Re: [Qemu-devel] [PATCH v4 3/3] dataplane: submit I/O as a batch

[Kevin Wolf]

Am 03.07.2014 um 11:49 hat Paolo Bonzini geschrieben:
> Il 03/07/2014 11:44, Kevin Wolf ha scritto:
> >Am 02.07.2014 um 14:18 hat Ming Lei geschrieben:
> >>Before commit 580b6b2aa2(dataplane: use the QEMU block
> >>layer for I/O), dataplane for virtio-blk submits block
> >>I/O as a batch.
> >>
> >>This commit 580b6b2aa2 replaces the custom linux AIO
> >>implementation(including submit I/O as a batch) with QEMU
> >>block layer, but this commit causes ~40% throughput regression
> >>on virtio-blk performance, and removing submitting I/O
> >>as a batch is one of the causes.
> >>
> >>This patch applies the newly 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 throughput 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 <address@hidden>
> >>Signed-off-by: Ming Lei <address@hidden>
> >
> >Reviewing this one doesn't make sense because commit b002254d
> >('virtio-blk: Unify {non-,}dataplane's request handlings') removes the
> >patched code. You need to patch common virtio-blk code now (which should
> >improve non-dataplane virtio-blk as well).
> 
> Actually no, the second and third hunk still apply.  The patch you
> mentioned unifies request processing, but the loops that fetches
> requests from the virtqueue are still separate.  This is because
> virtio-blk-dataplane still uses the vring API instead of the generic
> virtio API.  address_space_map/unmap is not thread-safe yet, vring
> avoids it.
> 
> Regarding the first hunk, it is not needed at all.

Right, I only saw the merge conflict on the first hunk and gave up. I
didn't even look at the unapplied patch to see that it's wrong and
should be dropped indeed.

Kevin