Re: [RFC PATCH v3 05/27] qcow2: Document the Extended L2 Entries feature

[Alberto Garcia]

On Thu 20 Feb 2020 04:16:12 PM CET, Eric Blake wrote:

>>>> +In these images standard data clusters are divided into 32 subclusters of 
>>>> the
>>>> +same size. They are contiguous and start from the beginning of the 
>>>> cluster.
>>>> +Subclusters can be allocated independently and the L2 entry contains 
>>>> information
>>>> +indicating the status of each one of them. Compressed data clusters don't 
>>>> have
>>>> +subclusters so they are treated like in images without this feature.
>>>
>>> Grammar; I'd suggest:
>>>
>>> ...don't have subclusters, so they are treated the same as in images
>>> without this feature.
>> 
>> Ok
>> 
>>> Are they truly the same, or do you still need to document that the
>>> extra 64 bits of the extended L2 entry are all zero?
>> 
>> It is documented later in the same patch ("Subcluster Allocation
>> Bitmap for compressed clusters").
>
> Yes, I saw the mention later.  I'm just wondering if we need to
> rearrange text to mention that the bits are reserved (set to 0, ignore
> on read) closer to the point where we document compressed clusters
> have no subclusters.

When I say that "compressed data clusters are treated the same as in
images without this feature" I mean that there are no semantic
changes. I don't think it's necessary to add anything else considering
that the sentence immediately after that one says that the L2 entry size
is now 128 bits, so it's not hard to guess that compressed cluster
descriptors must somehow be affected by this.

> We have 8 potential combinations (not all make sense):
>
> host   zero alloc
>    0      0    0     cluster unallocated, subcluster defers to backing
>    0      0    1     error (except maybe for external data file)

Correct (without the 'maybe')

>    0      1    0     cluster unallocated, subcluster reads as zero
>    0      1    1     error (except maybe for external data file)

This is an error in all cases.

>   addr    0    0     cluster allocated, subcluster defers to backing
>   addr    0    1     cluster allocated, subcluster reads from host
>   addr    1    0     cluster allocated, subcluster reads as zero
>   addr    1    1   error, or cluster allocated, subcluster reads as zero

The last one is also an error.

> Hmm - normally addr is non-zero (because the 0 addr is the metadata 
> cluster of qcow2), but with external data file, host addr 0 is required 
> for guest offset 0. How do subclusters play with external data files?

No difference:

    /* ... */ if (!(l2_entry & L2E_OFFSET_MASK)) {
        /* Offset 0 generally means unallocated, but it is ambiguous with
         * external data files because 0 is a valid offset there. However, all
         * clusters in external data files always have refcount 1, so we can
         * rely on QCOW_OFLAG_COPIED to disambiguate. */
        if (has_data_file(bs) && (l2_entry & QCOW_OFLAG_COPIED)) {
            return QCOW2_CLUSTER_NORMAL;
        } else {
            return QCOW2_CLUSTER_UNALLOCATED;
        }
    } /* ... */

This code doesn't change if there are subclusters, and is still used to
determine whether a cluster is allocated or not, and therefore whether
the subcluster allocation bits need to be checked or not.

> It makes sense to still have subclusters read as 0 or defer to backing
> with an external file (except maybe when raw external file is set).
> But you did word it as if the alloc bit is set, the "host cluster
> offset field must contain a valid offset" which includes an offset of
> 0 for external data file.

Yes, that is possible with subclusters (unless there's a bug).

> If we mandate 10 for the reads-as-zero form, then whether addr is
> valid is irrelevant. If we mandate 11 for the reads-as-zero form, then
> addr must be valid even though we don't reference addr.  Having
> written all that, I agree that either form should work, but also that
> mandating one form leaves the door open for a future extension to
> define meaning to the form we did not permit (that is, either 10 or 11
> becomes a reserved pattern that we can later give meaning to),
> vs. allowing both forms now and locking ourselves out of a future
> meaning.  And mandating addr to be valid even when reading zeroes
> doesn't use addr feels odd.

Yes, we definitely don't want to make 10 and 11 synonymous. One of them
should return an error and maybe in the future we can think of a new
meaning.

> So, I'm okay with your choice of picking 00, 01, and 10 as the
> mandated forms, and declaring 11 as invalid for now (but a possible
> future extension).  Maybe I'll change my mind when seeing what
> complexity it adds to the qcow2 reference implementation, but
> hopefully not.

>From the implementation point of view there's no difference in
complexity.

Berto