[Qemu-devel] [PATCH v2 0/3] linux-aio: introduce submit I/O at batch

[Qemu-devel] [PATCH v2 0/3] linux-aio: introduce submit I/O at batch

[Ming Lei]

Hi,

The commit 580b6b2aa2(dataplane: use the QEMU block layer for I/O)
introduces ~40% throughput regression on virtio-blk dataplane, and
one of causes is that submitting I/O at batch is removed.

This patchset trys to introduce this mechanism on block, at least,
linux-aio can benefit from that.

With these patches, it is observed that thoughout on virtio-blk
dataplane can be improved a lot, see data in commit log of patch
3/3.

It should be possible to apply the batch mechanism to other devices
(such as virtio-scsi) too.

V2:
	- define return value of bdrv_io_unplug as void, suggested by Paolo
	- avoid busy-wait for handling io_submit
V1:
	- move queuing io stuff into linux-aio.c as suggested by Paolo


Thanks,
--
Ming Lei

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

[Ming Lei]

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 <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 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);
@@ -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);
+    }
+}
+
+void bdrv_io_unplug(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (drv && drv->bdrv_io_unplug) {
+        drv->bdrv_io_unplug(bs);
+    } else if (bs->file) {
+        bdrv_io_unplug(bs->file);
+    }
+}
diff --git a/include/block/block.h b/include/block/block.h
index d0baf4f..ea627d2 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);
+void bdrv_io_unplug(BlockDriverState *bs);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 715c761..0d75ca6 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);
+    void (*bdrv_io_unplug)(BlockDriverState *bs);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.7.9.5

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 <pbonzini@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  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

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 <kwolf@redhat.com> 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 <pbonzini@redhat.com>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  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

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 <ming.lei@canonical.com> wrote:
> On Tue, Jul 1, 2014 at 7:18 PM, Kevin Wolf <kwolf@redhat.com> 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 <pbonzini@redhat.com>
>>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>>> ---
>>>  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

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 <ming.lei@canonical.com> wrote:
> > On Tue, Jul 1, 2014 at 7:18 PM, Kevin Wolf <kwolf@redhat.com> 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 <pbonzini@redhat.com>
> >>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >>> ---
> >>>  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

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

[Paolo Bonzini]

Il 01/07/2014 17:21, Kevin Wolf ha scritto:
>>>> 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.

Actually I thought about this in my review, and there's no reason for 
this not to work for image formats.

While bs->file is plugged, image formats will start executing their 
bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as 
necessary.  The reads and writes will accumulate in bs->file until it is 
unplugged, which is exactly the effect we want.

The change in bdrv_drain_all is ugly though.  I don't have a better 
idea, but I would like to understand better why it is needed.  Ming Lei, 
did you see a deadlock without it?

Paolo

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

[Ming Lei]

On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 01/07/2014 17:21, Kevin Wolf ha scritto:
>
>>>>> 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.
>
>
> Actually I thought about this in my review, and there's no reason for this
> not to work for image formats.
>
> While bs->file is plugged, image formats will start executing their
> bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
> necessary.  The reads and writes will accumulate in bs->file until it is
> unplugged, which is exactly the effect we want.

For some image formats, meta data need to be read first before
the payload can be read since how/what to read payload might
depend on content of meta data.

>
> The change in bdrv_drain_all is ugly though.  I don't have a better idea,
> but I would like to understand better why it is needed.  Ming Lei, did you
> see a deadlock without it?

Not yet, just for safe reason to make sure all queued data has chance
to be flushed. If you think it isn't necessary, I can remove it.

Thanks,
--
Ming Lei

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

[Ming Lei]

On Wed, Jul 2, 2014 at 8:46 AM, Ming Lei <ming.lei@canonical.com> wrote:
> On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 01/07/2014 17:21, Kevin Wolf ha scritto:
>>
>>>>>> 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.
>>
>>
>> Actually I thought about this in my review, and there's no reason for this
>> not to work for image formats.
>>
>> While bs->file is plugged, image formats will start executing their
>> bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
>> necessary.  The reads and writes will accumulate in bs->file until it is
>> unplugged, which is exactly the effect we want.
>
> For some image formats, meta data need to be read first before
> the payload can be read since how/what to read payload might
> depend on content of meta data.
>
>>
>> The change in bdrv_drain_all is ugly though.  I don't have a better idea,
>> but I would like to understand better why it is needed.  Ming Lei, did you
>> see a deadlock without it?
>
> Not yet, just for safe reason to make sure all queued data has chance
> to be flushed. If you think it isn't necessary, I can remove it.

