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 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
> 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?

Yes, I think so, even though the current implementation is just for
"fixing" dataplane and keeping simple as far as possible, because
reading both metadata and payload will forward to bs->file, and
bs->file->drv should have supported plug & unplug in case of
linux-aio.


Thanks,
--
Ming Lei