Re: [Qemu-devel] [PATCH 05/21] qcow2: Refcount overflow and qcow2_alloc_bytes()

[Max Reitz <mreitz@redhat.com>]

On 2014-11-11 at 17:13, Eric Blake wrote:
> On 11/11/2014 01:22 AM, Max Reitz wrote:
>> On 2014-11-10 at 22:12, Eric Blake wrote:
>>> On 11/10/2014 06:45 AM, Max Reitz wrote:
>>>> qcow2_alloc_bytes() may reuse a cluster multiple times, in which case
>>>> the refcount is increased accordingly. However, if this would lead to an
>>>> overflow the function should instead just not reuse this cluster and
>>>> allocate a new one.
>>> So if recount_order is 1 (2 bits per refcount, max refcount of 4
>> *max refcount of 3 (0b11)
> Oh right, because 0 is special.  Although I think I figured that out...
>
>>> ), and
>>> we encounter the same cluster 6 times (say by 5 back-to-back internal
>>> snapshots), does this code optimize to only 2 clusters (both with
>>> refcount 3) or does it result in each of the last 3 clusters spilling to
> ...when talking about 3 shares of a cluster.
>
>>> its own 1-ref cluster for a total of 4 clusters?  Short of Benoit's work
>>> on deduplication, is there even a way to avoid inefficient use of
>>> spilled clusters?
>> I'm not sure what you're referring to; maybe I should add that
>> qcow2_alloc_bytes() is used for allocating compressed clusters (which
>> ideally don't take up a full host cluster), so "reuse" in this context
>> just means that several compressed clusters share one host cluster.
> No, I was thinking about internal snapshots rather than compressed
> clusters (although there's probably some overlap on what happens).
>
>> Maybe you're referring to the following situation: We have the default
>> cluster size of 64k. Now we're trying to allocate 16k for each of the
>> compressed clusters A, B, C and D. D won't fit into that cluster because
>> the maximum refcount is three, so it will be put into a newly allocated
>> host cluster. Finally, we're trying to allocate 32k for a compressed
>> cluster E, which will then be put into the same cluster as D. We
>> therefore have the following allocation (each sub-box representing 16k):
>>
>> +---+---+---+---+   +---+---+---+---+
>> |A |B | C |   |   | D |   E | |
>> +---+---+---+---+   +---+---+---+---+
>>
>> whereas the ideal allocation would be:
>>
>> +---+---+---+---+   +---+---+---+---+
>> |A |B |   E   |   | C | D | | |
>> +---+---+---+---+   +---+---+---+---+
>>
>> This is a problem, but I think first it's a minor one (just use a
>> sufficiently large refcount width if you're going to use compressed
>> clusters) and second it's about compressed clusters, whose performance I
>> could hardly care less about, frankly.
> No, I was envisioning that we have a brand new image with one cluster
> allocated (cluster 1 has refcount 1), then 5 times in a row we do
> 'savevm' to take an internal snapshot.  If I understand your code
> correctly, the first two snapshots increase the refcount, so cluster 1
> has a refcount of 3. Then the next snapshot can't increase the refcount,
> so it instead copies the contents to cluster 2.

No, it just errors out.

qcow2_alloc_bytes() is only used for allocating space for a compressed 
cluster. When taking a snapshot, update_refcount() will be called to 
increase the clusters' refcounts, and that function will simply throw an 
error.

Max

> The fourth and fifth
> snapshots also see that cluster 1 is full, and allocate cluster 3 and 4;
> whereas a more efficient usage would increase the refcount of cluster 2
> instead of allocating.
>