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

[Qemu-devel] [PATCH v2 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.

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/group |  1 +
 3 files changed, 100 insertions(+), 17 deletions(-)
 create mode 100755 tests/qemu-iotests/062

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 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

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

[Eric Blake]

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

On 08/30/2013 02:36 AM, Max Reitz wrote:
> 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;

As long as you're touching this code, it might be worth s/for(/for (/ in
the line above.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v2 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/group |  1 +
 2 files changed, 65 insertions(+)
 create mode 100755 tests/qemu-iotests/062

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/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 v2 2/2] qemu-iotests: Snapshotting zero clusters

[Eric Blake]

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

On 08/30/2013 02:36 AM, Max Reitz wrote:
> 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/group |  1 +
>  2 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/062

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Eric Blake]

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

On 08/30/2013 07:07 AM, Eric Blake wrote:
> On 08/30/2013 02:36 AM, Max Reitz wrote:
>> 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/group |  1 +
>>  2 files changed, 65 insertions(+)
>>  create mode 100755 tests/qemu-iotests/062
> 
> ACK.

Sorry, I'm mixing list conventions.  That should be:

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Eric Blake]

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

On 08/30/2013 07:08 AM, Eric Blake wrote:
> On 08/30/2013 07:07 AM, Eric Blake wrote:
>> On 08/30/2013 02:36 AM, Max Reitz wrote:
>>> 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/group |  1 +
>>>  2 files changed, 65 insertions(+)
>>>  create mode 100755 tests/qemu-iotests/062
>>
>> ACK.
> 
> Sorry, I'm mixing list conventions.  That should be:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

and just now, I notice the v3 patch that provides the expected output.
Serves me right for trying to review first thing in the morning before
breakfast.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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