Re: [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta()

[Max Reitz <mreitz@redhat.com>]

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

On 26.10.19 23:25, Alberto Garcia wrote:
> handle_alloc() creates a QCowL2Meta structure in order to update the
> image metadata and perform the necessary copy-on-write operations.
> 
> This patch moves that code to a separate function so it can be used
> from other places.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 24 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8982b7b762..6c1dcdc781 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
>                          QCOW2_DISCARD_NEVER);
>  }
>  
> +/*
> + * For a given write request, create a new QCowL2Meta structure and
> + * add it to @m.
> + *
> + * @host_offset points to the beginning of the first cluster.

(I intended not to comment on such things on an RFC, but here I am...)

I’d call it host_cluster_offset to make clear that it points to a
cluster and isn’t the host offset for @guest_offset.

And now that I’ve gone this far already, I might as well say that I’d
like if it the comment noted that this function not only creates the
L2Meta structure but also adds it to the cluster_allocs list.

> + * @guest_offset and @bytes indicate the offset and length of the
> + * request.
> + *
> + * If @keep_old is true it means that the clusters were already
> + * allocated and will be overwritten. If false then the clusters are
> + * new and we have to decrease the reference count of the old ones.
> + */
> +static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
> +                              uint64_t guest_offset, uint64_t bytes,

And now I’m so deep into non-RFC-level review territory, I might as well
say I’d prefer @bytes to be an unsigned (or maybe even a plain int),
because anything more wouldn’t work.  (Not least because cow_end_to is
an unsigned).

Sorry...

Max

> +                              QCowL2Meta **m, bool keep_old)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    unsigned cow_start_from = 0;
> +    unsigned cow_start_to = offset_into_cluster(s, guest_offset);
> +    unsigned cow_end_from = cow_start_to + bytes;
> +    unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
> +    unsigned nb_clusters = size_to_clusters(s, cow_end_from);
> +    QCowL2Meta *old_m = *m;
> +
> +    *m = g_malloc0(sizeof(**m));
> +    **m = (QCowL2Meta) {
> +        .next           = old_m,
> +
> +        .alloc_offset   = host_offset,
> +        .offset         = start_of_cluster(s, guest_offset),
> +        .nb_clusters    = nb_clusters,
> +
> +        .keep_old_clusters = keep_old,
> +
> +        .cow_start = {
> +            .offset     = cow_start_from,
> +            .nb_bytes   = cow_start_to - cow_start_from,
> +        },
> +        .cow_end = {
> +            .offset     = cow_end_from,
> +            .nb_bytes   = cow_end_to - cow_end_from,
> +        },
> +    };
> +
> +    qemu_co_queue_init(&(*m)->dependent_requests);
> +    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
> +}
> +
>  /*
>   * Returns the number of contiguous clusters that can be used for an allocating
>   * write, but require COW to be performed (this includes yet unallocated space,
> @@ -1417,35 +1466,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
>      uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
>      int avail_bytes = nb_clusters << s->cluster_bits;
>      int nb_bytes = MIN(requested_bytes, avail_bytes);
> -    QCowL2Meta *old_m = *m;
> -
> -    *m = g_malloc0(sizeof(**m));
> -
> -    **m = (QCowL2Meta) {
> -        .next           = old_m,
> -
> -        .alloc_offset   = alloc_cluster_offset,
> -        .offset         = start_of_cluster(s, guest_offset),
> -        .nb_clusters    = nb_clusters,
> -
> -        .keep_old_clusters  = keep_old_clusters,
> -
> -        .cow_start = {
> -            .offset     = 0,
> -            .nb_bytes   = offset_into_cluster(s, guest_offset),
> -        },
> -        .cow_end = {
> -            .offset     = nb_bytes,
> -            .nb_bytes   = avail_bytes - nb_bytes,
> -        },
> -    };
> -    qemu_co_queue_init(&(*m)->dependent_requests);
> -    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
>  
>      *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset);
>      *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
>      assert(*bytes != 0);
>  
> +    calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
> +                      m, keep_old_clusters);
> +
>      return 1;
>  
>  fail:
> 



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