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

[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 --]

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

[Emanuele Giuseppe Esposito]

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>
---
 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 f425b00793..384e52d564 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;
@@ -1972,13 +1972,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)
 {
     GLOBAL_STATE_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 8d0590cc76..191f75ce25 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -988,9 +988,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,
@@ -1058,11 +1056,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.31.1

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

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:21AM -0400, Emanuele Giuseppe Esposito wrote:
> 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>
> ---
>  block/block-backend.c | 6 +++---
>  hw/block/virtio-blk.c | 4 ----
>  2 files changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH 3/8] virtio_blk_process_queued_requests: always run in a bh

[Emanuele Giuseppe Esposito]

This function in virtio_blk_data_plane_start is directly
invoked, accessing the queued requests from the main loop,
while the device has already switched to the iothread context.

The only place where calling virtio_blk_process_queued_requests
from the main loop is allowed is when blk_set_aio_context fails,
and we still need to process the requests.

Since the logic of the bh is exactly the same as
virtio_blk_dma_restart, so rename the function and make it public
so that we can utilize it here too.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f9224f23d2..03e10a36a4 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -234,8 +234,16 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
         goto fail_aio_context;
     }
 
+    blk_inc_in_flight(s->conf->conf.blk);
+    /*
+     * vblk->bh is only set in virtio_blk_dma_restart_cb, which
+     * is called only on vcpu start or stop.
+     * Therefore it must be null.
+     */
+    assert(vblk->bh == NULL);
     /* Process queued requests before the ones in vring */
-    virtio_blk_process_queued_requests(vblk, false);
+    vblk->bh = aio_bh_new(blk_get_aio_context(s->conf->conf.blk),
+                          virtio_blk_restart_bh, vblk);
 
     /* Kick right away to begin processing requests already in vring */
     for (i = 0; i < nvqs; i++) {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 191f75ce25..29a9c53ebc 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -855,7 +855,7 @@ void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh)
     aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
-static void virtio_blk_dma_restart_bh(void *opaque)
+void virtio_blk_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
@@ -882,7 +882,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
      */
     if (!s->bh && !virtio_bus_ioeventfd_enabled(bus)) {
         s->bh = aio_bh_new(blk_get_aio_context(s->conf.conf.blk),
-                           virtio_blk_dma_restart_bh, s);
+                           virtio_blk_restart_bh, s);
         blk_inc_in_flight(s->conf.conf.blk);
         qemu_bh_schedule(s->bh);
     }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d311c57cca..c334353b5a 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -92,5 +92,6 @@ typedef struct MultiReqBuffer {
 
 void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq);
 void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh);
+void virtio_blk_restart_bh(void *opaque);
 
 #endif
-- 
2.31.1

Re: [PATCH 3/8] virtio_blk_process_queued_requests: always run in a bh

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:22AM -0400, Emanuele Giuseppe Esposito wrote:
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index f9224f23d2..03e10a36a4 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -234,8 +234,16 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>          goto fail_aio_context;
>      }
>  
> +    blk_inc_in_flight(s->conf->conf.blk);

Missing comment explaining why the in-flight counter is incremented and
where the matching decrement operation is located.

I think you can get away without a comment if blk_inc_in_flight() is
right next to aio_bh_new(), but in this case there are a few lines of
code in between and it becomes unclear if there is a connection.

> +    /*
> +     * vblk->bh is only set in virtio_blk_dma_restart_cb, which
> +     * is called only on vcpu start or stop.
> +     * Therefore it must be null.
> +     */
> +    assert(vblk->bh == NULL);
>      /* Process queued requests before the ones in vring */

This comment makes an assumption about the order of file descriptor
handlers vs BHs in the event loop. I suggest removing the comment. There
is no reason for processing queued requests first anyway since
virtio-blk devices can complete requests in any order.

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

Re: [PATCH 3/8] virtio_blk_process_queued_requests: always run in a bh

[Emanuele Giuseppe Esposito]

