[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [RFC PATCH v3 05/27] qcow2: Document the Extended L2 Entries feature
Date: Thu, 20 Feb 2020 09:16:12 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 2/20/20 8:49 AM, Alberto Garcia wrote:
On Thu 20 Feb 2020 03:28:17 PM CET, Eric Blake wrote:
+An image uses Extended L2 Entries if bit 3 is set on the incompatible_features
+field of the header.
+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 
+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.


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.

By the way, this series treats an L2 entry as invalid if any of those
bits is not zero, but I think I'll change that. Conceivably those bits
could be used for a future compatible feature, but it can only be
compatible if the previous versions ignore those bits.

+        32 -  63    Subcluster reads as zeros (one bit per subcluster)
+                    1: the subcluster reads as zeros. In this case the
+                       allocation status bit must be unset. The host
+                       cluster offset field may or may not be set.

Why must the allocation bit be unset?  When we preallocate, we want a
cluster to reserve space, but still read as zero, so the combination
of both bits set makes sense to me.

Since 00 means unallocated and 01 allocated, there are two options left
to represent the "reads as zero" case: 10 and 11.

I think that one could argue for either one and there is no "right"
choice. I chose the former because I understood the allocation bit as
"the guest visible data is obtained from the raw data in that
subcluster" but the other option also makes sense.

My argument is that BOTH bit settings make sense:

10 - reads as zero, but subcluster is not allocated
11 - reads as zero, and subcluster is allocated

Oh, I see. I'm getting confused on the meanings of "allocated". Meaning 1: a host address is reserved for the guest address (pre-allocation sense). Meaning 2: guest reads come from this layer rather than from the backing layer (COW/COR sense).

Pre-allocation is ALWAYS done a cluster at a time (you only have ONE host offset, shared among all 32 subclusters, per L2 entry), so either all 32 subclusters have a preallocated location, or none of them do. What is left, then, is a determination of whether to read locally or from the backing file, AND when reading locally, whether to read from the pre-allocated space or to just read zeroes.

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)
  0      1    0     cluster unallocated, subcluster reads as zero
  0      1    1     error (except maybe for external data file)
 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

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? 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.

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.

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.

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]