diff mbox series

[1/2] qcow2: Make preallocate_co() resize the image to the correct size

Message ID adeb8b059917b141d5f5b3bd2a016262d3052c79.1599833007.git.berto@igalia.com
State New
Headers show
Series Make preallocate_co() resize the image to the correct size | expand

Commit Message

Alberto Garcia Sept. 11, 2020, 2:09 p.m. UTC 
This function preallocates metadata structures and then extends the
image to its new size, but that new size calculation is wrong because
it doesn't take into account that the host_offset variable is always
cluster-aligned.

This problem can be reproduced with preallocation=metadata when the
original size is not cluster-aligned but the new size is. In this case
the final image size will be shorter than expected.

   qemu-img create -f qcow2 img.qcow2 31k
   qemu-img resize --preallocation=metadata img.qcow2 128k

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c              |  1 +
 tests/qemu-iotests/125     | 40 +++++++++++++++++++++-----------------
 tests/qemu-iotests/125.out | 28 ++++++++++++++++++++++++--
 3 files changed, 49 insertions(+), 20 deletions(-)

Comments

Max Reitz Sept. 14, 2020, 12:14 p.m. UTC | #1 
On 11.09.20 16:09, Alberto Garcia wrote:
> This function preallocates metadata structures and then extends the
> image to its new size, but that new size calculation is wrong because
> it doesn't take into account that the host_offset variable is always
> cluster-aligned.
> 
> This problem can be reproduced with preallocation=metadata when the
> original size is not cluster-aligned but the new size is. In this case
> the final image size will be shorter than expected.
> 
>    qemu-img create -f qcow2 img.qcow2 31k
>    qemu-img resize --preallocation=metadata img.qcow2 128k
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c              |  1 +
>  tests/qemu-iotests/125     | 40 +++++++++++++++++++++-----------------
>  tests/qemu-iotests/125.out | 28 ++++++++++++++++++++++++--
>  3 files changed, 49 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
Max Reitz Sept. 15, 2020, 9:29 a.m. UTC | #2 
On 11.09.20 16:09, Alberto Garcia wrote:
> This function preallocates metadata structures and then extends the
> image to its new size, but that new size calculation is wrong because
> it doesn't take into account that the host_offset variable is always
> cluster-aligned.
> 
> This problem can be reproduced with preallocation=metadata when the
> original size is not cluster-aligned but the new size is. In this case
> the final image size will be shorter than expected.
> 
>    qemu-img create -f qcow2 img.qcow2 31k
>    qemu-img resize --preallocation=metadata img.qcow2 128k
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c              |  1 +
>  tests/qemu-iotests/125     | 40 +++++++++++++++++++++-----------------
>  tests/qemu-iotests/125.out | 28 ++++++++++++++++++++++++--
>  3 files changed, 49 insertions(+), 20 deletions(-)

The test additions make this test fail with compat=0.10.  Are you OK
with disabling compat=0.10 by squashing this in?

diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index 1f35598b2b..894d53f2bd 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -43,6 +43,10 @@ get_image_size_on_host()

 _supported_fmt qcow2
 _supported_proto file
+# Growing a file with a backing file (without preallocation=full or
+# =falloc) requires zeroing the newly added area, which is impossible
+# to do quickly for v2 images, and hence is unsupported.
+_unsupported_imgopts 'compat=0.10'

 if [ -z "$TEST_IMG_FILE" ]; then
     TEST_IMG_FILE=$TEST_IMG
Alberto Garcia Sept. 15, 2020, 9:37 a.m. UTC | #3 
On Tue 15 Sep 2020 11:29:22 AM CEST, Max Reitz wrote:
> On 11.09.20 16:09, Alberto Garcia wrote:
>> This function preallocates metadata structures and then extends the
>> image to its new size, but that new size calculation is wrong because
>> it doesn't take into account that the host_offset variable is always
>> cluster-aligned.
>> 
>> This problem can be reproduced with preallocation=metadata when the
>> original size is not cluster-aligned but the new size is. In this case
>> the final image size will be shorter than expected.
>> 
>>    qemu-img create -f qcow2 img.qcow2 31k
>>    qemu-img resize --preallocation=metadata img.qcow2 128k
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/qcow2.c              |  1 +
>>  tests/qemu-iotests/125     | 40 +++++++++++++++++++++-----------------
>>  tests/qemu-iotests/125.out | 28 ++++++++++++++++++++++++--
>>  3 files changed, 49 insertions(+), 20 deletions(-)
>
> The test additions make this test fail with compat=0.10.  Are you OK
> with disabling compat=0.10 by squashing this in?