Am 05/07/2022 um 16:23 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:22AM -0400, Emanuele Giuseppe Esposito wrote:
>> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
>> index f9224f23d2..03e10a36a4 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -234,8 +234,16 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
>>          goto fail_aio_context;
>>      }
>>  
>> +    blk_inc_in_flight(s->conf->conf.blk);
> 
> Missing comment explaining why the in-flight counter is incremented and
> where the matching decrement operation is located.
> 
> I think you can get away without a comment if blk_inc_in_flight() is
> right next to aio_bh_new(), but in this case there are a few lines of
> code in between and it becomes unclear if there is a connection.

I will simply add:

    /*
     * virtio_blk_restart_bh() code will take care of decrementing
     * in_flight counter.
     */

should make sense.

> 
>> +    /*
>> +     * vblk->bh is only set in virtio_blk_dma_restart_cb, which
>> +     * is called only on vcpu start or stop.
>> +     * Therefore it must be null.
>> +     */
>> +    assert(vblk->bh == NULL);
>>      /* Process queued requests before the ones in vring */
> 
> This comment makes an assumption about the order of file descriptor
> handlers vs BHs in the event loop. I suggest removing the comment. There
> is no reason for processing queued requests first anyway since
> virtio-blk devices can complete requests in any order.
> 

Ok, I guess you mean in a separate patch.

Thank you,
Emanuele

Re: [PATCH 3/8] virtio_blk_process_queued_requests: always run in a bh

[Stefan Hajnoczi]

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

On Fri, Jul 08, 2022 at 11:07:06AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:23 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:22AM -0400, Emanuele Giuseppe Esposito wrote:
> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> >> index f9224f23d2..03e10a36a4 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -234,8 +234,16 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> >>          goto fail_aio_context;
> >>      }
> >>  
> >> +    blk_inc_in_flight(s->conf->conf.blk);
> > 
> > Missing comment explaining why the in-flight counter is incremented and
> > where the matching decrement operation is located.
> > 
> > I think you can get away without a comment if blk_inc_in_flight() is
> > right next to aio_bh_new(), but in this case there are a few lines of
> > code in between and it becomes unclear if there is a connection.
> 
> I will simply add:
> 
>     /*
>      * virtio_blk_restart_bh() code will take care of decrementing
>      * in_flight counter.
>      */
> 
> should make sense.

Perfect.

> 
> > 
> >> +    /*
> >> +     * vblk->bh is only set in virtio_blk_dma_restart_cb, which
> >> +     * is called only on vcpu start or stop.
> >> +     * Therefore it must be null.
> >> +     */
> >> +    assert(vblk->bh == NULL);
> >>      /* Process queued requests before the ones in vring */
> > 
> > This comment makes an assumption about the order of file descriptor
> > handlers vs BHs in the event loop. I suggest removing the comment. There
> > is no reason for processing queued requests first anyway since
> > virtio-blk devices can complete requests in any order.
> > 
> 
> Ok, I guess you mean in a separate patch.

No, before this patch the comment was correct. Now it's questionable
because the (synchronous) call has been replaced with a BH.

I think it's appropriate to drop this comment in this patch.

Stefan

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

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

[Emanuele Giuseppe Esposito]

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 the main loop.

- realize: iothread is not even created yet.

Signed-off-by: Emanuele Giuseppe Esposito <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 29a9c53ebc..4e6421c35e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1032,6 +1032,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 d7ec023adf..0891ddb2ff 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"
@@ -223,6 +224,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;
     }
@@ -247,6 +250,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 0566ad7d00..6798039391 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -301,6 +301,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
     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 5d607aeaa0..2650504dd4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1977,6 +1977,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) {
@@ -2025,6 +2027,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 */
@@ -2882,6 +2886,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);
     }
@@ -3024,6 +3030,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.31.1

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

[Michael S. Tsirkin]

On Thu, Jun 09, 2022 at 10:37:23AM -0400, Emanuele Giuseppe Esposito wrote:
> 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 the main loop.
> 
> - realize: iothread is not even created yet.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Acked-by: Michael S. Tsirkin <mst@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 29a9c53ebc..4e6421c35e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1032,6 +1032,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 d7ec023adf..0891ddb2ff 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"
> @@ -223,6 +224,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;
>      }
> @@ -247,6 +250,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 0566ad7d00..6798039391 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -301,6 +301,8 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>      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 5d607aeaa0..2650504dd4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1977,6 +1977,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) {
> @@ -2025,6 +2027,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 */
> @@ -2882,6 +2886,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);
>      }
> @@ -3024,6 +3030,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.31.1

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

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:23AM -0400, Emanuele Giuseppe Esposito wrote:
> 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 the main loop.
> 
> - realize: iothread is not even created yet.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <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(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Emanuele Giuseppe Esposito]

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>
---
 hw/block/dataplane/virtio-blk.c |  4 ++++
 hw/block/virtio-blk.c           | 29 +++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 03e10a36a4..bda6b3e8de 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 4e6421c35e..2eb0408f92 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -51,6 +51,8 @@ static const VirtIOFeature feature_sizes[] = {
 
 static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
 {
+    GLOBAL_STATE_CODE();
+
     s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
         virtio_feature_get_config_size(feature_sizes, host_features));
 
@@ -865,6 +867,10 @@ void virtio_blk_restart_bh(void *opaque)
     virtio_blk_process_queued_requests(s, true);
 }
 
+/*
+ * 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)
 {
@@ -872,6 +878,8 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusState *bus = VIRTIO_BUS(qbus);
 
+    GLOBAL_STATE_CODE();
+
     if (!running) {
         return;
     }
@@ -894,8 +902,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
@@ -1064,11 +1078,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);
 
@@ -1082,11 +1102,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;
@@ -1135,6 +1161,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);
@@ -1143,6 +1170,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.31.1

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

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:24AM -0400, Emanuele Giuseppe Esposito wrote:
> 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>
> ---
>  hw/block/dataplane/virtio-blk.c |  4 ++++
>  hw/block/virtio-blk.c           | 29 +++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Emanuele Giuseppe Esposito]

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>
---
 hw/block/dataplane/virtio-blk.c |  4 ++++
 hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index bda6b3e8de..9dc6347350 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));
 
@@ -299,6 +301,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 2eb0408f92..e1aaa606ba 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -62,6 +62,8 @@ static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
+    IO_CODE();
+
     req->dev = s;
     req->vq = vq;
     req->qiov.size = 0;
@@ -80,6 +82,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);
@@ -99,6 +103,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! */
@@ -166,7 +172,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;
@@ -188,7 +196,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;
@@ -221,6 +231,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) {
@@ -262,6 +274,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);
     }
@@ -282,6 +296,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.
@@ -380,6 +396,7 @@ fail:
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
+    IO_CODE();
 
     status = virtio_blk_handle_scsi_req(req);
     if (status != -EINPROGRESS) {
@@ -395,6 +412,8 @@ static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
     int64_t sector_num = mrb->reqs[start]->sector_num;
     bool is_write = mrb->is_write;
 
+    IO_CODE();
+
     if (num_reqs > 1) {
         int i;
         struct iovec *tmp_iov = qiov->iov;
@@ -438,6 +457,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.
@@ -457,6 +478,8 @@ static void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
     uint32_t max_transfer;
     int64_t sector_num = 0;
 
+    IO_CODE();
+
     if (mrb->num_reqs == 1) {
         submit_requests(blk, mrb, 0, 1, -1);
         mrb->num_reqs = 0;
@@ -506,6 +529,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);
 
@@ -524,6 +549,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;
     }
@@ -550,6 +577,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);
@@ -628,6 +657,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;
@@ -778,6 +809,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);
 
@@ -811,6 +844,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().
-- 
2.31.1

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

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
> 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>
> ---
>  hw/block/dataplane/virtio-blk.c |  4 ++++
>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)

The definition of IO_CODE() is:

  I/O API functions. These functions are thread-safe, and therefore
  can run in any thread as long as the thread has called
  aio_context_acquire/release().

