[PATCH 0/8] virtio-blk: remove AioContext lock

[PATCH 0/8] virtio-blk: remove AioContext lock

[Stefan Hajnoczi]

This is a continuation of Emanuele Esposito's work to remove the AioContext
lock in virtio-blk. In the past it was necessary to acquire the AioContext lock
in order to do I/O. Paolo Bonzini and Emanuele have removed the need for the
AioContext in the core block layer code, with a few exceptions like blk_drain()
and blk_set_aio_context().

This patch series annotates virtio-blk functions with
IO_CODE()/GLOBAL_STATE_CODE() so it's clear in which context they are called.
It also removes unnecessary AioContext locks.

The end result is that virtio-blk rarely takes the AioContext lock. Future
patch series will add support for multiple IOThreads so that true multi-queue
can be achieved, but right now virtio-blk still has unprotected shared state
like s->rq so that needs to wait for another day.

Based-on: <20221102182337.252202-1-stefanha@redhat.com>

Emanuele Giuseppe Esposito (6):
  virtio_queue_aio_attach_host_notifier: remove AioContext lock
  block-backend: enable_write_cache should be atomic
  virtio: categorize callbacks in GS
  virtio-blk: mark GLOBAL_STATE_CODE functions
  virtio-blk: mark IO_CODE functions
  virtio-blk: remove unnecessary AioContext lock from function already
    safe

Stefan Hajnoczi (2):
  virtio-blk: don't acquire AioContext in virtio_blk_handle_vq()
  virtio-blk: minimize virtio_blk_reset() AioContext lock region

 include/block/aio-wait.h        |  4 +-
 block/block-backend.c           |  6 +--
 hw/block/dataplane/virtio-blk.c | 18 +++++--
 hw/block/virtio-blk.c           | 92 ++++++++++++++++++++++++---------
 hw/scsi/virtio-scsi-dataplane.c | 10 ++--
 hw/virtio/virtio-bus.c          |  5 ++
 hw/virtio/virtio-pci.c          |  2 +
 hw/virtio/virtio.c              |  8 +++
 util/aio-wait.c                 |  2 +-
 9 files changed, 106 insertions(+), 41 deletions(-)

-- 
2.38.1

[PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

virtio_queue_aio_attach_host_notifier() and
virtio_queue_aio_attach_host_notifier_nopoll() run always in the
main loop, so there is no need to protect them with AioContext
lock.

On the other side, virtio_queue_aio_detach_host_notifier() runs
in a bh in the iothread context, but it is always scheduled
(thus serialized) by the main loop. Therefore removing the
AioContext lock is safe.

In order to remove the AioContext lock it is necessary to switch
aio_wait_bh_oneshot() to AIO_WAIT_WHILE_UNLOCKED(). virtio-blk and
virtio-scsi are the only users of aio_wait_bh_oneshot() so it is
possible to make this change.

For now bdrv_set_aio_context() still needs the AioContext lock.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-2-eesposit@redhat.com>
---
 include/block/aio-wait.h        |  4 ++--
 hw/block/dataplane/virtio-blk.c | 10 ++++++----
 hw/block/virtio-blk.c           |  2 ++
 hw/scsi/virtio-scsi-dataplane.c | 10 ++++------
 util/aio-wait.c                 |  2 +-
 5 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index dd9a7f6461..fce6bfee3a 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -131,8 +131,8 @@ void aio_wait_kick(void);
  *
  * Run a BH in @ctx and wait for it to complete.
  *
- * Must be called from the main loop thread with @ctx acquired exactly once.
- * Note that main loop event processing may occur.
+ * Must be called from the main loop thread. @ctx must not be acquired by the
+ * caller. Note that main loop event processing may occur.
  */
 void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque);
 
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..975f5ca8c4 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -167,6 +167,8 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     Error *local_err = NULL;
     int r;
 
+    GLOBAL_STATE_CODE();
+
     if (vblk->dataplane_started || s->starting) {
         return 0;
     }
@@ -245,13 +247,11 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     }
 
     /* Get this show started by hooking up our callbacks */
-    aio_context_acquire(s->ctx);
     for (i = 0; i < nvqs; i++) {
         VirtQueue *vq = virtio_get_queue(s->vdev, i);
 
         virtio_queue_aio_attach_host_notifier(vq, s->ctx);
     }
-    aio_context_release(s->ctx);
     return 0;
 
   fail_aio_context:
@@ -301,6 +301,8 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
 
+    GLOBAL_STATE_CODE();
+
     if (!vblk->dataplane_started || s->stopping) {
         return;
     }
@@ -314,9 +316,10 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     s->stopping = true;
     trace_virtio_blk_data_plane_stop(s);
 
-    aio_context_acquire(s->ctx);
     aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
 
+    aio_context_acquire(s->ctx);
+
     /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
     blk_drain(s->conf->conf.blk);
 
@@ -325,7 +328,6 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
      * BlockBackend in the iothread, that's ok
      */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context(), NULL);
