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

[Ming Lei]

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, :-)
>
>>> @@ -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.


Thanks,
--
Ming Lei