[PATCH 0/2] qemu-img: fix getting stuck in infinite loop on

[PATCH 0/2] qemu-img: fix getting stuck in infinite loop on

[Andrey Drobyshev via]

Consider the following in-chain rebase case:

qemu-img create -f qcow2 base.qcow2 $(( 64 * 4 ))k
qemu-img create -f qcow2 -o backing_file=base.qcow2,backing_fmt=qcow2 inc1.qcow2 $(( 64 * 4 ))k
qemu-img create -f qcow2 -o backing_file=inc1.qcow2,backing_fmt=qcow2 inc2.qcow2 $(( 64 * 5 ))k
qemu-img rebase -f qcow2 -b base.qcow2 -F qcow2 inc2.qcow2

And then rebase operation gets stuck forever.

The 1st patch is a fix, the 2nd -- an additional test case to catch this
situation.

Andrey Drobyshev (2):
  qemu-img: rebase: stop when reaching EOF of old backing file
  qemu-iotests: 024: add rebasing test case for overlay_size >
    backing_size

 qemu-img.c                 |  7 ++++++
 tests/qemu-iotests/024     | 48 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 23 ++++++++++++++++++
 3 files changed, 78 insertions(+)

-- 
2.31.1

[PATCH 1/2] qemu-img: rebase: stop when reaching EOF of old backing file

[Andrey Drobyshev via]

In case when we're rebasing within one backing chain, and when target image
is larger than old backing file, bdrv_is_allocated_above() ends up setting
*pnum = 0.  As a result, target offset isn't getting incremented, and we
get stuck in an infinite for loop.  Let's detect this case and break the
loop, as there's no more data to be read and merged from the old backing.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 qemu-img.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 27f48051b0..55b6ce407c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3813,6 +3813,13 @@ static int img_rebase(int argc, char **argv)
                                  strerror(-ret));
                     goto out;
                 }
+                if (!n) {
+                    /*
+                     * We've reached EOF of the old backing, therefore there's
+                     * no more mergeable data.
+                     */
+                    break;
+                }
                 if (!ret) {
                     continue;
                 }
-- 
2.31.1

[PATCH 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size

[Andrey Drobyshev via]

Before previous commit, rebase was getting infitely stuck in case of
rebasing within the same backing chain and when overlay_size > backing_size.
Let's add this case to the rebasing test 024 to make sure it doesn't
break again.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/024     | 48 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 23 ++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 25a564a150..5b6c2d973c 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -199,6 +199,54 @@ echo
 # $BASE_OLD and $BASE_NEW)
 $QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
 
+# Check that rebase within the chain is working when
+# overlay_size > old_backing_size
+#
+# base_new <-- base_old <-- overlay
+#
+# Backing (new): 11 11 11 11
+# Backing (old): 22 22 22 22
+# Overlay:       -- -- -- -- --
+#
+# As a result, overlay should contain data identical to base_old, with the
+# last cluster remaining unallocated.
+
+echo
+echo "=== Test rebase within one backing chain ==="
+echo
+
+echo "Creating backing chain"
+echo
+
+TEST_IMG=$BASE_NEW _make_test_img $(( CLUSTER_SIZE * 4 ))
+TEST_IMG=$BASE_OLD _make_test_img -b "$BASE_NEW" -F $IMGFMT \
+    $(( CLUSTER_SIZE * 4 ))
+TEST_IMG=$OVERLAY _make_test_img -b "$BASE_OLD" -F $IMGFMT \
+    $(( CLUSTER_SIZE * 5 ))
+
+echo
+echo "Fill backing files with data"
+echo
+
+$QEMU_IO "$BASE_NEW" -c "write -P 0x11 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+$QEMU_IO "$BASE_OLD" -c "write -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+
+echo
+echo "Rebase onto another image in the same chain"
+echo
+
+$QEMU_IMG rebase -b "$BASE_NEW" -F $IMGFMT "$OVERLAY"
+
+# Verify the data is correct
+$QEMU_IO "$OVERLAY" -c "read -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+
+echo
+
+# Verify that cluster #5 remained unallocated
+$QEMU_IMG map "$OVERLAY" | _filter_qemu_img_map
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 973a5a3711..fb63cac07c 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -171,4 +171,27 @@ read 65536/65536 bytes at offset 196608
 Offset          Length          File
 0               0x30000         TEST_DIR/subdir/t.IMGFMT
 0x30000         0x10000         TEST_DIR/subdir/t.IMGFMT.base_new
+
+=== Test rebase within one backing chain ===
+
+Creating backing chain
+
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_new', fmt=IMGFMT size=262144
+Formatting 'TEST_DIR/subdir/t.IMGFMT.base_old', fmt=IMGFMT size=262144 backing_file=TEST_DIR/subdir/t.IMGFMT.base_new backing_fmt=IMGFMT
+Formatting 'TEST_DIR/subdir/t.IMGFMT', fmt=IMGFMT size=327680 backing_file=TEST_DIR/subdir/t.IMGFMT.base_old backing_fmt=IMGFMT
+
+Fill backing files with data
+
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Rebase onto another image in the same chain
+
+read 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Offset          Length          File
+0               0x40000         TEST_DIR/subdir/t.IMGFMT
 *** done
-- 
2.31.1

Re: [PATCH 1/2] qemu-img: rebase: stop when reaching EOF of old backing file

[Denis V. Lunev]

On 5/23/23 18:24, Andrey Drobyshev wrote:
> In case when we're rebasing within one backing chain, and when target image
> is larger than old backing file, bdrv_is_allocated_above() ends up setting
> *pnum = 0.  As a result, target offset isn't getting incremented, and we
> get stuck in an infinite for loop.  Let's detect this case and break the
> loop, as there's no more data to be read and merged from the old backing.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 27f48051b0..55b6ce407c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3813,6 +3813,13 @@ static int img_rebase(int argc, char **argv)
>                                    strerror(-ret));
>                       goto out;
>                   }
> +                if (!n) {
> +                    /*
> +                     * We've reached EOF of the old backing, therefore there's
> +                     * no more mergeable data.
> +                     */
> +                    break;
> +                }
>                   if (!ret) {
>                       continue;
>                   }
nope. It seems that this is wrong patch.