-
     aio_context_release(s->ctx);
 
     /*
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 1762517878..cdc6fd5979 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -100,6 +100,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     VirtIOBlock *s = next->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (next) {
         VirtIOBlockReq *req = next;
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 20bb91766e..f6f55d4511 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -91,6 +91,8 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    GLOBAL_STATE_CODE();
+
     if (s->dataplane_started ||
         s->dataplane_starting ||
         s->dataplane_fenced) {
@@ -138,20 +140,18 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     /*
      * These fields are visible to the IOThread so we rely on implicit barriers
-     * in aio_context_acquire() on the write side and aio_notify_accept() on
-     * the read side.
+     * in virtio_queue_aio_attach_host_notifier() on the write side and
+     * aio_notify_accept() on the read side.
      */
     s->dataplane_starting = false;
     s->dataplane_started = true;
 
-    aio_context_acquire(s->ctx);
     virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
     virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
 
     for (i = 0; i < vs->conf.num_queues; i++) {
         virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx);
     }
-    aio_context_release(s->ctx);
     return 0;
 
 fail_host_notifiers:
@@ -197,9 +197,7 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
     }
     s->dataplane_stopping = true;
 
-    aio_context_acquire(s->ctx);
     aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
-    aio_context_release(s->ctx);
 
     blk_drain_all(); /* ensure there are no in-flight requests */
 
diff --git a/util/aio-wait.c b/util/aio-wait.c
index 98c5accd29..80f26ee520 100644
--- a/util/aio-wait.c
+++ b/util/aio-wait.c
@@ -82,5 +82,5 @@ void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
     assert(qemu_get_current_aio_context() == qemu_get_aio_context());
 
     aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data);
-    AIO_WAIT_WHILE(ctx, !data.done);
+    AIO_WAIT_WHILE_UNLOCKED(ctx, !data.done);
 }
-- 
2.38.1

Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> virtio_queue_aio_attach_host_notifier() and
> virtio_queue_aio_attach_host_notifier_nopoll() run always in the
> main loop, so there is no need to protect them with AioContext
> lock.
>
> On the other side, virtio_queue_aio_detach_host_notifier() runs
> in a bh in the iothread context, but it is always scheduled
> (thus serialized) by the main loop. Therefore removing the
> AioContext lock is safe.
>
> In order to remove the AioContext lock it is necessary to switch
> aio_wait_bh_oneshot() to AIO_WAIT_WHILE_UNLOCKED(). virtio-blk and
> virtio-scsi are the only users of aio_wait_bh_oneshot() so it is
> possible to make this change.
>
> For now bdrv_set_aio_context() still needs the AioContext lock.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-2-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 2/8] block-backend: enable_write_cache should be atomic

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

It is read from IO_CODE and written with BQL held,
so setting it as atomic should be enough.

Also remove the aiocontext lock that was sporadically
taken around the set.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-3-eesposit@redhat.com>
---
 block/block-backend.c | 6 +++---
 hw/block/virtio-blk.c | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index c0c7d56c8d..949418cad4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -60,7 +60,7 @@ struct BlockBackend {
      * can be used to restore those options in the new BDS on insert) */
     BlockBackendRootState root_state;
 
-    bool enable_write_cache;
+    bool enable_write_cache; /* Atomic */
 
     /* I/O stats (display with "info blockstats"). */
     BlockAcctStats stats;
@@ -1939,13 +1939,13 @@ bool blk_is_sg(BlockBackend *blk)
 bool blk_enable_write_cache(BlockBackend *blk)
 {
     IO_CODE();
-    return blk->enable_write_cache;
+    return qatomic_read(&blk->enable_write_cache);
 }
 
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
 {
     IO_CODE();
-    blk->enable_write_cache = wce;
+    qatomic_set(&blk->enable_write_cache, wce);
 }
 
 void blk_activate(BlockBackend *blk, Error **errp)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cdc6fd5979..96d00103a4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -961,9 +961,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 
     memcpy(&blkcfg, config, s->config_size);
 
-    aio_context_acquire(blk_get_aio_context(s->blk));
     blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
-    aio_context_release(blk_get_aio_context(s->blk));
 }
 
 static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
@@ -1031,11 +1029,9 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
      * s->blk would erroneously be placed in writethrough mode.
      */
     if (!virtio_vdev_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) {
-        aio_context_acquire(blk_get_aio_context(s->blk));
         blk_set_enable_write_cache(s->blk,
                                    virtio_vdev_has_feature(vdev,
                                                            VIRTIO_BLK_F_WCE));
-        aio_context_release(blk_get_aio_context(s->blk));
     }
 }
 
-- 
2.38.1

Re: [PATCH 2/8] block-backend: enable_write_cache should be atomic

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> It is read from IO_CODE and written with BQL held,
> so setting it as atomic should be enough.
>
> Also remove the aiocontext lock that was sporadically
> taken around the set.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-3-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 3/8] virtio: categorize callbacks in GS

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

