Re: [RFC PATCH v3 07/27] qcow2: Add subcluster-related fields to BDRVQcow2State

[Eric Blake]

On 12/22/19 5:36 AM, Alberto Garcia wrote:
> This patch adds the following new fields to BDRVQcow2State:
>
> - subclusters_per_cluster: Number of subclusters in a cluster
> - subcluster_size: The size of each subcluster, in bytes
> - subcluster_bits: No. of bits so 1 << subcluster_bits = subcluster_size
>
> Images without subclusters are treated as if they had exactly one,
> with subcluster_size = cluster_size.

The qcow2 spec changes earlier in the series made it sound like your 
choices are exactly 1 or 32,

> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>   block/qcow2.c | 5 +++++
>   block/qcow2.h | 5 +++++
>   2 files changed, 10 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3866b47946..cbd857e9c7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1378,6 +1378,11 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>           }
>       }
>   
> +    s->subclusters_per_cluster =
> +        has_subclusters(s) ? QCOW_MAX_SUBCLUSTERS_PER_CLUSTER : 1;

which matches your code here (other than the name of the constant)...

> +    s->subcluster_size = s->cluster_size / s->subclusters_per_cluster;
> +    s->subcluster_bits = ctz32(s->subcluster_size);
> +
>       /* Check support for various header values */
>       if (header.refcount_order > 6) {
>           error_setg(errp, "Reference count entry width too large; may not "
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 1db3fc5dbc..941330cfc9 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -78,6 +78,8 @@
>   /* The cluster reads as all zeros */
>   #define QCOW_OFLAG_ZERO (1ULL << 0)
>   
> +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32
> +

...but this name sounds like other values (2, 4, 8, 16) might be 
possible?  Is this just leftovers from earlier spins of the series 
before we decided to mandate that clusters must be at least 16k if 
subclusters are enabled (so that subclusters are at least 512 bytes)?

Once we get the right name for the constant, the rest of the patch makes 
sense.

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