[PATCH 0/6] virtio-blk: iothread-vq-mapping cleanups

[PATCH 0/6] virtio-blk: iothread-vq-mapping cleanups

[Stefan Hajnoczi]

Kevin Wolf identified some issues with the virtio-blk iothread-vq-mapping patch
series that was merged at the end of 2023:
1. s->rq is restarted from one AioContext and races with the other
   iothread-vq-mapping AioContexts.
2. Failure to set the AioContext is no longer fatal since the IO_CODE APIs can
   be called from any thread. We can relax the error handling behavior.
3. Starting dataplane must self-trigger the ioeventfd even in a drained
   section. Failure to do so could lead to an unresponsive virtio-blk device.

This patch series addresses these issues. The first few patches merge the
hw/block/dataplane/virtio-blk.c code into hw/block/virtio-blk.c so that s->rq
can easily be restarted in the correct AioContexts.

Stefan Hajnoczi (6):
  virtio-blk: move dataplane code into virtio-blk.c
  virtio-blk: rename dataplane create/destroy functions
  virtio-blk: rename dataplane to ioeventfd
  virtio-blk: restart s->rq reqs in vq AioContexts
  virtio-blk: tolerate failure to set BlockBackend AioContext
  virtio-blk: always set ioeventfd during startup

 meson.build                     |   1 -
 hw/block/dataplane/trace.h      |   1 -
 hw/block/dataplane/virtio-blk.h |  34 ---
 include/hw/virtio/virtio-blk.h  |  16 +-
 hw/block/dataplane/virtio-blk.c | 404 -------------------------------
 hw/block/virtio-blk.c           | 412 +++++++++++++++++++++++++++++---
 hw/block/dataplane/meson.build  |   1 -
 hw/block/dataplane/trace-events |   5 -
 8 files changed, 391 insertions(+), 483 deletions(-)
 delete mode 100644 hw/block/dataplane/trace.h
 delete mode 100644 hw/block/dataplane/virtio-blk.h
 delete mode 100644 hw/block/dataplane/virtio-blk.c
 delete mode 100644 hw/block/dataplane/trace-events

-- 
2.43.0

[PATCH 1/6] virtio-blk: move dataplane code into virtio-blk.c

[Stefan Hajnoczi]

The dataplane code used to be significantly different from the
non-dataplane code and therefore had a separate source file.

Over time the difference has gotten smaller because the I/O code paths
were unified. Nowadays the distinction between the VirtIOBlock and
VirtIOBlockDataPlane structs is more of an inconvenience that hinders
code simplification.

Move hw/block/dataplane/virtio-blk.c into hw/block/virtio-blk.c, merging
VirtIOBlockDataPlane's fields into VirtIOBlock.

hw/block/virtio-blk.c used VirtIOBlock->dataplane to check if
virtio_blk_data_plane_create() was successful. This is not necessary
because ->dataplane_started and ->dataplane_disabled can be used
instead. This patch makes those changes in order to drop
VirtIOBlock->dataplane.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 meson.build                     |   1 -
 hw/block/dataplane/trace.h      |   1 -
 hw/block/dataplane/virtio-blk.h |  34 ---
 include/hw/virtio/virtio-blk.h  |  12 +-
 hw/block/dataplane/virtio-blk.c | 404 --------------------------------
 hw/block/virtio-blk.c           | 362 ++++++++++++++++++++++++++--
 hw/block/dataplane/meson.build  |   1 -
 hw/block/dataplane/trace-events |   5 -
 8 files changed, 357 insertions(+), 463 deletions(-)
 delete mode 100644 hw/block/dataplane/trace.h
 delete mode 100644 hw/block/dataplane/virtio-blk.h
 delete mode 100644 hw/block/dataplane/virtio-blk.c
 delete mode 100644 hw/block/dataplane/trace-events

diff --git a/meson.build b/meson.build
index 38deb9363c..38745771f9 100644
--- a/meson.build
+++ b/meson.build
@@ -3270,7 +3270,6 @@ if have_system
     'hw/arm',
     'hw/audio',
     'hw/block',
-    'hw/block/dataplane',
     'hw/char',
     'hw/display',
     'hw/dma',
diff --git a/hw/block/dataplane/trace.h b/hw/block/dataplane/trace.h
deleted file mode 100644
index 240cc59834..0000000000
--- a/hw/block/dataplane/trace.h
+++ /dev/null
@@ -1 +0,0 @@
-#include "trace/trace-hw_block_dataplane.h"
diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
deleted file mode 100644
index 1a806fe447..0000000000
--- a/hw/block/dataplane/virtio-blk.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Dedicated thread for virtio-blk I/O processing
- *
- * Copyright 2012 IBM, Corp.
- * Copyright 2012 Red Hat, Inc. and/or its affiliates
- *
- * Authors:
- *   Stefan Hajnoczi <stefanha@redhat.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#ifndef HW_DATAPLANE_VIRTIO_BLK_H
-#define HW_DATAPLANE_VIRTIO_BLK_H
-
-#include "hw/virtio/virtio.h"
-
-typedef struct VirtIOBlockDataPlane VirtIOBlockDataPlane;
-
-bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
-                                  VirtIOBlockDataPlane **dataplane,
-                                  Error **errp);
-void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
-void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq);
-
-int virtio_blk_data_plane_start(VirtIODevice *vdev);
-void virtio_blk_data_plane_stop(VirtIODevice *vdev);
-
-void virtio_blk_data_plane_detach(VirtIOBlockDataPlane *s);
-void virtio_blk_data_plane_attach(VirtIOBlockDataPlane *s);
-
-#endif /* HW_DATAPLANE_VIRTIO_BLK_H */
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5e4091e4da..fecffdc303 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -50,8 +50,6 @@ struct VirtIOBlkConf
     bool x_enable_wce_if_config_wce;
 };
 
