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
signature.asc
Description: PGP signature
