[Qemu-block] [PATCH v2 10/11] blockdev: Block device IO during blockdev-backup transaction
Signed-off-by: Fam Zheng f...@redhat.com
---
blockdev.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index ae52d27..bd28183 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1649,6 +1649,7 @@ typedef struct BlockdevBackupState {
BlockDriverState *bs;
BlockJob *job;
AioContext *aio_context;
+Error *blocker;
} BlockdevBackupState;
static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
@@ -1685,6 +1686,10 @@ static void blockdev_backup_prepare(BlkTransactionState
*common, Error **errp)
}
aio_context_acquire(state-aio_context);
+state-bs = bs;
+error_setg(state-blocker, blockdev-backup in progress);
+bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, state-blocker);
+
qmp_blockdev_backup(backup-device, backup-target,
backup-sync,
backup-has_speed, backup-speed,
@@ -1696,7 +1701,6 @@ static void blockdev_backup_prepare(BlkTransactionState
*common, Error **errp)
return;
}
-state-bs = bs;
state-job = state-bs-job;
}
@@ -1715,6 +1719,10 @@ static void blockdev_backup_clean(BlkTransactionState
*common)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
+if (state-bs) {
+bdrv_op_unblock(state-bs, BLOCK_OP_TYPE_DEVICE_IO, state-blocker);
+error_free(state-blocker);
+}
if (state-aio_context) {
aio_context_release(state-aio_context);
}
--
2.4.0
Re: [Qemu-block] [PATCH v2 10/11] blockdev: Block device IO during blockdev-backup transaction
On 13/05/2015 19:28, Fam Zheng wrote: +state-bs = bs; +error_setg(state-blocker, blockdev-backup in progress); +bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, state-blocker); + qmp_blockdev_backup(backup-device, backup-target, backup-sync, backup-has_speed, backup-speed, @@ -1696,7 +1701,6 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) return; } -state-bs = bs; I don't understand this. Jobs could pause/resume themselves by adding a DEVICE_IO notifier on the targets. However, block backups is the one job that cannot do this, because I/O on the source triggers I/O on the target. So if we consider this idea worthwhile, and decide that pausing device I/O on the target should pause the block job, the backup job actually has to prevent *adding a DEVICE_IO blocker* on the target. This meta-block is not possible in your design, which is a pity because on the surface it looked nicer than mine. FWIW, my original idea was: - bdrv_pause checks if there is an operation blocker for PAUSE. if it is there, it fails - otherwise, bdrv_pause invokes a notifier list if this is the outermost call. if not the outermost call, it does nothing - bdrv_resume does the same, but does not need a blocker - drive-backup should block PAUSE on its target Also, should the blockers (either DEVICE_IO in your design, or PAUSE in mine) be included in bdrv_op_block_all. I would say no in your case; here is the proof: - block-backup doesn't like DEVICE_IO blockers on the target - block-backup calls bdrv_op_block_all on the target - hence, bdrv_op_block_all shouldn't block DEVICE_IO block_job_create is another suspicious caller of bdrv_op_block_all. It probably shouldn't block neither PAUSE nor DEVICE_IO. Paolo
Re: [Qemu-block] [PATCH v2 10/11] blockdev: Block device IO during blockdev-backup transaction
On 13/05/2015 16:55, Fam Zheng wrote: So if we consider this idea worthwhile, and decide that pausing device I/O on the target should pause the block job, the backup job actually has to prevent *adding a DEVICE_IO blocker* on the target. When do we need to pause device IO on the target? For drive-backup the target is anonymous and no device exists on it, so it's out of question. For blockdev-backup, users can have a device attached, but I'm not sure we should pause the job in this case. Well, it depends on the meaning you give to pause/device_io blocker. For me it was I want exclusive ownership of the disk, no one else should write. For blockdev-backup for example you could mirror the backup destination somewhere else, and mirror needs to drain the source. For that to make sense, the block job has to be paused. Of course this is contrived, but it's better to make the design generic. This meta-block is not possible in your design, which is a pity because on the surface it looked nicer than mine. FWIW, my original idea was: - bdrv_pause checks if there is an operation blocker for PAUSE. if it is there, it fails It's more complicated, because bdrv_drain_all can't fail now, if we want bdrv_pause there, then it could fail, and will become much harder to use. I think we don't need bdrv_pause there. See my reply to patch 11---we want it in the callers. In your idea, who will block PAUSE, and why? For example, blockdev-backup/drive-backup can block PAUSE on the target. But maybe they only need to add an op blocker notifier, and use it to block device I/O on the source and drain it. So your idea is safe. Good! Another idea was to block PAUSE in virtio-scsi until it's fixed. Paolo - otherwise, bdrv_pause invokes a notifier list if this is the outermost call. if not the outermost call, it does nothing - bdrv_resume does the same, but does not need a blocker - drive-backup should block PAUSE on its target
