[Qemu-devel] [PATCH 0/4] qcow2: Some fixes

[Qemu-devel] [PATCH 0/4] qcow2: Some fixes

[Max Reitz]

This series contains some fixes for issues reported by Eric.


Max Reitz (4):
  qcow2: Fix preallocation size formula
  qcow2: Reuse preallocated zero clusters
  qcow2: Discard preallocated zero clusters
  iotests: Extend test 066

 block/qcow2.h              |   3 ++
 block/qcow2-cluster.c      |  83 ++++++++++++++++++++---------
 block/qcow2.c              |   9 ++--
 tests/qemu-iotests/066     | 128 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/066.out |  46 ++++++++++++++++
 5 files changed, 239 insertions(+), 30 deletions(-)

-- 
2.12.2

[Qemu-devel] [PATCH 1/4] qcow2: Fix preallocation size formula

[Max Reitz]

When calculating the number of reftable entries, we should actually use
the number of refblocks and not (wrongly[1]) re-calculate it.

[1] "Wrongly" means: Dividing the number of clusters by the number of
    entries per refblock and rounding down instead of up.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5c1573c999..f5a72a4d2d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2139,7 +2139,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
          * too, as long as the bulk is allocated here). Therefore, using
          * floating point arithmetic is fine. */
         int64_t meta_size = 0;
-        uint64_t nreftablee, nrefblocke, nl1e, nl2e;
+        uint64_t nreftablee, nrefblocke, nl1e, nl2e, refblock_count;
         int64_t aligned_total_size = align_offset(total_size, cluster_size);
         int refblock_bits, refblock_size;
         /* refcount entry size in bytes */
@@ -2182,11 +2182,12 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         nrefblocke = (aligned_total_size + meta_size + cluster_size)
                    / (cluster_size - rces - rces * sizeof(uint64_t)
                                                  / cluster_size);
-        meta_size += DIV_ROUND_UP(nrefblocke, refblock_size) * cluster_size;
+        refblock_count = DIV_ROUND_UP(nrefblocke, refblock_size);
+        meta_size += refblock_count * cluster_size;
 
         /* total size of refcount tables */
-        nreftablee = nrefblocke / refblock_size;
-        nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t));
+        nreftablee = align_offset(refblock_count,
+                                  cluster_size / sizeof(uint64_t));
         meta_size += nreftablee * sizeof(uint64_t);
 
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
-- 
2.12.2

[Qemu-devel] [PATCH 2/4] qcow2: Reuse preallocated zero clusters

[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 <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h         |  3 ++
 block/qcow2-cluster.c | 80 +++++++++++++++++++++++++++++++++++----------------
 2 files changed, 59 insertions(+), 24 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index f8aeb08794..8731f24b82 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 100398c565..fb91fd8979 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;
@@ -835,7 +841,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);
@@ -1132,8 +1138,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);
@@ -1170,31 +1177,54 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
      * wrong with our code. */
     assert(nb_clusters > 0);
 
-    qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+    if (qcow2_get_cluster_type(entry) == QCOW2_CLUSTER_ZERO &&
+        (entry & L2E_OFFSET_MASK) != 0 && (entry & QCOW_OFLAG_COPIED) &&
+        (!*host_offset ||
+         start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK)))
+    {
+        /* 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[l2_index], QCOW_OFLAG_COPIED);
+        assert(preallocated_nb_clusters > 0);
 
-    /* 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, &alloc_cluster_offset,
-                                  &nb_clusters);
-    if (ret < 0) {
-        goto fail;
-    }
+        nb_clusters = preallocated_nb_clusters;
+        alloc_cluster_offset = entry & L2E_OFFSET_MASK;
 
-    /* Can't extend contiguous allocation */
-    if (nb_clusters == 0) {
-        *bytes = 0;
-        return 0;
+        /* We want to reuse these clusters, so qcow2_alloc_cluster_link_l2()
+         * should not free them. */
+        keep_old_clusters = true;
     }
 
-    /* !*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. */
+    qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+
     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, &alloc_cluster_offset,
+                                      &nb_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;
+        }
     }
 
     /*
@@ -1225,6 +1255,8 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_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),
-- 
2.12.2

[Qemu-devel] [PATCH 3/4] qcow2: Discard preallocated zero clusters

[Max Reitz]

In discard_single_l2(), we completely discard normal clusters instead of
simply turning them into preallocated zero clusters. That means we
should probably do the same with such preallocated zero clusters:
Discard them instead of keeping them allocated.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fb91fd8979..31077d8102 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1511,7 +1511,8 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
                 break;
 
             case QCOW2_CLUSTER_ZERO:
-                if (!full_discard) {
+                /* Preallocated zero clusters should be discarded in any case */
+                if (!full_discard && (old_l2_entry & L2E_OFFSET_MASK) == 0) {
                     continue;
                 }
                 break;