-struct VirtIOBlockDataPlane;
-
 struct VirtIOBlockReq;
 struct VirtIOBlock {
     VirtIODevice parent_obj;
@@ -64,7 +62,15 @@ struct VirtIOBlock {
     VMChangeStateEntry *change;
     bool dataplane_disabled;
     bool dataplane_started;
-    struct VirtIOBlockDataPlane *dataplane;
+    bool dataplane_starting;
+    bool dataplane_stopping;
+
+    /*
+     * The AioContext for each virtqueue. The BlockDriverState will use the
+     * first element as its AioContext.
+     */
+    AioContext **vq_aio_context;
+
     uint64_t host_features;
     size_t config_size;
     BlockRAMRegistrar blk_ram_registrar;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
deleted file mode 100644
index ba22732497..0000000000
--- a/hw/block/dataplane/virtio-blk.c
+++ /dev/null
@@ -1,404 +0,0 @@
-/*
- * Dedicated thread for virtio-blk I/O processing
- *
- * Copyright 2012 IBM, Corp.
- * Copyright 2012 Red Hat, Inc. and/or its affiliates
- *
- * Authors:
- *   Stefan Hajnoczi <stefanha@redhat.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu/osdep.h"
-#include "qapi/error.h"
-#include "trace.h"
-#include "qemu/iov.h"
-#include "qemu/main-loop.h"
-#include "qemu/thread.h"
-#include "qemu/error-report.h"
-#include "hw/virtio/virtio-blk.h"
-#include "virtio-blk.h"
-#include "block/aio.h"
-#include "hw/virtio/virtio-bus.h"
-#include "qom/object_interfaces.h"
-
-struct VirtIOBlockDataPlane {
-    bool starting;
-    bool stopping;
-
-    VirtIOBlkConf *conf;
-    VirtIODevice *vdev;
-
-    /*
-     * The AioContext for each virtqueue. The BlockDriverState will use the
-     * first element as its AioContext.
-     */
-    AioContext **vq_aio_context;
-};
-
-/* Raise an interrupt to signal guest, if necessary */
-void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq)
-{
-    virtio_notify_irqfd(s->vdev, vq);
-}
-
-/* Generate vq:AioContext mappings from a validated iothread-vq-mapping list */
-static void
-apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
-                 AioContext **vq_aio_context, uint16_t num_queues)
-{
-    IOThreadVirtQueueMappingList *node;
-    size_t num_iothreads = 0;
-    size_t cur_iothread = 0;
-
-    for (node = iothread_vq_mapping_list; node; node = node->next) {
-        num_iothreads++;
-    }
-
-    for (node = iothread_vq_mapping_list; node; node = node->next) {
-        IOThread *iothread = iothread_by_id(node->value->iothread);
-        AioContext *ctx = iothread_get_aio_context(iothread);
-
-        /* Released in virtio_blk_data_plane_destroy() */
-        object_ref(OBJECT(iothread));
-
-        if (node->value->vqs) {
-            uint16List *vq;
-
-            /* Explicit vq:IOThread assignment */
-            for (vq = node->value->vqs; vq; vq = vq->next) {
-                vq_aio_context[vq->value] = ctx;
-            }
-        } else {
-            /* Round-robin vq:IOThread assignment */
-            for (unsigned i = cur_iothread; i < num_queues;
-                 i += num_iothreads) {
-                vq_aio_context[i] = ctx;
-            }
-        }
-
-        cur_iothread++;
-    }
-}
-
-/* Context: BQL held */
-bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
-                                  VirtIOBlockDataPlane **dataplane,
-                                  Error **errp)
-{
-    VirtIOBlockDataPlane *s;
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-
-    *dataplane = NULL;
-
-    if (conf->iothread || conf->iothread_vq_mapping_list) {
-        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
-            error_setg(errp,
-                       "device is incompatible with iothread "
-                       "(transport does not support notifiers)");
-            return false;
-        }
-        if (!virtio_device_ioeventfd_enabled(vdev)) {
-            error_setg(errp, "ioeventfd is required for iothread");
-            return false;
-        }
-
-        /* If dataplane is (re-)enabled while the guest is running there could
-         * be block jobs that can conflict.
-         */
-        if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
-            error_prepend(errp, "cannot start virtio-blk dataplane: ");
-            return false;
-        }
-    }
-    /* Don't try if transport does not support notifiers. */
-    if (!virtio_device_ioeventfd_enabled(vdev)) {
-        return false;
-    }
-
-    s = g_new0(VirtIOBlockDataPlane, 1);
-    s->vdev = vdev;
-    s->conf = conf;
-    s->vq_aio_context = g_new(AioContext *, conf->num_queues);
-
-    if (conf->iothread_vq_mapping_list) {
-        apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
-                         conf->num_queues);
-    } else if (conf->iothread) {
-        AioContext *ctx = iothread_get_aio_context(conf->iothread);
-        for (unsigned i = 0; i < conf->num_queues; i++) {
-            s->vq_aio_context[i] = ctx;
-        }
-
-        /* Released in virtio_blk_data_plane_destroy() */
-        object_ref(OBJECT(conf->iothread));
-    } else {
-        AioContext *ctx = qemu_get_aio_context();
-        for (unsigned i = 0; i < conf->num_queues; i++) {
-            s->vq_aio_context[i] = ctx;
-        }
-    }
-
-    *dataplane = s;
-
-    return true;
-}
-
-/* Context: BQL held */
-void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
-{
-    VirtIOBlock *vblk;
-    VirtIOBlkConf *conf;
-
-    if (!s) {
-        return;
-    }
-
-    vblk = VIRTIO_BLK(s->vdev);
-    assert(!vblk->dataplane_started);
-    conf = s->conf;
-
-    if (conf->iothread_vq_mapping_list) {
-        IOThreadVirtQueueMappingList *node;
-
-        for (node = conf->iothread_vq_mapping_list; node; node = node->next) {
-            IOThread *iothread = iothread_by_id(node->value->iothread);
-            object_unref(OBJECT(iothread));
-        }
-    }
-
-    if (conf->iothread) {
-        object_unref(OBJECT(conf->iothread));
-    }
-
-    g_free(s->vq_aio_context);
-    g_free(s);
-}
-
-/* Context: BQL held */
-int virtio_blk_data_plane_start(VirtIODevice *vdev)
-{
-    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
-    VirtIOBlockDataPlane *s = vblk->dataplane;
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vblk)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    unsigned i;
-    unsigned nvqs = s->conf->num_queues;
-    Error *local_err = NULL;
-    int r;
-
-    if (vblk->dataplane_started || s->starting) {
-        return 0;
-    }
-
-    s->starting = true;
-
-    /* Set up guest notifier (irq) */
-    r = k->set_guest_notifiers(qbus->parent, nvqs, true);
-    if (r != 0) {
-        error_report("virtio-blk failed to set guest notifier (%d), "
-                     "ensure -accel kvm is set.", r);
-        goto fail_guest_notifiers;
-    }
-
-    /*
-     * Batch all the host notifiers in a single transaction to avoid
-     * quadratic time complexity in address_space_update_ioeventfds().
-     */
-    memory_region_transaction_begin();
-
-    /* Set up virtqueue notify */
-    for (i = 0; i < nvqs; i++) {
-        r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
-        if (r != 0) {
-            int j = i;
-
-            fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
-            while (i--) {
-                virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-            }
-
-            /*
-             * The transaction expects the ioeventfds to be open when it
-             * commits. Do it now, before the cleanup loop.
-             */
-            memory_region_transaction_commit();
-
-            while (j--) {
-                virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), j);
-            }
-            goto fail_host_notifiers;
-        }
-    }
-
-    memory_region_transaction_commit();
-
-    trace_virtio_blk_data_plane_start(s);
-
-    r = blk_set_aio_context(s->conf->conf.blk, s->vq_aio_context[0],
-                            &local_err);
-    if (r < 0) {
-        error_report_err(local_err);
-        goto fail_aio_context;
-    }
-
-    /*
-     * These fields must be visible to the IOThread when it processes the
-     * virtqueue, otherwise it will think dataplane has not started yet.
-     *
-     * Make sure ->dataplane_started is false when blk_set_aio_context() is
-     * called above so that draining does not cause the host notifier to be
-     * detached/attached prematurely.
-     */
-    s->starting = false;
-    vblk->dataplane_started = true;
-    smp_wmb(); /* paired with aio_notify_accept() on the read side */
-
-    /* Get this show started by hooking up our callbacks */
-    if (!blk_in_drain(s->conf->conf.blk)) {
-        for (i = 0; i < nvqs; i++) {
-            VirtQueue *vq = virtio_get_queue(s->vdev, i);
-            AioContext *ctx = s->vq_aio_context[i];
-
-            /* Kick right away to begin processing requests already in vring */
-            event_notifier_set(virtio_queue_get_host_notifier(vq));
-
-            virtio_queue_aio_attach_host_notifier(vq, ctx);
-        }
-    }
-    return 0;
-
-  fail_aio_context:
-    memory_region_transaction_begin();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-    }
-
-    memory_region_transaction_commit();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
-    }
-  fail_host_notifiers:
-    k->set_guest_notifiers(qbus->parent, nvqs, false);
-  fail_guest_notifiers:
-    vblk->dataplane_disabled = true;
-    s->starting = false;
-    return -ENOSYS;
-}
-
-/* Stop notifications for new requests from guest.
- *
- * Context: BH in IOThread
- */
-static void virtio_blk_data_plane_stop_vq_bh(void *opaque)
-{
-    VirtQueue *vq = opaque;
-    EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq);
-
-    virtio_queue_aio_detach_host_notifier(vq, qemu_get_current_aio_context());
-
-    /*
-     * Test and clear notifier after disabling event, in case poll callback
-     * didn't have time to run.
-     */
-    virtio_queue_host_notifier_read(host_notifier);
-}
-
-/* Context: BQL held */
-void virtio_blk_data_plane_stop(VirtIODevice *vdev)
-{
-    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
-    VirtIOBlockDataPlane *s = vblk->dataplane;
-    BusState *qbus = qdev_get_parent_bus(DEVICE(vblk));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    unsigned i;
-    unsigned nvqs = s->conf->num_queues;
-
-    if (!vblk->dataplane_started || s->stopping) {
-        return;
-    }
-
-    /* Better luck next time. */
-    if (vblk->dataplane_disabled) {
-        vblk->dataplane_disabled = false;
-        vblk->dataplane_started = false;
-        return;
-    }
-    s->stopping = true;
-    trace_virtio_blk_data_plane_stop(s);
-
-    if (!blk_in_drain(s->conf->conf.blk)) {
-        for (i = 0; i < nvqs; i++) {
-            VirtQueue *vq = virtio_get_queue(s->vdev, i);
-            AioContext *ctx = s->vq_aio_context[i];
-
-            aio_wait_bh_oneshot(ctx, virtio_blk_data_plane_stop_vq_bh, vq);
-        }
-    }
-
-    /*
-     * Batch all the host notifiers in a single transaction to avoid
-     * quadratic time complexity in address_space_update_ioeventfds().
-     */
-    memory_region_transaction_begin();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-    }
-
-    /*
-     * The transaction expects the ioeventfds to be open when it
-     * commits. Do it now, before the cleanup loop.
-     */
-    memory_region_transaction_commit();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
-    }
-
-    /*
-     * Set ->dataplane_started to false before draining so that host notifiers
-     * are not detached/attached anymore.
-     */
-    vblk->dataplane_started = false;
-
-    /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
-    blk_drain(s->conf->conf.blk);
-
-    /*
-     * Try to switch bs back to the QEMU main loop. If other users keep the
-     * BlockBackend in the iothread, that's ok
-     */
-    blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context(), NULL);
-
-    /* Clean up guest notifier (irq) */
-    k->set_guest_notifiers(qbus->parent, nvqs, false);
-
-    s->stopping = false;
-}
-
-void virtio_blk_data_plane_detach(VirtIOBlockDataPlane *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s->vdev);
-
-    for (uint16_t i = 0; i < s->conf->num_queues; i++) {
-        VirtQueue *vq = virtio_get_queue(vdev, i);
-        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
-    }
-}
-
-void virtio_blk_data_plane_attach(VirtIOBlockDataPlane *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s->vdev);
-
-    for (uint16_t i = 0; i < s->conf->num_queues; i++) {
-        VirtQueue *vq = virtio_get_queue(vdev, i);
-        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
-    }
-}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b7a344ca97..510cb4248d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -27,7 +27,6 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 #include "hw/virtio/virtio-blk.h"
-#include "dataplane/virtio-blk.h"
 #include "scsi/constants.h"
 #ifdef __linux__
 # include <scsi/sg.h>