Another interface like bdrv_io_flush() may be needed for
such purpose, and the accumulated IOs is really required to be
flushed before doing flush.

I will add the interface in v4.

Thanks,
--
Ming Lei

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

[Paolo Bonzini]

Il 02/07/2014 02:46, Ming Lei ha scritto:
> On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 01/07/2014 17:21, Kevin Wolf ha scritto:
>>>>>> 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.
>>
>>
>> Actually I thought about this in my review, and there's no reason for this
>> not to work for image formats.
>>
>> While bs->file is plugged, image formats will start executing their
>> bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
>> necessary.  The reads and writes will accumulate in bs->file until it is
>> unplugged, which is exactly the effect we want.
>
> For some image formats, meta data need to be read first before
> the payload can be read since how/what to read payload might
> depend on content of meta data.

Yes, but everything is done asynchronously so it should just work.  You have

     plug(bs)
       plug(bs->file)
     read
       start coroutine for image format read
         issue metadata read on bs->file
           bdrv_read ultimately calls bdrv_aio_readv on bs->file
             I/O not submitted because bs->file is plugged
         coroutine yields
     unplug(bs)
       unplug(bs->file)
         bs->file now submits metadata read
     ...
     metadata read completes
       coroutine re-entered
         image format read restarts, unaware of plug/unplug

If things do _not_ work as above I would like to understand where my 
reasoning is wrong.

On top of this, there _could_ be reasons for formats to implement 
plug/unplug themselves.  They could coalesce metadata reads or 
copy-on-write operations, for example.  This however is independent from 
the default behavior, which IMO is "plugging should propagate along 
bs->file" (and possibly bs->backing_hd too, but not now because that 
opens more cans of worms).

>> The change in bdrv_drain_all is ugly though.  I don't have a better idea,
>> but I would like to understand better why it is needed.  Ming Lei, did you
>> see a deadlock without it?
>
> Not yet, just for safe reason to make sure all queued data has chance
> to be flushed. If you think it isn't necessary, I can remove it.

If you could make it an assertion, that would also be great.  (BTW, it's 
probably best to add a nesting count to plug/unplug).

Paolo

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

[Kevin Wolf]

Am 02.07.2014 um 10:18 hat Paolo Bonzini geschrieben:
> Il 02/07/2014 02:46, Ming Lei ha scritto:
> >On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>Il 01/07/2014 17:21, Kevin Wolf ha scritto:
> >>>>>>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.
> >>
> >>
> >>Actually I thought about this in my review, and there's no reason for this
> >>not to work for image formats.
> >>
> >>While bs->file is plugged, image formats will start executing their
> >>bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
> >>necessary.  The reads and writes will accumulate in bs->file until it is
> >>unplugged, which is exactly the effect we want.
> >
> >For some image formats, meta data need to be read first before
> >the payload can be read since how/what to read payload might
> >depend on content of meta data.
> 
> Yes, but everything is done asynchronously so it should just work.  You have
> 
>     plug(bs)
>       plug(bs->file)
>     read
>       start coroutine for image format read
>         issue metadata read on bs->file
>           bdrv_read ultimately calls bdrv_aio_readv on bs->file
>             I/O not submitted because bs->file is plugged
>         coroutine yields
>     unplug(bs)
>       unplug(bs->file)
>         bs->file now submits metadata read
>     ...
>     metadata read completes
>       coroutine re-entered
>         image format read restarts, unaware of plug/unplug
> 
> If things do _not_ work as above I would like to understand where my
> reasoning is wrong.

This is my understanding of how things work, yes. It means that
plug applies only to the very first I/O that is made against the lower
layer. This means that we may queue L2 table reads, but the actual
payload requests are sent one by one.

In fact, currently, only one request would do the metadata read and hold
a coroutine lock for it. The request would be delayed until the unplug,
but we wouldn't get any batching in this case. We may actually benefit
from it when we get metadata cache hits and the first thing to happen is
the payload request.

Okay, looks safe to enable it even for image formats.

> On top of this, there _could_ be reasons for formats to implement
> plug/unplug themselves.  They could coalesce metadata reads or
> copy-on-write operations, for example.  This however is independent
> from the default behavior, which IMO is "plugging should propagate
> along bs->file" (and possibly bs->backing_hd too, but not now
> because that opens more cans of worms).

Yes, enabling it for bs->backing_hd was my alternative option for the
case that we keep bs->file. What can of worms do you see there? From the
reasoning above, I can't see why bs->backing_hd should be any different
from bs->file.

