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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-11 at 20:49, Eric Blake wrote:
> On 11/11/2014 09:18 AM, Max Reitz wrote:
>
>>> 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.
> That's okay for now (always better for an initial feature to be
> conservative, then expand it later if there is demand).  But I wonder if
> we could be made smarter in the future and auto-COW any cluster that
> would otherwise exceed max refcount.  Thus, for a refcount_order=0
> (width=1) image, a snapshot now doubles the size of the image (as every
> single cluster would COW into a new cluster) rather than erroring out.
> Food for thought; maybe worth injecting comments into this series
> (whether in code or in commit messages, as appropriate) pointing out
> that we thought about the future possibility even though we chose not to
> allow it for now.

Ah, right, thank you. Yes, that sounds like a good idea, I'll see to it 
at some later point in time.

I think adding comments will be hard because the snapshot functions 
aren't really modified. They just try to increase the refcount and that 
may now fail earlier than it did for a refcount width of 16 bits, so 
there's no real change in behavior there, it's just that it's now 
reasonably possible to hit that case. I will add appropriate comments to 
the test case (which tests this snapshotting issue), though.

Max