@@ -66,7 +65,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     iov_discard_undo(&req->outhdr_undo);
     virtqueue_push(req->vq, &req->elem, req->in_len);
     if (s->dataplane_started && !s->dataplane_disabled) {
-        virtio_blk_data_plane_notify(s->dataplane, req->vq);
+        virtio_notify_irqfd(vdev, req->vq);
     } else {
         virtio_notify(vdev, req->vq);
     }
@@ -1142,7 +1141,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
-    if (s->dataplane && !s->dataplane_started) {
+    if (!s->dataplane_disabled && !s->dataplane_started) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().
          */
@@ -1546,16 +1545,34 @@ static void virtio_blk_resize(void *opaque)
     aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
 }
 
+static void virtio_blk_data_plane_detach(VirtIOBlock *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
+        VirtQueue *vq = virtio_get_queue(vdev, i);
+        virtio_queue_aio_detach_host_notifier(vq, s->vq_aio_context[i]);
+    }
+}
+
+static void virtio_blk_data_plane_attach(VirtIOBlock *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    for (uint16_t i = 0; i < s->conf.num_queues; i++) {
+        VirtQueue *vq = virtio_get_queue(vdev, i);
+        virtio_queue_aio_attach_host_notifier(vq, s->vq_aio_context[i]);
+    }
+}
+
 /* Suspend virtqueue ioeventfd processing during drain */
 static void virtio_blk_drained_begin(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
-    if (!s->dataplane || !s->dataplane_started) {
-        return;
+    if (s->dataplane_started) {
+        virtio_blk_data_plane_detach(s);
     }
-
-    virtio_blk_data_plane_detach(s->dataplane);
 }
 
 /* Resume virtqueue ioeventfd processing after drain */
@@ -1563,11 +1580,9 @@ static void virtio_blk_drained_end(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
-    if (!s->dataplane || !s->dataplane_started) {
-        return;
+    if (s->dataplane_started) {
+        virtio_blk_data_plane_attach(s);
     }
-
-    virtio_blk_data_plane_attach(s->dataplane);
 }
 
 static const BlockDevOps virtio_block_ops = {
@@ -1576,6 +1591,326 @@ static const BlockDevOps virtio_block_ops = {
     .drained_end   = virtio_blk_drained_end,
 };
 
+/* Generate vq:AioContext mappings from a validated iothread-vq-mapping list */
+static void
+apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
+                 AioContext **vq_aio_context, uint16_t num_queues)
+{
+    IOThreadVirtQueueMappingList *node;
+    size_t num_iothreads = 0;
+    size_t cur_iothread = 0;
+
+    for (node = iothread_vq_mapping_list; node; node = node->next) {
+        num_iothreads++;
+    }
+
+    for (node = iothread_vq_mapping_list; node; node = node->next) {
+        IOThread *iothread = iothread_by_id(node->value->iothread);
+        AioContext *ctx = iothread_get_aio_context(iothread);
+
+        /* Released in virtio_blk_data_plane_destroy() */
+        object_ref(OBJECT(iothread));
+
+        if (node->value->vqs) {
+            uint16List *vq;
+
+            /* Explicit vq:IOThread assignment */
+            for (vq = node->value->vqs; vq; vq = vq->next) {
+                vq_aio_context[vq->value] = ctx;
+            }
+        } else {
+            /* Round-robin vq:IOThread assignment */
+            for (unsigned i = cur_iothread; i < num_queues;
+                 i += num_iothreads) {
+                vq_aio_context[i] = ctx;
+            }
+        }
+
+        cur_iothread++;
+    }
+}
+
+/* Context: BQL held */
+static bool virtio_blk_data_plane_create(VirtIOBlock *s, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    VirtIOBlkConf *conf = &s->conf;
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    if (conf->iothread || conf->iothread_vq_mapping_list) {
+        if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
+            error_setg(errp,
+                       "device is incompatible with iothread "
+                       "(transport does not support notifiers)");
+            return false;
+        }
+        if (!virtio_device_ioeventfd_enabled(vdev)) {
+            error_setg(errp, "ioeventfd is required for iothread");
+            return false;
+        }
+
+        /*
+         * If dataplane is (re-)enabled while the guest is running there could
+         * be block jobs that can conflict.
+         */
+        if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
+            error_prepend(errp, "cannot start virtio-blk dataplane: ");
+            return false;
+        }
+    }
+    /* Don't try if transport does not support notifiers. */
+    if (!virtio_device_ioeventfd_enabled(vdev)) {
+        s->dataplane_disabled = true;
+        return false;
+    }
+
+    s->vq_aio_context = g_new(AioContext *, conf->num_queues);
+
+    if (conf->iothread_vq_mapping_list) {
+        apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
+                         conf->num_queues);
+    } else if (conf->iothread) {
+        AioContext *ctx = iothread_get_aio_context(conf->iothread);
+        for (unsigned i = 0; i < conf->num_queues; i++) {
+            s->vq_aio_context[i] = ctx;
+        }
+
+        /* Released in virtio_blk_data_plane_destroy() */
+        object_ref(OBJECT(conf->iothread));
+    } else {
+        AioContext *ctx = qemu_get_aio_context();
+        for (unsigned i = 0; i < conf->num_queues; i++) {
+            s->vq_aio_context[i] = ctx;
+        }
+    }
+
+    return true;
+}
+
+/* Context: BQL held */
+static void virtio_blk_data_plane_destroy(VirtIOBlock *s)
+{
+    VirtIOBlkConf *conf = &s->conf;
+
+    assert(!s->dataplane_started);
+
+    if (conf->iothread_vq_mapping_list) {
+        IOThreadVirtQueueMappingList *node;
+
+        for (node = conf->iothread_vq_mapping_list; node; node = node->next) {
+            IOThread *iothread = iothread_by_id(node->value->iothread);
+            object_unref(OBJECT(iothread));
+        }
+    }
+
+    if (conf->iothread) {
+        object_unref(OBJECT(conf->iothread));
+    }
+
+    g_free(s->vq_aio_context);
+    s->vq_aio_context = NULL;
+}
+
+/* Context: BQL held */
+static int virtio_blk_data_plane_start(VirtIODevice *vdev)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    unsigned i;
+    unsigned nvqs = s->conf.num_queues;
+    Error *local_err = NULL;
+    int r;
+
+    if (s->dataplane_started || s->dataplane_starting) {
+        return 0;
+    }
+
+    s->dataplane_starting = true;
+
+    /* Set up guest notifier (irq) */
+    r = k->set_guest_notifiers(qbus->parent, nvqs, true);
+    if (r != 0) {
+        error_report("virtio-blk failed to set guest notifier (%d), "
+                     "ensure -accel kvm is set.", r);
+        goto fail_guest_notifiers;
+    }
+
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
+    /* Set up virtqueue notify */
+    for (i = 0; i < nvqs; i++) {
+        r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
+        if (r != 0) {
+            int j = i;
+
+            fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
+            while (i--) {
+                virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+            }
+
+            /*
+             * The transaction expects the ioeventfds to be open when it
+             * commits. Do it now, before the cleanup loop.
+             */
+            memory_region_transaction_commit();
+
+            while (j--) {
+                virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), j);
+            }
+            goto fail_host_notifiers;
+        }
+    }
+
+    memory_region_transaction_commit();
+
+    r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
+                            &local_err);
+    if (r < 0) {
+        error_report_err(local_err);
+        goto fail_aio_context;
+    }
+
+    /*
+     * These fields must be visible to the IOThread when it processes the
+     * virtqueue, otherwise it will think dataplane has not started yet.
+     *
+     * Make sure ->dataplane_started is false when blk_set_aio_context() is
+     * called above so that draining does not cause the host notifier to be
+     * detached/attached prematurely.
+     */
+    s->dataplane_starting = false;
+    s->dataplane_started = true;
+    smp_wmb(); /* paired with aio_notify_accept() on the read side */
+
+    /* Get this show started by hooking up our callbacks */
+    if (!blk_in_drain(s->conf.conf.blk)) {
+        for (i = 0; i < nvqs; i++) {
+            VirtQueue *vq = virtio_get_queue(vdev, i);
+            AioContext *ctx = s->vq_aio_context[i];
+
+            /* Kick right away to begin processing requests already in vring */
+            event_notifier_set(virtio_queue_get_host_notifier(vq));
+
+            virtio_queue_aio_attach_host_notifier(vq, ctx);
+        }
+    }
+    return 0;
+
+  fail_aio_context:
+    memory_region_transaction_begin();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+    }
+
+    memory_region_transaction_commit();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
+    }
+  fail_host_notifiers:
+    k->set_guest_notifiers(qbus->parent, nvqs, false);
+  fail_guest_notifiers:
+    s->dataplane_disabled = true;
+    s->dataplane_starting = false;
+    return -ENOSYS;
+}
+
+/* Stop notifications for new requests from guest.
+ *
+ * Context: BH in IOThread
+ */
+static void virtio_blk_data_plane_stop_vq_bh(void *opaque)
+{
+    VirtQueue *vq = opaque;
+    EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq);
+
+    virtio_queue_aio_detach_host_notifier(vq, qemu_get_current_aio_context());
+
+    /*
+     * Test and clear notifier after disabling event, in case poll callback
+     * didn't have time to run.
+     */
+    virtio_queue_host_notifier_read(host_notifier);
+}
+
+/* Context: BQL held */
+static void virtio_blk_data_plane_stop(VirtIODevice *vdev)
+{
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
+    BusState *qbus = qdev_get_parent_bus(DEVICE(s));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    unsigned i;
+    unsigned nvqs = s->conf.num_queues;
+
+    if (!s->dataplane_started || s->dataplane_stopping) {
+        return;
+    }
+
+    /* Better luck next time. */
+    if (s->dataplane_disabled) {
+        s->dataplane_disabled = false;
+        s->dataplane_started = false;
+        return;
+    }
+    s->dataplane_stopping = true;
+
+    if (!blk_in_drain(s->conf.conf.blk)) {
+        for (i = 0; i < nvqs; i++) {
+            VirtQueue *vq = virtio_get_queue(vdev, i);
+            AioContext *ctx = s->vq_aio_context[i];
+
+            aio_wait_bh_oneshot(ctx, virtio_blk_data_plane_stop_vq_bh, vq);
+        }
+    }
+
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+    }
+
+    /*
+     * The transaction expects the ioeventfds to be open when it
+     * commits. Do it now, before the cleanup loop.
+     */
+    memory_region_transaction_commit();
+
+    for (i = 0; i < nvqs; i++) {
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
+    }
+
+    /*
+     * Set ->dataplane_started to false before draining so that host notifiers
+     * are not detached/attached anymore.
+     */
+    s->dataplane_started = false;
+
+    /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
+    blk_drain(s->conf.conf.blk);
+
+    /*
+     * Try to switch bs back to the QEMU main loop. If other users keep the
+     * BlockBackend in the iothread, that's ok
+     */
+    blk_set_aio_context(s->conf.conf.blk, qemu_get_aio_context(), NULL);
+
+    /* Clean up guest notifier (irq) */
+    k->set_guest_notifiers(qbus->parent, nvqs, false);
+
+    s->dataplane_stopping = false;
+}
+
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1680,7 +2015,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
     }
     qemu_coroutine_inc_pool_size(conf->num_queues * conf->queue_size / 2);