All the callbacks below are always running in the main loop.

The callbacks are the following:
- start/stop_ioeventfd: these are the callbacks where
  blk_set_aio_context(iothread) is done, so they are called in the main
  loop.

- save and load: called during migration, when VM is stopped from the
  main loop.

- reset: before calling this callback, stop_ioeventfd is invoked, so
  it can only run in the main loop.

- set_status: going through all the callers we can see it is called
  from a MemoryRegionOps callback, which always run in a vcpu thread and
  hold the BQL.

- realize: iothread is not even created yet.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-5-eesposit@redhat.com>
---
 hw/block/virtio-blk.c  | 2 ++
 hw/virtio/virtio-bus.c | 5 +++++
 hw/virtio/virtio-pci.c | 2 ++
 hw/virtio/virtio.c     | 8 ++++++++
 4 files changed, 17 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 96d00103a4..96bc11d2fe 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1005,6 +1005,8 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+    GLOBAL_STATE_CODE();
+
     if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
         assert(!s->dataplane_started);
     }
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 896feb37a1..74cdf4bd27 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
@@ -224,6 +225,8 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
     int r;
 
+    GLOBAL_STATE_CODE();
+
     if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) {
         return -ENOSYS;
     }
@@ -248,6 +251,8 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
     VirtIODevice *vdev;
     VirtioDeviceClass *vdc;
 
+    GLOBAL_STATE_CODE();
+
     if (!bus->ioeventfd_started) {
         return;
     }
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index a1c9dfa7bb..4f9a94f61b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -313,6 +313,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     uint16_t vector;
     hwaddr pa;
 
