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

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.

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

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