-    virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
+    virtio_blk_data_plane_create(s, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         for (i = 0; i < conf->num_queues; i++) {
@@ -1717,8 +2052,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev)
 
     blk_drain(s->blk);
     del_boot_device_lchs(dev, "/disk@0,0");
-    virtio_blk_data_plane_destroy(s->dataplane);
-    s->dataplane = NULL;
+    virtio_blk_data_plane_destroy(s);
     for (i = 0; i < conf->num_queues; i++) {
         virtio_del_queue(vdev, i);
     }
diff --git a/hw/block/dataplane/meson.build b/hw/block/dataplane/meson.build
index 025b3b061b..11a5eba2f4 100644
--- a/hw/block/dataplane/meson.build
+++ b/hw/block/dataplane/meson.build
@@ -1,2 +1 @@
-system_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
 specific_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c'))
diff --git a/hw/block/dataplane/trace-events b/hw/block/dataplane/trace-events
deleted file mode 100644
index 38fc3e7507..0000000000
--- a/hw/block/dataplane/trace-events
+++ /dev/null
@@ -1,5 +0,0 @@
-# See docs/devel/tracing.rst for syntax documentation.
-
-# virtio-blk.c
-virtio_blk_data_plane_start(void *s) "dataplane %p"
-virtio_blk_data_plane_stop(void *s) "dataplane %p"
-- 
2.43.0

[PATCH 2/6] virtio-blk: rename dataplane create/destroy functions

[Stefan Hajnoczi]

virtio_blk_data_plane_create() and virtio_blk_data_plane_destroy() are
actually about s->vq_aio_context[] rather than managing
dataplane-specific state.

As a prerequisite to using s->vq_aio_context[] in all code paths (even
when dataplane is not used), rename these functions to reflect that they
just manage s->vq_aio_context and call them regardless of whether or not
dataplane is in use.

Note that virtio-blk supports running with -device
virtio-blk-pci,ioevent=off where the vCPU thread enters the device
emulation code. In this mode ioeventfd is not used for virtqueue
processing. However, we still want to initialize s->vq_aio_context[] to
qemu_aio_context in that case since I/O completion callbacks will be
invoked in the main loop thread.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 510cb4248d..47494ebadd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1608,7 +1608,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
         IOThread *iothread = iothread_by_id(node->value->iothread);
         AioContext *ctx = iothread_get_aio_context(iothread);
 
-        /* Released in virtio_blk_data_plane_destroy() */
+        /* Released in virtio_blk_vq_aio_context_cleanup() */
         object_ref(OBJECT(iothread));
 
         if (node->value->vqs) {
@@ -1631,7 +1631,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
 }
 
 /* Context: BQL held */
-static bool virtio_blk_data_plane_create(VirtIOBlock *s, Error **errp)
+static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
     VirtIOBlkConf *conf = &s->conf;
@@ -1659,11 +1659,6 @@ static bool virtio_blk_data_plane_create(VirtIOBlock *s, Error **errp)
             return false;
         }
     }
