Re: [RFC PATCH v2 05/26] qcow2: Document the Extended L2 Entries feature

From: Max Reitz <mreitz@redhat.com>
To: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Anton Nefedov <anton.nefedov@virtuozzo.com>,
	qemu-block@nongnu.org,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"Denis V . Lunev" <den@openvz.org>
Subject: Re: [RFC PATCH v2 05/26] qcow2: Document the Extended L2 Entries feature
Date: Wed, 30 Oct 2019 17:23:30 +0100	[thread overview]
Message-ID: <0c738172-732f-eee0-3396-5a9576a4addf@redhat.com> (raw)
In-Reply-To: <57cddd5db9c79406fba70a9bf46c436f6fc9ccd3.1572125022.git.berto@igalia.com>

[-- Attachment #1.1: Type: text/plain, Size: 3909 bytes --]

On 26.10.19 23:25, 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(-)

Sounds good, just a bit of bit nit-picking from my side.

> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..d34261f955 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt

[...]

> @@ -524,6 +535,57 @@ file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>  no backing file or the backing file is smaller than the image, they shall read
>  zeros for all parts that are not covered by the backing file.
>  
> +== 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.
> +
> +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)

I seem to remember that someone proposed this bit ordering to you, but I
wonder why.  So far everything in qcow2 starts from the least
significant bit, for example refcounts (“If refcount_bits implies a
sub-byte width, note that bit 0 means the least significant bit in this
context”), feature bits, and sub-byte structure descriptions in general
(which you reference directly with “bit x”).

Soo...  What’s the reason for doing it the other way around here?

> +        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.
> +                    0: no effect.
> +
> +                    Bits are assigned starting from the most significant one.
> +                    (i.e. bit x is used for subcluster 63 - x)
> +
> +Subcluster Allocation Bitmap (for compressed clusters):

Going from nit-picking to bike shedding: I’d drop the parentheses.

Max

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]