[Qemu-devel] [PATCH 0/3] Drop virtio-{blk,scsi} op blockers

[Qemu-devel] [PATCH 0/3] Drop virtio-{blk,scsi} op blockers

[Fam Zheng]

We are ready to get rid of dataplane's op blockers altogether. Most operations
are already unblocked in virtio-blk, and those remained for virtio-scsi only
because we haven't got around to add counterpart unblocking code.

The first patch fixes an existing bug with blockdev-backup. Then the op
blockers are removed from both devices.


Fam Zheng (3):
  blockdev-backup: Don't move target AioContext if it's attached
  virtio-blk: Remove op blocker for dataplane
  virtio-scsi: Remove op blocker for dataplane

 blockdev.c                      | 12 +++++++-
 hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
 hw/scsi/virtio-scsi.c           | 62 ----------------------------------------
 include/hw/virtio/virtio-scsi.h | 11 -------
 4 files changed, 11 insertions(+), 137 deletions(-)

-- 
2.8.2

[Qemu-devel] [PATCH 1/3] blockdev-backup: Don't move target AioContext if it's attached

[Fam Zheng]

If the BDS is attached, it will want to stay on the AioContext where its
BlockBackend is. Don't call bdrv_set_aio_context in this case.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 1892b8e..eb15593 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3376,8 +3376,18 @@ void do_blockdev_backup(const char *device, const char *target,
     }
     target_bs = blk_bs(target_blk);
 
+    if (bdrv_get_aio_context(target_bs) != aio_context) {
+        if (!target_bs->blk) {
+            /* The target BDS is not attached, we can safely move it to another
+             * AioContext. */
+            bdrv_set_aio_context(target_bs, aio_context);
+        } else {
+            error_setg(errp, "Target is attached to a different thread from "
+                             "source.");
+            goto out;
+        }
+    }
     bdrv_ref(target_bs);
-    bdrv_set_aio_context(target_bs, aio_context);
     backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
                  on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
-- 
2.8.2

[Qemu-devel] [PATCH 2/3] virtio-blk: Remove op blocker for dataplane

[Fam Zheng]

Block layer is prepared to unspecialize dataplane, an evidence is this
almost complete list of unblocked operations. It has all types except
two (actually three if DATAPLANE itself counts but blockdev.c makes sure
attaching twice is not possible): MIRROR_TARGET and BACKUP_TARGET.

blockdev-mirror refuses to start if target is attached, so the first is
not a problem.

By removing BACKUP_TARGET, blockdev-backup will become permissive to
write to a virtio-blk dataplane disk, but that is not worse than
non-dataplane given the latter is already possible. In either case,
blockdev.c always checks the target and source are on the same
AioContext, or bring them together if possible.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
 1 file changed, 63 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3cb97c9..2073f9a 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -37,8 +37,6 @@ 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
@@ -46,9 +44,6 @@ struct VirtIOBlockDataPlane {
      */
     IOThread *iothread;
     AioContext *ctx;
-
-    /* Operation blocker on BDS */
-    Error *blocker;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -68,54 +63,6 @@ static void notify_guest_bh(void *opaque)
     event_notifier_set(s->guest_notifier);
 }
 
-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_SOURCE, 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);
-}
-
-static void data_plane_remove_op_blockers(VirtIOBlockDataPlane *s)
-{
-    if (s->blocker) {
-        blk_op_unblock_all(s->conf->conf.blk, s->blocker);
-        error_free(s->blocker);
-        s->blocker = NULL;
-    }
-}
-
-static void data_plane_blk_insert_notifier(Notifier *n, void *data)
-{
-    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
-                                           insert_notifier);
-    assert(s->conf->conf.blk == data);
-    data_plane_set_up_op_blockers(s);
-}
-
-static void data_plane_blk_remove_notifier(Notifier *n, void *data)
-{
-    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
-                                           remove_notifier);
-    assert(s->conf->conf.blk == data);
-    data_plane_remove_op_blockers(s);
-}
-
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
                                   VirtIOBlockDataPlane **dataplane,