-- 
2.12.2

[Qemu-devel] [PATCH 4/4] iotests: Extend test 066

[Max Reitz]

066 was supposed to be a test "for discarding preallocated zero
clusters", but it did so incompletely: While it did check the image
file's integrity after the operation, it did not confirm that the
clusters are indeed freed. This patch adds this test.

In addition, new cases for writing to preallocated zero clusters are
added.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/066     | 128 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/066.out |  46 ++++++++++++++++
 2 files changed, 173 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
index c2116a3088..8638217736 100755
--- a/tests/qemu-iotests/066
+++ b/tests/qemu-iotests/066
@@ -1,6 +1,6 @@
 #!/bin/bash
 #
-# Test case for discarding preallocated zero clusters in qcow2
+# Test case for preallocated zero clusters in qcow2
 #
 # Copyright (C) 2013 Red Hat, Inc.
 #
@@ -55,8 +55,134 @@ _make_test_img $IMG_SIZE
 $QEMU_IO -c "write 0 256k" -c "write -z 0 256k" -c "write 64M 512" \
 	 -c "discard 0 $IMG_SIZE" -c "read -P 0 0 $IMG_SIZE" "$TEST_IMG" \
          | _filter_qemu_io
+
 # Check the image (there shouldn't be any leaks)
 _check_test_img
+# Map the image (we want all clusters to be gone)
+$QEMU_IMG map "$TEST_IMG"
+
+_cleanup_test_img
+
+
+echo
+echo '=== Writing to preallocated zero clusters ==='
+echo
+
+_make_test_img $IMG_SIZE
+
+# Create data clusters (not aligned to an L2 table)
+$QEMU_IO -c 'write -P 42 1M 256k' "$TEST_IMG" | _filter_qemu_io
+orig_map=$($QEMU_IMG map --output=json "$TEST_IMG")
+
+# Convert the data clusters to preallocated zero clusters
+$QEMU_IO -c 'write -z 1M 256k' "$TEST_IMG" | _filter_qemu_io
+
+# Now write to them (with a COW needed for the head and tail)
+$QEMU_IO -c "write -P 23 $(((1024 + 32) * 1024)) 192k" "$TEST_IMG" \
+    | _filter_qemu_io
+
+# Check metadata correctness
+_check_test_img
+
+# Check data correctness
+$QEMU_IO -c "read -P  0 $(( 1024             * 1024)) 32k" \
+         -c "read -P 23 $(((1024 + 32)       * 1024)) 192k" \
+         -c "read -P  0 $(((1024 + 32 + 192) * 1024)) 32k" \
+         "$TEST_IMG" \
+         | _filter_qemu_io
+
+# Check that we have actually reused the original area
+new_map=$($QEMU_IMG map --output=json "$TEST_IMG")
+if [ "$new_map" = "$orig_map" ]; then
+    echo 'Successfully reused original clusters.'
+else
+    echo 'Failed to reuse original clusters.'
+    echo 'Original map:'
+    echo "$orig_map"
+    echo 'New map:'
+    echo "$new_map"
+fi
+
+_cleanup_test_img
+
+
+echo
+echo '=== Writing to a snapshotted preallocated zero cluster ==='
+echo
+
+_make_test_img 64k
+
+# Create a preallocated zero cluster
+$QEMU_IO -c 'write -P 42 0 64k' -c 'write -z 0 64k' "$TEST_IMG" \
+    | _filter_qemu_io
+
+# Snapshot it
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+
+# Write to the cluster
+$QEMU_IO -c 'write -P 23 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+# Check metadata correctness
+_check_test_img
+
+# Check data correctness
+$QEMU_IO -c 'read -P 23 0 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -a foo "$TEST_IMG"
+$QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+_cleanup_test_img
+
+
+echo
+echo '=== Consecutive write to a preallocated zero cluster ==='
+echo
+
+_make_test_img 192k
+
+# Create three normal clusters
+$QEMU_IO -c 'write -P 42 0 192k' "$TEST_IMG" | _filter_qemu_io
+orig_map=$($QEMU_IMG map --output=json "$TEST_IMG")
+
+# Make the middle cluster a preallocated zero cluster
+$QEMU_IO -c 'write -z 64k 64k' "$TEST_IMG" | _filter_qemu_io
+
+# Try to overwrite everything: This should reuse the whole range. To test that
+# this only issues a single continuous write request, use blkdebug.
+$QEMU_IO -c 'write -P 42 0 192k' \
+    "json:{
+        'driver': '$IMGFMT',
+        'file': {
+            'driver': 'blkdebug',
+            'image.filename': '$TEST_IMG',
+            'set-state': [{
+                'event': 'write_aio',
+                'new_state': 2
+            }],
+            'inject-error': [{
+                'event': 'write_aio',
+                'state': 2
+            }]
+        }
+    }" \
+    | _filter_qemu_io
+
+# Check metadata correctness
+_check_test_img
+
+# Check that we have actually reused the original area
+new_map=$($QEMU_IMG map --output=json "$TEST_IMG")
+if [ "$new_map" = "$orig_map" ]; then
+    echo 'Successfully reused original clusters.'
+else
+    echo 'Failed to reuse original clusters.'
+    echo 'Original map:'
+    echo "$orig_map"
+    echo 'New map:'
+    echo "$new_map"
+fi
+
+_cleanup_test_img
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/066.out b/tests/qemu-iotests/066.out
index 7c1f31a1b1..3d9da9bd0b 100644
--- a/tests/qemu-iotests/066.out
+++ b/tests/qemu-iotests/066.out
@@ -14,4 +14,50 @@ discard 67109376/67109376 bytes at offset 0
 read 67109376/67109376 bytes at offset 0
 64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.