+    GLOBAL_STATE_CODE();
+
     switch (addr) {
     case VIRTIO_PCI_GUEST_FEATURES:
         /* Guest does not negotiate properly?  We have to assume nothing. */
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9683b2e158..468e8f5ad0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2422,6 +2422,8 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     trace_virtio_set_status(vdev, val);
 
+    GLOBAL_STATE_CODE();
+
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) &&
             val & VIRTIO_CONFIG_S_FEATURES_OK) {
@@ -2515,6 +2517,8 @@ void virtio_reset(void *opaque)
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     int i;
 
+    GLOBAL_STATE_CODE();
+
     virtio_set_status(vdev, 0);
     if (current_cpu) {
         /* Guest initiated reset */
@@ -3357,6 +3361,8 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
     uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff);
     int i;
 
+    GLOBAL_STATE_CODE();
+
     if (k->save_config) {
         k->save_config(qbus->parent, f);
     }
@@ -3508,6 +3514,8 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
 
+    GLOBAL_STATE_CODE();
+
     /*
      * We poison the endianness to ensure it does not get used before
      * subsections have been loaded.
-- 
2.38.1

Re: [PATCH 3/8] virtio: categorize callbacks in GS

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> All the callbacks below are always running in the main loop.
>
> The callbacks are the following:
> - start/stop_ioeventfd: these are the callbacks where
>   blk_set_aio_context(iothread) is done, so they are called in the main
>   loop.
>
> - save and load: called during migration, when VM is stopped from the
>   main loop.
>
> - reset: before calling this callback, stop_ioeventfd is invoked, so
>   it can only run in the main loop.
>
> - set_status: going through all the callers we can see it is called
>   from a MemoryRegionOps callback, which always run in a vcpu thread and
>   hold the BQL.
>
> - realize: iothread is not even created yet.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-5-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 4/8] virtio-blk: mark GLOBAL_STATE_CODE functions

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Just as done in the block API, mark functions in virtio-blk
that are always called in the main loop with BQL held.

We know such functions are GS because they all are callbacks
from virtio.c API that has already classified them as GS.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-6-eesposit@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  4 ++++
 hw/block/virtio-blk.c           | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 975f5ca8c4..728c9cd86c 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -89,6 +89,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
+    GLOBAL_STATE_CODE();
+
     *dataplane = NULL;
 
     if (conf->iothread) {
@@ -140,6 +142,8 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
 {
     VirtIOBlock *vblk;
 
+    GLOBAL_STATE_CODE();
+
     if (!s) {
         return;
     }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 96bc11d2fe..02b213a140 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -845,11 +845,17 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
+/*
+ * Only called when VM is started or stopped in cpus.c.
+ * No iothread runs in parallel
+ */
 static void virtio_blk_dma_restart_cb(void *opaque, bool running,
                                       RunState state)
 {
     VirtIOBlock *s = opaque;
 
+    GLOBAL_STATE_CODE();
+
     if (!running) {
         return;
     }
@@ -867,8 +873,14 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     AioContext *ctx;
     VirtIOBlockReq *req;
 
+    GLOBAL_STATE_CODE();
+
     ctx = blk_get_aio_context(s->blk);
     aio_context_acquire(ctx);
+    /*
+     * This drain together with ->stop_ioeventfd() in virtio_pci_reset()
+     * stops all Iothreads.
+     */
     blk_drain(s->blk);
 
     /* We drop queued requests after blk_drain() because blk_drain() itself can
@@ -1037,11 +1049,17 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     }
 }
 
+/*
+ * VM is stopped while doing migration, so iothread has
+ * no requests to process.
+ */
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     VirtIOBlockReq *req = s->rq;
 
+    GLOBAL_STATE_CODE();
+
     while (req) {
         qemu_put_sbyte(f, 1);
 
@@ -1055,11 +1073,17 @@ static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_sbyte(f, 0);
 }
 
+/*
+ * VM is stopped while doing migration, so iothread has
+ * no requests to process.
+ */
 static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
                                   int version_id)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+    GLOBAL_STATE_CODE();
+
     while (qemu_get_sbyte(f)) {
         unsigned nvqs = s->conf.num_queues;
         unsigned vq_idx = 0;
@@ -1108,6 +1132,7 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
+/* Iothread is not yet created */
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1116,6 +1141,8 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     Error *err = NULL;
     unsigned i;
 
+    GLOBAL_STATE_CODE();
+
     if (!conf->conf.blk) {
         error_setg(errp, "drive property not set");
         return;
-- 
2.38.1

Re: [PATCH 4/8] virtio-blk: mark GLOBAL_STATE_CODE functions

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> Just as done in the block API, mark functions in virtio-blk
> that are always called in the main loop with BQL held.
>
> We know such functions are GS because they all are callbacks
> from virtio.c API that has already classified them as GS.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-6-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 5/8] virtio-blk: mark IO_CODE functions

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Just as done in the block API, mark functions in virtio-blk
that are called also from iothread(s).

We know such functions are IO because many are blk_* callbacks,
running always in the device iothread, and remaining are propagated
from the leaf IO functions (if a function calls a IO_CODE function,
itself is categorized as IO_CODE too).

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-7-eesposit@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  4 +++
 hw/block/virtio-blk.c           | 45 ++++++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 728c9cd86c..3593ac0e7b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -63,6 +63,8 @@ static void notify_guest_bh(void *opaque)
     unsigned long bitmap[BITS_TO_LONGS(nvqs)];
     unsigned j;
 
+    IO_CODE();
+
     memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
     memset(s->batch_notify_vqs, 0, sizeof(bitmap));
 
@@ -288,6 +290,8 @@ static void virtio_blk_data_plane_stop_bh(void *opaque)
     VirtIOBlockDataPlane *s = opaque;
     unsigned i;
 
+    IO_CODE();
+
     for (i = 0; i < s->conf->num_queues; i++) {
         VirtQueue *vq = virtio_get_queue(s->vdev, i);
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 02b213a140..f8fcf25292 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -39,6 +39,8 @@
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
+    IO_CODE();
+
     req->dev = s;
     req->vq = vq;
     req->qiov.size = 0;
@@ -57,6 +59,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     trace_virtio_blk_req_complete(vdev, req, status);
 
     stb_p(&req->in->status, status);
@@ -76,6 +80,8 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     VirtIOBlock *s = req->dev;
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
 
+    IO_CODE();
+
     if (action == BLOCK_ERROR_ACTION_STOP) {
         /* Break the link as the next request is going to be parsed from the
          * ring again. Otherwise we may end up doing a double completion! */
@@ -143,7 +149,9 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
     VirtIOBlockReq *req = opaque;
     VirtIOBlock *s = req->dev;
 
+    IO_CODE();
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, 0, true)) {
             goto out;
@@ -165,7 +173,9 @@ static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
     bool is_write_zeroes = (virtio_ldl_p(VIRTIO_DEVICE(s), &req->out.type) &
                             ~VIRTIO_BLK_T_BARRIER) == VIRTIO_BLK_T_WRITE_ZEROES;
 
+    IO_CODE();
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, false, is_write_zeroes)) {
             goto out;
@@ -198,6 +208,8 @@ static void virtio_blk_ioctl_complete(void *opaque, int status)
     struct virtio_scsi_inhdr *scsi;
     struct sg_io_hdr *hdr;
 
+    IO_CODE();
+
     scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
 
     if (status) {
@@ -239,6 +251,8 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s, VirtQueue *vq)
 {
     VirtIOBlockReq *req = virtqueue_pop(vq, sizeof(VirtIOBlockReq));
 
+    IO_CODE();
+
     if (req) {
         virtio_blk_init_request(s, vq, req);
     }
@@ -259,6 +273,8 @@ static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
     BlockAIOCB *acb;
 #endif
 
+    IO_CODE();
+
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
      * and the SCSI command block.
@@ -357,6 +373,7 @@ fail:
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
+    IO_CODE();
 
     status = virtio_blk_handle_scsi_req(req);
     if (status != -EINPROGRESS) {
@@ -374,6 +391,8 @@ static inline void submit_requests(VirtIOBlock *s, MultiReqBuffer *mrb,
     bool is_write = mrb->is_write;
     BdrvRequestFlags flags = 0;
 
+    IO_CODE();
+
     if (num_reqs > 1) {
         int i;
         struct iovec *tmp_iov = qiov->iov;
@@ -423,6 +442,8 @@ static int multireq_compare(const void *a, const void *b)
     const VirtIOBlockReq *req1 = *(VirtIOBlockReq **)a,
                          *req2 = *(VirtIOBlockReq **)b;
 
+    IO_CODE();
+
     /*
      * Note that we can't simply subtract sector_num1 from sector_num2
      * here as that could overflow the return value.
@@ -442,6 +463,8 @@ static void virtio_blk_submit_multireq(VirtIOBlock *s, MultiReqBuffer *mrb)
     uint32_t max_transfer;
     int64_t sector_num = 0;
 
+    IO_CODE();
+
     if (mrb->num_reqs == 1) {
         submit_requests(s, mrb, 0, 1, -1);
         mrb->num_reqs = 0;
@@ -491,6 +514,8 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     VirtIOBlock *s = req->dev;
 
+    IO_CODE();
+
     block_acct_start(blk_get_stats(s->blk), &req->acct, 0,
                      BLOCK_ACCT_FLUSH);
 
@@ -509,6 +534,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
     uint64_t total_sectors;
 
+    IO_CODE();
+
     if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return false;
     }
@@ -535,6 +562,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
     uint8_t err_status;
     int bytes;
 
+    IO_CODE();
+
     sector = virtio_ldq_p(vdev, &dwz_hdr->sector);
     num_sectors = virtio_ldl_p(vdev, &dwz_hdr->num_sectors);
     flags = virtio_ldl_p(vdev, &dwz_hdr->flags);
@@ -613,6 +642,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
         virtio_error(vdev, "virtio-blk missing headers");
         return -1;
@@ -763,6 +794,8 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
     MultiReqBuffer mrb = {};
     bool suppress_notifications = virtio_queue_get_notification(vq);
 
+    IO_CODE();
+
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_io_plug(s->blk);
 
@@ -796,6 +829,8 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
+    IO_CODE();
+
     if (s->dataplane && !s->dataplane_started) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().
@@ -846,8 +881,9 @@ static void virtio_blk_dma_restart_bh(void *opaque)
 }
 
 /*
- * Only called when VM is started or stopped in cpus.c.
- * No iothread runs in parallel
+ * Only called when VM is started or stopped in cpus.c. When running is true
+ * ->start_ioeventfd() has already been called. When running is false
+ * ->stop_ioeventfd() has not yet been called.
  */
 static void virtio_blk_dma_restart_cb(void *opaque, bool running,
                                       RunState state)
@@ -867,6 +903,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
             virtio_blk_dma_restart_bh, s);
 }
 
+/* ->stop_ioeventfd() has already been called by virtio_bus_reset() */
 static void virtio_blk_reset(VirtIODevice *vdev)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
@@ -877,10 +914,6 @@ static void virtio_blk_reset(VirtIODevice *vdev)
 
     ctx = blk_get_aio_context(s->blk);
     aio_context_acquire(ctx);
-    /*
-     * This drain together with ->stop_ioeventfd() in virtio_pci_reset()
-     * stops all Iothreads.
-     */
     blk_drain(s->blk);
 
     /* We drop queued requests after blk_drain() because blk_drain() itself can
-- 
2.38.1

Re: [PATCH 5/8] virtio-blk: mark IO_CODE functions

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> Just as done in the block API, mark functions in virtio-blk
> that are called also from iothread(s).
>
> We know such functions are IO because many are blk_* callbacks,
> running always in the device iothread, and remaining are propagated
> from the leaf IO functions (if a function calls a IO_CODE function,
> itself is categorized as IO_CODE too).
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-7-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 6/8] virtio-blk: remove unnecessary AioContext lock from function already safe

[Stefan Hajnoczi]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

AioContext lock was introduced in b9e413dd375 and in this instance
it is used to protect these 3 functions:
- virtio_blk_handle_rw_error
- virtio_blk_req_complete
- block_acct_done

Now that all three of the above functions are protected with
their own locks, we can get rid of the AioContext lock.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20220609143727.1151816-9-eesposit@redhat.com>
---
 hw/block/virtio-blk.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f8fcf25292..faea045178 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -108,7 +108,6 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 
     IO_CODE();
 
-    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (next) {
         VirtIOBlockReq *req = next;
         next = req->mr_next;
@@ -141,7 +140,6 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
         block_acct_done(blk_get_stats(s->blk), &req->acct);
         virtio_blk_free_request(req);
     }
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
 static void virtio_blk_flush_complete(void *opaque, int ret)
@@ -150,20 +148,16 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
     VirtIOBlock *s = req->dev;
 
     IO_CODE();
-    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
 
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, 0, true)) {
-            goto out;
+            return;
         }
     }
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     block_acct_done(blk_get_stats(s->blk), &req->acct);
     virtio_blk_free_request(req);
-
-out:
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
 static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
@@ -174,11 +168,10 @@ static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
                             ~VIRTIO_BLK_T_BARRIER) == VIRTIO_BLK_T_WRITE_ZEROES;
 
     IO_CODE();
-    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
 
     if (ret) {
         if (virtio_blk_handle_rw_error(req, -ret, false, is_write_zeroes)) {
-            goto out;
+            return;
         }
     }
 
@@ -187,9 +180,6 @@ static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
         block_acct_done(blk_get_stats(s->blk), &req->acct);
     }
     virtio_blk_free_request(req);
-
-out:
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
 #ifdef __linux__
@@ -238,10 +228,8 @@ static void virtio_blk_ioctl_complete(void *opaque, int status)
     virtio_stl_p(vdev, &scsi->data_len, hdr->dxfer_len);
 
 out:
-    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     virtio_blk_req_complete(req, status);
     virtio_blk_free_request(req);
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
     g_free(ioctl_req);
 }
 
@@ -852,7 +840,6 @@ static void virtio_blk_dma_restart_bh(void *opaque)
 
     s->rq = NULL;
 
-    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (req) {
         VirtIOBlockReq *next = req->next;
         if (virtio_blk_handle_request(req, &mrb)) {
@@ -876,8 +863,6 @@ static void virtio_blk_dma_restart_bh(void *opaque)
 
     /* Paired with inc in virtio_blk_dma_restart_cb() */
     blk_dec_in_flight(s->conf.conf.blk);
-
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
 /*
-- 
2.38.1

Re: [PATCH 6/8] virtio-blk: remove unnecessary AioContext lock from function already safe

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> AioContext lock was introduced in b9e413dd375 and in this instance
> it is used to protect these 3 functions:
> - virtio_blk_handle_rw_error
> - virtio_blk_req_complete
> - block_acct_done
>
> Now that all three of the above functions are protected with
> their own locks, we can get rid of the AioContext lock.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20220609143727.1151816-9-eesposit@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 7/8] virtio-blk: don't acquire AioContext in virtio_blk_handle_vq()

[Stefan Hajnoczi]

There is no need to acquire AioContext in virtio_blk_handle_vq() because
no APIs used in the function require it and nothing else in the
virtio-blk code requires mutual exclusion anymore.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index faea045178..771d87cfbe 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -784,7 +784,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
 
     IO_CODE();
 
-    aio_context_acquire(blk_get_aio_context(s->blk));
     blk_io_plug(s->blk);
 
     do {
@@ -810,7 +809,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
     }
 
     blk_io_unplug(s->blk);
-    aio_context_release(blk_get_aio_context(s->blk));
 }
 
 static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.38.1

Re: [PATCH 7/8] virtio-blk: don't acquire AioContext in virtio_blk_handle_vq()

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> There is no need to acquire AioContext in virtio_blk_handle_vq() because
> no APIs used in the function require it and nothing else in the
> virtio-blk code requires mutual exclusion anymore.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/virtio-blk.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index faea045178..771d87cfbe 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -784,7 +784,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>  
>      IO_CODE();
>  
> -    aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_io_plug(s->blk);
>  
>      do {
> @@ -810,7 +809,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>      }
>  
>      blk_io_unplug(s->blk);
> -    aio_context_release(blk_get_aio_context(s->blk));
>  }
>  
>  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> 

As already discussed offline, this might be problematic with the work I
am done.

Basically I am trying to replace the AioContext lock with a rwlock for
graph modifications, and in order to use it we must convert all
BlockDriver IO functions in coroutines, because they traverse the graph
and must take the read lock (defined as coroutine_fn).

This implies that for now we need to implement blk_* and bdrv_*
functions in a similar fashion as generated_co_wrapper, therefore
creating a coroutine and polling waiting for it.
And polling uses AIO_WAIT_WHILE, which assumes the AioContext lock to be
taken.

In the future, we will use AIO_WAIT_WHILE_UNLOCKED, as you did in patch
1, but right now it's definitely too early to do it for a g_c_w function.

For this specific case, I see that blk_ioplug/unplug is not called in a
lot of places:
- here, and it's ok
- virtio-scsi, and I think you are going to cover this too soon
- xen-block

So if you manage to make all callers aiocontext-free, then I can just
rebase on top of your series and use AIO_WAIT_WHILE_POLL for
blk_ioplug/unplug when I convert it in coroutine.

Thank you,
Emanuele

[PATCH 8/8] virtio-blk: minimize virtio_blk_reset() AioContext lock region

[Stefan Hajnoczi]

blk_drain() needs the lock because it calls AIO_WAIT_WHILE().

The s->rq loop doesn't need the lock because dataplane has been stopped
when virtio_blk_reset() is called.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 771d87cfbe..0b411b3065 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -898,6 +898,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     ctx = blk_get_aio_context(s->blk);
     aio_context_acquire(ctx);
     blk_drain(s->blk);
+    aio_context_release(ctx);
 
     /* We drop queued requests after blk_drain() because blk_drain() itself can
      * produce them. */
@@ -908,8 +909,6 @@ static void virtio_blk_reset(VirtIODevice *vdev)
         virtio_blk_free_request(req);
     }
 
-    aio_context_release(ctx);
-
     assert(!s->dataplane_started);
     blk_set_enable_write_cache(s->blk, s->original_wce);
 }
-- 
2.38.1

Re: [PATCH 8/8] virtio-blk: minimize virtio_blk_reset() AioContext lock region

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> blk_drain() needs the lock because it calls AIO_WAIT_WHILE().
>
> The s->rq loop doesn't need the lock because dataplane has been stopped
> when virtio_blk_reset() is called.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>


Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[PATCH 0/8] virtio-blk: removal of AioContext lock

[Emanuele Giuseppe Esposito]

This serie aims to free virtio-blk (and in the future all
virtio devices) from the AioContext lock.

First step is to understand which functions are running in
the main loop and which are in iothreads.
Because many functions in virtio-blk are callbacks called
in some other virtio (pci, mmio, bus and so on) callbacks,
this is not trivial.
Patches 4-5-6 aim to split at least virtio-blk API.

There are two main things to consider when comparing this serie
with the block layer API split:

- sometimes we have data that is accessed by both IO and GS
  functions, but never together. For example, when the main
  loop access some data, iothread is guaranteed to be stopped.

- taking into account the multiqueue setup:
  this work aims to allow an iothread to access multiple
  virtio queues, while assigning the same queue to only one
  iothread. Currently, we have a single iothread running,
  so if we know that the main loop is not interfering, we
  are safe. However, if we want to consider multiple iothreads
  concurrently running, we need to take additional precautions.

A good example of the above is in patch 7.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Emanuele Giuseppe Esposito (8):
  virtio_queue_aio_attach_host_notifier: remove AioContext lock
  block-backend: enable_write_cache should be atomic
  virtio_blk_process_queued_requests: always run in a bh
  virtio: categorize callbacks in GS
  virtio-blk: mark GLOBAL_STATE_CODE functions
  virtio-blk: mark IO_CODE functions
  VirtIOBlock: protect rq with its own lock
  virtio-blk: remove unnecessary AioContext lock from function already
    safe

 block/block-backend.c           |   6 +-
 hw/block/dataplane/virtio-blk.c |  32 +++++++-
 hw/block/virtio-blk.c           | 132 ++++++++++++++++++++++++--------
 hw/scsi/virtio-scsi-dataplane.c |  12 ++-
 hw/virtio/virtio-bus.c          |   5 ++
 hw/virtio/virtio-pci.c          |   2 +
 hw/virtio/virtio.c              |   8 ++
 include/hw/virtio/virtio-blk.h  |   6 +-
 8 files changed, 163 insertions(+), 40 deletions(-)

-- 
2.31.1

[PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Emanuele Giuseppe Esposito]

virtio_queue_aio_attach_host_notifier() and
virtio_queue_aio_attach_host_notifier_nopoll() run always in the
main loop, so there is no need to protect them with AioContext
lock.

On the other side, virtio_queue_aio_detach_host_notifier() runs
in a bh in the iothread context, but it is always scheduled
(thus serialized) by the main loop. Therefore removing the
AioContext lock is safe, but unfortunately we can't do it
right now since bdrv_set_aio_context() and
aio_wait_bh_oneshot() still need to have it.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 14 ++++++++++++--
 hw/block/virtio-blk.c           |  2 ++
 hw/scsi/virtio-scsi-dataplane.c | 12 ++++++++++--
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 49276e46f2..f9224f23d2 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -167,6 +167,8 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     Error *local_err = NULL;
     int r;
 
+    GLOBAL_STATE_CODE();
+
     if (vblk->dataplane_started || s->starting) {
         return 0;
     }
@@ -243,13 +245,11 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     }
 
     /* Get this show started by hooking up our callbacks */
-    aio_context_acquire(s->ctx);
     for (i = 0; i < nvqs; i++) {
         VirtQueue *vq = virtio_get_queue(s->vdev, i);
 
         virtio_queue_aio_attach_host_notifier(vq, s->ctx);
     }
-    aio_context_release(s->ctx);
     return 0;
 
   fail_aio_context:
@@ -304,6 +304,8 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
 
+    GLOBAL_STATE_CODE();
+
     if (!vblk->dataplane_started || s->stopping) {
         return;
     }
@@ -318,6 +320,14 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
+    /*
+     * TODO: virtio_blk_data_plane_stop_bh() does not need the AioContext lock,
+     * because even though virtio_queue_aio_detach_host_notifier() runs in
+     * Iothread context, such calls are serialized by the BQL held (this
+     * function runs in the main loop).
+     * On the other side, virtio_queue_aio_attach_host_notifier* always runs
+     * in the main loop, therefore it doesn't need the AioContext lock.
+     */
     aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
 
     /* Drain and try to switch bs back to the QEMU main loop. If other users
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e9ba752f6b..8d0590cc76 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -121,6 +121,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     VirtIOBlock *s = next->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
+    IO_CODE();
+
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (next) {
         VirtIOBlockReq *req = next;
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 8bb6e6acfc..7080e9caa9 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -91,6 +91,8 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
+    GLOBAL_STATE_CODE();
+
     if (s->dataplane_started ||
         s->dataplane_starting ||
         s->dataplane_fenced) {
@@ -136,7 +138,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    aio_context_acquire(s->ctx);
     virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx);
     virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx);
 
@@ -146,7 +147,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     s->dataplane_starting = false;
     s->dataplane_started = true;
-    aio_context_release(s->ctx);
     return 0;
 
 fail_host_notifiers:
@@ -193,6 +193,14 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
     s->dataplane_stopping = true;
 
     aio_context_acquire(s->ctx);
+    /*
+     * TODO: virtio_scsi_dataplane_stop_bh() does not need the AioContext lock,
+     * because even though virtio_queue_aio_detach_host_notifier() runs in
+     * Iothread context, such calls are serialized by the BQL held (this
+     * function runs in the main loop).
+     * On the other side, virtio_queue_aio_attach_host_notifier* always runs
+     * in the main loop, therefore it doesn't need the AioContext lock.
+     */
     aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
     aio_context_release(s->ctx);
 
-- 
2.31.1

Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:20AM -0400, Emanuele Giuseppe Esposito wrote:
> @@ -146,7 +147,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>  
>      s->dataplane_starting = false;
>      s->dataplane_started = true;
> -    aio_context_release(s->ctx);
>      return 0;

This looks risky because s->dataplane_started is accessed by IO code and
there is a race condition here. Maybe you can refactor the code along
the lines of virtio-blk to avoid the race.

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

Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Emanuele Giuseppe Esposito]

