[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v3 03/27] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC cluster
From: |
Eric Blake |
Subject: |
Re: [RFC PATCH v3 03/27] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied() |
Date: |
Thu, 20 Feb 2020 08:53:53 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 12/22/19 5:36 AM, Alberto Garcia wrote:
When writing to a qcow2 file there are two functions that take a
virtual offset and return a host offset, possibly allocating new
clusters if necessary:
- handle_copied() looks for normal data clusters that are already
allocated and have a reference count of 1. In those clusters we
can simply write the data and there is no need to perform any
copy-on-write.
- handle_alloc() looks for clusters that do need copy-on-write,
either because they haven't been allocated yet, because their
reference count is != 1 or because they are ZERO_ALLOC clusters.
The ZERO_ALLOC case is a bit special because those are clusters that
are already allocated and they could perfectly be dealt with in
handle_copied() (as long as copy-on-write is performed when required).
In fact, there is extra code specifically for them in handle_alloc()
that tries to reuse the existing allocation if possible and frees them
otherwise.
This patch changes the handling of ZERO_ALLOC clusters so the
semantics of these two functions are now like this:
- handle_copied() looks for clusters that are already allocated and
which we can overwrite (NORMAL and ZERO_ALLOC clusters with a
reference count of 1).
- handle_alloc() looks for clusters for which we need a new
allocation (all other cases).
One importante difference after this change is that clusters found in
important
handle_copied() may now require copy-on-write, but this will be anyway
necessary once we add support for subclusters.
necessary anyway
Signed-off-by: Alberto Garcia <address@hidden>
---
block/qcow2-cluster.c | 226 +++++++++++++++++++++++-------------------
1 file changed, 126 insertions(+), 100 deletions(-)
A bit of an increase in code size, but I think it does reduce the
overall complexity to treat ZERO_ALLOC like normal. The patch is big,
but I don't see any sane way to split it. Overall, I like it.
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index e078bddcc2..9387f15866 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1069,18 +1112,20 @@ static void calculate_l2_meta(BlockDriverState *bs,
QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
}
-/* Returns true if writing to a cluster requires COW */
-static bool cluster_needs_cow(BlockDriverState *bs, uint64_t l2_entry)
+/* Returns true if writing to the cluster pointed to by @l2_entry
+ * requires a new allocation (that is, if the cluster is unallocated
+ * or has refcount > 1 and therefore cannot be written in-place). */
syntax check wants you to wing this comment, now.
+static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry)
The rename makes sense.
@@ -1337,9 +1400,10 @@ static int do_alloc_cluster_offset(BlockDriverState *bs,
uint64_t guest_offset,
}
/*
- * Allocates new clusters for an area that either is yet unallocated or needs a
- * copy on write. If *host_offset is not INV_OFFSET, clusters are only
- * allocated if the new allocation can match the specified host offset.
+ * Allocates new clusters for an area that either is yet unallocated or
+ * cannot be overwritten in-place. If *host_offset is not INV_OFFSET,
s/either is yet/is either still/
+ * clusters are only allocated if the new allocation can match the specified
+ * host offset.
*
* Note that guest_offset may not be cluster aligned. In this case, the
* returned *host_offset points to exact byte referenced by guest_offset and
Findings are minor and you can fix them up when dropping RFC.
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
- Re: [RFC PATCH v3 03/27] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied(),
Eric Blake <=