> >>The change in bdrv_drain_all is ugly though.  I don't have a better idea,
> >>but I would like to understand better why it is needed.  Ming Lei, did you
> >>see a deadlock without it?
> >
> >Not yet, just for safe reason to make sure all queued data has chance
> >to be flushed. If you think it isn't necessary, I can remove it.
> 
> If you could make it an assertion, that would also be great.  (BTW,
> it's probably best to add a nesting count to plug/unplug).

Agreed.

Another point I was thinking about (not for this series, but in a
follow-up) is what to do with bdrv_aio_multiwrite(). We could either
leave the two different interfaces there, just that multiwrite should
probably call plug/unplug before it sends its requests (this would apply
the improvements to virtio-blk without dataplane; could be done easily
even now), or we try to implement request merging with the plug/unplug
interface and convert the callers. Not sure how we would do the latter,
though, because we don't have a list of requests any more, except in the
lowest layer that actually queues.

Or should we have a default plug/unplug implementation in block.c that
queues any reads and writes, and on unplug merges requests, then plugs
bs->file and sends the requests to the driver?

Kevin

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

[Paolo Bonzini]

Il 02/07/2014 10:38, Kevin Wolf ha scritto:
> > On top of this, there _could_ be reasons for formats to implement
> > plug/unplug themselves.  They could coalesce metadata reads or
> > copy-on-write operations, for example.  This however is independent
> > from the default behavior, which IMO is "plugging should propagate
> > along bs->file" (and possibly bs->backing_hd too, but not now
> > because that opens more cans of worms).
>
> Yes, enabling it for bs->backing_hd was my alternative option for the
> case that we keep bs->file. What can of worms do you see there? From the
> reasoning above, I can't see why bs->backing_hd should be any different
> from bs->file.

The only one I thought of, is someone changing the backing file chain 
while the backing file is plugged.  IIRC bs->file doesn't change when 
you do bdrv_reopen.

Though changing the backing file chain probably would call 
bdrv_drain_all too.

> Another point I was thinking about (not for this series, but in a
> follow-up) is what to do with bdrv_aio_multiwrite(). We could either
> leave the two different interfaces there, just that multiwrite should
> probably call plug/unplug before it sends its requests (this would apply
> the improvements to virtio-blk without dataplane; could be done easily
> even now), or we try to implement request merging with the plug/unplug
> interface and convert the callers. Not sure how we would do the latter,
> though, because we don't have a list of requests any more, except in the
> lowest layer that actually queues.
>
> Or should we have a default plug/unplug implementation in block.c that
> queues any reads and writes, and on unplug merges requests, then plugs
> bs->file and sends the requests to the driver?

Longer term this is a good point.  Plug/unplug certainly provides many 
optimization opportunities, but as long as we expose the right API to 
the device model we need not care about it now.

In fact, there are plenty of other places where we can plug/unplug: 
mirroring, I/O throttling, image formats could also use aio to submit 
requests to a fragmented bs->file together and plug/unplug around the 
loop.  Benchmarking is needed of course to see if this actually improves 
performance (unlikely except in the last case) or at least reduces CPU 
usage.

Paolo

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

[Ming Lei]

On Wed, Jul 2, 2014 at 4:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/07/2014 02:46, Ming Lei ha scritto:
>
>> On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>>>
>>> Il 01/07/2014 17:21, Kevin Wolf ha scritto:
>>>>>>>
>>>>>>> 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.
>>>
>>>
>>>
>>> Actually I thought about this in my review, and there's no reason for
>>> this
>>> not to work for image formats.
>>>
>>> While bs->file is plugged, image formats will start executing their
>>> bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
>>> necessary.  The reads and writes will accumulate in bs->file until it is
>>> unplugged, which is exactly the effect we want.
>>
>>
>> For some image formats, meta data need to be read first before
>> the payload can be read since how/what to read payload might
>> depend on content of meta data.
>
>
> Yes, but everything is done asynchronously so it should just work.  You have
>
>     plug(bs)
>       plug(bs->file)
>     read
>       start coroutine for image format read
>         issue metadata read on bs->file
>           bdrv_read ultimately calls bdrv_aio_readv on bs->file
>             I/O not submitted because bs->file is plugged
>         coroutine yields
>     unplug(bs)
>       unplug(bs->file)
>         bs->file now submits metadata read
>     ...
>     metadata read completes
>       coroutine re-entered
>         image format read restarts, unaware of plug/unplug

Then start to read payload in original path, but no plug/unplug any
more. Also another request may follows, and another plug&unplug
comes too, which makes thing more complicated, so I suggest to
enable plug&unplug only for raw driver now.

And in future we can use explicit bdrv_io_flush() to handle the
dependency in non-raw format drivers.

