[v1,1/3] block: block: introduce bdrv_io_plug() and bdrv_io_unplug()

From: Ming Lei <tom.leiming@gmail.com>

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.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block.c                   |   22 ++++++++++++++++++++++
 include/block/block.h     |    3 +++
 include/block/block_int.h |    4 ++++
 3 files changed, 29 insertions(+)

Il 30/06/2014 17:47, Ming Lei ha scritto:
> From: Ming Lei <tom.leiming@gmail.com>
>
> 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.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  block.c                   |   22 ++++++++++++++++++++++
>  include/block/block.h     |    3 +++
>  include/block/block_int.h |    4 ++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/block.c b/block.c
> index 217f523..2b4ec5b 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);
> @@ -5774,3 +5775,24 @@ 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);
> +    }
> +}
> +
> +int bdrv_io_unplug(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (drv && drv->bdrv_io_unplug) {
> +        return drv->bdrv_io_unplug(bs);
> +    } else if (bs->file) {
> +        return bdrv_io_unplug(bs->file);
> +    }
> +    return 0;

I think this should return void (and that's how you use it in patch 3 
indeed).  If you fix this you can add my Reviewed-by tag.

Paolo

> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index d0baf4f..ccced61 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -578,4 +578,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs);
>   */
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context);
>
> +void bdrv_io_plug(BlockDriverState *bs);
> +int bdrv_io_unplug(BlockDriverState *bs);
> +
>  #endif
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 715c761..b5412fa 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -257,6 +257,10 @@ struct BlockDriver {
>      void (*bdrv_attach_aio_context)(BlockDriverState *bs,
>                                      AioContext *new_context);
>
> +    /* io queue for linux-aio */
> +    void (*bdrv_io_plug)(BlockDriverState *bs);
> +    int (*bdrv_io_unplug)(BlockDriverState *bs);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>
>

On Tue, Jul 1, 2014 at 12:10 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 30/06/2014 17:47, Ming Lei ha scritto:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> 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.
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  block.c                   |   22 ++++++++++++++++++++++
>>  include/block/block.h     |    3 +++
>>  include/block/block_int.h |    4 ++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 217f523..2b4ec5b 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);
>> @@ -5774,3 +5775,24 @@ 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);
>> +    }
>> +}
>> +
>> +int bdrv_io_unplug(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (drv && drv->bdrv_io_unplug) {
>> +        return drv->bdrv_io_unplug(bs);
>> +    } else if (bs->file) {
>> +        return bdrv_io_unplug(bs->file);
>> +    }
>> +    return 0;
>
>
> I think this should return void (and that's how you use it in patch 3
> indeed).  If you fix this you can add my Reviewed-by tag.

It can be used to trace how many IO are submitted at batch,
otherwise device can't know this information at all.

Thanks,
--
Ming Lei

Il 30/06/2014 18:15, Ming Lei ha scritto:
>>> >> +int bdrv_io_unplug(BlockDriverState *bs)
>>> >> +{
>>> >> +    BlockDriver *drv = bs->drv;
>>> >> +    if (drv && drv->bdrv_io_unplug) {
>>> >> +        return drv->bdrv_io_unplug(bs);
>>> >> +    } else if (bs->file) {
>>> >> +        return bdrv_io_unplug(bs->file);
>>> >> +    }
>>> >> +    return 0;
>> >
>> >
>> > I think this should return void (and that's how you use it in patch 3
>> > indeed).  If you fix this you can add my Reviewed-by tag.
> It can be used to trace how many IO are submitted at batch,
> otherwise device can't know this information at all.

Having a return value however suggests that bdrv_io_unplug can fail.  So 
this should be documented.  For now, I'd prefer to keep it simple.

Paolo

On Tue, Jul 1, 2014 at 12:18 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 30/06/2014 18:15, Ming Lei ha scritto:
>
>>>> >> +int bdrv_io_unplug(BlockDriverState *bs)
>>>> >> +{
>>>> >> +    BlockDriver *drv = bs->drv;
>>>> >> +    if (drv && drv->bdrv_io_unplug) {
>>>> >> +        return drv->bdrv_io_unplug(bs);
>>>> >> +    } else if (bs->file) {
>>>> >> +        return bdrv_io_unplug(bs->file);
>>>> >> +    }
>>>> >> +    return 0;
>>>
>>> >
>>> >
>>> > I think this should return void (and that's how you use it in patch 3
>>> > indeed).  If you fix this you can add my Reviewed-by tag.
>>
>> It can be used to trace how many IO are submitted at batch,
>> otherwise device can't know this information at all.
>
>
> Having a return value however suggests that bdrv_io_unplug can fail.  So
> this should be documented.  For now, I'd prefer to keep it simple.

Fair enough, will change it to void in v2.

Thanks,
--
Ming Lei