-    /* Don't try if transport does not support notifiers. */
-    if (!virtio_device_ioeventfd_enabled(vdev)) {
-        s->dataplane_disabled = true;
-        return false;
-    }
 
     s->vq_aio_context = g_new(AioContext *, conf->num_queues);
 
@@ -1676,7 +1671,7 @@ static bool virtio_blk_data_plane_create(VirtIOBlock *s, Error **errp)
             s->vq_aio_context[i] = ctx;
         }
 
-        /* Released in virtio_blk_data_plane_destroy() */
+        /* Released in virtio_blk_vq_aio_context_cleanup() */
         object_ref(OBJECT(conf->iothread));
     } else {
         AioContext *ctx = qemu_get_aio_context();
@@ -1689,7 +1684,7 @@ static bool virtio_blk_data_plane_create(VirtIOBlock *s, Error **errp)
 }
 
 /* Context: BQL held */
-static void virtio_blk_data_plane_destroy(VirtIOBlock *s)
+static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s)
 {
     VirtIOBlkConf *conf = &s->conf;
 
@@ -2015,7 +2010,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output);
     }
     qemu_coroutine_inc_pool_size(conf->num_queues * conf->queue_size / 2);
-    virtio_blk_data_plane_create(s, &err);
+
+    /* Don't start dataplane if transport does not support notifiers. */
+    if (!virtio_device_ioeventfd_enabled(vdev)) {
+        s->dataplane_disabled = true;
+    }
+
+    virtio_blk_vq_aio_context_init(s, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         for (i = 0; i < conf->num_queues; i++) {
@@ -2052,7 +2053,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev)
 
     blk_drain(s->blk);
     del_boot_device_lchs(dev, "/disk@0,0");
-    virtio_blk_data_plane_destroy(s);
+    virtio_blk_vq_aio_context_cleanup(s);
     for (i = 0; i < conf->num_queues; i++) {
         virtio_del_queue(vdev, i);
     }
-- 
2.43.0

[PATCH 3/6] virtio-blk: rename dataplane to ioeventfd

[Stefan Hajnoczi]

The dataplane code is really about using ioeventfd. It's used both for
IOThreads (what we think of as dataplane) and for the core virtio-pci
code's ioeventfd feature (which is enabled by default and used when no
IOThread has been specified). Rename the code to reflect this.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-blk.h |  8 ++--
 hw/block/virtio-blk.c          | 78 +++++++++++++++++-----------------
 2 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fecffdc303..833a9a344f 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -60,10 +60,10 @@ struct VirtIOBlock {
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;
-    bool dataplane_disabled;
-    bool dataplane_started;
-    bool dataplane_starting;
-    bool dataplane_stopping;
+    bool ioeventfd_disabled;
+    bool ioeventfd_started;
+    bool ioeventfd_starting;
+    bool ioeventfd_stopping;
 
     /*
      * The AioContext for each virtqueue. The BlockDriverState will use the
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 47494ebadd..e342cb2cfb 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -64,7 +64,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     iov_discard_undo(&req->inhdr_undo);
     iov_discard_undo(&req->outhdr_undo);
     virtqueue_push(req->vq, &req->elem, req->in_len);
-    if (s->dataplane_started && !s->dataplane_disabled) {
+    if (s->ioeventfd_started && !s->ioeventfd_disabled) {
         virtio_notify_irqfd(vdev, req->vq);
     } else {
         virtio_notify(vdev, req->vq);
@@ -1141,12 +1141,12 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
-    if (!s->dataplane_disabled && !s->dataplane_started) {
+    if (!s->ioeventfd_disabled && !s->ioeventfd_started) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
-         * dataplane here instead of waiting for .set_status().
+         * ioeventfd here instead of waiting for .set_status().
          */
         virtio_device_start_ioeventfd(vdev);
-        if (!s->dataplane_disabled) {
+        if (!s->ioeventfd_disabled) {
             return;
         }
     }
@@ -1213,7 +1213,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     VirtIOBlockReq *req;
 
     /* Dataplane has stopped... */
-    assert(!s->dataplane_started);
+    assert(!s->ioeventfd_started);
 
     /* ...but requests may still be in flight. */
     blk_drain(s->blk);
@@ -1380,7 +1380,7 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
-        assert(!s->dataplane_started);
+        assert(!s->ioeventfd_started);
     }
 
     if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