>
> If things do _not_ work as above I would like to understand where my
> reasoning is wrong.
>
> On top of this, there _could_ be reasons for formats to implement
> plug/unplug themselves.  They could coalesce metadata reads or copy-on-write
> operations, for example.  This however is independent from the default
> behavior, which IMO is "plugging should propagate along bs->file" (and
> possibly bs->backing_hd too, but not now because that opens more cans of
> worms).
>
>
>>> The change in bdrv_drain_all is ugly though.  I don't have a better idea,
>>> but I would like to understand better why it is needed.  Ming Lei, did
>>> you
>>> see a deadlock without it?
>>
>>
>> Not yet, just for safe reason to make sure all queued data has chance
>> to be flushed. If you think it isn't necessary, I can remove it.
>
>
> If you could make it an assertion, that would also be great.  (BTW, it's
> probably best to add a nesting count to plug/unplug).

I'd rather to add it in future if we really need that.


Thanks,
--
Ming Lei

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

[Paolo Bonzini]

Il 02/07/2014 10:39, Ming Lei ha scritto:
> Then start to read payload in original path, but no plug/unplug any
> more. Also another request may follows, and another plug&unplug
> comes too, which makes thing more complicated, so I suggest to
> enable plug&unplug only for raw driver now.

That's just a performance issue (and actually one that wasn't in 2.0 
because qcow2 on dataplane wasn't supported there).  In many cases the 
cache hit of the qcow2 metadata cache can be very high, and avoiding 
plug/unplug would prevent an easy performance bonus.

I don't especially like plug/unplug as an API (I think it's better to 
extend aio_multiwrite to include other kind of requests), but:

- either we have qualms on the correctness of it, and then we should 
live with the regressions

- or if the patches are not messy and reverting them is easy, we should 
go for it.  This is what we did for dataplane in the first place, and we 
can keep doing it in the 2.1 dataplane code.

> > If you could make it an assertion, that would also be great.  (BTW, it's
> > probably best to add a nesting count to plug/unplug).
>
> I'd rather to add it in future if we really need that.

I think this is a matter of correctness as Kevin's review pointed out.

Paolo

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

[Kevin Wolf]

Am 02.07.2014 um 10:56 hat Paolo Bonzini geschrieben:
> Il 02/07/2014 10:39, Ming Lei ha scritto:
> >Then start to read payload in original path, but no plug/unplug any
> >more. Also another request may follows, and another plug&unplug
> >comes too, which makes thing more complicated, so I suggest to
> >enable plug&unplug only for raw driver now.
> 
> That's just a performance issue (and actually one that wasn't in 2.0
> because qcow2 on dataplane wasn't supported there).  In many cases
> the cache hit of the qcow2 metadata cache can be very high, and
> avoiding plug/unplug would prevent an easy performance bonus.
> 
> I don't especially like plug/unplug as an API (I think it's better
> to extend aio_multiwrite to include other kind of requests), but:
> 
> - either we have qualms on the correctness of it, and then we should
> live with the regressions
> 
> - or if the patches are not messy and reverting them is easy, we
> should go for it.  This is what we did for dataplane in the first
> place, and we can keep doing it in the 2.1 dataplane code.

Fully agree. This series is small enough and obviously fixes a
dataplane problem, so at least for 2.1 we should go for it.

My thoughts in the other mail were more about where to go in the long
term. We need to have a decision about what API we commit to - something
multiwrite-like or something plug/unplug-like - before we want to start
converting everything to that interface.

This is why I think we should be thinking about how to implement certain
optimisations (like the request merging with plug/unplug, as I mentioned;
or mixing read and writes in one batch with multiwrite) in both models.
Only when we have a reasonbly good idea of what the result would look
like in either case we can make an informed decision.

Kevin

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

[Ming Lei]

On Wed, Jul 2, 2014 at 5:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 02.07.2014 um 10:56 hat Paolo Bonzini geschrieben:
>> Il 02/07/2014 10:39, Ming Lei ha scritto:
>> >Then start to read payload in original path, but no plug/unplug any
>> >more. Also another request may follows, and another plug&unplug
>> >comes too, which makes thing more complicated, so I suggest to
>> >enable plug&unplug only for raw driver now.
>>
>> That's just a performance issue (and actually one that wasn't in 2.0
>> because qcow2 on dataplane wasn't supported there).  In many cases
>> the cache hit of the qcow2 metadata cache can be very high, and
>> avoiding plug/unplug would prevent an easy performance bonus.
>>
>> I don't especially like plug/unplug as an API (I think it's better
>> to extend aio_multiwrite to include other kind of requests), but:
>>
>> - either we have qualms on the correctness of it, and then we should
>> live with the regressions
>>
>> - or if the patches are not messy and reverting them is easy, we
>> should go for it.  This is what we did for dataplane in the first
>> place, and we can keep doing it in the 2.1 dataplane code.
>
> Fully agree. This series is small enough and obviously fixes a
> dataplane problem, so at least for 2.1 we should go for it.
>
> My thoughts in the other mail were more about where to go in the long
> term. We need to have a decision about what API we commit to - something
> multiwrite-like or something plug/unplug-like - before we want to start
> converting everything to that interface.
>
> This is why I think we should be thinking about how to implement certain
> optimisations (like the request merging with plug/unplug, as I mentioned;
> or mixing read and writes in one batch with multiwrite) in both models.
> Only when we have a reasonbly good idea of what the result would look
> like in either case we can make an informed decision.

