[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qcow2: Fix preallocation on images with unaligned sizes
From: |
Max Reitz |
Subject: |
Re: [PATCH] qcow2: Fix preallocation on images with unaligned sizes |
Date: |
Tue, 16 Jun 2020 17:07:22 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
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
signature.asc
Description: OpenPGP digital signature