@@ -1545,7 +1545,7 @@ static void virtio_blk_resize(void *opaque)
     aio_bh_schedule_oneshot(qemu_get_aio_context(), virtio_resize_cb, vdev);
 }
 
-static void virtio_blk_data_plane_detach(VirtIOBlock *s)
+static void virtio_blk_ioeventfd_detach(VirtIOBlock *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
@@ -1555,7 +1555,7 @@ static void virtio_blk_data_plane_detach(VirtIOBlock *s)
     }
 }
 
-static void virtio_blk_data_plane_attach(VirtIOBlock *s)
+static void virtio_blk_ioeventfd_attach(VirtIOBlock *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
@@ -1570,8 +1570,8 @@ static void virtio_blk_drained_begin(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
-    if (s->dataplane_started) {
-        virtio_blk_data_plane_detach(s);
+    if (s->ioeventfd_started) {
+        virtio_blk_ioeventfd_detach(s);
     }
 }
 
@@ -1580,8 +1580,8 @@ static void virtio_blk_drained_end(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
-    if (s->dataplane_started) {
-        virtio_blk_data_plane_attach(s);
+    if (s->ioeventfd_started) {
+        virtio_blk_ioeventfd_attach(s);
     }
 }
 
@@ -1651,11 +1651,11 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
         }
 
         /*
-         * If dataplane is (re-)enabled while the guest is running there could
+         * If ioeventfd is (re-)enabled while the guest is running there could
          * be block jobs that can conflict.
          */
         if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
-            error_prepend(errp, "cannot start virtio-blk dataplane: ");
+            error_prepend(errp, "cannot start virtio-blk ioeventfd: ");
             return false;
         }
     }
