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

[Stefan Hajnoczi <stefanha@redhat.com>]

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