| Message ID | 20230919165804.439110-2-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: rebase: add compression support | expand |
19.09.2023 19:57, 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 <andrey.drobyshev@virtuozzo.com> > Reviewed-by: Denis V. Lunev <den@openvz.org> > Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Is this change not for -stable anymore? First version of this patch has been Cc'd to stable, this one is not. Thanks, /mjt
On 11/1/23 11:50, Michael Tokarev wrote: > 19.09.2023 19:57, 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 <andrey.drobyshev@virtuozzo.com> >> Reviewed-by: Denis V. Lunev <den@openvz.org> >> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > > Is this change not for -stable anymore? First version of this patch has > been > Cc'd to stable, this one is not. > > Thanks, > > /mjt Hi Michael, Since this series is already merged in master, I'm not sure whether it's necessary to forward this particular patch to qemu-stable, or it should rather be cherry-picked to -stable by one of the block maintainers. Kevin, maybe you can clarify? Andrey
01.11.2023 18:38, Andrey Drobyshev wrote: > Hi Michael, > > Since this series is already merged in master, I'm not sure whether it's > necessary to forward this particular patch to qemu-stable, or it should > rather be cherry-picked to -stable by one of the block maintainers. It's been my job lately to pick something up for stable, once it gets applied to master. But it is usually the patch author or subsystem maintainer to mark a change as a candidate for -stable, - I can't decide about every change out there, since I don't have enough expertise in every area. You Cc: qemu-stable@ and I pick it up. I do look at stuff being applied to master from time to time though, and ask if I see something which might be worth to pick, as in this case. BTW, there's another change in this series which might be a good candidate too, - "qemu-img: rebase: use backing files' BlockBackend for buffer alignment". Once again, I dunno if it's worth to pick it up or not, it's basically up to you to decide. Basically, you understand much better what the implications are and if the change fixes actual bug. /mjt
Am 01.11.2023 um 18:51 hat Michael Tokarev geschrieben: > 01.11.2023 18:38, Andrey Drobyshev wrote: > > Hi Michael, > > > > Since this series is already merged in master, I'm not sure whether it's > > necessary to forward this particular patch to qemu-stable, or it should > > rather be cherry-picked to -stable by one of the block maintainers. > > It's been my job lately to pick something up for stable, once it gets > applied to master. But it is usually the patch author or subsystem > maintainer to mark a change as a candidate for -stable, - I can't decide > about every change out there, since I don't have enough expertise in > every area. You Cc: qemu-stable@ and I pick it up. I do look at stuff > being applied to master from time to time though, and ask if I see > something which might be worth to pick, as in this case. I didn't even think about it for this patch, but yes, it sounds like a candidate for stable to me. > BTW, there's another change in this series which might be a good candidate > too, - "qemu-img: rebase: use backing files' BlockBackend for buffer > alignment". Once again, I dunno if it's worth to pick it up or not, > it's basically up to you to decide. Basically, you understand much > better what the implications are and if the change fixes actual bug. It's an optimisation, I wouldn't pick that one. Kevin
diff --git a/qemu-img.c b/qemu-img.c index a48edb7101..50660ba920 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3805,6 +3805,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 @@ -3817,9 +3819,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; + } } /*