Actually linux-aio can support to submit read/write to multi files, and
virtio-scsi does have the use case, so in future io queue should be
per aio-context as I posted 1st time.  And I am wondering if multiwrite-like
APIs can fit in this situation.


Thanks,
--
Ming Lei

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

[Kevin Wolf]

Am 02.07.2014 um 11:29 hat Ming Lei geschrieben:
> On Wed, Jul 2, 2014 at 5:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 02.07.2014 um 10:56 hat Paolo Bonzini geschrieben:
> >> Il 02/07/2014 10:39, Ming Lei ha scritto:
> >> >Then start to read payload in original path, but no plug/unplug any
> >> >more. Also another request may follows, and another plug&unplug
> >> >comes too, which makes thing more complicated, so I suggest to
> >> >enable plug&unplug only for raw driver now.
> >>
> >> That's just a performance issue (and actually one that wasn't in 2.0
> >> because qcow2 on dataplane wasn't supported there).  In many cases
> >> the cache hit of the qcow2 metadata cache can be very high, and
> >> avoiding plug/unplug would prevent an easy performance bonus.
> >>
> >> I don't especially like plug/unplug as an API (I think it's better
> >> to extend aio_multiwrite to include other kind of requests), but:
> >>
> >> - either we have qualms on the correctness of it, and then we should
> >> live with the regressions
> >>
> >> - or if the patches are not messy and reverting them is easy, we
> >> should go for it.  This is what we did for dataplane in the first
> >> place, and we can keep doing it in the 2.1 dataplane code.
> >
> > Fully agree. This series is small enough and obviously fixes a
> > dataplane problem, so at least for 2.1 we should go for it.
> >
> > My thoughts in the other mail were more about where to go in the long
> > term. We need to have a decision about what API we commit to - something
> > multiwrite-like or something plug/unplug-like - before we want to start
> > converting everything to that interface.
> >
> > This is why I think we should be thinking about how to implement certain
> > optimisations (like the request merging with plug/unplug, as I mentioned;
> > or mixing read and writes in one batch with multiwrite) in both models.
> > Only when we have a reasonbly good idea of what the result would look
> > like in either case we can make an informed decision.
> 
> Actually linux-aio can support to submit read/write to multi files, and
> virtio-scsi does have the use case, so in future io queue should be
> per aio-context as I posted 1st time.  And I am wondering if multiwrite-like
> APIs can fit in this situation.

Though where would you get the requests for two different files from,
within the same bdrv_plug/unplug block?

Hm, okay, I guess backing files might be a valid point. Anything else?

Kevin

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

[Ming Lei]

On Wed, Jul 2, 2014 at 5:49 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 02.07.2014 um 11:29 hat Ming Lei geschrieben:
>> On Wed, Jul 2, 2014 at 5:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 02.07.2014 um 10:56 hat Paolo Bonzini geschrieben:
>> >> Il 02/07/2014 10:39, Ming Lei ha scritto:
>> >> >Then start to read payload in original path, but no plug/unplug any
>> >> >more. Also another request may follows, and another plug&unplug
>> >> >comes too, which makes thing more complicated, so I suggest to
>> >> >enable plug&unplug only for raw driver now.
>> >>
>> >> That's just a performance issue (and actually one that wasn't in 2.0
>> >> because qcow2 on dataplane wasn't supported there).  In many cases
>> >> the cache hit of the qcow2 metadata cache can be very high, and
>> >> avoiding plug/unplug would prevent an easy performance bonus.
>> >>
>> >> I don't especially like plug/unplug as an API (I think it's better
>> >> to extend aio_multiwrite to include other kind of requests), but:
>> >>
>> >> - either we have qualms on the correctness of it, and then we should
>> >> live with the regressions
>> >>
>> >> - or if the patches are not messy and reverting them is easy, we
>> >> should go for it.  This is what we did for dataplane in the first
>> >> place, and we can keep doing it in the 2.1 dataplane code.
>> >
>> > Fully agree. This series is small enough and obviously fixes a
>> > dataplane problem, so at least for 2.1 we should go for it.
>> >
>> > My thoughts in the other mail were more about where to go in the long
>> > term. We need to have a decision about what API we commit to - something
>> > multiwrite-like or something plug/unplug-like - before we want to start
>> > converting everything to that interface.
>> >
>> > This is why I think we should be thinking about how to implement certain
>> > optimisations (like the request merging with plug/unplug, as I mentioned;
>> > or mixing read and writes in one batch with multiwrite) in both models.
>> > Only when we have a reasonbly good idea of what the result would look
>> > like in either case we can make an informed decision.
>>
>> Actually linux-aio can support to submit read/write to multi files, and
>> virtio-scsi does have the use case, so in future io queue should be
>> per aio-context as I posted 1st time.  And I am wondering if multiwrite-like
>> APIs can fit in this situation.
>
> Though where would you get the requests for two different files from,
> within the same bdrv_plug/unplug block?

