Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Put the code for setting up and removing op blockers into an own
> function, respectively. Then, we can invoke those functions whenever a
> BDS is removed from an virtio-blk BB or inserted into it.
> 
> Signed-off-by: Max Reitz <address@hidden>

Do you know of a case where this is observable? I don't think you can
change the medium of a virtio-blk device, and blk_set_bs() isn't
converted to a wrapper around blk_remove/insert_bs() yet. If this patch
is necessary to fix something observable, the latter is probably a bug.

> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c42ddeb..4c95d5b 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
>      EventNotifier *guest_notifier;  /* irq */
>      QEMUBH *bh;                     /* bh for guest notification */
>  
> +    Notifier insert_notifier, remove_notifier;
> +
>      /* Note that these EventNotifiers are assigned by value.  This is
>       * fine as long as you do not call event_notifier_cleanup on them
>       * (because you don't own the file descriptor or handle; you just
> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
>      blk_io_unplug(s->conf->conf.blk);
>  }
>  
> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> +{
> +    assert(!s->blocker);
> +    error_setg(&s->blocker, "block device is in use by data plane");
> +    blk_op_block_all(s->conf->conf.blk, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, 
> s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, 
> s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, 
> s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> +                   s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
> +    blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
> +}

This makes me wonder: What do we even block here any more? If I didn't
miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
why this needs to be blocked, or if we simply forgot to enable it.

Kevin