Re: [Qemu-block] [PATCH 1/1] qcow2: avoid extra flushes in qcow2

[Pavel Borzenkov]

On Wed, Jun 01, 2016 at 12:07:01PM +0200, Kevin Wolf wrote:
> Am 01.06.2016 um 11:12 hat Denis V. Lunev geschrieben:
> > qcow2_cache_flush() calls bdrv_flush() unconditionally after writing
> > cache entries of a particular cache. This can lead to as many as
> > 2 additional fdatasyncs inside bdrv_flush.
> > 
> > We can simply skip all fdatasync calls inside qcow2_co_flush_to_os
> > as bdrv_flush for sure will do the job.
> 
> This looked wrong at first because flushes are needed to keep the right
> order of writes to the different caches. However, I see that you keep
> the flush in qcow2_cache_flush_dependency(), so in the code this is
> actually fine.
> 
> Can you make that more explicit in the commit message?
> 
> > This seriously affects the
> > performance of database operations inside the guest.
> > 
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Pavel Borzenkov <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > CC: Max Reitz <address@hidden>
> 
> Do you have performance numbers for master and with your patch applied?
> (No performance related patch should come without numbers in its commit
> message!)
> 
> What I find interesting is that this seems to help even though
> duplicated flushes should actually be really cheap because there is no
> new data that could be flushed in the second request. Makes me wonder if
> guests send duplicated flushes, too, and whether we should optimise
> that.
> 
> Maybe it would also be interesting to measure how things perform if we
> removed the flush from qcow2_cache_flush_dependency(). This would be
> incorrect code (corruption possible after host crash), but I'd like to
> know how much performance we actually lose here. This is performance
> that could potentially be gained by using a journal.

Here are the results of the following testcase: sequential write of 8Gb
file by 64Kb blocks, on unallocated qcow2 image, with fsync() after each
64 block. Lazy refcounts are disabled, so we have a dependent cache
here. Results from SSD machine are as follows (every result is a 10
iterations average):

w/o patches: ~420 blocks/sec
with Den's patch: ~650 blocks/sec
with Den's patch + qcow2_cache_flush_dependency() switched to
qcow2_cache_flush_nosync(): ~720 blocks/sec

> 
> Kevin