I think it is doable if io queue is per aio_context.

The requests can be sent to different luns(files), see virtio_scsi_handle_cmd().

Thanks,
--
Ming Lei

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

[Paolo Bonzini]

Il 02/07/2014 12:02, Ming Lei ha scritto:
>>> Actually linux-aio can support to submit read/write to multi files, and
>>> virtio-scsi does have the use case, so in future io queue should be
>>> per aio-context as I posted 1st time.  And I am wondering if multiwrite-like
>>> APIs can fit in this situation.
>>
>> Though where would you get the requests for two different files from,
>> within the same bdrv_plug/unplug block?
>
> I think it is doable if io queue is per aio_context.

Yes, but that would basically mean moving linux-aio functionality to 
AioContext.  Effectively it's a rewrite of linux-aio.c.

Paolo

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

[Ming Lei]

On Wed, Jul 2, 2014 at 6:24 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/07/2014 12:02, Ming Lei ha scritto:
>
>>>> Actually linux-aio can support to submit read/write to multi files, and
>>>> virtio-scsi does have the use case, so in future io queue should be
>>>> per aio-context as I posted 1st time.  And I am wondering if
>>>> multiwrite-like
>>>> APIs can fit in this situation.
>>>
>>>
>>> Though where would you get the requests for two different files from,
>>> within the same bdrv_plug/unplug block?
>>
>>
>> I think it is doable if io queue is per aio_context.
>
>
> Yes, but that would basically mean moving linux-aio functionality to
> AioContext.  Effectively it's a rewrite of linux-aio.c.

IMO, we just need to move io_queue, plug and unplug out of linux-aio.c,
not a rewrite.

Thanks,
--
Ming Lei

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

[Ming Lei]

On Wed, Jul 2, 2014 at 4:56 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/07/2014 10:39, Ming Lei ha scritto:
>
>> Then start to read payload in original path, but no plug/unplug any
>> more. Also another request may follows, and another plug&unplug
>> comes too, which makes thing more complicated, so I suggest to
>> enable plug&unplug only for raw driver now.
>
>
> That's just a performance issue (and actually one that wasn't in 2.0 because
> qcow2 on dataplane wasn't supported there).  In many cases the cache hit of
> the qcow2 metadata cache can be very high, and avoiding plug/unplug would
> prevent an easy performance bonus.

I just gave a test on qcow2 image with dataplane on, and looks it
can work, so I will take original way to implement the api.

But making full use of plug/unplug for non-raw formats need
changes in format drivers in future.

>
> I don't especially like plug/unplug as an API (I think it's better to extend
> aio_multiwrite to include other kind of requests), but:
>
> - either we have qualms on the correctness of it, and then we should live
> with the regressions
>
> - or if the patches are not messy and reverting them is easy, we should go
> for it.  This is what we did for dataplane in the first place, and we can
> keep doing it in the 2.1 dataplane code.

OK, let's keep the API's as it now.

Actually I have thoughts in mind for plug&unplug changes too,
and now it is better to make it a bit simple,

>
>> > If you could make it an assertion, that would also be great.  (BTW, it's
>> > probably best to add a nesting count to plug/unplug).
>>
>> I'd rather to add it in future if we really need that.
>
>
> I think this is a matter of correctness as Kevin's review pointed out.

OK, specifically it is needed for non-raw format driver.

Thanks,
--
Ming Lei

[Qemu-devel] [PATCH v2 2/3] linux-aio: implement io plug and unplug

