[Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters

[Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters

[Max Reitz]

When creating a snapshots, the refcount adjustment function basically
adjusted the refcounts of any cluster with a non-zero L2 entry. However,
the L2 entry of unallocated zero clusters is exactly one (therefore
non-zero) which led to wrong refcount updates on cluster 0 (the main
qcow2 header), since the cluster index for these unallocated zero clusters
is 0.

This series fixes this by taking all cluster types specifically into
account using qcow2_get_cluster_type and handling them accordingly.
Furthermore, a short test is added.

v3:
 - forgot 062.out in v2; now added

v2:
 - more thorough commit message
 - take pre-allocated zero clusters into account
 - abort on unknown cluster type
 - a test case

Max Reitz (2):
  qcow2-refcount: Snapshot update for zero clusters
  qemu-iotests: Snapshotting zero clusters

 block/qcow2-refcount.c     | 52 +++++++++++++++++++++++++------------
 tests/qemu-iotests/062     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/062.out |  9 +++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 109 insertions(+), 17 deletions(-)
 create mode 100755 tests/qemu-iotests/062
 create mode 100644 tests/qemu-iotests/062.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/2] qcow2-refcount: Snapshot update for zero clusters

[Max Reitz]

Account for all cluster types in qcow2_update_snapshot_refcounts;
this prevents this function from updating the refcount of unallocated
zero clusters which effectively led to wrong adjustments of the refcount
of cluster 0 (the main qcow2 header). This in turn resulted in images
with (unallocated) zero clusters having a cluster 0 refcount greater
than one after creating a snapshot.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 52 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1244693..a61224a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -861,11 +861,14 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
             }
 
             for(j = 0; j < s->l2_size; j++) {
+                uint64_t cluster_index;
+
                 offset = be64_to_cpu(l2_table[j]);
-                if (offset != 0) {
-                    old_offset = offset;
-                    offset &= ~QCOW_OFLAG_COPIED;
-                    if (offset & QCOW_OFLAG_COMPRESSED) {
+                old_offset = offset;
+                offset &= ~QCOW_OFLAG_COPIED;
+
+                switch (qcow2_get_cluster_type(offset)) {
+                    case QCOW2_CLUSTER_COMPRESSED:
                         nb_csectors = ((offset >> s->csize_shift) &
                                        s->csize_mask) + 1;
                         if (addend != 0) {
@@ -880,8 +883,16 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                         }
                         /* compressed clusters are never modified */
                         refcount = 2;
-                    } else {
-                        uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
+                        break;
+
+                    case QCOW2_CLUSTER_NORMAL:
+                    case QCOW2_CLUSTER_ZERO:
+                        cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
+                        if (!cluster_index) {
+                            /* unallocated */
+                            refcount = 0;
+                            break;
+                        }
                         if (addend != 0) {
                             refcount = update_cluster_refcount(bs, cluster_index, addend,
                                                                QCOW2_DISCARD_SNAPSHOT);
@@ -893,19 +904,26 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                             ret = refcount;
                             goto fail;
                         }
-                    }
+                        break;
 
-                    if (refcount == 1) {
-                        offset |= QCOW_OFLAG_COPIED;
-                    }
-                    if (offset != old_offset) {
-                        if (addend > 0) {
-                            qcow2_cache_set_dependency(bs, s->l2_table_cache,
-                                s->refcount_block_cache);
-                        }
-                        l2_table[j] = cpu_to_be64(offset);
-                        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+                    case QCOW2_CLUSTER_UNALLOCATED:
+                        refcount = 0;
+                        break;
+
+                    default:
+                        abort();
+                }
+
+                if (refcount == 1) {
+                    offset |= QCOW_OFLAG_COPIED;
+                }
+                if (offset != old_offset) {
+                    if (addend > 0) {
+                        qcow2_cache_set_dependency(bs, s->l2_table_cache,
+                            s->refcount_block_cache);
                     }
+                    l2_table[j] = cpu_to_be64(offset);
+                    qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
                 }
             }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/2] qemu-iotests: Snapshotting zero clusters

[Max Reitz]

This test creates an image with unallocated zero clusters, then creates
a snapshot. Afterwards, there should be neither any errors nor leaks.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/062     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/062.out |  9 +++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 74 insertions(+)
 create mode 100755 tests/qemu-iotests/062
 create mode 100644 tests/qemu-iotests/062.out

diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
new file mode 100755
index 0000000..0511246
--- /dev/null
+++ b/tests/qemu-iotests/062
@@ -0,0 +1,64 @@
+#!/bin/bash
+#
+# Test case for snapshotting images with unallocated zero clusters in
+# qcow2
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+IMGOPTS="compat=1.1"
+IMG_SIZE=64M
+
+echo
+echo "=== Testing snapshotting an image with zero clusters ==="
+echo
+_make_test_img $IMG_SIZE
+# Write some zero clusters
+$QEMU_IO -c "write -z 0 256k" "$TEST_IMG" | _filter_qemu_io
+# Create a snapshot
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+# Check the image (there shouldn't be any errors or leaks)
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/062.out b/tests/qemu-iotests/062.out
new file mode 100644
index 0000000..442d761
--- /dev/null
+++ b/tests/qemu-iotests/062.out
@@ -0,0 +1,9 @@
+QA output created by 062
+
+=== Testing snapshotting an image with zero clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 43c05d6..de53b9e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -64,3 +64,4 @@
 055 rw auto
 056 rw auto backing
 059 rw auto
+062 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters

[Kevin Wolf]

Am 30.08.2013 um 10:40 hat Max Reitz geschrieben:
> When creating a snapshots, the refcount adjustment function basically
> adjusted the refcounts of any cluster with a non-zero L2 entry. However,
> the L2 entry of unallocated zero clusters is exactly one (therefore
> non-zero) which led to wrong refcount updates on cluster 0 (the main
> qcow2 header), since the cluster index for these unallocated zero clusters
> is 0.
> 
> This series fixes this by taking all cluster types specifically into
> account using qcow2_get_cluster_type and handling them accordingly.
> Furthermore, a short test is added.
> 
> v3:
>  - forgot 062.out in v2; now added
> 
> v2:
>  - more thorough commit message
>  - take pre-allocated zero clusters into account
>  - abort on unknown cluster type
>  - a test case
> 
> Max Reitz (2):
>   qcow2-refcount: Snapshot update for zero clusters
>   qemu-iotests: Snapshotting zero clusters
> 
>  block/qcow2-refcount.c     | 52 +++++++++++++++++++++++++------------
>  tests/qemu-iotests/062     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/062.out |  9 +++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 109 insertions(+), 17 deletions(-)
>  create mode 100755 tests/qemu-iotests/062
>  create mode 100644 tests/qemu-iotests/062.out

Thanks, applied to the block branch.

Kevin