Re: [Qemu-devel] [PATCH v10 03/17] qcow2: Reuse preallocated zero clusters

[Max Reitz]

On 27.04.2017 03:46, Eric Blake wrote:
> From: Max Reitz 
> 
> Instead of just freeing preallocated zero clusters and completely
> allocating them from scratch, reuse them.
> 
> We cannot do this in handle_copied(), however, since this is a COW
> operation. Therefore, we have to add the new logic to handle_alloc() and
> simply return the existing offset if it exists. The only catch is that
> we have to convince qcow2_alloc_cluster_link_l2() not to free the old
> clusters (because we have reused them).
> 
> Reported-by: Eric Blake 
> Signed-off-by: Max Reitz 
> Signed-off-by: Eric Blake 
> 
> ---
> v10: new patch. Max hasn't posted the patch directly on list, but
> did mention it here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg03936.html
> and that he would post it once he has tests. Well, my later patches
> add a test that requires this one :)  The other two patches that
> he mentioned there are also good, but not essential for my series
> (and I didn't want to write tests to expose the behavior difference,
> because it would deprive Max of that fun).

Well, the main reason I didn't send the patches yet is because I was
tired while writing them ("Date: Sat Apr 22 01:17:52 2017 +0200") and I
wanted to take another look before sending them. I guess now's as good a
time as any.

> ---
>  block/qcow2.h |  3 ++
>  block/qcow2-cluster.c | 83 
> +++
>  2 files changed, 60 insertions(+), 26 deletions(-)

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index d1063df..db3d937 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c

[...]

> @@ -1192,31 +1199,53 @@ static int handle_alloc(BlockDriverState *bs, 
> uint64_t guest_offset,
>   * wrong with our code. */
>  assert(nb_clusters > 0);
> 
> +if (!*host_offset && qcow2_get_cluster_type(entry) == QCOW2_CLUSTER_ZERO 
> &&
> +(entry & L2E_OFFSET_MASK) != 0 && (entry & QCOW_OFLAG_COPIED))

*host_offset works with this, too, if
start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK).

If !(entry & QCOW_OFLAG_COPIED), we should check whether the refcount
maybe is 1 and then set OFLAG_COPIED. But that is something we don't
even do for normal clusters yet, so it's something to fix another day.

