Re: [PATCH v4 11/30] qcow2: Add l2_entry_size()

[Vladimir Sementsov-Ogievskiy]

17.03.2020 21:16, Alberto Garcia wrote:
> qcow2 images with subclusters have 128-bit L2 entries. The first 64
> bits contain the same information as traditional images and the last
> 64 bits form a bitmap with the status of each individual subcluster.
>
> Because of that we cannot assume that L2 entries are sizeof(uint64_t)
> anymore. This function returns the proper value for the image.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>   block/qcow2.h          |  9 +++++++++
>   block/qcow2-cluster.c  | 12 ++++++------
>   block/qcow2-refcount.c | 14 ++++++++------
>   block/qcow2.c          |  8 ++++----
>   4 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 06929072d2..1eb4b46807 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -80,6 +80,10 @@
>   
>   #define QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER 32
>   
> +/* Size of normal and extended L2 entries */
> +#define L2E_SIZE_NORMAL   (sizeof(uint64_t))
> +#define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2)
> +
>   #define MIN_CLUSTER_BITS 9
>   #define MAX_CLUSTER_BITS 21
>   
> @@ -506,6 +510,11 @@ static inline bool has_subclusters(BDRVQcow2State *s)
>       return false;
>   }
>   
> +static inline size_t l2_entry_size(BDRVQcow2State *s)
> +{
> +    return has_subclusters(s) ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL;
> +}
> +
>   static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
>                                       int idx)
>   {
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index cd48ab0223..41a23c5305 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -208,7 +208,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
>                      uint64_t l2_offset, uint64_t **l2_slice)
>   {
>       BDRVQcow2State *s = bs->opaque;
> -    int start_of_slice = sizeof(uint64_t) *
> +    int start_of_slice = l2_entry_size(s) *
>           (offset_to_l2_index(s, offset) - offset_to_l2_slice_index(s, offset));
>   
>       return qcow2_cache_get(bs, s->l2_table_cache, l2_offset + start_of_slice,
> @@ -281,7 +281,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index)
>   
>       /* allocate a new l2 entry */
>   
> -    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
> +    l2_offset = qcow2_alloc_clusters(bs, s->l2_size * l2_entry_size(s));

hmm. s->l2_size * l2_entry_size, isn't it just s->cluster_size always? Maybe, 
just refactor these things?


>       if (l2_offset < 0) {
>           ret = l2_offset;
>           goto fail;
> @@ -305,7 +305,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index)

[...]

> @@ -1425,7 +1425,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>           bs->encrypted = true;
>       }
>   
> -    s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
> +    s->l2_bits = s->cluster_bits - ctz32(l2_entry_size(s));
>       s->l2_size = 1 << s->l2_bits;
>       /* 2^(s->refcount_order - 3) is the refcount width in bytes */
>       s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3);
> @@ -4104,7 +4104,7 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>            *  preallocation. All that matters is that we will not have to 
> allocate
>            *  new refcount structures for them.) */
>           nb_new_l2_tables = DIV_ROUND_UP(nb_new_data_clusters,
> -                                        s->cluster_size / sizeof(uint64_t));
> +                                        s->cluster_size / l2_entry_size(s));

Isn't it just s->l2_size ?

>           /* The cluster range may not be aligned to L2 boundaries, so add one 
> L2
>            * table for a potential head/tail */
>           nb_new_l2_tables++;


Conversions looks correct, but how to check that we have converted everything?

Trying at least

   cd block; git grep 'sizeof(uint64_t)' qcow2* | grep -v 'l1_size \*' | grep 
-v 'l1_sz \*' | grep -v refcount | grep -v reftable

I found this not converted chunk:

    /* total size of L2 tables */
    nl2e = aligned_total_size / cluster_size;
    nl2e = ROUND_UP(nl2e, cluster_size / sizeof(uint64_t));
    meta_size += nl2e * sizeof(uint64_t);


Hmm. How to avoid it? Maybe, at least, refactor the code, to drop all 
sizeof(uint64_t), converting them to L2_ENTRY_SIZE, L1_ENTRY_SIZE, 
REFTABLE_ENTRY_SIZE etc?
And all occurrences of pure '8' (not many of them exist)

--
Best regards,
Vladimir