@@ -158,13 +105,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     s->ctx = iothread_get_aio_context(s->iothread);
     s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
 
-    s->insert_notifier.notify = data_plane_blk_insert_notifier;
-    s->remove_notifier.notify = data_plane_blk_remove_notifier;
-    blk_add_insert_bs_notifier(conf->conf.blk, &s->insert_notifier);
-    blk_add_remove_bs_notifier(conf->conf.blk, &s->remove_notifier);
-
-    data_plane_set_up_op_blockers(s);
-
     *dataplane = s;
 }
 
@@ -176,9 +116,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     virtio_blk_data_plane_stop(s);
-    data_plane_remove_op_blockers(s);
-    notifier_remove(&s->insert_notifier);
-    notifier_remove(&s->remove_notifier);
     qemu_bh_delete(s->bh);
     object_unref(OBJECT(s->iothread));
     g_free(s);
-- 
2.8.2

[Qemu-devel] [PATCH 3/3] virtio-scsi: Remove op blocker for dataplane

[Fam Zheng]

The previous patch dropped all op blockers from virtio-blk data plane.
The situation of virtio-scsi is exactly the same it can drop them too.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi.c           | 62 -----------------------------------------
 include/hw/virtio/virtio-scsi.h | 11 --------
 2 files changed, 73 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 30415c6..d26f490 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -773,22 +773,6 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
     }
 }
 
-static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
-{
-    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
-                                                n, n);
-    assert(cn->sd->conf.blk == data);
-    blk_op_block_all(cn->sd->conf.blk, cn->s->blocker);
-}
-
-static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
-{
-    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
-                                                n, n);
-    assert(cn->sd->conf.blk == data);
-    blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker);
-}
-
 static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
@@ -797,29 +781,13 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     SCSIDevice *sd = SCSI_DEVICE(dev);
 
     if (s->ctx && !s->dataplane_fenced) {
-        VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
-
         if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
             return;
         }
-        blk_op_block_all(sd->conf.blk, s->blocker);
         aio_context_acquire(s->ctx);
         blk_set_aio_context(sd->conf.blk, s->ctx);
         aio_context_release(s->ctx);
 
-        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 (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
@@ -835,7 +803,6 @@ 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 (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
         virtio_scsi_push_event(s, sd,
@@ -843,28 +810,6 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                VIRTIO_SCSI_EVT_RESET_REMOVED);
     }
 
-    if (s->ctx) {
-        blk_op_unblock_all(sd->conf.blk, s->blocker);
-    }
-
-    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
-        if (insert_notifier->sd == sd) {
-            notifier_remove(&insert_notifier->n);
-            QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
-            g_free(insert_notifier);
-            break;
-        }
-    }
-
-    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
-        if (remove_notifier->sd == sd) {
-            notifier_remove(&remove_notifier->n);
-            QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
-            g_free(remove_notifier);
-            break;
-        }
-    }
-
     qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -950,11 +895,6 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
 
     register_savevm(dev, "virtio-scsi", virtio_scsi_id++, 1,
                     virtio_scsi_save, virtio_scsi_load, s);
-
-    error_setg(&s->blocker, "block device is in use by data plane");
-
-    QTAILQ_INIT(&s->insert_notifiers);
-    QTAILQ_INIT(&s->remove_notifiers);
 }
 
 static void virtio_scsi_instance_init(Object *obj)
@@ -980,8 +920,6 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(dev);
 
-    error_free(s->blocker);
-
     unregister_savevm(dev, "virtio-scsi", s);
     virtio_scsi_common_unrealize(dev, errp);
 }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index ba2f5ce..b515669 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -68,13 +68,6 @@ typedef struct VirtIOSCSICommon {
     VirtQueue **cmd_vqs;
 } VirtIOSCSICommon;
 
