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

[Eric Blake]

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

> 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