> +{
> +/* Try to reuse preallocated zero clusters; contiguous normal 
> clusters
> + * would be fine, too, but count_cow_clusters() above has limited
> + * nb_clusters already to a range of COW clusters */
> +int preallocated_nb_clusters =
> +count_contiguous_clusters(nb_clusters, s->cluster_size, l2_table,

s/l2_table/_table[l2_index]/

> +  QCOW_OFLAG_COPIED);
> +
> +if (preallocated_nb_clusters) {

preallocated_nb_clusters must be at least 1, so an assertion would be
better.

Max

> +nb_clusters = preallocated_nb_clusters;
> +alloc_cluster_offset = entry & L2E_OFFSET_MASK;
> +
> +/* We want to reuse these clusters, so 
> qcow2_alloc_cluster_link_l2()
> + * should not free them. */
> +keep_old_clusters = true;
> +}
> +}
> +
>  qcow2_cache_put(bs, s->l2_table_cache, (void **) _table);
> 
> -/* Allocate, if necessary at a given offset in the image file */
> -alloc_cluster_offset = start_of_cluster(s, *host_offset);
> -ret = do_alloc_cluster_offset(bs, guest_offset, _cluster_offset,
> -  _clusters);
> -if (ret < 0) {
> -goto fail;
> -}
> -
> -/* Can't extend contiguous allocation */
> -if (nb_clusters == 0) {
> -*bytes = 0;
> -return 0;
> -}
> -
> -/* !*host_offset would overwrite the image header and is reserved for "no
> - * host offset preferred". If 0 was a valid host offset, it'd trigger the
> - * following overlap check; do that now to avoid having an invalid value 
> in
> - * *host_offset. */
>  if (!alloc_cluster_offset) {
> -ret = qcow2_pre_write_overlap_check(bs, 0, alloc_cluster_offset,
> -nb_clusters * s->cluster_size);
> -assert(ret < 0);
> -goto fail;
> +/* Allocate, if necessary at a given offset in the image file */
> +alloc_cluster_offset = start_of_cluster(s, *host_offset);
> +ret = do_alloc_cluster_offset(bs, guest_offset, 
> _cluster_offset,
> +  _clusters);
> +if (ret < 0) {
> +goto fail;
> +}
> +
> +/* Can't extend contiguous allocation */
> +if (nb_clusters == 0) {
> +*bytes = 0;
> +return 0;
> +}
> +
> +/* !*host_offset would overwrite the image header and is reserved for
> + * "no host offset preferred". If 0 was a valid host offset, it'd
> + * trigger 

[Qemu-devel] [PATCH v10 03/17] qcow2: Reuse preallocated zero clusters

[Eric Blake]

From: Max Reitz 

Instead of just freeing preallocated zero clusters and completely
allocating them from scratch, reuse them.

We cannot do this in handle_copied(), however, since this is a COW
operation. Therefore, we have to add the new logic to handle_alloc() and
simply return the existing offset if it exists. The only catch is that
we have to convince qcow2_alloc_cluster_link_l2() not to free the old
clusters (because we have reused them).

Reported-by: Eric Blake 
Signed-off-by: Max Reitz 
Signed-off-by: Eric Blake 

---
v10: new patch. Max hasn't posted the patch directly on list, but
did mention it here:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg03936.html
and that he would post it once he has tests. Well, my later patches
add a test that requires this one :)  The other two patches that
he mentioned there are also good, but not essential for my series
(and I didn't want to write tests to expose the behavior difference,
because it would deprive Max of that fun).
---
 block/qcow2.h |  3 ++
 block/qcow2-cluster.c | 83 +++
 2 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index f8aeb08..8731f24 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -322,6 +322,9 @@ typedef struct QCowL2Meta
 /** Number of newly allocated clusters */
 int nb_clusters;

+/** Do not free the old clusters */
+bool keep_old_clusters;
+
 /**
  * Requests that overlap with this allocation and wait to be restarted
  * when the allocating request has completed.
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d1063df..db3d937 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -309,14 +309,20 @@ static int count_contiguous_clusters(int nb_clusters, int 
cluster_size,
 uint64_t *l2_table, uint64_t stop_flags)
 {
 int i;
+int first_cluster_type;
 uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
 uint64_t first_entry = be64_to_cpu(l2_table[0]);
 uint64_t offset = first_entry & mask;

-if (!offset)
+if (!offset) {
 return 0;
+}

-assert(qcow2_get_cluster_type(first_entry) == QCOW2_CLUSTER_NORMAL);
+/* must be allocated */
+first_cluster_type = qcow2_get_cluster_type(first_entry);
+assert(first_cluster_type == QCOW2_CLUSTER_NORMAL ||
+   (first_cluster_type == QCOW2_CLUSTER_ZERO &&
+(first_entry & L2E_OFFSET_MASK) != 0));

 for (i = 0; i < nb_clusters; i++) {
 uint64_t l2_entry = be64_to_cpu(l2_table[i]) & mask;
@@ -857,7 +863,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
  * Don't discard clusters that reach a refcount of 0 (e.g. compressed
  * clusters), the next write will reuse them anyway.
  */
-if (j != 0) {
+if (!m->keep_old_clusters && j != 0) {
 for (i = 0; i < j; i++) {
 qcow2_free_any_clusters(bs, be64_to_cpu(old_cluster[i]), 1,
 QCOW2_DISCARD_NEVER);
@@ -1154,8 +1160,9 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 uint64_t entry;
 uint64_t nb_clusters;
 int ret;
+bool keep_old_clusters = false;

-uint64_t alloc_cluster_offset;
+uint64_t alloc_cluster_offset = 0;

 trace_qcow2_handle_alloc(qemu_coroutine_self(), guest_offset, *host_offset,
  *bytes);
@@ -1192,31 +1199,53 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
  * wrong with our code. */
 assert(nb_clusters > 0);

+if (!*host_offset && qcow2_get_cluster_type(entry) == QCOW2_CLUSTER_ZERO &&
+(entry & L2E_OFFSET_MASK) != 0 && (entry & QCOW_OFLAG_COPIED))
+{
+/* Try to reuse preallocated zero clusters; contiguous normal clusters
+ * would be fine, too, but count_cow_clusters() above has limited
+ * nb_clusters already to a range of COW clusters */
+int preallocated_nb_clusters =
+count_contiguous_clusters(nb_clusters, s->cluster_size, l2_table,
+  QCOW_OFLAG_COPIED);
+
+if (preallocated_nb_clusters) {
+nb_clusters = preallocated_nb_clusters;
+alloc_cluster_offset = entry & L2E_OFFSET_MASK;
+
+/* We want to reuse these clusters, so 
qcow2_alloc_cluster_link_l2()
+ * should not free them. */
+keep_old_clusters = true;
+}
+}
+
 qcow2_cache_put(bs, s->l2_table_cache, (void **) _table);

-/* Allocate, if necessary at a given offset in the image file */
-alloc_cluster_offset = start_of_cluster(s, *host_offset);
-ret = do_alloc_cluster_offset(bs, guest_offset, _cluster_offset,
-  _clusters);
-if (ret < 0) {
-goto fail;
-}
-
-/* Can't extend contiguous