-typedef struct VirtIOSCSIBlkChangeNotifier {
-    Notifier n;
-    struct VirtIOSCSI *s;
-    SCSIDevice *sd;
-    QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
-} VirtIOSCSIBlkChangeNotifier;
-
 typedef struct VirtIOSCSI {
     VirtIOSCSICommon parent_obj;
 
@@ -85,14 +78,10 @@ typedef struct VirtIOSCSI {
     /* Fields for dataplane below */
     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
-    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
-    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
-
     bool dataplane_started;
     bool dataplane_starting;
     bool dataplane_stopping;
     bool dataplane_fenced;
-    Error *blocker;
     uint32_t host_features;
 } VirtIOSCSI;
 
-- 
2.8.2

Re: [Qemu-devel] [PATCH 2/3] virtio-blk: Remove op blocker for dataplane

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:24:09PM +0800, Fam Zheng wrote:
> Block layer is prepared to unspecialize dataplane, an evidence is this
> almost complete list of unblocked operations. It has all types except
> two (actually three if DATAPLANE itself counts but blockdev.c makes sure
> attaching twice is not possible): MIRROR_TARGET and BACKUP_TARGET.
> 
> blockdev-mirror refuses to start if target is attached, so the first is
> not a problem.
> 
> By removing BACKUP_TARGET, blockdev-backup will become permissive to
> write to a virtio-blk dataplane disk, but that is not worse than
> non-dataplane given the latter is already possible. In either case,
> blockdev.c always checks the target and source are on the same
> AioContext, or bring them together if possible.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
>  1 file changed, 63 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 3cb97c9..2073f9a 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -37,8 +37,6 @@ 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
> @@ -46,9 +44,6 @@ struct VirtIOBlockDataPlane {
>       */
>      IOThread *iothread;
>      AioContext *ctx;
> -
> -    /* Operation blocker on BDS */
> -    Error *blocker;
>  };
>  
>  /* Raise an interrupt to signal guest, if necessary */
> @@ -68,54 +63,6 @@ static void notify_guest_bh(void *opaque)
>      event_notifier_set(s->guest_notifier);
>  }
>  
> -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_SOURCE, 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);
> -}
> -
> -static void data_plane_remove_op_blockers(VirtIOBlockDataPlane *s)
> -{
> -    if (s->blocker) {
> -        blk_op_unblock_all(s->conf->conf.blk, s->blocker);
> -        error_free(s->blocker);
> -        s->blocker = NULL;
> -    }
> -}
> -
> -static void data_plane_blk_insert_notifier(Notifier *n, void *data)
> -{
> -    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
> -                                           insert_notifier);
> -    assert(s->conf->conf.blk == data);
> -    data_plane_set_up_op_blockers(s);
> -}
> -
> -static void data_plane_blk_remove_notifier(Notifier *n, void *data)
> -{
> -    VirtIOBlockDataPlane *s = container_of(n, VirtIOBlockDataPlane,
> -                                           remove_notifier);
> -    assert(s->conf->conf.blk == data);
> -    data_plane_remove_op_blockers(s);
> -}
> -
>  /* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>                                    VirtIOBlockDataPlane **dataplane,
> @@ -158,13 +105,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>      s->ctx = iothread_get_aio_context(s->iothread);
>      s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
>  
> -    s->insert_notifier.notify = data_plane_blk_insert_notifier;
> -    s->remove_notifier.notify = data_plane_blk_remove_notifier;
> -    blk_add_insert_bs_notifier(conf->conf.blk, &s->insert_notifier);
> -    blk_add_remove_bs_notifier(conf->conf.blk, &s->remove_notifier);
> -
> -    data_plane_set_up_op_blockers(s);
> -
>      *dataplane = s;
>  }
>  
> @@ -176,9 +116,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>      }
>  
>      virtio_blk_data_plane_stop(s);
> -    data_plane_remove_op_blockers(s);
> -    notifier_remove(&s->insert_notifier);
> -    notifier_remove(&s->remove_notifier);
>      qemu_bh_delete(s->bh);
>      object_unref(OBJECT(s->iothread));
>      g_free(s);
> -- 
> 2.8.2

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Remove op blocker for dataplane

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:24:10PM +0800, Fam Zheng wrote:
> The previous patch dropped all op blockers from virtio-blk data plane.
> The situation of virtio-scsi is exactly the same it can drop them too.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/scsi/virtio-scsi.c           | 62 -----------------------------------------
>  include/hw/virtio/virtio-scsi.h | 11 --------
>  2 files changed, 73 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 30415c6..d26f490 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -773,22 +773,6 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
>      }
>  }
>  
> -static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
> -{
> -    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
> -                                                n, n);
> -    assert(cn->sd->conf.blk == data);
> -    blk_op_block_all(cn->sd->conf.blk, cn->s->blocker);
> -}
> -
> -static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
> -{
> -    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
> -                                                n, n);
> -    assert(cn->sd->conf.blk == data);
> -    blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker);
> -}
> -
>  static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                  Error **errp)
>  {
> @@ -797,29 +781,13 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      SCSIDevice *sd = SCSI_DEVICE(dev);
>  
>      if (s->ctx && !s->dataplane_fenced) {
> -        VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
> -
>          if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
>              return;
>          }
> -        blk_op_block_all(sd->conf.blk, s->blocker);
>          aio_context_acquire(s->ctx);
>          blk_set_aio_context(sd->conf.blk, s->ctx);
>          aio_context_release(s->ctx);
>  
> -        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 (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
> @@ -835,7 +803,6 @@ 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 (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
>          virtio_scsi_push_event(s, sd,
> @@ -843,28 +810,6 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 VIRTIO_SCSI_EVT_RESET_REMOVED);
>      }
>  
> -    if (s->ctx) {
> -        blk_op_unblock_all(sd->conf.blk, s->blocker);
> -    }
> -
> -    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
> -        if (insert_notifier->sd == sd) {
> -            notifier_remove(&insert_notifier->n);
> -            QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> -            g_free(insert_notifier);
> -            break;
> -        }
> -    }
> -
> -    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> -        if (remove_notifier->sd == sd) {
> -            notifier_remove(&remove_notifier->n);
> -            QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> -            g_free(remove_notifier);
> -            break;
> -        }
> -    }
> -
>      qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }
>  
> @@ -950,11 +895,6 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
>  
>      register_savevm(dev, "virtio-scsi", virtio_scsi_id++, 1,
>                      virtio_scsi_save, virtio_scsi_load, s);
> -
> -    error_setg(&s->blocker, "block device is in use by data plane");
> -
> -    QTAILQ_INIT(&s->insert_notifiers);
> -    QTAILQ_INIT(&s->remove_notifiers);
>  }
>  
>  static void virtio_scsi_instance_init(Object *obj)
> @@ -980,8 +920,6 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
>  {
>      VirtIOSCSI *s = VIRTIO_SCSI(dev);
>  
> -    error_free(s->blocker);
> -
>      unregister_savevm(dev, "virtio-scsi", s);
>      virtio_scsi_common_unrealize(dev, errp);
>  }
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index ba2f5ce..b515669 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -68,13 +68,6 @@ typedef struct VirtIOSCSICommon {
>      VirtQueue **cmd_vqs;
>  } VirtIOSCSICommon;
>  
> -typedef struct VirtIOSCSIBlkChangeNotifier {
> -    Notifier n;
> -    struct VirtIOSCSI *s;
> -    SCSIDevice *sd;
> -    QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
> -} VirtIOSCSIBlkChangeNotifier;
> -
>  typedef struct VirtIOSCSI {
>      VirtIOSCSICommon parent_obj;
>  
> @@ -85,14 +78,10 @@ typedef struct VirtIOSCSI {
>      /* Fields for dataplane below */
>      AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
>  
> -    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
> -    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
> -
>      bool dataplane_started;
>      bool dataplane_starting;
>      bool dataplane_stopping;
>      bool dataplane_fenced;
> -    Error *blocker;
>      uint32_t host_features;
>  } VirtIOSCSI;
>  
> -- 
> 2.8.2