Yes, thanks

Berto
diff mbox series

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 77c43ce178..1cb5daf39a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3135,6 +3135,7 @@  static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
             error_setg_errno(errp, -ret, "Allocating clusters failed");
             goto out;
         }
+        host_offset += offset_into_cluster(s, offset);
 
         for (m = meta; m != NULL; m = m->next) {
             m->prealloc = true;
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index 7cb1c19730..1f35598b2b 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -168,24 +168,28 @@  done
 $QEMU_IMG create -f raw "$TEST_IMG.base" 128k | _filter_img_create
 $QEMU_IO -c 'write -q -P 1 0 128k' -f raw "$TEST_IMG.base"
 for orig_size in 31k 33k; do
-    echo "--- Resizing image from $orig_size to 96k ---"
-    _make_test_img -F raw -b "$TEST_IMG.base" -o cluster_size=64k "$orig_size"
-    $QEMU_IMG resize -f "$IMGFMT" --preallocation=full "$TEST_IMG" 96k
-    # The first part of the image should contain data from the backing file
-    $QEMU_IO -c "read -q -P 1 0 ${orig_size}" "$TEST_IMG"
-    # The resized part of the image should contain zeroes
-    $QEMU_IO -c "read -q -P 0 ${orig_size} 63k" "$TEST_IMG"
-    # If the image does not have an external data file we can also verify its
-    # actual size. The resized image should have 7 clusters:
-    # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
-    if ! _get_data_file "$TEST_IMG" > /dev/null; then
-        expected_file_length=$((65536 * 7))
-        file_length=$(stat -c '%s' "$TEST_IMG_FILE")
-        if [ "$file_length" != "$expected_file_length" ]; then
-            echo "ERROR: file length $file_length (expected $expected_file_length)"
-        fi
-    fi
-    echo
+    for dst_size in 96k 128k; do
+        for prealloc in metadata full; do
+            echo "--- Resizing image from $orig_size to $dst_size (preallocation=$prealloc) ---"
+            _make_test_img -F raw -b "$TEST_IMG.base" -o cluster_size=64k "$orig_size"
+            $QEMU_IMG resize -f "$IMGFMT" --preallocation="$prealloc" "$TEST_IMG" "$dst_size"
+            # The first part of the image should contain data from the backing file
+            $QEMU_IO -c "read -q -P 1 0 ${orig_size}" "$TEST_IMG"
+            # The resized part of the image should contain zeroes
+            $QEMU_IO -c "read -q -P 0 ${orig_size} 63k" "$TEST_IMG"
+            # If the image does not have an external data file we can also verify its
+            # actual size. The resized image should have 7 clusters:
+            # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
+            if ! _get_data_file "$TEST_IMG" > /dev/null; then
+                expected_file_length=$((65536 * 7))
+                file_length=$(stat -c '%s' "$TEST_IMG_FILE")
+                if [ "$file_length" != "$expected_file_length" ]; then
+                    echo "ERROR: file length $file_length (expected $expected_file_length)"
+                fi
+            fi
+            echo
+        done
+    done
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/125.out b/tests/qemu-iotests/125.out
index 7f76f7af20..63a6e9e8a9 100644
--- a/tests/qemu-iotests/125.out
+++ b/tests/qemu-iotests/125.out
@@ -768,11 +768,35 @@  wrote 81920/81920 bytes at offset 2048000
 80 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=raw size=131072
---- Resizing image from 31k to 96k ---
+--- Resizing image from 31k to 96k (preallocation=metadata) ---
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
 Image resized.
 
---- Resizing image from 33k to 96k ---
+--- Resizing image from 31k to 96k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 31k to 128k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 31k to 128k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 96k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 96k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 128k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 128k (preallocation=full) ---
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
 Image resized.