I'm not sure it matches with the exact semantics you have in mind. Are
they really allowed to be called from any thread and even from multiple
threads? Or maybe just from the BlockBackend's AioContext thread?

We need to be very careful to define these terms precisely and avoid
applying them in cases that are similar but different as that will cause
problems in the future.

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Emanuele Giuseppe Esposito]

Am 05/07/2022 um 16:39 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
>> 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>
>> ---
>>  hw/block/dataplane/virtio-blk.c |  4 ++++
>>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+)
> 
> The definition of IO_CODE() is:
> 
>   I/O API functions. These functions are thread-safe, and therefore
>   can run in any thread as long as the thread has called
>   aio_context_acquire/release().
> 
> I'm not sure it matches with the exact semantics you have in mind. Are
> they really allowed to be called from any thread and even from multiple
> threads? Or maybe just from the BlockBackend's AioContext thread?

I think it is just from the BlockBackend's AioContext thread. But I
classified blk_* functions as IO_CODE.

What is your opinion on that?

> 
> We need to be very careful to define these terms precisely and avoid
> applying them in cases that are similar but different as that will cause
> problems in the future.
> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

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

[Stefan Hajnoczi]

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

On Fri, Jul 08, 2022 at 11:19:43AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:39 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:25AM -0400, Emanuele Giuseppe Esposito wrote:
> >> 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>
> >> ---
> >>  hw/block/dataplane/virtio-blk.c |  4 ++++
> >>  hw/block/virtio-blk.c           | 35 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 39 insertions(+)
> > 
> > The definition of IO_CODE() is:
> > 
> >   I/O API functions. These functions are thread-safe, and therefore
> >   can run in any thread as long as the thread has called
> >   aio_context_acquire/release().
> > 
> > I'm not sure it matches with the exact semantics you have in mind. Are
> > they really allowed to be called from any thread and even from multiple
> > threads? Or maybe just from the BlockBackend's AioContext thread?
> 
> I think it is just from the BlockBackend's AioContext thread. But I
> classified blk_* functions as IO_CODE.
> 
> What is your opinion on that?

There is a difference between blk_*() APIs and device emulation code.
Device emulation code controls exactly where it runs (vCPU thread, main
loop, IOThread). blk_*() APIs may be called from more of contexts and
they have no control over it.

I'd like to make sure that the annotations match the actual usage that
the code was designed for.

Stefan

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

[PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Emanuele Giuseppe Esposito]

s->rq is pointing to the the VirtIOBlockReq list, and this list is
read/written in:

virtio_blk_reset = main loop, but caller calls ->stop_ioeventfd() and
drains, so no iothread runs in parallel
virtio_blk_save_device = main loop, but VM is stopped (migration), so
iothread has no work on request list
virtio_blk_load_device = same as save_device
virtio_blk_device_realize = iothread is not created yet
virtio_blk_handle_rw_error = io, here is why we need synchronization.
s is device state and is shared accross all queues. Right now there
is no problem, because iothread and main loop never access it at
the same time, but if we introduce 1 iothread -> n virtqueue and
1 virtqueue -> 1 iothread mapping we might have two iothreads
accessing the list at the same time
virtio_blk_process_queued_requests: io, same problem as above.

Therefore we need a virtio-blk to protect s->rq list.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/block/virtio-blk.c          | 38 ++++++++++++++++++++++++++--------
 include/hw/virtio/virtio-blk.h |  5 ++++-
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e1aaa606ba..88c61457e1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -109,8 +109,10 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         /* 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! */
         req->mr_next = NULL;
