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
