Re: [Qemu-devel] [PATCH for-2.12] qcow2: Reset free_cluster_index when allocating a new refcount block

[Eric Blake <eblake@redhat.com>]

On 03/21/2018 04:28 AM, Alberto Garcia wrote:
> On Tue 20 Mar 2018 06:54:15 PM CET, Eric Blake wrote:
> 
>>> When we try to allocate new clusters we first look for available ones
>>> starting from s->free_cluster_index and once we find them we increase
>>> their reference counts. Before we get to call update_refcount() to do
>>> this last step s->free_cluster_index is already pointing to the next
>>> cluster after the ones we are trying to allocate.
>>>
>>   > During update_refcount() it may happen however that we also need to
>>   > allocate a new refcount block in order to store the refcounts of
>>   > these new clusters
>>
>> Your changes to test 121 covers this...
>>
>>   > (and to complicate things further that may also require us to grow
>>   > the refcount table).
>>
>> ...but not this.  Is it worth also trying to cover this case in the
>> testsuite as well?
> 
> I checked and the patch doesn't really fix that scenario. There's a
> different problem that I haven't debugged completely yet, because of two
> reasons:
> 
>   - One difference is that when we grow the refcount table we actually
>     allocate a new one, so s->free_cluster_index points to the beginning
>     of the image (where the previous table was) and any holes left during
>     the process are allocated after that (depending on how much data we
>     write though).

Yeah, that can make the test harder to reason about, and is slightly 
more sensitive to our internal algorithm - but it also explains why you 
checked for index > start rather than unconditionally assigning index = 
start.

> 
>   - This scenario is harder to reach: in order to fill a 1-cluster
>     refcount table the size of the image needs to be larger than
>     (cluster_size³ / refcount_bits) bytes, that's 16TB with the default
>     parameters. So although it can be reproduced easily if you reduce the
>     cluster size I think it's very infrequent under normal conditions.

Yes, 16TB for default size, but only 2M for the best-case (512-byte 
cluster, 64-bit refcount), so still easy to write a test for.

> 
> But yes, it's a task left for the future.

Fair enough.

> 
>>> +            /* If the caller needs to restart the search for free clusters,
>>> +             * try the same ones first to see if they're still free. */
>>> +            if (ret == -EAGAIN) {
>>> +                if (s->free_cluster_index > (start >> s->cluster_bits)) {
>>> +                    s->free_cluster_index = (start >> s->cluster_bits);
>>> +                }
>>
>> Is there any harm in making this assignment unconditional, instead of
>> only doing it when free_cluster_index has grown larger than start?
> 
> It can happen that it is smaller than 'start' if we were moving the
> refcount table to a new location, so we want to keep the lowest value.

Okay, then my R-b is sufficient on the patch as-is.

> 
>> [And unrelated, but it might be nice to do a followup cleanup to track
>> free_cluster_offset by bytes instead of having to shift
>> free_cluster_index everywhere]
> 
> I've actually just seen that we already have free_byte_offset, we use
> that for compressed clusters, so it might be possible to use that one...

free_byte_offset really IS a byte offset, as it can point mid-cluster. 
But having EVERYTHING be byte-based seems like it will make reasoning 
about the code easier to do.

> 
> I'll put that in my TODO list.
> 
> Berto
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org