[PATCH] qcow2: Fix preallocation on images with unaligned sizes

[PATCH] qcow2: Fix preallocation on images with unaligned sizes

[Alberto Garcia]

When resizing an image with qcow2_co_truncate() using the falloc or
full preallocation modes the code assumes that both the old and new
sizes are cluster-aligned.

There are two problems with this:

  1) The calculation of how many clusters are involved does not always
     get the right result.

     Example: creating a 60KB image and resizing it (with
     preallocation=full) to 80KB won't allocate the second cluster.

  2) No copy-on-write is performed, so in the previous example if
     there is a backing file then the first 60KB of the first cluster
     won't be filled with data from the backing file.

This patch fixes both issues.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c              | 17 ++++++++++++++---
 tests/qemu-iotests/125     | 21 +++++++++++++++++++++
 tests/qemu-iotests/125.out |  9 +++++++++
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e6757e..e20590c3b7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4239,8 +4239,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
             old_file_size = ROUND_UP(old_file_size, s->cluster_size);
         }
 
-        nb_new_data_clusters = DIV_ROUND_UP(offset - old_length,
-                                            s->cluster_size);
+        nb_new_data_clusters = (ROUND_UP(offset, s->cluster_size) -
+            start_of_cluster(s, old_length)) >> s->cluster_bits;
 
         /* This is an overestimation; we will not actually allocate space for
          * these in the file but just make sure the new refcount structures are
@@ -4317,10 +4317,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
             int64_t nb_clusters = MIN(
                 nb_new_data_clusters,
                 s->l2_slice_size - offset_to_l2_slice_index(s, guest_offset));
-            QCowL2Meta allocation = {
+            unsigned cow_start_length = offset_into_cluster(s, guest_offset);
+            QCowL2Meta allocation;
+            guest_offset = start_of_cluster(s, guest_offset);
+            allocation = (QCowL2Meta) {
                 .offset       = guest_offset,
                 .alloc_offset = host_offset,
                 .nb_clusters  = nb_clusters,
+                .cow_start    = {
+                    .offset       = 0,
+                    .nb_bytes     = cow_start_length,
+                },
+                .cow_end      = {
+                    .offset       = nb_clusters << s->cluster_bits,
+                    .nb_bytes     = 0,
+                },
             };
             qemu_co_queue_init(&allocation.dependent_requests);
 
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index d510984045..5c249b4b23 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -164,6 +164,27 @@ for GROWTH_SIZE in 16 48 80; do
 done
 done
 
+# Test image resizing using preallocation and unaligned offsets
+$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"
+    # The resized image should have 7 clusters:
+    # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
+    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
+    echo
+done
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/125.out b/tests/qemu-iotests/125.out
index 596905f533..7f76f7af20 100644
--- a/tests/qemu-iotests/125.out
+++ b/tests/qemu-iotests/125.out
@@ -767,4 +767,13 @@ wrote 2048000/2048000 bytes at offset 0
 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 ---
+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 ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
 *** done
-- 
2.20.1

Re: [PATCH] qcow2: Fix preallocation on images with unaligned sizes

[Eric Blake]

On 6/10/20 4:46 AM, Alberto Garcia wrote:
> When resizing an image with qcow2_co_truncate() using the falloc or
> full preallocation modes the code assumes that both the old and new
> sizes are cluster-aligned.
> 
> There are two problems with this:
> 
>    1) The calculation of how many clusters are involved does not always
>       get the right result.
> 
>       Example: creating a 60KB image and resizing it (with
>       preallocation=full) to 80KB won't allocate the second cluster.
> 
>    2) No copy-on-write is performed, so in the previous example if
>       there is a backing file then the first 60KB of the first cluster
>       won't be filled with data from the backing file.
> 
> This patch fixes both issues.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.c              | 17 ++++++++++++++---
>   tests/qemu-iotests/125     | 21 +++++++++++++++++++++
>   tests/qemu-iotests/125.out |  9 +++++++++
>   3 files changed, 44 insertions(+), 3 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qcow2: Fix preallocation on images with unaligned sizes

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4013 bytes --]

On 10.06.20 11:46, Alberto Garcia wrote:
> When resizing an image with qcow2_co_truncate() using the falloc or
> full preallocation modes the code assumes that both the old and new
> sizes are cluster-aligned.
> 
> There are two problems with this:
> 
>   1) The calculation of how many clusters are involved does not always
>      get the right result.
> 
>      Example: creating a 60KB image and resizing it (with
>      preallocation=full) to 80KB won't allocate the second cluster.
> 
>   2) No copy-on-write is performed, so in the previous example if
>      there is a backing file then the first 60KB of the first cluster
>      won't be filled with data from the backing file.
> 
> This patch fixes both issues.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c              | 17 ++++++++++++++---
>  tests/qemu-iotests/125     | 21 +++++++++++++++++++++
>  tests/qemu-iotests/125.out |  9 +++++++++
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0cd2e6757e..e20590c3b7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4317,10 +4317,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>              int64_t nb_clusters = MIN(
>                  nb_new_data_clusters,
>                  s->l2_slice_size - offset_to_l2_slice_index(s, guest_offset));
> -            QCowL2Meta allocation = {
> +            unsigned cow_start_length = offset_into_cluster(s, guest_offset);
> +            QCowL2Meta allocation;
> +            guest_offset = start_of_cluster(s, guest_offset);
> +            allocation = (QCowL2Meta) {
>                  .offset       = guest_offset,
>                  .alloc_offset = host_offset,
>                  .nb_clusters  = nb_clusters,
> +                .cow_start    = {
> +                    .offset       = 0,
> +                    .nb_bytes     = cow_start_length,
> +                },
> +                .cow_end      = {
> +                    .offset       = nb_clusters << s->cluster_bits,
> +                    .nb_bytes     = 0,
> +                },

(I don’t think we need to set .cow_end, but well, it doesn’t hurt either.)

Anyway, overall, the code change looks good to me, thanks!

>              };
>              qemu_co_queue_init(&allocation.dependent_requests);
>  
> diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
> index d510984045..5c249b4b23 100755
> --- a/tests/qemu-iotests/125
> +++ b/tests/qemu-iotests/125
> @@ -164,6 +164,27 @@ for GROWTH_SIZE in 16 48 80; do
>  done
>  done
>  
> +# Test image resizing using preallocation and unaligned offsets
> +$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"
> +    # The resized image should have 7 clusters:
> +    # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
> +    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

Just one thing: Can we skip this check if $IMGOPTS has a data_file?
(i.e. if _get_data_file "$TEST_IMG" returns true)

Because if it has, the data clusters will be in that file and this is
bound to fail.

Max


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