Re: [Qemu-devel] [PATCH 0/3] Drop virtio-{blk,scsi} op blockers

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:24:07PM +0800, Fam Zheng wrote:
> We are ready to get rid of dataplane's op blockers altogether. Most operations
> are already unblocked in virtio-blk, and those remained for virtio-scsi only
> because we haven't got around to add counterpart unblocking code.
> 
> The first patch fixes an existing bug with blockdev-backup. Then the op
> blockers are removed from both devices.

Seems to be appropriate for block tree, not virtio.
Thus ack on virtio patches only.

> 
> Fam Zheng (3):
>   blockdev-backup: Don't move target AioContext if it's attached
>   virtio-blk: Remove op blocker for dataplane
>   virtio-scsi: Remove op blocker for dataplane
> 
>  blockdev.c                      | 12 +++++++-
>  hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
>  hw/scsi/virtio-scsi.c           | 62 ----------------------------------------
>  include/hw/virtio/virtio-scsi.h | 11 -------
>  4 files changed, 11 insertions(+), 137 deletions(-)
> 
> -- 
> 2.8.2

Re: [Qemu-devel] [PATCH 1/3] blockdev-backup: Don't move target AioContext if it's attached

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Wed, May 18, 2016 at 04:24:08PM +0800, Fam Zheng wrote:
> If the BDS is attached, it will want to stay on the AioContext where its
> BlockBackend is. Don't call bdrv_set_aio_context in this case.

