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 <[email protected]> > --- > 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
