Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
On 10/26/23 09:32, Michael Tokarev wrote: > 01.06.2023 22:28, 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 proceed >> further down the loop body, as the offsets beyond the old backing size >> need >> to be explicitly zeroed. > > Ping? Has this been forgotten? It's a few months already.. > > /mjt Hi Michael, It's not forgotten, there's already v3 of this series and it's already taken to the block branch by Kevin: https://lists.nongnu.org/archive/html/qemu-block/2023-09/msg00593.html Andrey
Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
01.06.2023 22:28, 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 proceed further down the loop body, as the offsets beyond the old backing size need to be explicitly zeroed. Ping? Has this been forgotten? It's a few months already.. /mjt
Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
On 01.06.23 21:28, Andrey Drobyshev via 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 --- qemu-img.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Hanna Czenczek
Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
On 6/1/23 21:28, 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
---
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;
+}
}
/*
for the clarity:
Reviewed-by: Denis V. Lunev
Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
On 6/2/23 00:18, Michael Tokarev wrote: > 01.06.2023 22:28, 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 proceed >> further down the loop body, as the offsets beyond the old backing size >> need >> to be explicitly zeroed. >> >> Signed-off-by: Andrey Drobyshev > > It looks like you forgot the Reviewed-by: Denis V. Lunev here and > in the subsequent patch. Yes, you're right, thanks for pointing that out. > > Should this be backported to -stable? Not that I've seen this issue, > it's a quite specific and somewhat rare case.. I guess in the vast majority of cases the sizes of images within the same backing chain are equal. But as long as it's legal to have them unequal, a bug remains a bug. > > Thanks, > > /mjt
Re: [PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
01.06.2023 22:28, 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 proceed further down the loop body, as the offsets beyond the old backing size need to be explicitly zeroed. Signed-off-by: Andrey Drobyshev It looks like you forgot the Reviewed-by: Denis V. Lunev here and in the subsequent patch. Should this be backported to -stable? Not that I've seen this issue, it's a quite specific and somewhat rare case.. Thanks, /mjt
[PATCH 1/6] qemu-img: rebase: stop when reaching EOF of old backing file
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
---
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
