[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtio-blk: simplify virtio_blk_dma_restart_cb()
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH] virtio-blk: simplify virtio_blk_dma_restart_cb() |
Date: |
Thu, 10 Nov 2022 10:03:21 -0500 |
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
signature.asc
Description: PGP signature