Re: [Qemu-devel] [PATCH v2 1/3] block: block: introduce bdrv_io_plug() and bdrv_io_unplug()

[Kevin Wolf]

Am 01.07.2014 um 09:51 hat Ming Lei geschrieben:
> This patch introduces these two APIs so that following
> patches can support queuing I/O requests and submitting them
> at batch for improving I/O performance.
> 
> Reviewed-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Ming Lei <address@hidden>
> ---
>  block.c                   |   21 +++++++++++++++++++++
>  include/block/block.h     |    3 +++
>  include/block/block_int.h |    4 ++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 217f523..fea9e43 100644
> --- a/block.c
> +++ b/block.c
> @@ -1910,6 +1910,7 @@ void bdrv_drain_all(void)
>              bool bs_busy;
>  
>              aio_context_acquire(aio_context);
> +            bdrv_io_unplug(bs);
>              bdrv_start_throttled_reqs(bs);
>              bs_busy = bdrv_requests_pending(bs);
>              bs_busy |= aio_poll(aio_context, bs_busy);

This means that bdrv_io_plug/unplug() are not paired as I would have
expected. I find the name not very descriptive anyway (I probably
wouldn't have guessed what it does from its name if it weren't in this
series), so maybe we should consider renaming it?

Perhaps something like bdrv_req_batch_start() and
bdrv_req_batch_submit(), but I'm open for different suggestions.

> @@ -5774,3 +5775,23 @@ bool bdrv_is_first_non_filter(BlockDriverState 
> *candidate)
>  
>      return false;
>  }
> +
> +void bdrv_io_plug(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (drv && drv->bdrv_io_plug) {
> +        drv->bdrv_io_plug(bs);
> +    } else if (bs->file) {
> +        bdrv_io_plug(bs->file);
> +    }
> +}

Does this bs->file forwarding work for more than the raw driver? For
example, if drv is an image format driver that needs to read some
metadata from the image before it can submit the payload, does this
still do what you were intending?

Kevin