iris ~/tmp/1 $ qemu-img create -f qcow2 base.qcow2 $(( 65 * 4 ))k
Formatting 'base.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off 
compression_type=zlib size=266240 lazy_refcounts=off refcount_bits=16
iris ~/tmp/1 $ qemu-img create -f qcow2 -o 
backing_file=base.qcow2,backing_fmt=qcow2 inc1.qcow2 $(( 64 * 4 ))
Formatting 'inc1.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off 
compression_type=zlib size=256 backing_file=base.qcow2 backing_fmt=qcow2 
lazy_refcounts=off refcount_bits=16
iris ~/tmp/1 $ qemu-img create -f qcow2 -o 
backing_file=inc1.qcow2,backing_fmt=qcow2 inc2.qcow2 $(( 64 * 5 ))k
Formatting 'inc2.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off 
compression_type=zlib size=327680 backing_file=inc1.qcow2 
backing_fmt=qcow2 lazy_refcounts=off refcount_bits=16
iris ~/tmp/1 $ qemu-io -c "write -P 0xae 256k 4k" base.qcow2
wrote 4096/4096 bytes at offset 262144
4 KiB, 1 ops; 00.01 sec (471.447 KiB/sec and 117.8617 ops/sec)
iris ~/tmp/1 $ qemu-io -c "read -P 0xae 256k 4k" base.qcow2
read 4096/4096 bytes at offset 262144
4 KiB, 1 ops; 00.00 sec (56.076 MiB/sec and 14355.4407 ops/sec)
iris ~/tmp/1 $ qemu-io -c "read -P 0xae 256k 4k" inc2.qcow2
Pattern verification failed at offset 262144, 4096 bytes
read 4096/4096 bytes at offset 262144
4 KiB, 1 ops; 00.00 sec (827.771 MiB/sec and 211909.3028 ops/sec)
iris ~/tmp/1 $ qemu-io -c "read -P 0 256k 4k" inc2.qcow2
read 4096/4096 bytes at offset 262144
4 KiB, 1 ops; 00.00 sec (838.611 MiB/sec and 214684.4139 ops/sec)
iris ~/tmp/1 $


iris ~/tmp/1 $ /home/den/src/qemu/build/qemu-img rebase -f qcow2 -b 
base.qcow2 -F qcow2 inc2.qcow2
iris ~/tmp/1 $ qemu-io -c "read -P 0 256k 4k" inc2.qcow2
Pattern verification failed at offset 262144, 4096 bytes
read 4096/4096 bytes at offset 262144
4 KiB, 1 ops; 00.00 sec (88.052 MiB/sec and 22541.3069 ops/sec)
iris ~/tmp/1 $

the problem is the following:
[----0xAE] <- base
[----] <- inc1
[--------] <- inc2

In this case last 4k of base should be read as 0x00 (this offset is not
written in inc2 and beyond end of the inc1).

This means that all non-present clusters in inc2 MUST be zeroed
during rebasing to base.

Something like this...

Den

