Re: [Qemu-devel] [PATCH v4 05/11] block: Move BDS close notifiers into BB

[Max Reitz <mreitz@redhat.com>]

On 2015-02-27 at 11:43, Max Reitz wrote:
> The only remaining user of the BDS close notifiers is NBD which uses
> them to determine when a BDS tree is being ejected. This patch removes
> the BDS-level close notifiers and adds a notifier list to the
> BlockBackend structure that is invoked whenever a BDS is removed.
>
> Symmetrically to that, another notifier list is added that is invoked
> whenever a BDS is inserted. The dataplane implementations for virtio-blk
> and virtio-scsi use both notifier types for setting up and removing op
> blockers. This is not only important for setting up the op blockers on
> insertion, but also for removing them on ejection since bdrv_delete()
> asserts that there are no op blockers set up.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block.c                         |  7 ----
>   block/block-backend.c           | 19 +++++++--
>   blockdev-nbd.c                  | 36 +----------------
>   hw/block/dataplane/virtio-blk.c | 77 +++++++++++++++++++++++++++---------
>   hw/scsi/virtio-scsi.c           | 87 ++++++++++++++++++++++++++++++++++++++---
>   include/block/block.h           |  1 -
>   include/block/block_int.h       |  2 -
>   include/hw/virtio/virtio-scsi.h | 10 +++++
>   include/sysemu/block-backend.h  |  3 +-
>   nbd.c                           | 13 ++++++
>   10 files changed, 182 insertions(+), 73 deletions(-)

[snip]

> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 5469bad..570f2fd 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -755,6 +755,37 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
>       }
>   }
>   
> +static void virtio_scsi_set_up_op_blockers(VirtIOSCSI *s, SCSIDevice *sd)
> +{
> +    if (!s->blocker) {
> +        error_setg(&s->blocker, "block device is in use by data plane");

This will no longer be necessary with v2 of my virtio-scsi op blocker patch.

> +    }
> +    blk_op_block_all(sd->conf.blk, s->blocker);
> +}
> +
> +static void virtio_scsi_remove_op_blockers(VirtIOSCSI *s, SCSIDevice *sd)
> +{
> +    if (s->blocker) {
> +        blk_op_unblock_all(sd->conf.blk, s->blocker);
> +    }
> +}
> +
> +static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
> +{
> +    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
> +                                                n, n);
> +    assert(cn->sd->conf.blk == data);
> +    virtio_scsi_set_up_op_blockers(cn->s, cn->sd);
> +}
> +
> +static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
> +{
> +    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
> +                                                n, n);
> +    assert(cn->sd->conf.blk == data);
> +    virtio_scsi_remove_op_blockers(cn->s, cn->sd);
> +}
> +
>   static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                   Error **errp)
>   {
> @@ -763,12 +794,26 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       SCSIDevice *sd = SCSI_DEVICE(dev);
>   
>       if (s->ctx && !s->dataplane_disabled) {
> +        VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
> +
> +        insert_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
> +        insert_notifier->n.notify = virtio_scsi_blk_insert_notifier;
> +        insert_notifier->s = s;
> +        insert_notifier->sd = sd;
> +        blk_add_insert_bs_notifier(sd->conf.blk, &insert_notifier->n);
> +        QTAILQ_INSERT_TAIL(&s->insert_notifiers, insert_notifier, next);
> +
> +        remove_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
> +        remove_notifier->n.notify = virtio_scsi_blk_remove_notifier;
> +        remove_notifier->s = s;
> +        remove_notifier->sd = sd;
> +        blk_add_remove_bs_notifier(sd->conf.blk, &remove_notifier->n);
> +        QTAILQ_INSERT_TAIL(&s->remove_notifiers, remove_notifier, next);
> +
>           if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
>               return;
>           }
> -        assert(!s->blocker);
> -        error_setg(&s->blocker, "block device is in use by data plane");

Same for these two lines removed (they just don't exist any more).

> -        blk_op_block_all(sd->conf.blk, s->blocker);
> +        virtio_scsi_set_up_op_blockers(s, sd);
>       }
>   
>       if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
> @@ -784,6 +829,7 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
>       VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>       SCSIDevice *sd = SCSI_DEVICE(dev);
> +    VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
>   
>       if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
>           virtio_scsi_push_event(s, sd,
> @@ -791,11 +837,39 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                  VIRTIO_SCSI_EVT_RESET_REMOVED);
>       }
>   
> -    if (s->ctx && s->blocker) {
> -        blk_op_unblock_all(sd->conf.blk, s->blocker);
> +    if (s->ctx) {
> +        virtio_scsi_remove_op_blockers(s, sd);
> +    }
> +
> +    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
> +        if (insert_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (insert_notifier) {
> +        notifier_remove(&insert_notifier->n);
> +        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> +        g_free(insert_notifier);
> +    }
> +
> +    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> +        if (remove_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (remove_notifier) {
> +        notifier_remove(&remove_notifier->n);
> +        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> +        g_free(remove_notifier);
> +    }
> +
> +    if (QTAILQ_EMPTY(&s->insert_notifiers) &&
> +        QTAILQ_EMPTY(&s->remove_notifiers))
> +    {
>           error_free(s->blocker);
>           s->blocker = NULL;
>       }
> +

And for this last block.

Max