What should the user do when they hit this error?

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1892b8e..eb15593 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3376,8 +3376,18 @@ void do_blockdev_backup(const char *device, const char *target,
>      }
>      target_bs = blk_bs(target_blk);
>  
> +    if (bdrv_get_aio_context(target_bs) != aio_context) {
> +        if (!target_bs->blk) {
> +            /* The target BDS is not attached, we can safely move it to another
> +             * AioContext. */
> +            bdrv_set_aio_context(target_bs, aio_context);
> +        } else {
> +            error_setg(errp, "Target is attached to a different thread from "
> +                             "source.");
> +            goto out;
> +        }
> +    }
>      bdrv_ref(target_bs);
> -    bdrv_set_aio_context(target_bs, aio_context);
>      backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
>                   on_target_error, block_job_cb, bs, txn, &local_err);
>      if (local_err != NULL) {
> -- 
> 2.8.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Drop virtio-{blk,scsi} op blockers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On Wed, May 18, 2016 at 04:24:07PM +0800, Fam Zheng wrote:
> We are ready to get rid of dataplane's op blockers altogether. Most operations
> are already unblocked in virtio-blk, and those remained for virtio-scsi only
> because we haven't got around to add counterpart unblocking code.
> 
> The first patch fixes an existing bug with blockdev-backup. Then the op
> blockers are removed from both devices.
> 
> 
> Fam Zheng (3):
>   blockdev-backup: Don't move target AioContext if it's attached
>   virtio-blk: Remove op blocker for dataplane
>   virtio-scsi: Remove op blocker for dataplane
> 
>  blockdev.c                      | 12 +++++++-
>  hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
>  hw/scsi/virtio-scsi.c           | 62 ----------------------------------------

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

We may still need to refine how aio context contraints between BDSes
work (my reply to Patch 1) but this series doesn't change semantics
there.  Looks good.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Drop virtio-{blk,scsi} op blockers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

On Wed, May 18, 2016 at 04:24:07PM +0800, Fam Zheng wrote:
> We are ready to get rid of dataplane's op blockers altogether. Most operations
> are already unblocked in virtio-blk, and those remained for virtio-scsi only
> because we haven't got around to add counterpart unblocking code.
> 
> The first patch fixes an existing bug with blockdev-backup. Then the op
> blockers are removed from both devices.
> 
> 
> Fam Zheng (3):
>   blockdev-backup: Don't move target AioContext if it's attached
>   virtio-blk: Remove op blocker for dataplane
>   virtio-scsi: Remove op blocker for dataplane
> 
>  blockdev.c                      | 12 +++++++-
>  hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
>  hw/scsi/virtio-scsi.c           | 62 ----------------------------------------
>  include/hw/virtio/virtio-scsi.h | 11 -------
>  4 files changed, 11 insertions(+), 137 deletions(-)

Please rebase on qemu.git/master because Kevin's "[PULL 23/31] block:
Remove BlockDriverState.blk" drops BDS->blk.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/3] blockdev-backup: Don't move target AioContext if it's attached

[Kevin Wolf]

Am 18.05.2016 um 10:24 hat Fam Zheng geschrieben:
> If the BDS is attached, it will want to stay on the AioContext where its
> BlockBackend is. Don't call bdrv_set_aio_context in this case.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1892b8e..eb15593 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3376,8 +3376,18 @@ void do_blockdev_backup(const char *device, const char *target,
>      }
>      target_bs = blk_bs(target_blk);
>  
> +    if (bdrv_get_aio_context(target_bs) != aio_context) {
> +        if (!target_bs->blk) {

How should this ever happen when we have target_bs = blk_bs(target_blk)
two lines above?

> +            /* The target BDS is not attached, we can safely move it to another
> +             * AioContext. */
> +            bdrv_set_aio_context(target_bs, aio_context);
> +        } else {
> +            error_setg(errp, "Target is attached to a different thread from "
> +                             "source.");
> +            goto out;
> +        }
> +    }

