Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/2] virtio-blk: Use blk_drain() to drain IO requests

[Markus Armbruster]

Just spotted this in my git-pull...

Alexander Yarygin <address@hidden> writes:

> Each call of the virtio_blk_reset() function calls blk_drain_all(),
> which works for all existing BlockDriverStates, while draining only
> one is needed.
>
> This patch replaces blk_drain_all() by blk_drain() in
> virtio_blk_reset(). virtio_blk_data_plane_stop() should be called
> after draining because it restores vblk->complete_request.
>
> Cc: "Michael S. Tsirkin" <address@hidden>
> 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>
> ---
>  hw/block/virtio-blk.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e6afe97..d8a906f 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int 
> running,
>  static void virtio_blk_reset(VirtIODevice *vdev)
>  {
>      VirtIOBlock *s = VIRTIO_BLK(vdev);
> -
> -    if (s->dataplane) {
> -        virtio_blk_data_plane_stop(s->dataplane);
> -    }
> +    AioContext *ctx;
>  
>      /*
>       * This should cancel pending requests, but can't do nicely until there
>       * are per-device request lists.
>       */
> -    blk_drain_all();
> +    ctx = blk_get_aio_context(s->blk);
> +    aio_context_acquire(ctx);
> +    blk_drain(s->blk);
> +
> +    if (s->dataplane) {
> +        virtio_blk_data_plane_stop(s->dataplane);
> +    }
> +    aio_context_release(ctx);
> +
>      blk_set_enable_write_cache(s->blk, s->original_wce);
>  }

>From bdrv_drain_all()'s comment:

 * Note that completion of an asynchronous I/O operation can trigger any
 * number of other I/O operations on other devices---for example a coroutine
 * can be arbitrarily complex and a constant flow of I/O can come until the
 * coroutine is complete.  Because of this, it is not possible to have a
 * function to drain a single device's I/O queue.

>From bdrv_drain()'s comment:

 * See the warning in bdrv_drain_all().  This function can only be called if
 * you are sure nothing can generate I/O because you have op blockers
 * installed.

blk_drain() and blk_drain_all() are trivial wrappers.

Ignorant questions:

* Why does blk_drain() suffice here?

* Is blk_drain() (created in PATCH 1) even a safe interface?