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