[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta()
From: |
Max Reitz |
Subject: |
Re: [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta() |
Date: |
Wed, 30 Oct 2019 13:04:02 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
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 <address@hidden>
> ---
> 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:
>
signature.asc
Description: OpenPGP digital signature
- [RFC PATCH v2 00/26] Add subcluster allocation to qcow2, Alberto Garcia, 2019/10/26
- [RFC PATCH v2 16/26] qcow2: Add subcluster support to discard_in_l2_slice(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 01/26] qcow2: Add calculate_l2_meta(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 25/26] qcow2: Allow preallocation and backing files if extended_l2 is set, Alberto Garcia, 2019/10/26
- [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_meta(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 21/26] qcow2: Clear the L2 bitmap when allocating a compressed cluster, Alberto Garcia, 2019/10/26
- [RFC PATCH v2 08/26] qcow2: Add offset_to_sc_index(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 06/26] qcow2: Add dummy has_subclusters() function, Alberto Garcia, 2019/10/26
- [RFC PATCH v2 10/26] qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 22/26] qcow2: Add subcluster support to handle_alloc_space(), Alberto Garcia, 2019/10/26
- [RFC PATCH v2 05/26] qcow2: Document the Extended L2 Entries feature, Alberto Garcia, 2019/10/26