Kevin

Re: [Qemu-devel] [PATCH 1/3] blockdev-backup: Don't move target AioContext if it's attached

[Fam Zheng]

On Fri, 05/20 10:03, Kevin Wolf wrote:
> Am 18.05.2016 um 10:24 hat Fam Zheng geschrieben:
> > If the BDS is attached, it will want to stay on the AioContext where its
> > BlockBackend is. Don't call bdrv_set_aio_context in this case.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 1892b8e..eb15593 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3376,8 +3376,18 @@ void do_blockdev_backup(const char *device, const char *target,
> >      }
> >      target_bs = blk_bs(target_blk);
> >  
> > +    if (bdrv_get_aio_context(target_bs) != aio_context) {
> > +        if (!target_bs->blk) {
> 
> How should this ever happen when we have target_bs = blk_bs(target_blk)
> two lines above?

I must have made a mistake with git or my editor, I meant to change it to
bdrv_lookup_bs above.

Fam

> 
> > +            /* The target BDS is not attached, we can safely move it to another
> > +             * AioContext. */
> > +            bdrv_set_aio_context(target_bs, aio_context);
> > +        } else {
> > +            error_setg(errp, "Target is attached to a different thread from "
> > +                             "source.");
> > +            goto out;
> > +        }
> > +    }
> 
> Kevin

Re: [Qemu-devel] [PATCH 1/3] blockdev-backup: Don't move target AioContext if it's attached

[Fam Zheng]

On Thu, 05/19 13:42, Stefan Hajnoczi wrote:
> On Wed, May 18, 2016 at 04:24:08PM +0800, Fam Zheng wrote:
> > If the BDS is attached, it will want to stay on the AioContext where its
> > BlockBackend is. Don't call bdrv_set_aio_context in this case.
> 
> What should the user do when they hit this error?

The user should backup to a dedicate target newly inserted with blockdev-add.
On this error, the user must be using a dataplane enabled virtual disk as
backup target, which is not supported.  The mirror job already refuses any
attached device, but for backup we want to support image fleecing and colo, in
which case the target is attached to NBD.

Fam

> 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 1892b8e..eb15593 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3376,8 +3376,18 @@ void do_blockdev_backup(const char *device, const char *target,
> >      }
> >      target_bs = blk_bs(target_blk);
> >  
> > +    if (bdrv_get_aio_context(target_bs) != aio_context) {
> > +        if (!target_bs->blk) {
> > +            /* The target BDS is not attached, we can safely move it to another
> > +             * AioContext. */
> > +            bdrv_set_aio_context(target_bs, aio_context);
> > +        } else {
> > +            error_setg(errp, "Target is attached to a different thread from "
> > +                             "source.");
> > +            goto out;
> > +        }
> > +    }
> >      bdrv_ref(target_bs);
> > -    bdrv_set_aio_context(target_bs, aio_context);
> >      backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
> >                   on_target_error, block_job_cb, bs, txn, &local_err);
> >      if (local_err != NULL) {
> > -- 
> > 2.8.2
> >