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

[Eric Blake <eblake@redhat.com>]

On 12/22/19 5:36 AM, Alberto Garcia wrote:
> Subcluster allocation in qcow2 is implemented by extending the
> existing L2 table entries and adding additional information to
> indicate the allocation status of each subcluster.
> 
> This patch documents the changes to the qcow2 format and how they
> affect the calculation of the L2 cache size.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   docs/interop/qcow2.txt | 68 ++++++++++++++++++++++++++++++++++++++++--
>   docs/qcow2-cache.txt   | 19 +++++++++++-
>   2 files changed, 83 insertions(+), 4 deletions(-)

This adds a new feature bit; where is the corresponding patch to qcow2.c 
to advertise the feature bit name in the optional feature name table?

/me reads ahead

good, patch 25 covers it.  Quick comment added there as a result.


> +== Extended L2 Entries ==
> +
> +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.

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?

> +
> +The size of an extended L2 entry is 128 bits so the number of entries per table
> +is calculated using this formula:
> +
> +    l2_entries = (cluster_size / (2 * sizeof(uint64_t)))
> +
> +The first 64 bits have the same format as the standard L2 table entry described
> +in the previous section, with the exception of bit 0 of the standard cluster
> +descriptor.
> +
> +The last 64 bits contain a subcluster allocation bitmap with this format:
> +
> +Subcluster Allocation Bitmap (for standard clusters):
> +
> +    Bit  0 -  31:   Allocation status (one bit per subcluster)
> +
> +                    1: the subcluster is allocated. In this case the
> +                       host cluster offset field must contain a valid
> +                       offset.
> +                    0: the subcluster is not allocated. In this case
> +                       read requests shall go to the backing file or
> +                       return zeros if there is no backing file data.
> +
> +                    Bits are assigned starting from the most significant one.
> +                    (i.e. bit x is used for subcluster 31 - x)
> +

Missing trailing '.'

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

> +                    0: no effect.
> +
> +                    Bits are assigned starting from the most significant one.
> +                    (i.e. bit x is used for subcluster 63 - x)

and again.

> +
> +Subcluster Allocation Bitmap (for compressed clusters):
> +
> +    Bit  0 -  63:   Reserved (set to 0)
> +                    Compressed clusters don't have subclusters,
> +                    so this field is not used.
>   
>   == Snapshots ==
>   
> diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
> index d57f409861..04eb4ce2f1 100644
> --- a/docs/qcow2-cache.txt
> +++ b/docs/qcow2-cache.txt
> @@ -1,6 +1,6 @@
>   qcow2 L2/refcount cache configuration
>   =====================================
> -Copyright (C) 2015, 2018 Igalia, S.L.
> +Copyright (C) 2015, 2018-2019 Igalia, S.L.

Our review is late; you could add 2020 if desired, now.

>   Author: Alberto Garcia <berto@igalia.com>
>   
>   This work is licensed under the terms of the GNU GPL, version 2 or
> @@ -222,3 +222,20 @@ support this functionality, and is 0 (disabled) on other platforms.
>   This functionality currently relies on the MADV_DONTNEED argument for
>   madvise() to actually free the memory. This is a Linux-specific feature,
>   so cache-clean-interval is not supported on other systems.
> +
> +
> +Extended L2 Entries
> +-------------------
> +All numbers shown in this document are valid for qcow2 images with normal
> +64-bit L2 entries.
> +
> +Images with extended L2 entries need twice as much L2 metadata, so the L2
> +cache size must be twice as large for the same disk space.
> +
> +   disk_size = l2_cache_size * cluster_size / 16
> +
> +i.e.
> +
> +   l2_cache_size = disk_size * 16 / cluster_size
> +
> +Refcount blocks are not affected by this.
> 

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