-        req->next = s->rq;
-        s->rq = req;
+        WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+            req->next = s->rq;
+            s->rq = req;
+        }
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         if (acct_failed) {
@@ -860,10 +862,16 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh)
 {
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;
     MultiReqBuffer mrb = {};
 
-    s->rq = NULL;
+    IO_CODE();
+
+    /* Detach queue from s->rq and process everything here */
+    WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+        req = s->rq;
+        s->rq = NULL;
+    }
 
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     while (req) {
@@ -896,6 +904,7 @@ void virtio_blk_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
 
+    IO_CODE();
     qemu_bh_delete(s->bh);
     s->bh = NULL;
 
@@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
      * stops all Iothreads.
      */
     blk_drain(s->blk);
+    aio_context_release(ctx);
 
     /* We drop queued requests after blk_drain() because blk_drain() itself can
      * produce them. */
+    qemu_mutex_lock(&s->req_mutex);
     while (s->rq) {
         req = s->rq;
         s->rq = req->next;
+        qemu_mutex_unlock(&s->req_mutex);
         virtqueue_detach_element(req->vq, &req->elem, 0);
         virtio_blk_free_request(req);
+        qemu_mutex_lock(&s->req_mutex);
     }
-
-    aio_context_release(ctx);
+    qemu_mutex_unlock(&s->req_mutex);
 
     assert(!s->dataplane_started);
     blk_set_enable_write_cache(s->blk, s->original_wce);
@@ -1120,10 +1132,14 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;
 
     GLOBAL_STATE_CODE();
 
+    WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+        req = s->rq;
+    }
+
     while (req) {
         qemu_put_sbyte(f, 1);
 
@@ -1165,8 +1181,10 @@ static int virtio_blk_load_device(VirtIODevice *vdev, QEMUFile *f,
 
         req = qemu_get_virtqueue_element(vdev, f, sizeof(VirtIOBlockReq));
         virtio_blk_init_request(s, virtio_get_queue(vdev, vq_idx), req);
-        req->next = s->rq;
-        s->rq = req;
+        WITH_QEMU_LOCK_GUARD(&s->req_mutex) {
+            req->next = s->rq;
+            s->rq = req;
+        }
     }
 
     return 0;
@@ -1272,6 +1290,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 
     virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
 
+    qemu_mutex_init(&s->req_mutex);
     s->blk = conf->conf.blk;
     s->rq = NULL;
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
@@ -1318,6 +1337,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev)
     qemu_coroutine_dec_pool_size(conf->num_queues * conf->queue_size / 2);
     qemu_del_vm_change_state_handler(s->change);
     blockdev_mark_auto_del(s->blk);
