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(-)

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.
 
-- 
2.20.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>

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

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