Am 05/07/2022 um 16:11 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:20AM -0400, Emanuele Giuseppe Esposito wrote:
>> @@ -146,7 +147,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>>  
>>      s->dataplane_starting = false;
>>      s->dataplane_started = true;
>> -    aio_context_release(s->ctx);
>>      return 0;
> 
> This looks risky because s->dataplane_started is accessed by IO code and
> there is a race condition here. Maybe you can refactor the code along
> the lines of virtio-blk to avoid the race.
> 

Uhmm could you explain why is virtio-blk also safe here?
And what is currently protecting dataplane_started (in both blk and
scsi, as I don't see any other AioContext lock taken)?

Because I see that for example virtio_blk_req_complete is IO_CODE, so it
could theoretically read dataplane_started while it is being changed in
dataplane_stop? Even though I guess it doesn't because we disable and
clean the host notifier before modifying it?

But if so, I don't get what is the difference with scsi code, and why we
need to protect only that instance with the aiocontext lock?

Thank you,
Emanuele

Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

[Stefan Hajnoczi]

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

On Fri, Jul 08, 2022 at 11:01:37AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:11 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:20AM -0400, Emanuele Giuseppe Esposito wrote:
> >> @@ -146,7 +147,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
> >>  
> >>      s->dataplane_starting = false;
> >>      s->dataplane_started = true;
> >> -    aio_context_release(s->ctx);
> >>      return 0;
> > 
> > This looks risky because s->dataplane_started is accessed by IO code and
> > there is a race condition here. Maybe you can refactor the code along
> > the lines of virtio-blk to avoid the race.
> > 
> 
> Uhmm could you explain why is virtio-blk also safe here?
> And what is currently protecting dataplane_started (in both blk and
> scsi, as I don't see any other AioContext lock taken)?

dataplane_started is assigned before the host notifier is set up, which
I'm assuming is an implicit write barrier.

> Because I see that for example virtio_blk_req_complete is IO_CODE, so it
> could theoretically read dataplane_started while it is being changed in
> dataplane_stop? Even though I guess it doesn't because we disable and
> clean the host notifier before modifying it?

virtio_blk_data_plane_stop() has:

  aio_context_acquire(s->ctx);
  aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);

  /* Drain and 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);

  aio_context_release(s->ctx);

and disables host notifiers. At that point the IOThread no longer
receives virtqueue kicks and all in-flight requests have completed.
dataplane_started is only written afterwards so there is no race with
virtio_blk_req_complete().

> 
> But if so, I don't get what is the difference with scsi code, and why we
> need to protect only that instance with the aiocontext lock?

The race condition I pointed out is not with virtio_blk_req_complete()
and data_plane_stop(). It's data_plane_start() racing with
virtio_blk_req_complete().

The virtio-scsi dataplane code is different for historical reasons and
happens to have the race. I don't think the virtio-blk code is affected.

Stefan

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