[Qemu-devel] [PATCH 0/2] Correct bitmap size in zero cluster expansion

[Qemu-devel] [PATCH 0/2] Correct bitmap size in zero cluster expansion

[Max Reitz]

The current version of the zero cluster expansion uses the guest disk
size for determining the size of the expanded_clusters bitmap, however,
it is addressed using host offsets. This leads to an assertion failing if
the host image size exceeds the guest disk size. This is fixed by using
the host image size instead for allocating the bitmap.

This however uncovers another problem: If the host image is growable, it
may grow during the zero cluster expansion due to cluster allocations. If
this happens, the bitmap has to be resized accordingly.

Max Reitz (2):
  qcow2: Correct bitmap size in zero expansion
  qemu-iotests: Preallocated zero clusters in 061

 block/qcow2-cluster.c      | 38 +++++++++++++++++++++++++++-----------
 tests/qemu-iotests/061     |  9 +++++++++
 tests/qemu-iotests/061.out | 11 +++++++++++
 3 files changed, 47 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] qcow2: Correct bitmap size in zero expansion

[Max Reitz]

Since the expanded_clusters bitmap is addressed using host offsets in
the underlying image file, the correct size to use for allocating the
bitmap is not determined by the guest disk image but by the underlying
host image file.

Furthermore, this size may change during the expansion due to cluster
allocations on growable image files. In this case, the bitmap needs to
be resized as well to reflect the growth.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 738ff73..a8e17f1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1509,8 +1509,8 @@ fail:
  * i.e., the number of bits in expanded_clusters.
  */
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
-                                      int l1_size, uint8_t *expanded_clusters,
-                                      uint64_t nb_clusters)
+                                      int l1_size, uint8_t **expanded_clusters,
+                                      uint64_t *nb_clusters)
 {
     BDRVQcowState *s = bs->opaque;
     bool is_active_l1 = (l1_table == s->l1_table);
@@ -1553,8 +1553,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
 
             if (cluster_type == QCOW2_CLUSTER_NORMAL) {
                 cluster_index = offset >> s->cluster_bits;
-                assert((cluster_index >= 0) && (cluster_index < nb_clusters));
-                if (expanded_clusters[cluster_index / 8] &
+                assert((cluster_index >= 0) && (cluster_index < *nb_clusters));
+                if ((*expanded_clusters)[cluster_index / 8] &
                     (1 << (cluster_index % 8))) {
                     /* Probably a shared L2 table; this cluster was a zero
                      * cluster which has been expanded, its refcount
@@ -1612,8 +1612,25 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             l2_dirty = true;
 
             cluster_index = offset >> s->cluster_bits;
-            assert((cluster_index >= 0) && (cluster_index < nb_clusters));
-            expanded_clusters[cluster_index / 8] |= 1 << (cluster_index % 8);
+
+            if (cluster_index >= *nb_clusters) {
+                uint64_t old_bitmap_size = (*nb_clusters + 7) / 8;
+                uint64_t new_bitmap_size;
+                /* The offset may lie beyond the old end of the underlying image
+                 * file for growable files only */
+                assert(bs->file->growable);
+                *nb_clusters = size_to_clusters(s, bs->file->total_sectors *
+                                                BDRV_SECTOR_SIZE);
+                new_bitmap_size = (*nb_clusters + 7) / 8;
+                *expanded_clusters = g_realloc(*expanded_clusters,
+                                               new_bitmap_size);
+                /* clear the newly allocated space */
+                memset(&(*expanded_clusters)[old_bitmap_size], 0,
+                       new_bitmap_size - old_bitmap_size);
+            }
+
+            assert((cluster_index >= 0) && (cluster_index < *nb_clusters));
+            (*expanded_clusters)[cluster_index / 8] |= 1 << (cluster_index % 8);
         }
 
         if (is_active_l1) {
@@ -1672,18 +1689,17 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t *l1_table = NULL;
-    int cluster_to_sector_bits = s->cluster_bits - BDRV_SECTOR_BITS;
     uint64_t nb_clusters;
     uint8_t *expanded_clusters;
     int ret;
     int i, j;
 
-    nb_clusters = (bs->total_sectors + (1 << cluster_to_sector_bits) - 1)
-            >> cluster_to_sector_bits;
+    nb_clusters = size_to_clusters(s, bs->file->total_sectors *
+                                   BDRV_SECTOR_SIZE);
     expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
 
     ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size,
-                                     expanded_clusters, nb_clusters);
+                                     &expanded_clusters, &nb_clusters);
     if (ret < 0) {
         goto fail;
     }
@@ -1717,7 +1733,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
         }
 
         ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size,
-                                         expanded_clusters, nb_clusters);
+                                         &expanded_clusters, &nb_clusters);
         if (ret < 0) {
             goto fail;
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] qemu-iotests: Preallocated zero clusters in 061

[Max Reitz]

Add a test case for zero cluster expansion on an image completely filled
with preallocated zero clusters to test 061.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/061     |  9 +++++++++
 tests/qemu-iotests/061.out | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 5f04bfa..fa9319d 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -200,6 +200,15 @@ $QEMU_IMG snapshot -a foo "$TEST_IMG"
 _check_test_img
 $QEMU_IO -c "read -P 0 0 128k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing preallocated zero expansion on full image ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG" _make_test_img 64M
+$QEMU_IO -c "write -P 0x2a 0 64M" "$TEST_IMG" -c "write -z 0 64M" | _filter_qemu_io
+$QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+$QEMU_IO -c "read -P 0 0 64M" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index d42127f..4027e00 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -373,4 +373,15 @@ read 131072/131072 bytes at offset 0
 No errors were found on the image.
 read 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing preallocated zero expansion on full image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 67108864/67108864 bytes at offset 0
+64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 67108864/67108864 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.
+read 67108864/67108864 bytes at offset 0
+64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] Correct bitmap size in zero cluster expansion

[Kevin Wolf]

Am 25.09.2013 um 12:07 hat Max Reitz geschrieben:
> The current version of the zero cluster expansion uses the guest disk
> size for determining the size of the expanded_clusters bitmap, however,
> it is addressed using host offsets. This leads to an assertion failing if
> the host image size exceeds the guest disk size. This is fixed by using
> the host image size instead for allocating the bitmap.
> 
> This however uncovers another problem: If the host image is growable, it
> may grow during the zero cluster expansion due to cluster allocations. If
> this happens, the bitmap has to be resized accordingly.
> 
> Max Reitz (2):
>   qcow2: Correct bitmap size in zero expansion
>   qemu-iotests: Preallocated zero clusters in 061
> 
>  block/qcow2-cluster.c      | 38 +++++++++++++++++++++++++++-----------
>  tests/qemu-iotests/061     |  9 +++++++++
>  tests/qemu-iotests/061.out | 11 +++++++++++
>  3 files changed, 47 insertions(+), 11 deletions(-)

Thanks, applied to the block branch.

Kevin