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

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

[Fam Zheng]

v2: Switch to bdrv_lookup_bs on target_bs. [Kevin]
    Rebase onto master.
    Add Michael's ack-by in virtio patches.

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 (4):
  blockdev-backup: Use bdrv_lookup_bs on target
  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                      | 23 ++++++++-------
 hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
 hw/scsi/virtio-scsi.c           | 62 ----------------------------------------
 include/hw/virtio/virtio-scsi.h | 11 -------
 4 files changed, 13 insertions(+), 146 deletions(-)

-- 
2.8.2

[Qemu-devel] [PATCH v2 1/4] blockdev-backup: Use bdrv_lookup_bs on target

[Fam Zheng]

This allows backing up to a BDS that has not been attached to any BB.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 40e4e6f..026ead0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3346,7 +3346,7 @@ void do_blockdev_backup(const char *device, const char *target,
                          BlockdevOnError on_target_error,
                          BlockJobTxn *txn, Error **errp)
 {
-    BlockBackend *blk, *target_blk;
+    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     Error *local_err = NULL;
@@ -3377,18 +3377,11 @@ void do_blockdev_backup(const char *device, const char *target,
     }
     bs = blk_bs(blk);
 
-    target_blk = blk_by_name(target);
-    if (!target_blk) {
-        error_setg(errp, "Device '%s' not found", target);
+    target_bs = bdrv_lookup_bs(target, target, errp);
+    if (!target_bs) {
         goto out;
     }
 
-    if (!blk_is_available(target_blk)) {
-        error_setg(errp, "Device '%s' has no medium", target);
-        goto out;
-    }
-    target_bs = blk_bs(target_blk);
-
     bdrv_ref(target_bs);
     bdrv_set_aio_context(target_bs, aio_context);
     backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
-- 
2.8.2

[Qemu-devel] [PATCH v2 2/4] 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 026ead0..85eb7d7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3382,8 +3382,18 @@ void do_blockdev_backup(const char *device, const char *target,
         goto out;
     }
 
+    if (bdrv_get_aio_context(target_bs) != aio_context) {
+        if (!bdrv_has_blk(target_bs)) {
+            /* 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 v2 3/4] 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>
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

[Qemu-devel] [PATCH v2 4/4] 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>
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 v2 0/4] Drop virtio-{blk,scsi} op blockers

[Fam Zheng]

On Mon, 05/23 10:19, Fam Zheng wrote:
> v2: Switch to bdrv_lookup_bs on target_bs. [Kevin]
>     Rebase onto master.
>     Add Michael's ack-by in virtio patches.
> 
> 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.

Without this series, completing a mirror job on virtio-blk-dataplane disk fails
an assertion (thanks to Yanbin Duan <yduan@redhat.com> for providing the
backtrace):

block.c:2338: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed.
 
(gdb) bt
#0  0x00007f10d44915f7 in raise () from /lib64/libc.so.6
#1  0x00007f10d4492ce8 in abort () from /lib64/libc.so.6
#2  0x00007f10d448a566 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007f10d448a612 in __assert_fail () from /lib64/libc.so.6
#4  0x00007f10dc9f856f in bdrv_delete (bs=0x7f10de16e800) at block.c:2338
#5  bdrv_unref (bs=0x7f10de16e800) at block.c:3376
#6  0x00007f10dc9fb353 in block_job_defer_to_main_loop_bh (opaque=0x7f10de038310) at blockjob.c:476
#7  0x00007f10dc9f3fcd in aio_bh_call (bh=<optimized out>) at async.c:66
#8  aio_bh_poll (ctx=ctx@entry=0x7f10de109980) at async.c:94
#9  0x00007f10dc9fdb70 in aio_dispatch (ctx=0x7f10de109980) at aio-posix.c:308
#10 0x00007f10dc9f3dbe in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>,
    user_data=<optimized out>) at async.c:233
---Type <return> to continue, or q <return> to quit---
#11 0x00007f10d528f79a in g_main_context_dispatch () from /lib64/libglib-2.0.so.0
#12 0x00007f10dc9fc460 in glib_pollfds_poll () at main-loop.c:213
#13 os_host_main_loop_wait (timeout=<optimized out>) at main-loop.c:258
#14 main_loop_wait (nonblocking=<optimized out>) at main-loop.c:506
#15 0x00007f10dc7c655f in main_loop () at vl.c:1934
#16 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4667

This is because the bdrv_replace_in_backing_chain skipped the BlockBackend's
"remove notifier" which is responsible for dropping the dataplane op blockers.

Now we are dropping the notifier handlers, the crash is gone.  Therefore this
should probably be included in qemu-stable@nongnu.org because it affacts 2.6
too.

Fam

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

[Stefan Hajnoczi]

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

On Mon, May 23, 2016 at 10:19:34AM +0800, Fam Zheng wrote:
> v2: Switch to bdrv_lookup_bs on target_bs. [Kevin]
>     Rebase onto master.
>     Add Michael's ack-by in virtio patches.
> 
> 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 (4):
>   blockdev-backup: Use bdrv_lookup_bs on target
>   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                      | 23 ++++++++-------
>  hw/block/dataplane/virtio-blk.c | 63 -----------------------------------------
>  hw/scsi/virtio-scsi.c           | 62 ----------------------------------------
>  include/hw/virtio/virtio-scsi.h | 11 -------
>  4 files changed, 13 insertions(+), 146 deletions(-)
> 
> -- 
> 2.8.2
> 
> 

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

Stefan

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