Re: [PATCH v4 20/30] qcow2: Add subcluster support to discard_in_l2_slice()

[Max Reitz]

On 10.04.20 14:47, Alberto Garcia wrote:
> On Thu 09 Apr 2020 12:05:12 PM CEST, Max Reitz wrote:
>>>          switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
>>>          case QCOW2_CLUSTER_UNALLOCATED:
>>> -            if (full_discard || !bs->backing) {
>>> +            if (full_discard) {
>>> +                /* If the image has extended L2 entries we can only
>>> +                 * skip this operation if the L2 bitmap is zero. */
>>> +                uint64_t bitmap = has_subclusters(s) ?
>>> +                    get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
>>
>> Isn’t this bitmap only valid for standard clusters?  In this case, the
>> whole cluster is unallocated, so the bitmap shouldn’t be relevant,
>> AFAIU.
> 
> I'm not sure if I follow you.
> 
> An unallocated cluster can still have QCOW_OFLAG_SUB_ZERO set in some of
> its subclusters. Those read as zeroes and the rest go to the backing
> file.

Hm, right, this is the only way to have non-preallocated zero clusters
after all.

I suppose I read the spec wrong and assumed somehow that unallocated
clusters don’t use “standard cluster descriptors”, so their bitmap usage
would be undefined.  Don’t know how that happened.

> After a full discard all subclusters should be completely deallocated so
> those bits should be cleared.
> 
> If the bitmap is already 0 (the whole cluster is already unallocated) or
> if the image does not have extended L2 entries (which also means that
> the whole cluster is already unallocated) then we can skip the discard.

Yep, seems right.

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature