* [Qemu-devel] [PATCH v3 1/2] qcow2-refcount: Snapshot update for zero clusters
2013-08-30 8:40 [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters Max Reitz
@ 2013-08-30 8:40 ` Max Reitz
2013-08-30 8:40 ` [Qemu-devel] [PATCH v3 2/2] qemu-iotests: Snapshotting " Max Reitz
2013-08-30 9:47 ` [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for " Kevin Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Max Reitz @ 2013-08-30 8:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH v3 2/2] qemu-iotests: Snapshotting zero clusters
2013-08-30 8:40 [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters Max Reitz
2013-08-30 8:40 ` [Qemu-devel] [PATCH v3 1/2] qcow2-refcount: " Max Reitz
@ 2013-08-30 8:40 ` Max Reitz
2013-08-30 9:47 ` [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for " Kevin Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Max Reitz @ 2013-08-30 8:40 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters
2013-08-30 8:40 [Qemu-devel] [PATCH v3 0/2] qcow2: Snapshot update for zero clusters Max Reitz
2013-08-30 8:40 ` [Qemu-devel] [PATCH v3 1/2] qcow2-refcount: " Max Reitz
2013-08-30 8:40 ` [Qemu-devel] [PATCH v3 2/2] qemu-iotests: Snapshotting " Max Reitz
@ 2013-08-30 9:47 ` Kevin Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Kevin Wolf @ 2013-08-30 9:47 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 4+ messages in thread