| Message ID | 20220609143727.1151816-4-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: removal of AioContext lock | expand |
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
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
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(-)