Re: [PATCH 1/2] qemu-img: rebase: stop when reaching EOF of old backing file

[Andrey Drobyshev]

On 5/24/23 11:30, Denis V. Lunev wrote:
> On 5/23/23 18:24, Andrey Drobyshev wrote:
>> In case when we're rebasing within one backing chain, and when target
>> image
>> is larger than old backing file, bdrv_is_allocated_above() ends up
>> setting
>> *pnum = 0.  As a result, target offset isn't getting incremented, and we
>> get stuck in an infinite for loop.  Let's detect this case and break the
>> loop, as there's no more data to be read and merged from the old backing.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>   qemu-img.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 27f48051b0..55b6ce407c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -3813,6 +3813,13 @@ static int img_rebase(int argc, char **argv)
>>                                    strerror(-ret));
>>                       goto out;
>>                   }
>> +                if (!n) {
>> +                    /*
>> +                     * We've reached EOF of the old backing,
>> therefore there's
>> +                     * no more mergeable data.
>> +                     */
>> +                    break;
>> +                }
>>                   if (!ret) {
>>                       continue;
>>                   }
> nope. It seems that this is wrong patch.
> 
> iris ~/tmp/1 $ qemu-img create -f qcow2 base.qcow2 $(( 65 * 4 ))k
> Formatting 'base.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off
> compression_type=zlib size=266240 lazy_refcounts=off refcount_bits=16
> iris ~/tmp/1 $ qemu-img create -f qcow2 -o
> backing_file=base.qcow2,backing_fmt=qcow2 inc1.qcow2 $(( 64 * 4 ))
> Formatting 'inc1.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off
> compression_type=zlib size=256 backing_file=base.qcow2 backing_fmt=qcow2
> lazy_refcounts=off refcount_bits=16
> iris ~/tmp/1 $ qemu-img create -f qcow2 -o
> backing_file=inc1.qcow2,backing_fmt=qcow2 inc2.qcow2 $(( 64 * 5 ))k
> Formatting 'inc2.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off
> compression_type=zlib size=327680 backing_file=inc1.qcow2
> backing_fmt=qcow2 lazy_refcounts=off refcount_bits=16
> iris ~/tmp/1 $ qemu-io -c "write -P 0xae 256k 4k" base.qcow2
> wrote 4096/4096 bytes at offset 262144
> 4 KiB, 1 ops; 00.01 sec (471.447 KiB/sec and 117.8617 ops/sec)
> iris ~/tmp/1 $ qemu-io -c "read -P 0xae 256k 4k" base.qcow2
> read 4096/4096 bytes at offset 262144
> 4 KiB, 1 ops; 00.00 sec (56.076 MiB/sec and 14355.4407 ops/sec)
> iris ~/tmp/1 $ qemu-io -c "read -P 0xae 256k 4k" inc2.qcow2
> Pattern verification failed at offset 262144, 4096 bytes
> read 4096/4096 bytes at offset 262144
> 4 KiB, 1 ops; 00.00 sec (827.771 MiB/sec and 211909.3028 ops/sec)
> iris ~/tmp/1 $ qemu-io -c "read -P 0 256k 4k" inc2.qcow2
> read 4096/4096 bytes at offset 262144
> 4 KiB, 1 ops; 00.00 sec (838.611 MiB/sec and 214684.4139 ops/sec)
> iris ~/tmp/1 $
> 
> 
> iris ~/tmp/1 $ /home/den/src/qemu/build/qemu-img rebase -f qcow2 -b
> base.qcow2 -F qcow2 inc2.qcow2
> iris ~/tmp/1 $ qemu-io -c "read -P 0 256k 4k" inc2.qcow2
> Pattern verification failed at offset 262144, 4096 bytes
> read 4096/4096 bytes at offset 262144
> 4 KiB, 1 ops; 00.00 sec (88.052 MiB/sec and 22541.3069 ops/sec)
> iris ~/tmp/1 $
> 
> the problem is the following:
> [----0xAE] <- base
> [----] <- inc1
> [--------] <- inc2
> 
> In this case last 4k of base should be read as 0x00 (this offset is not
> written in inc2 and beyond end of the inc1).
> 
> This means that all non-present clusters in inc2 MUST be zeroed
> during rebasing to base.
> 
> Something like this...
> 
> Den

Thanks for pointing that out, it seems indeed that breaking the loop
just yet isn't a good idea.  Since the offsets beyond the old backing
size were read as zeroes before the rebase, this should remain the case
afterwards as well.  Which means we should explicitly zero the overlay
clusters which go beyond the old backing size.

I'll alter the patches accordingly (including the test) and send v2.

Andrey