[Ming Lei]

This patch implements .bdrv_io_plug and .bdrv_io_unplug
callbacks for linux-aio Block Drivers, so that submitting
I/O at batch can be supported on linux-aio.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/linux-aio.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 block/raw-aio.h   |    2 ++
 block/raw-posix.c |   30 +++++++++++++++++++
 3 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/block/linux-aio.c b/block/linux-aio.c
index f0a2c08..195f0e3 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -25,6 +25,8 @@
  */
 #define MAX_EVENTS 128
 
+#define MAX_QUEUED_IO  128
+
 struct qemu_laiocb {
     BlockDriverAIOCB common;
     struct qemu_laio_state *ctx;
@@ -36,9 +38,19 @@ struct qemu_laiocb {
     QLIST_ENTRY(qemu_laiocb) node;
 };
 
+struct laio_queue {
+    struct iocb *iocbs[MAX_QUEUED_IO];
+    bool plugged;
+    unsigned int size;
+    unsigned int idx;
+};
+
 struct qemu_laio_state {
     io_context_t ctx;
     EventNotifier e;
+
+    /* io queue for submit at batch */
+    struct laio_queue io_q;
 };
 
 static inline ssize_t io_event_ret(struct io_event *ev)
@@ -135,6 +147,70 @@ static const AIOCBInfo laio_aiocb_info = {
     .cancel             = laio_cancel,
 };
 
+static void ioq_init(struct laio_queue *io_q)
+{
+    io_q->size = MAX_QUEUED_IO;
+    io_q->idx = 0;
+    io_q->plugged = false;
+}
+
+static int ioq_submit(struct qemu_laio_state *s)
+{
+    int ret, i = 0;
+    int len = s->io_q.idx;
+
+    do {
+        ret = io_submit(s->ctx, len, s->io_q.iocbs);
+    } while (i++ < 3 && ret == -EAGAIN);
+
+    /* empty io queue */
+    s->io_q.idx = 0;
+
+    if (ret >= 0)
+      return 0;
+
+    for (i = 0; i < len; i++) {
+        struct qemu_laiocb *laiocb =
+            container_of(s->io_q.iocbs[i], struct qemu_laiocb, iocb);
+
+        laiocb->ret = ret;
+        qemu_laio_process_completion(s, laiocb);
+    }
+    return ret;
+}
+
+static void ioq_enqueue(struct qemu_laio_state *s, struct iocb *iocb)
+{
+    unsigned int idx = s->io_q.idx;
+
+    s->io_q.iocbs[idx++] = iocb;
+    s->io_q.idx = idx;
+
+    /* submit immediately if queue is full */
+    if (idx == s->io_q.size)
+        ioq_submit(s);
+}
+
+void laio_io_plug(BlockDriverState *bs, void *aio_ctx)
+{
+    struct qemu_laio_state *s = aio_ctx;
+
+    s->io_q.plugged = true;
+}
+
+int laio_io_unplug(BlockDriverState *bs, void *aio_ctx)
+{
+    struct qemu_laio_state *s = aio_ctx;
+    int ret = 0;
+
+    if (s->io_q.idx > 0) {
+        ret = ioq_submit(s);
+    }
+    s->io_q.plugged = false;
+
+    return ret;
+}
+
 BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void *aio_ctx, int fd,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque, int type)
@@ -168,8 +244,12 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void *aio_ctx, int fd,
     }
     io_set_eventfd(&laiocb->iocb, event_notifier_get_fd(&s->e));
 
-    if (io_submit(s->ctx, 1, &iocbs) < 0)
-        goto out_free_aiocb;
+    if (!s->io_q.plugged) {
+        if (io_submit(s->ctx, 1, &iocbs) < 0)
+            goto out_free_aiocb;
+    } else {
+        ioq_enqueue(s, iocbs);
+    }
     return &laiocb->common;
 
 out_free_aiocb:
@@ -204,6 +284,8 @@ void *laio_init(void)
         goto out_close_efd;
     }
 
+    ioq_init(&s->io_q);
+
     return s;
 
 out_close_efd:
diff --git a/block/raw-aio.h b/block/raw-aio.h
index 8cf084e..ed47c3d 100644
--- a/block/raw-aio.h
+++ b/block/raw-aio.h
@@ -40,6 +40,8 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void *aio_ctx, int fd,
         BlockDriverCompletionFunc *cb, void *opaque, int type);
 void laio_detach_aio_context(void *s, AioContext *old_context);
 void laio_attach_aio_context(void *s, AioContext *new_context);
