Re: [PATCH] virtio-blk: simplify virtio_blk_dma_restart_cb()

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On Thu, Nov 10, 2022 at 07:27:59AM -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 02, 2022 at 02:23:37PM -0400, Stefan Hajnoczi wrote:
> > virtio_blk_dma_restart_cb() is tricky because the BH must deal with
> > virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() being called.
> > 
> > There are two issues with the code:
> > 
> > 1. virtio_blk_realize() should use qdev_add_vm_change_state_handler()
> >    instead of qemu_add_vm_change_state_handler(). This ensures the
> >    ordering with virtio_init()'s vm change state handler that calls
> >    virtio_blk_data_plane_start()/virtio_blk_data_plane_stop() is
> >    well-defined. Then blk's AioContext is guaranteed to be up-to-date in
> >    virtio_blk_dma_restart_cb() and it's no longer necessary to have a
> >    special case for virtio_blk_data_plane_start().
> > 
> > 2. Only blk_drain() waits for virtio_blk_dma_restart_cb()'s
> >    blk_inc_in_flight() to be decremented. The bdrv_drain() family of
> >    functions do not wait for BlockBackend's in_flight counter to reach
> >    zero. virtio_blk_data_plane_stop() relies on blk_set_aio_context()'s
> >    implicit drain, but that's a bdrv_drain() and not a blk_drain().
> >    Note that virtio_blk_reset() already correctly relies on blk_drain().
> >    If virtio_blk_data_plane_stop() switches to blk_drain() then we can
> >    properly wait for pending virtio_blk_dma_restart_bh() calls.
> > 
> > Once these issues are taken care of the code becomes simpler. This
> > change is in preparation for multiple IOThreads in virtio-blk where we
> > need to clean up the multi-threading behavior.
> > 
> > I ran the reproducer from commit 49b44549ace7 ("virtio-blk: On restart,
> > process queued requests in the proper context") to check that there is
> > no regression.
> > 
> > Cc: Sergio Lopez <slp@redhat.com>
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> block tree I presume?

Thanks, merged into my block-next tree.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]