Re: [Qemu-devel] [PATCH v2 10/11] blockdev: Block device IO during blockdev-backup transaction

[Paolo Bonzini]

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