+    qemu_mutex_destroy(&s->req_mutex);
     virtio_cleanup(vdev);
 }
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index c334353b5a..5cb59994a8 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,7 +53,6 @@ struct VirtIOBlockReq;
 struct VirtIOBlock {
     VirtIODevice parent_obj;
     BlockBackend *blk;
-    void *rq;
     QEMUBH *bh;
     VirtIOBlkConf conf;
     unsigned short sector_mask;
@@ -64,6 +63,10 @@ struct VirtIOBlock {
     struct VirtIOBlockDataPlane *dataplane;
     uint64_t host_features;
     size_t config_size;
+
+    /* While the VM is running, req_mutex protects rq.  */
+    QemuMutex req_mutex;
+    struct VirtIOBlockReq *rq;
 };
 
 typedef struct VirtIOBlockReq {
-- 
2.31.1

Re: [PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * stops all Iothreads.
>       */
>      blk_drain(s->blk);
> +    aio_context_release(ctx);
>  
>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>       * produce them. */
> +    qemu_mutex_lock(&s->req_mutex);
>      while (s->rq) {
>          req = s->rq;
>          s->rq = req->next;
> +        qemu_mutex_unlock(&s->req_mutex);
>          virtqueue_detach_element(req->vq, &req->elem, 0);
>          virtio_blk_free_request(req);
> +        qemu_mutex_lock(&s->req_mutex);

Why is req_mutex dropped temporarily? At this point we don't really need
the req_mutex (all I/O should be stopped and drained), but maybe we
should do:

  WITH_QEMU_MUTEX(&s->req_mutex) {
      req = s->rq;
      s->rq = NULL;
  }

  ...process req list...

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Emanuele Giuseppe Esposito]

Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>>       * stops all Iothreads.
>>       */
>>      blk_drain(s->blk);
>> +    aio_context_release(ctx);
>>  
>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>>       * produce them. */
>> +    qemu_mutex_lock(&s->req_mutex);
>>      while (s->rq) {
>>          req = s->rq;
>>          s->rq = req->next;
>> +        qemu_mutex_unlock(&s->req_mutex);
>>          virtqueue_detach_element(req->vq, &req->elem, 0);
>>          virtio_blk_free_request(req);
>> +        qemu_mutex_lock(&s->req_mutex);
> 
> Why is req_mutex dropped temporarily? At this point we don't really need
> the req_mutex (all I/O should be stopped and drained), but maybe we
> should do:

Agree that maybe it is not useful to drop the mutex temporarily.

Regarding why req_mutex is not needed, yes I guess it isn't. Should I
get rid of this hunk at all, and maybe leave a comment like "no
synchronization needed, due to drain + ->stop_ioeventfd()"?

> 
>   WITH_QEMU_MUTEX(&s->req_mutex) {
>       req = s->rq;
>       s->rq = NULL;
>   }
> 
>   ...process req list...

Not sure what you mean here, we are looping on s->rq, so do we need to
protect also that? and why setting it to NULL? Sorry I am a little bit
lost here.

Thank you,
Emanuele

> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Re: [PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Emanuele Giuseppe Esposito]

Am 08/07/2022 um 11:33 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
>> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
>>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>>>       * stops all Iothreads.
>>>       */
>>>      blk_drain(s->blk);
>>> +    aio_context_release(ctx);
>>>  
>>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
>>>       * produce them. */
>>> +    qemu_mutex_lock(&s->req_mutex);
>>>      while (s->rq) {
>>>          req = s->rq;
>>>          s->rq = req->next;
>>> +        qemu_mutex_unlock(&s->req_mutex);
>>>          virtqueue_detach_element(req->vq, &req->elem, 0);
>>>          virtio_blk_free_request(req);
>>> +        qemu_mutex_lock(&s->req_mutex);
>>
>> Why is req_mutex dropped temporarily? At this point we don't really need
>> the req_mutex (all I/O should be stopped and drained), but maybe we
>> should do:
> 
> Agree that maybe it is not useful to drop the mutex temporarily.
> 
> Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> get rid of this hunk at all, and maybe leave a comment like "no
> synchronization needed, due to drain + ->stop_ioeventfd()"?

Actually, regarding this, I found why I added the lock:

https://patchew.org/QEMU/20220426085114.199647-1-eesposit@redhat.com/#584d7d1a-94cc-9ebb-363b-2fddb8d79f5b@redhat.com

So maybe it's better to add it.

> 
>>
>>   WITH_QEMU_MUTEX(&s->req_mutex) {
>>       req = s->rq;
>>       s->rq = NULL;
>>   }
>>
>>   ...process req list...
> 
> Not sure what you mean here, we are looping on s->rq, so do we need to
> protect also that? and why setting it to NULL? Sorry I am a little bit
> lost here.
> 
> Thank you,
> Emanuele
> 
>>
>> Otherwise:
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>

Re: [PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Stefan Hajnoczi]

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

On Fri, Jul 08, 2022 at 01:22:58PM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 08/07/2022 um 11:33 schrieb Emanuele Giuseppe Esposito:
> > 
> > 
> > Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> >> On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> >>> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
> >>>       * stops all Iothreads.
> >>>       */
> >>>      blk_drain(s->blk);
> >>> +    aio_context_release(ctx);
> >>>  
> >>>      /* We drop queued requests after blk_drain() because blk_drain() itself can
> >>>       * produce them. */
> >>> +    qemu_mutex_lock(&s->req_mutex);
> >>>      while (s->rq) {
> >>>          req = s->rq;
> >>>          s->rq = req->next;
> >>> +        qemu_mutex_unlock(&s->req_mutex);
> >>>          virtqueue_detach_element(req->vq, &req->elem, 0);
> >>>          virtio_blk_free_request(req);
> >>> +        qemu_mutex_lock(&s->req_mutex);
> >>
> >> Why is req_mutex dropped temporarily? At this point we don't really need
> >> the req_mutex (all I/O should be stopped and drained), but maybe we
> >> should do:
> > 
> > Agree that maybe it is not useful to drop the mutex temporarily.
> > 
> > Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> > get rid of this hunk at all, and maybe leave a comment like "no
> > synchronization needed, due to drain + ->stop_ioeventfd()"?
> 
> Actually, regarding this, I found why I added the lock:
> 
> https://patchew.org/QEMU/20220426085114.199647-1-eesposit@redhat.com/#584d7d1a-94cc-9ebb-363b-2fddb8d79f5b@redhat.com
> 
> So maybe it's better to add it.

I don't see anything obvious in Paolo's email that you linked. I think
he was saying it's safest to use a lock but not that it's necessary.

Can you clarify what you mean?

Stefan

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

Re: [PATCH 7/8] VirtIOBlock: protect rq with its own lock

[Stefan Hajnoczi]

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

On Fri, Jul 08, 2022 at 11:33:28AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 16:45 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:26AM -0400, Emanuele Giuseppe Esposito wrote:
> >> @@ -946,17 +955,20 @@ static void virtio_blk_reset(VirtIODevice *vdev)
> >>       * stops all Iothreads.
> >>       */
> >>      blk_drain(s->blk);
> >> +    aio_context_release(ctx);
> >>  
> >>      /* We drop queued requests after blk_drain() because blk_drain() itself can
> >>       * produce them. */
> >> +    qemu_mutex_lock(&s->req_mutex);
> >>      while (s->rq) {
> >>          req = s->rq;
> >>          s->rq = req->next;
> >> +        qemu_mutex_unlock(&s->req_mutex);
> >>          virtqueue_detach_element(req->vq, &req->elem, 0);
> >>          virtio_blk_free_request(req);
> >> +        qemu_mutex_lock(&s->req_mutex);
> > 
> > Why is req_mutex dropped temporarily? At this point we don't really need
> > the req_mutex (all I/O should be stopped and drained), but maybe we
> > should do:
> 
> Agree that maybe it is not useful to drop the mutex temporarily.
> 
> Regarding why req_mutex is not needed, yes I guess it isn't. Should I
> get rid of this hunk at all, and maybe leave a comment like "no
> synchronization needed, due to drain + ->stop_ioeventfd()"?
> 
> > 
> >   WITH_QEMU_MUTEX(&s->req_mutex) {
> >       req = s->rq;
> >       s->rq = NULL;
> >   }
> > 
> >   ...process req list...
> 
> Not sure what you mean here, we are looping on s->rq, so do we need to
> protect also that? and why setting it to NULL? Sorry I am a little bit
> lost here.

During reset we need to free the s->rq list and set the head pointer to
NULL.

If we want to access s->rq under s->req_mutex for consistency, then we
can fetch the list head into a local variable, drop the lock, and then
process the list (new items will not be added to the list anymore).

FWIW I think accessing s->rq under req_mutex for consistency is fine.
That makes the code easier to understand (no special case) and reduces
the danger of copy-pasting code into a context where a lock is required.

Stefan

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

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

[Emanuele Giuseppe Esposito]

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>
---
 hw/block/virtio-blk.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 88c61457e1..ce8efd8381 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -133,7 +133,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;
@@ -166,7 +165,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)
@@ -175,20 +173,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)
@@ -199,11 +193,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;
         }
     }
 
@@ -212,9 +205,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__
@@ -263,10 +253,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);
 }
 
@@ -873,7 +861,6 @@ void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh)
         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)) {
@@ -897,7 +884,6 @@ void virtio_blk_process_queued_requests(VirtIOBlock *s, bool is_bh)
     if (is_bh) {
         blk_dec_in_flight(s->conf.conf.blk);
     }
-    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
 void virtio_blk_restart_bh(void *opaque)
-- 
2.31.1

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

[Stefan Hajnoczi]

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

On Thu, Jun 09, 2022 at 10:37:27AM -0400, Emanuele Giuseppe Esposito wrote:
> 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>
> ---
>  hw/block/virtio-blk.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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