Re: [Qemu-devel] [PATCH 1/2] block-backend: Introduce blk_drain() and replace blk_drain_all()

[Stefan Hajnoczi]

On Wed, Jun 03, 2015 at 04:46:14PM +0300, Alexander Yarygin wrote:
> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while only one
> BlockDriverState needs to be drained.
> 
> This patch introduces the blk_drain() function and replaces
> blk_drain_all() on it in virtio_blk_reset().
> 
> Cc: Christian Borntraeger <address@hidden>
> Cc: Cornelia Huck <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Alexander Yarygin <address@hidden>
> ---
>  block/block-backend.c          | 5 +++++
>  hw/block/virtio-blk.c          | 2 +-
>  include/sysemu/block-backend.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 93e46f3..aee8a12 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -700,6 +700,11 @@ int blk_flush_all(void)
>      return bdrv_flush_all();
>  }
>  
> +void blk_drain(BlockBackend *blk)
> +{
> +    bdrv_drain(blk->bs);
> +}
> +
>  void blk_drain_all(void)
>  {
>      bdrv_drain_all();
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..abaca58 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    blk_drain(s->blk);
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }

I noticed a bug in the current code when reviewing this:

If the 'backup' block job is active when the virtio-blk device is reset,
bs is moved to the global AioContext while the target stays in the
iothread AioContext.  Accesses will be made to target without acquiring
its AioContext.

This is not your fault but calling blk_drain() increases the chance that
this will deadlock.  The bs request cannot make progress until target
completes the I/O request.  Since target doesn't pump events when we
call aio_poll() on bs, draining bs will result in a hang if there are
write requests pending on bs.

I'll write a patch to address this case.

Stefan

pgpQnWFViq9Yq.pgp
Description: PGP signature