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

[Kevin Wolf]

Am 01.07.2014 um 16:39 hat Ming Lei geschrieben:
> On Tue, Jul 1, 2014 at 9:31 PM, Ming Lei <address@hidden> wrote:
> > On Tue, Jul 1, 2014 at 7:18 PM, Kevin Wolf <address@hidden> wrote:
> >> 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
> 
> Maybe new interface of bdrv_io_flush() or bdrv_io_commit() is better
> for the above situation.
> 
> >> 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.
> >
> > The term of plug/unplug have been used in block subsystem of
> > linux kernel for long time, just like pipe with faucet, :-)

Fair enough. I don't think it's obvious when you don't know the kernel
code, but consistency might be more important.

> >>> @@ -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?
> 
> Sorry for not understanding the problem, and you are right, these
> patches can't support other formats, and for solving the dependency,
> changes to image format driver should be needed.

Then let's drop the bs->file recursion here and add an explicit
.bdrv_io_plug/unplug callback to the raw driver.

Kevin