+void laio_io_plug(BlockDriverState *bs, void *aio_ctx);
+int laio_io_unplug(BlockDriverState *bs, void *aio_ctx);
 #endif
 
 #ifdef _WIN32
diff --git a/block/raw-posix.c b/block/raw-posix.c
index dacf4fb..ce548e1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1054,6 +1054,26 @@ static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
                        cb, opaque, type);
 }
 
+static void raw_aio_plug(BlockDriverState *bs)
+{
+#ifdef CONFIG_LINUX_AIO
+    BDRVRawState *s = bs->opaque;
+    if (s->use_aio) {
+        laio_io_plug(bs, s->aio_ctx);
+    }
+#endif
+}
+
+static void raw_aio_unplug(BlockDriverState *bs)
+{
+#ifdef CONFIG_LINUX_AIO
+    BDRVRawState *s = bs->opaque;
+    if (s->use_aio) {
+        laio_io_unplug(bs, s->aio_ctx);
+    }
+#endif
+}
+
 static BlockDriverAIOCB *raw_aio_readv(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque)
@@ -1503,6 +1523,8 @@ static BlockDriver bdrv_file = {
     .bdrv_aio_flush = raw_aio_flush,
     .bdrv_aio_discard = raw_aio_discard,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_io_plug = raw_aio_plug,
+    .bdrv_io_unplug = raw_aio_unplug,
 
     .bdrv_truncate = raw_truncate,
     .bdrv_getlength = raw_getlength,
@@ -1902,6 +1924,8 @@ static BlockDriver bdrv_host_device = {
     .bdrv_aio_flush	= raw_aio_flush,
     .bdrv_aio_discard   = hdev_aio_discard,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_io_plug = raw_aio_plug,
+    .bdrv_io_unplug = raw_aio_unplug,
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength	= raw_getlength,
@@ -2047,6 +2071,8 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_aio_writev    = raw_aio_writev,
     .bdrv_aio_flush	= raw_aio_flush,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_io_plug = raw_aio_plug,
+    .bdrv_io_unplug = raw_aio_unplug,
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength      = raw_getlength,
@@ -2175,6 +2201,8 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_aio_writev    = raw_aio_writev,
     .bdrv_aio_flush	= raw_aio_flush,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_io_plug = raw_aio_plug,
+    .bdrv_io_unplug = raw_aio_unplug,
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength      = raw_getlength,
@@ -2309,6 +2337,8 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_aio_writev    = raw_aio_writev,
     .bdrv_aio_flush	= raw_aio_flush,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_io_plug = raw_aio_plug,
+    .bdrv_io_unplug = raw_aio_unplug,
 
     .bdrv_truncate      = raw_truncate,
     .bdrv_getlength      = raw_getlength,
-- 
1.7.9.5

[Qemu-devel] [PATCH v2 3/3] dataplane: submit I/O at batch

[Ming Lei]

Before commit 580b6b2aa2(dataplane: use the Qemu block
layer for I/O), dataplane for virtio-blk submits block
I/O at batch.

This commit 580b6b2aa2 replaces the custom linux AIO
implementation(including I/O batch) with Qemu block
layer, but this commit causes ~40% throughput regression
on virtio-blk performance, and removing submitting I/O
at batch is one of the cause.

This patch applys the new introduced bdrv_io_plug() and
bdrv_io_unplug() interfaces to support submitting I/O
at batch for Qemu block layer, and in my test, the change
can improve thoughput by ~30% with 'aio=native'.

Following my fio test script:

	[global]
	direct=1
	size=4G
	bsrange=4k-4k
	timeout=40
	numjobs=4
	ioengine=libaio
	iodepth=64
	filename=/dev/vdc
	group_reporting=1

	[f]
	rw=randread

Result on one of my small machine(host: x86_64, 2cores, 4thread, guest: 4cores):
	- qemu master: 59K IOPS
	- qemu master with these patches: 81K IOPS
	- 2.0.0 release(dataplane using custom linux aio): 104K IOPS

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/block/dataplane/virtio-blk.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c10b7b7..8fefcce 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -289,6 +289,7 @@ static void handle_notify(EventNotifier *e)
     int ret;
 
     event_notifier_test_and_clear(&s->host_notifier);
+    bdrv_io_plug(s->blk->conf.bs);
     for (;;) {
         /* Disable guest->host notifies to avoid unnecessary vmexits */
         vring_disable_notification(s->vdev, &s->vring);
@@ -322,6 +323,7 @@ static void handle_notify(EventNotifier *e)
             break;
         }
     }
+    bdrv_io_unplug(s->blk->conf.bs);
 }
 
 /* Context: QEMU global mutex held */
-- 
1.7.9.5