+Offset          Length          Mapped to       File
+
+=== Writing to preallocated zero clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67109376
+wrote 262144/262144 bytes at offset 1048576
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 262144/262144 bytes at offset 1048576
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 196608/196608 bytes at offset 1081344
+192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+read 32768/32768 bytes at offset 1048576
+32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 196608/196608 bytes at offset 1081344
+192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 32768/32768 bytes at offset 1277952
+32 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Successfully reused original clusters.
+
+=== Writing to a snapshotted preallocated zero cluster ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Consecutive write to a preallocated zero cluster ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=196608
+wrote 196608/196608 bytes at offset 0
+192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 196608/196608 bytes at offset 0
+192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+Successfully reused original clusters.
 *** done
-- 
2.12.2

Re: [Qemu-devel] [PATCH 1/4] qcow2: Fix preallocation size formula

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On 05/03/2017 06:11 PM, Max Reitz wrote:
> When calculating the number of reftable entries, we should actually use
> the number of refblocks and not (wrongly[1]) re-calculate it.
> 
> [1] "Wrongly" means: Dividing the number of clusters by the number of
>     entries per refblock and rounding down instead of up.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/4] qcow2: Reuse preallocated zero clusters

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On 05/03/2017 06:11 PM, Max Reitz wrote:
> 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 <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.h         |  3 ++
>  block/qcow2-cluster.c | 80 +++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 59 insertions(+), 24 deletions(-)
> 

>  
> -    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));

