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

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase
@ 2023-05-25 18:02 Andrey Drobyshev via
  2023-05-25 18:02 ` [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file Andrey Drobyshev via
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andrey Drobyshev via @ 2023-05-25 18:02 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman, andrey.drobyshev, den

v1 -> v2:

  * Avoid breaking the loop just yet, as the offsets beyond the old
    backing size need to be explicitly zeroed;
  * Amend the commit message accordingly;
  * Alter the added test case to take the last zeroed cluster into
    consideration.

v1: https://lists.nongnu.org/archive/html/qemu-block/2023-05/msg00674.html

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                 | 13 ++++++++-
 tests/qemu-iotests/024     | 57 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file
  2023-05-25 18:02 [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev via
@ 2023-05-25 18:02 ` Andrey Drobyshev via
  2023-05-26 12:12   ` Denis V. Lunev
  2023-05-25 18:02 ` [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size Andrey Drobyshev via
  2023-06-01 20:54 ` [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev
  2 siblings, 1 reply; 6+ messages in thread
From: Andrey Drobyshev via @ 2023-05-25 18:02 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman, andrey.drobyshev, den

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 proceed
further down the loop body, as the offsets beyond the old backing size need
to be explicitly zeroed.

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

diff --git a/qemu-img.c b/qemu-img.c
index 27f48051b0..78433f3746 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3801,6 +3801,8 @@ static int img_rebase(int argc, char **argv)
             }
 
             if (prefix_chain_bs) {
+                uint64_t bytes = n;
+
                 /*
                  * If cluster wasn't changed since prefix_chain, we don't need
                  * to take action
@@ -3813,9 +3815,18 @@ static int img_rebase(int argc, char **argv)
                                  strerror(-ret));
                     goto out;
                 }
-                if (!ret) {
+                if (!ret && n) {
                     continue;
                 }
+                if (!n) {
+                    /*
+                     * If we've reached EOF of the old backing, it means that
+                     * offsets beyond the old backing size were read as zeroes.
+                     * Now we will need to explicitly zero the cluster in
+                     * order to preserve that state after the rebase.
+                     */
+                    n = bytes;
+                }
             }
 
             /*
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file
  2023-05-25 18:02 ` [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file Andrey Drobyshev via
@ 2023-05-26 12:12   ` Denis V. Lunev
  0 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-05-26 12:12 UTC (permalink / raw)
  To: Andrey Drobyshev, qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman

On 5/25/23 20:02, 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 proceed
> further down the loop body, as the offsets beyond the old backing size need
> to be explicitly zeroed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qemu-img.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 27f48051b0..78433f3746 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3801,6 +3801,8 @@ static int img_rebase(int argc, char **argv)
>               }
>   
>               if (prefix_chain_bs) {
> +                uint64_t bytes = n;
> +
>                   /*
>                    * If cluster wasn't changed since prefix_chain, we don't need
>                    * to take action
> @@ -3813,9 +3815,18 @@ static int img_rebase(int argc, char **argv)
>                                    strerror(-ret));
>                       goto out;
>                   }
> -                if (!ret) {
> +                if (!ret && n) {
>                       continue;
>                   }
> +                if (!n) {
> +                    /*
> +                     * If we've reached EOF of the old backing, it means that
> +                     * offsets beyond the old backing size were read as zeroes.
> +                     * Now we will need to explicitly zero the cluster in
> +                     * order to preserve that state after the rebase.
> +                     */
> +                    n = bytes;
> +                }
>               }
>   
>               /*
Revieved-by: Denis V. Lunev <den@openvz.org>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size
  2023-05-25 18:02 [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev via
  2023-05-25 18:02 ` [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file Andrey Drobyshev via
@ 2023-05-25 18:02 ` Andrey Drobyshev via
  2023-05-26 12:13   ` Denis V. Lunev
  2023-06-01 20:54 ` [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev
  2 siblings, 1 reply; 6+ messages in thread
From: Andrey Drobyshev via @ 2023-05-25 18:02 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman, andrey.drobyshev, den

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     | 57 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 25a564a150..98a7c8fd65 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -199,6 +199,63 @@ 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 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 * 5 ))
+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 * 5 ))" \
+    | _filter_qemu_io
+$QEMU_IO "$BASE_OLD" -c "write -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+
+echo
+echo "Check the last cluster is zeroed in overlay before the rebase"
+echo
+$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
+echo "Rebase onto another image in the same chain"
+echo
+
+$QEMU_IMG rebase -b "$BASE_NEW" -F $IMGFMT "$OVERLAY"
+
+echo "Verify that data is read the same before and after rebase"
+echo
+
+# Verify the first 4 clusters are still read the same as in the old base
+$QEMU_IO "$OVERLAY" -c "read -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
+    | _filter_qemu_io
+# Verify the last cluster still reads as zeroes
+$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
+    | _filter_qemu_io
+
+echo
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 973a5a3711..245fe8b1d1 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -171,4 +171,34 @@ 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=327680
+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 327680/327680 bytes at offset 0
+320 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)
+
+Check the last cluster is zeroed in overlay before the rebase
+
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Rebase onto another image in the same chain
+
+Verify that data is read the same before and after rebase
+
+read 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size
  2023-05-25 18:02 ` [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size Andrey Drobyshev via
@ 2023-05-26 12:13   ` Denis V. Lunev
  0 siblings, 0 replies; 6+ messages in thread
From: Denis V. Lunev @ 2023-05-26 12:13 UTC (permalink / raw)
  To: Andrey Drobyshev, qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman

On 5/25/23 20:02, Andrey Drobyshev wrote:
> 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     | 57 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
>   2 files changed, 87 insertions(+)
>
> diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
> index 25a564a150..98a7c8fd65 100755
> --- a/tests/qemu-iotests/024
> +++ b/tests/qemu-iotests/024
> @@ -199,6 +199,63 @@ 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 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 * 5 ))
> +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 * 5 ))" \
> +    | _filter_qemu_io
> +$QEMU_IO "$BASE_OLD" -c "write -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
> +    | _filter_qemu_io
> +
> +echo
> +echo "Check the last cluster is zeroed in overlay before the rebase"
> +echo
> +$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
> +    | _filter_qemu_io
> +
> +echo
> +echo "Rebase onto another image in the same chain"
> +echo
> +
> +$QEMU_IMG rebase -b "$BASE_NEW" -F $IMGFMT "$OVERLAY"
> +
> +echo "Verify that data is read the same before and after rebase"
> +echo
> +
> +# Verify the first 4 clusters are still read the same as in the old base
> +$QEMU_IO "$OVERLAY" -c "read -P 0x22 0 $(( CLUSTER_SIZE * 4 ))" \
> +    | _filter_qemu_io
> +# Verify the last cluster still reads as zeroes
> +$QEMU_IO "$OVERLAY" -c "read -P 0x00 $(( CLUSTER_SIZE * 4 )) $CLUSTER_SIZE" \
> +    | _filter_qemu_io
> +
> +echo
>   
>   # success, all done
>   echo "*** done"
> diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
> index 973a5a3711..245fe8b1d1 100644
> --- a/tests/qemu-iotests/024.out
> +++ b/tests/qemu-iotests/024.out
> @@ -171,4 +171,34 @@ 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=327680
> +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 327680/327680 bytes at offset 0
> +320 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)
> +
> +Check the last cluster is zeroed in overlay before the rebase
> +
> +read 65536/65536 bytes at offset 262144
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +Rebase onto another image in the same chain
> +
> +Verify that data is read the same before and after rebase
> +
> +read 262144/262144 bytes at offset 0
> +256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +read 65536/65536 bytes at offset 262144
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
>   *** done
Reviewed-by: Denis V. Lunev <den@openvz.org>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase
  2023-05-25 18:02 [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev via
  2023-05-25 18:02 ` [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file Andrey Drobyshev via
  2023-05-25 18:02 ` [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size Andrey Drobyshev via
@ 2023-06-01 20:54 ` Andrey Drobyshev
  2 siblings, 0 replies; 6+ messages in thread
From: Andrey Drobyshev @ 2023-06-01 20:54 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, kwolf, shmuel.eiderman, den

On 5/25/23 21:02, Andrey Drobyshev wrote:
> v1 -> v2:
> 
>   * Avoid breaking the loop just yet, as the offsets beyond the old
>     backing size need to be explicitly zeroed;
>   * Amend the commit message accordingly;
>   * Alter the added test case to take the last zeroed cluster into
>     consideration.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-05/msg00674.html
> 
> 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                 | 13 ++++++++-
>  tests/qemu-iotests/024     | 57 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/024.out | 30 ++++++++++++++++++++
>  3 files changed, 99 insertions(+), 1 deletion(-)
> 

Since there're no comments so far, I've included this same bugfix into
the bigger series regarding "qemu-img rebase".  Please refer to
https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00068.html.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-06-01 20:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-25 18:02 [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev via
2023-05-25 18:02 ` [PATCH v2 1/2] qemu-img: rebase: stop when reaching EOF of old backing file Andrey Drobyshev via
2023-05-26 12:12   ` Denis V. Lunev
2023-05-25 18:02 ` [PATCH v2 2/2] qemu-iotests: 024: add rebasing test case for overlay_size > backing_size Andrey Drobyshev via
2023-05-26 12:13   ` Denis V. Lunev
2023-06-01 20:54 ` [PATCH v2 0/2] qemu-img: fix getting stuck in infinite loop on in-chain rebase Andrey Drobyshev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).