Re: [Qemu-devel] [PATCH][RFC][0/2] REF+/REF- optimization

[Frediano Ziglio]

2011/9/13 Kevin Wolf <address@hidden>:
> Am 13.09.2011 09:53, schrieb Frediano Ziglio:
>> These patches try to trade-off between leaks and speed for clusters
>> refcounts.
>>
>> Refcount increments (REF+ or refp) are handled in a different way from
>> decrements (REF- or refm). The reason it that posting or not flushing
>> a REF- cause "just" a leak while posting a REF+ cause a corruption.
>>
>> To optimize REF- I just used an array to store offsets then when a
>> flush is requested or array reach a limit (currently 1022) the array
>> is sorted and written to disk. I use an array with offset instead of
>> ranges to support compression (an offset could appear multiple times
>> in the array).
>> I consider this patch quite ready.
>
> Ok, first of all let's clarify what this optimises. I don't think it
> changes anything at all for the writeback cache modes, because these
> already do most operations in memory only. So this must be about
> optimising some operations with cache=writethrough. REF- isn't about
> normal cluster allocation, it is about COW with internal snapshots or
> bdrv_discard. Do you have benchmarks for any of them?
>
> I strongly disagree with your approach for REF-. We already have a
> cache, and introducing a second one sounds like a bad idea. I think we
> could get a very similar effect if we introduced a
> qcow2_cache_entry_mark_dirty_wb() that marks a given refcount block as
> dirty, but at the same time tells the cache that even in write-through
> mode it can still treat this block as write-back. This should require
> much less code changes.
>
> But let's measure the effects first, I suspect that for cluster
> allocation it doesn't help much because every REF- comes with a REF+.
>
>> To optimize REF+ I mark a range as allocated and use this range to
>> get new ones (avoiding writing refcount to disk). When a flush is
>> requested or in some situations (like snapshot) this cache is disabled
>> and flushed (written as REF-).
>> I do not consider this patch ready, it works and pass all io-tests
>> but for instance I would avoid allocating new clusters for refcount
>> during preallocation.
>
> The only question here is if improving cache=writethrough cluster
> allocation performance is worth the additional complexity in the already
> complex refcounting code.
>
> The alternative that was discussed before is the dirty bit approach that
> is used in QED and would allow us to use writeback for all refcount
> blocks, regardless of REF- or REF+. It would be an easier approach
> requiring less code changes, but it comes with the cost of requiring an
> fsck after a qemu crash.
>
>> End speed up is quite visible allocating clusters (more then 20%).
>
> What benchmark do you use for testing this?
>
> Kevin
>

Here you are some results (kb/s)

with patch (ref-, ref+), qcow2s is qcow2 with a snapshot

run   raw    qcow2  qcow2s
1     22748  4878   4792
2     29557  15839  23144

without
run   raw    qcow2  qcow2s
1     21979  4308   1021
2     26249  13776  24182

Frediano