I wonder if we should have separate QCOW2_CLUSTER_ZERO_PLAIN and
QCOW2_CLUSTER_ZERO_ALLOCATED, so we don't have to keep checking
first_entry & L2E_OFFSET_MASK.  But I think that can be a separate
followup if we want it.


Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/4] qcow2: Discard preallocated zero clusters

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On 05/03/2017 06:11 PM, Max Reitz wrote:
> In discard_single_l2(), we completely discard normal clusters instead of
> simply turning them into preallocated zero clusters. That means we
> should probably do the same with such preallocated zero clusters:
> Discard them instead of keeping them allocated.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index fb91fd8979..31077d8102 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1511,7 +1511,8 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
>                  break;
>  
>              case QCOW2_CLUSTER_ZERO:
> -                if (!full_discard) {
> +                /* Preallocated zero clusters should be discarded in any case */
> +                if (!full_discard && (old_l2_entry & L2E_OFFSET_MASK) == 0) {

Again, if we had a separate QCOW2_CLUSTER_* for plain vs. pre-allocated
zero, this might be easier to write.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 4/4] iotests: Extend test 066

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

On 05/03/2017 06:11 PM, Max Reitz wrote:
> 066 was supposed to be a test "for discarding preallocated zero
> clusters", but it did so incompletely: While it did check the image
> file's integrity after the operation, it did not confirm that the
> clusters are indeed freed. This patch adds this test.
> 
> In addition, new cases for writing to preallocated zero clusters are
> added.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/066     | 128 ++++++++++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/066.out |  46 ++++++++++++++++
>  2 files changed, 173 insertions(+), 1 deletion(-)
> 
> @@ -55,8 +55,134 @@ _make_test_img $IMG_SIZE
>  $QEMU_IO -c "write 0 256k" -c "write -z 0 256k" -c "write 64M 512" \
>  	 -c "discard 0 $IMG_SIZE" -c "read -P 0 0 $IMG_SIZE" "$TEST_IMG" \
>           | _filter_qemu_io
> +
>  # Check the image (there shouldn't be any leaks)
>  _check_test_img
> +# Map the image (we want all clusters to be gone)
> +$QEMU_IMG map "$TEST_IMG"

The human-readable qemu-img map ignores allocated but reads-as-zeros
clusters.  Maybe it shouldn't, but only --output=json does the right
thing (actually, even then it doesn't, without my pending patch that I
am rebasing on top of your series [1]).

But I think this is sufficient for test 66, since I'm adding more tests
in my pending series.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05224.html


> +# Create three normal clusters
> +$QEMU_IO -c 'write -P 42 0 192k' "$TEST_IMG" | _filter_qemu_io
> +orig_map=$($QEMU_IMG map --output=json "$TEST_IMG")
> +
> +# Make the middle cluster a preallocated zero cluster
> +$QEMU_IO -c 'write -z 64k 64k' "$TEST_IMG" | _filter_qemu_io
> +
> +# Try to overwrite everything: This should reuse the whole range. To test that
> +# this only issues a single continuous write request, use blkdebug.
> +$QEMU_IO -c 'write -P 42 0 192k' \
> +    "json:{
> +        'driver': '$IMGFMT',
> +        'file': {
> +            'driver': 'blkdebug',
> +            'image.filename': '$TEST_IMG',
> +            'set-state': [{
> +                'event': 'write_aio',
> +                'new_state': 2
> +            }],
> +            'inject-error': [{
> +                'event': 'write_aio',
> +                'state': 2
> +            }]
> +        }
> +    }" \
> +    | _filter_qemu_io

blkdebug is cool!

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 4/4] iotests: Extend test 066

[Kevin Wolf]

Am 04.05.2017 um 01:11 hat Max Reitz geschrieben:
> 066 was supposed to be a test "for discarding preallocated zero
> clusters", but it did so incompletely: While it did check the image
> file's integrity after the operation, it did not confirm that the
> clusters are indeed freed. This patch adds this test.
> 
> In addition, new cases for writing to preallocated zero clusters are
> added.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> +echo
> +echo '=== Writing to a snapshotted preallocated zero cluster ==='
> +echo
> +
> +_make_test_img 64k
> +
> +# Create a preallocated zero cluster
> +$QEMU_IO -c 'write -P 42 0 64k' -c 'write -z 0 64k' "$TEST_IMG" \
> +    | _filter_qemu_io
> +
> +# Snapshot it
> +$QEMU_IMG snapshot -c foo "$TEST_IMG"
> +
> +# Write to the cluster
> +$QEMU_IO -c 'write -P 23 0 64k' "$TEST_IMG" | _filter_qemu_io
> +
> +# Check metadata correctness
> +_check_test_img
> +
> +# Check data correctness
> +$QEMU_IO -c 'read -P 23 0 64k' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IMG snapshot -a foo "$TEST_IMG"
> +$QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io

This doesn't actually test that we allocated a new cluster, just that
the zero flag from the snapshot L2 table still takes effect. If we
wanted to test this, we could either add some more snapshotting and
writes in order to cause a second COW of the zero cluster, or at least
do a 'qemu-img map'.

Of course, all of this is perfectly fine as a follow-up.

Kevin

Re: [Qemu-devel] [PATCH 0/4] qcow2: Some fixes

[Kevin Wolf]

Am 04.05.2017 um 01:11 hat Max Reitz geschrieben:
> This series contains some fixes for issues reported by Eric.

Thanks, applied to the block branch (though I think it's a new feature
rather than a fix for the most part).

Kevin