Re: [Qemu-devel] [PATCH 0/3] block: introduce submit I/O at batch

[Ming Lei]

Hi Paolo,

On Mon, Jun 30, 2014 at 7:10 PM, Paolo Bonzini <address@hidden> wrote:
> Il 30/06/2014 11:49, Ming Lei ha scritto:
>
>> 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.
>
>
> The basic idea of the code is great, however I do not think it is necessary
> to add the logic in AioContext.  You are right that io_submit supports
> multiple I/O, but right now we have one io_context_t per BlockDriverState so
> it is somewhat premature.  Note that AioContext in QEMU means something else
> than io_context_t.

I added the io queue into AioContext because the io queue can only
be used in the attached context(or thread), that said the io queue has to
be put into per context instance.

Or could you suggest where we put the io queue for submitting at
batch?  At the beginning, I put it into bs, but looks like bdrv_io_plug
and bdrv_io_unplug have to handle bs->file and bs itself.

>
> I don't think it's necessary to walk backing_hd too (especially because
> we're close to release and this cannot be a regression---dataplane in 2.0
> didn't support backing files at all!).  We need to keep the patches as
> simple as possible.

OK, I will not consider backing_hd case in v1, also the patchset will
not only improve dataplane, and other devices should benefit from
it too.

These patches themselves should be simple, and most of code are
add-only, if bdrv_io_plug()/bdrv_io_unplug() isn't used, they are very close
to noop.

I just wrote a draft patch to apply the mechanism on virtio-scsi, and
the improvement is obvious.

>
> You can just add bdrv_plug/bdrv_unplug callbacks to block/linux-aio.c and,
> in block.c, something like
>
>     BlockDriver *drv = bs->drv;
>     if (drv && drv->bdrv_plug) {
>         drv->bdrv_plug(bdrv);
>     } else if (bs->file) {
>         bdrv_plug(bs->file);
>     }
>
> and place all the queuing logic in linux-aio.c.  It should be obvious to the
> reviewer that the patch only affects dataplane.

As I explained above, the io queue has to be per context(thread), so
I am wondering if something like above is simpler since linux-aio still
need to get context information from bs->aio_context.

>
> (Also, note that QEMU doesn't use __ as the prefix for internal function).

OK, I will follow the QEMU code style.

Thanks,
--
Ming Lei