@@ -1688,7 +1688,7 @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s)
 {
     VirtIOBlkConf *conf = &s->conf;
 
-    assert(!s->dataplane_started);
+    assert(!s->ioeventfd_started);
 
     if (conf->iothread_vq_mapping_list) {
         IOThreadVirtQueueMappingList *node;
@@ -1708,7 +1708,7 @@ static void virtio_blk_vq_aio_context_cleanup(VirtIOBlock *s)
 }
 
 /* Context: BQL held */
-static int virtio_blk_data_plane_start(VirtIODevice *vdev)
+static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
@@ -1718,11 +1718,11 @@ static int virtio_blk_data_plane_start(VirtIODevice *vdev)
     Error *local_err = NULL;
     int r;
 
-    if (s->dataplane_started || s->dataplane_starting) {
+    if (s->ioeventfd_started || s->ioeventfd_starting) {
         return 0;
     }
 
-    s->dataplane_starting = true;
+    s->ioeventfd_starting = true;
 
     /* Set up guest notifier (irq) */
     r = k->set_guest_notifiers(qbus->parent, nvqs, true);
@@ -1773,14 +1773,14 @@ static int virtio_blk_data_plane_start(VirtIODevice *vdev)
 
     /*
      * These fields must be visible to the IOThread when it processes the
-     * virtqueue, otherwise it will think dataplane has not started yet.
+     * virtqueue, otherwise it will think ioeventfd has not started yet.
      *
-     * Make sure ->dataplane_started is false when blk_set_aio_context() is
+     * Make sure ->ioeventfd_started is false when blk_set_aio_context() is
      * called above so that draining does not cause the host notifier to be
      * detached/attached prematurely.
      */
-    s->dataplane_starting = false;
-    s->dataplane_started = true;
+    s->ioeventfd_starting = false;
+    s->ioeventfd_started = true;
     smp_wmb(); /* paired with aio_notify_accept() on the read side */
 
     /* Get this show started by hooking up our callbacks */
@@ -1812,8 +1812,8 @@ static int virtio_blk_data_plane_start(VirtIODevice *vdev)
   fail_host_notifiers:
     k->set_guest_notifiers(qbus->parent, nvqs, false);
   fail_guest_notifiers:
-    s->dataplane_disabled = true;
-    s->dataplane_starting = false;
+    s->ioeventfd_disabled = true;
+    s->ioeventfd_starting = false;
     return -ENOSYS;
 }
 
@@ -1821,7 +1821,7 @@ static int virtio_blk_data_plane_start(VirtIODevice *vdev)
  *
  * Context: BH in IOThread
  */
-static void virtio_blk_data_plane_stop_vq_bh(void *opaque)
+static void virtio_blk_ioeventfd_stop_vq_bh(void *opaque)
 {
     VirtQueue *vq = opaque;
     EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq);
@@ -1836,7 +1836,7 @@ static void virtio_blk_data_plane_stop_vq_bh(void *opaque)
 }
 
 /* Context: BQL held */
-static void virtio_blk_data_plane_stop(VirtIODevice *vdev)
+static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     BusState *qbus = qdev_get_parent_bus(DEVICE(s));
@@ -1844,24 +1844,24 @@ static void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     unsigned i;
     unsigned nvqs = s->conf.num_queues;
 
-    if (!s->dataplane_started || s->dataplane_stopping) {
+    if (!s->ioeventfd_started || s->ioeventfd_stopping) {
         return;
     }
 
     /* Better luck next time. */
-    if (s->dataplane_disabled) {
-        s->dataplane_disabled = false;
-        s->dataplane_started = false;
+    if (s->ioeventfd_disabled) {
+        s->ioeventfd_disabled = false;
+        s->ioeventfd_started = false;
         return;
     }
-    s->dataplane_stopping = true;
+    s->ioeventfd_stopping = true;
 
     if (!blk_in_drain(s->conf.conf.blk)) {
         for (i = 0; i < nvqs; i++) {
             VirtQueue *vq = virtio_get_queue(vdev, i);
             AioContext *ctx = s->vq_aio_context[i];
 
-            aio_wait_bh_oneshot(ctx, virtio_blk_data_plane_stop_vq_bh, vq);
+            aio_wait_bh_oneshot(ctx, virtio_blk_ioeventfd_stop_vq_bh, vq);
         }
     }
 
@@ -1886,10 +1886,10 @@ static void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     }
 
     /*
-     * Set ->dataplane_started to false before draining so that host notifiers
+     * Set ->ioeventfd_started to false before draining so that host notifiers
      * are not detached/attached anymore.
      */
-    s->dataplane_started = false;
+    s->ioeventfd_started = false;
 
     /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
     blk_drain(s->conf.conf.blk);
@@ -1903,7 +1903,7 @@ static void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, nvqs, false);
 
-    s->dataplane_stopping = false;
+    s->ioeventfd_stopping = false;
 }
 
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
@@ -2011,9 +2011,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     }
     qemu_coroutine_inc_pool_size(conf->num_queues * conf->queue_size / 2);
 
-    /* Don't start dataplane if transport does not support notifiers. */
+    /* Don't start ioeventfd if transport does not support notifiers. */
     if (!virtio_device_ioeventfd_enabled(vdev)) {
-        s->dataplane_disabled = true;
+        s->ioeventfd_disabled = true;
     }
 
     virtio_blk_vq_aio_context_init(s, &err);
@@ -2137,8 +2137,8 @@ static void virtio_blk_class_init(ObjectClass *klass, void *data)
     vdc->reset = virtio_blk_reset;
     vdc->save = virtio_blk_save_device;
     vdc->load = virtio_blk_load_device;
-    vdc->start_ioeventfd = virtio_blk_data_plane_start;
-    vdc->stop_ioeventfd = virtio_blk_data_plane_stop;
+    vdc->start_ioeventfd = virtio_blk_start_ioeventfd;
+    vdc->stop_ioeventfd = virtio_blk_stop_ioeventfd;
 }
 
 static const TypeInfo virtio_blk_info = {
-- 
2.43.0

[PATCH 4/6] virtio-blk: restart s->rq reqs in vq AioContexts

[Stefan Hajnoczi]

A virtio-blk device with the iothread-vq-mapping parameter has
per-virtqueue AioContexts. It is not thread-safe to process s->rq
requests in the BlockBackend AioContext since that may be different from
the virtqueue's AioContext to which this request belongs. The code
currently races and could crash.

Adapt virtio_blk_dma_restart_cb() to first split s->rq into per-vq lists
and then schedule a BH each vq's AioContext as necessary. This way
requests are safely processed in their vq's AioContext.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 44 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e342cb2cfb..4525988d92 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1156,16 +1156,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlockReq *req = opaque;
+    VirtIOBlock *s = req->dev; /* we're called with at least one request */
 
-    VirtIOBlockReq *req;
     MultiReqBuffer mrb = {};
 
-    WITH_QEMU_LOCK_GUARD(&s->rq_lock) {
-        req = s->rq;
-        s->rq = NULL;
-    }
-
     while (req) {
         VirtIOBlockReq *next = req->next;
         if (virtio_blk_handle_request(req, &mrb)) {
@@ -1195,16 +1190,43 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
                                       RunState state)
 {
     VirtIOBlock *s = opaque;
+    uint16_t num_queues = s->conf.num_queues;
 
     if (!running) {
         return;
     }
 
-    /* Paired with dec in virtio_blk_dma_restart_bh() */
-    blk_inc_in_flight(s->conf.conf.blk);
+    /* Split the device-wide s->rq request list into per-vq request lists */
+    g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
+    VirtIOBlockReq *rq;
 
-    aio_bh_schedule_oneshot(blk_get_aio_context(s->conf.conf.blk),
-            virtio_blk_dma_restart_bh, s);
+    WITH_QEMU_LOCK_GUARD(&s->rq_lock) {
+        rq = s->rq;
+        s->rq = NULL;
+    }
+
+    while (rq) {
+        VirtIOBlockReq *next = rq->next;
+        uint16_t idx = virtio_get_queue_index(rq->vq);
+
+        rq->next = vq_rq[idx];
+        vq_rq[idx] = rq;
+        rq = next;
+    }
+
+    /* Schedule a BH to submit the requests in each vq's AioContext */
+    for (uint16_t i = 0; i < num_queues; i++) {
+        if (!vq_rq[i]) {
+            continue;
+        }
+
+        /* Paired with dec in virtio_blk_dma_restart_bh() */
+        blk_inc_in_flight(s->conf.conf.blk);
+
+        aio_bh_schedule_oneshot(s->vq_aio_context[i],
+                                virtio_blk_dma_restart_bh,
+                                vq_rq[i]);
+    }
 }
 
 static void virtio_blk_reset(VirtIODevice *vdev)
-- 
2.43.0

[PATCH 5/6] virtio-blk: tolerate failure to set BlockBackend AioContext

[Stefan Hajnoczi]

We no longer rely on setting the AioContext since the block layer
IO_CODE APIs can be called from any thread. Now it's just a hint to help
block jobs and other operations co-locate themselves in a thread with
the guest I/O requests. Keep going if setting the AioContext fails.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4525988d92..73248d15c8 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1786,11 +1786,14 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
+    /*
+     * Try to change the AioContext so that block jobs and other operations can
+     * co-locate their activity in the same AioContext. If it fails, nevermind.
+     */
     r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
                             &local_err);
     if (r < 0) {
-        error_report_err(local_err);
-        goto fail_aio_context;
+        warn_report_err(local_err);
     }
 
     /*
@@ -1819,18 +1822,6 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
     }
     return 0;
 
-  fail_aio_context:
-    memory_region_transaction_begin();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
-    }
-
-    memory_region_transaction_commit();
-
-    for (i = 0; i < nvqs; i++) {
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
-    }
   fail_host_notifiers:
     k->set_guest_notifiers(qbus->parent, nvqs, false);
   fail_guest_notifiers:
-- 
2.43.0

[PATCH 6/6] virtio-blk: always set ioeventfd during startup

[Stefan Hajnoczi]

When starting ioeventfd it is common practice to set the event notifier
so that the ioeventfd handler is triggered to run immediately. There may
be no requests waiting to be processed, but the idea is that if a
request snuck in then we guarantee that it will be detected.

One scenario where self-triggering the ioeventfd is necessary is when
virtio_blk_handle_output() is called from a vCPU thread before the
VIRTIO Device Status transitions to DRIVER_OK. In that case we need to
self-trigger the ioeventfd so that the kick handled by the vCPU thread
causes the vq AioContext thread to take over handling the request(s).

Fixes: b6948ab01df0 ("virtio-blk: add iothread-vq-mapping parameter")
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 73248d15c8..227d83569f 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1809,14 +1809,14 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
     smp_wmb(); /* paired with aio_notify_accept() on the read side */
 
     /* Get this show started by hooking up our callbacks */
-    if (!blk_in_drain(s->conf.conf.blk)) {
-        for (i = 0; i < nvqs; i++) {
-            VirtQueue *vq = virtio_get_queue(vdev, i);
-            AioContext *ctx = s->vq_aio_context[i];
+    for (i = 0; i < nvqs; i++) {
+        VirtQueue *vq = virtio_get_queue(vdev, i);
+        AioContext *ctx = s->vq_aio_context[i];
 
-            /* Kick right away to begin processing requests already in vring */
-            event_notifier_set(virtio_queue_get_host_notifier(vq));
+        /* Kick right away to begin processing requests already in vring */
+        event_notifier_set(virtio_queue_get_host_notifier(vq));
 
+        if (!blk_in_drain(s->conf.conf.blk)) {
             virtio_queue_aio_attach_host_notifier(vq, ctx);
         }
     }
-- 
2.43.0

Re: [PATCH 0/6] virtio-blk: iothread-vq-mapping cleanups

[Kevin Wolf]

Am 19.01.2024 um 14:57 hat Stefan Hajnoczi geschrieben:
> Kevin Wolf identified some issues with the virtio-blk iothread-vq-mapping patch
> series that was merged at the end of 2023:
> 1. s->rq is restarted from one AioContext and races with the other
>    iothread-vq-mapping AioContexts.
> 2. Failure to set the AioContext is no longer fatal since the IO_CODE APIs can
>    be called from any thread. We can relax the error handling behavior.
> 3. Starting dataplane must self-trigger the ioeventfd even in a drained
>    section. Failure to do so could lead to an unresponsive virtio-blk device.
> 
> This patch series addresses these issues. The first few patches merge the
> hw/block/dataplane/virtio-blk.c code into hw/block/virtio-blk.c so that s->rq
> can easily be restarted in the correct AioContexts.

Thanks, applied to the block branch.

Kevin