| Message ID | 20191120184501.28159-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix resize (extending) of short overlays | expand |
On 11/20/19 12:44 PM, Kevin Wolf wrote: > When extending the size of an image that has a backing file larger than > its old size, make sure that the backing file data doesn't become > visible in the guest, but the added area is properly zeroed out. > > Consider the following scenario where the overlay is shorter than its > backing file: > > base.qcow2: AAAAAAAA > overlay.qcow2: BBBB > > When resizing (extending) overlay.qcow2, the new blocks should not stay > unallocated and make the additional As from base.qcow2 visible like > before this patch, but zeros should be read. > > A similar case happens with the various variants of a commit job when an > intermediate file is short (- for unallocated): > > base.qcow2: A-A-AAAA > mid.qcow2: BB-B > top.qcow2: C--C--C- > > After commit top.qcow2 to mid.qcow2, the following happens: > > mid.qcow2: CB-C00C0 (correct result) > mid.qcow2: CB-C--C- (before this fix) > > Without the fix, blocks that previously read as zeros on top.qcow2 > suddenly turn into A. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/io.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > + if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) { > + int64_t backing_len; > + > + backing_len = bdrv_getlength(backing_bs(bs)); > + if (backing_len < 0) { > + ret = backing_len; > + goto out; > + } > + > + if (backing_len > old_size) { > + ret = bdrv_co_do_pwrite_zeroes( > + bs, old_size, MIN(new_bytes, backing_len - old_size), > + BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP); > + if (ret < 0) { > + goto out; > + } > + } > + } Note that if writing zeroes is not fast, and it turns out that we copy a lot of data rather than unallocated sections from the image being committed, that this can actually slow things down (doing a bulk pre-zero doubles up data I/O unless it is fast, which is why we added BDRV_REQ_NO_FALLBACK to avoid slow pre-zeroing). However, the complication of zeroing only the unallocated clusters rather than a bulk pre-zeroing for something that is an unlikely corner case (how often do you create an overlay shorter than the backing file?) is not worth the extra code maintenance (unlike in the 'qemu-img convert' case where it was worth the optimization). So I'm fine with how you fixed it here. Reviewed-by: Eric Blake <eblake@redhat.com>
On 20.11.19 19:44, Kevin Wolf wrote: > When extending the size of an image that has a backing file larger than > its old size, make sure that the backing file data doesn't become > visible in the guest, but the added area is properly zeroed out. > > Consider the following scenario where the overlay is shorter than its > backing file: > > base.qcow2: AAAAAAAA > overlay.qcow2: BBBB > > When resizing (extending) overlay.qcow2, the new blocks should not stay > unallocated and make the additional As from base.qcow2 visible like > before this patch, but zeros should be read. > > A similar case happens with the various variants of a commit job when an > intermediate file is short (- for unallocated): > > base.qcow2: A-A-AAAA > mid.qcow2: BB-B > top.qcow2: C--C--C- > > After commit top.qcow2 to mid.qcow2, the following happens: > > mid.qcow2: CB-C00C0 (correct result) > mid.qcow2: CB-C--C- (before this fix) > > Without the fix, blocks that previously read as zeros on top.qcow2 > suddenly turn into A. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/io.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) Zeroing the intersection may take some time. So is it right for QMP’s block_resize to do this, seeing it is a synchronous operation? As far as I can tell, jobs actually have the same problem. I don’t think mirror or commit have a pause point before truncating, so they still block the monitor there, don’t they? Max
21.11.2019 11:59, Max Reitz wrote: > On 20.11.19 19:44, Kevin Wolf wrote: >> When extending the size of an image that has a backing file larger than >> its old size, make sure that the backing file data doesn't become >> visible in the guest, but the added area is properly zeroed out. >> >> Consider the following scenario where the overlay is shorter than its >> backing file: >> >> base.qcow2: AAAAAAAA >> overlay.qcow2: BBBB >> >> When resizing (extending) overlay.qcow2, the new blocks should not stay >> unallocated and make the additional As from base.qcow2 visible like >> before this patch, but zeros should be read. >> >> A similar case happens with the various variants of a commit job when an >> intermediate file is short (- for unallocated): >> >> base.qcow2: A-A-AAAA >> mid.qcow2: BB-B >> top.qcow2: C--C--C- >> >> After commit top.qcow2 to mid.qcow2, the following happens: >> >> mid.qcow2: CB-C00C0 (correct result) >> mid.qcow2: CB-C--C- (before this fix) >> >> Without the fix, blocks that previously read as zeros on top.qcow2 >> suddenly turn into A. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/io.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) > > Zeroing the intersection may take some time. So is it right for QMP’s > block_resize to do this, seeing it is a synchronous operation? > > As far as I can tell, jobs actually have the same problem. I don’t > think mirror or commit have a pause point before truncating, so they > still block the monitor there, don’t they? > > Max > For mirror there is alternative way: we may somehow cheat with block-status to consider space after EOF as allocated on upper level, then mirror will ZERO it by itself..
Am 20.11.2019 um 22:15 hat Eric Blake geschrieben: > On 11/20/19 12:44 PM, Kevin Wolf wrote: > > When extending the size of an image that has a backing file larger than > > its old size, make sure that the backing file data doesn't become > > visible in the guest, but the added area is properly zeroed out. > > > > Consider the following scenario where the overlay is shorter than its > > backing file: > > > > base.qcow2: AAAAAAAA > > overlay.qcow2: BBBB > > > > When resizing (extending) overlay.qcow2, the new blocks should not stay > > unallocated and make the additional As from base.qcow2 visible like > > before this patch, but zeros should be read. > > > > A similar case happens with the various variants of a commit job when an > > intermediate file is short (- for unallocated): > > > > base.qcow2: A-A-AAAA > > mid.qcow2: BB-B > > top.qcow2: C--C--C- > > > > After commit top.qcow2 to mid.qcow2, the following happens: > > > > mid.qcow2: CB-C00C0 (correct result) > > mid.qcow2: CB-C--C- (before this fix) > > > > Without the fix, blocks that previously read as zeros on top.qcow2 > > suddenly turn into A. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/io.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > > + if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) { > > + int64_t backing_len; > > + > > + backing_len = bdrv_getlength(backing_bs(bs)); > > + if (backing_len < 0) { > > + ret = backing_len; > > + goto out; > > + } > > + > > + if (backing_len > old_size) { > > + ret = bdrv_co_do_pwrite_zeroes( > > + bs, old_size, MIN(new_bytes, backing_len - old_size), > > + BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP); > > + if (ret < 0) { > > + goto out; > > + } > > + } > > + } > > Note that if writing zeroes is not fast, and it turns out that we copy a lot > of data rather than unallocated sections from the image being committed, > that this can actually slow things down (doing a bulk pre-zero doubles up > data I/O unless it is fast, which is why we added BDRV_REQ_NO_FALLBACK to > avoid slow pre-zeroing). However, the complication of zeroing only the > unallocated clusters rather than a bulk pre-zeroing I'm not sure how there could already be any allocated clusters in the area that we just added? (Apart from preallocation, of course, which is why this is restricted to PREALLOC_MODE_OFF.) You're right that if this truncate was called in the context of a commit block job rather than a resize, the zeros might be overwritten later. I'm not sure if leaving clusters unallocated while the job is running (and might be cancelled) is right, though. On the other hand, while the job is running, the target image on its own is invalid anyway. > for something that is an unlikely corner case (how often do you create > an overlay shorter than the backing file?) is not worth the extra code > maintenance (unlike in the 'qemu-img convert' case where it was worth > the optimization). So I'm fine with how you fixed it here. Also note that raw doesn't support backing files and qcow2 generally supports zero writes. If you use an external data file and backing file at the same time and your file system doesn't support zero writes, you could run into the problem. Or if you use a more unusual image format (including qcow2 v2). So it's really two corner cases combined. Kevin
Am 21.11.2019 um 09:59 hat Max Reitz geschrieben: > On 20.11.19 19:44, Kevin Wolf wrote: > > When extending the size of an image that has a backing file larger than > > its old size, make sure that the backing file data doesn't become > > visible in the guest, but the added area is properly zeroed out. > > > > Consider the following scenario where the overlay is shorter than its > > backing file: > > > > base.qcow2: AAAAAAAA > > overlay.qcow2: BBBB > > > > When resizing (extending) overlay.qcow2, the new blocks should not stay > > unallocated and make the additional As from base.qcow2 visible like > > before this patch, but zeros should be read. > > > > A similar case happens with the various variants of a commit job when an > > intermediate file is short (- for unallocated): > > > > base.qcow2: A-A-AAAA > > mid.qcow2: BB-B > > top.qcow2: C--C--C- > > > > After commit top.qcow2 to mid.qcow2, the following happens: > > > > mid.qcow2: CB-C00C0 (correct result) > > mid.qcow2: CB-C--C- (before this fix) > > > > Without the fix, blocks that previously read as zeros on top.qcow2 > > suddenly turn into A. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/io.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > Zeroing the intersection may take some time. So is it right for QMP’s > block_resize to do this, seeing it is a synchronous operation? What else would be right? Returning an error? Common cases (raw and qcow2 v3 without external data files) are quick anyway. > As far as I can tell, jobs actually have the same problem. I don’t > think mirror or commit have a pause point before truncating, so they > still block the monitor there, don’t they? Do you really need a pause point? They call bdrv_co_truncate() from inside the job coroutine, so it will yield. I would expect that this is enough. But in fact, all jobs have a pause point before even calling .run(), so even if that made a difference, it should still be fine. Kevin
On 21.11.19 12:34, Kevin Wolf wrote: > Am 21.11.2019 um 09:59 hat Max Reitz geschrieben: >> On 20.11.19 19:44, Kevin Wolf wrote: >>> When extending the size of an image that has a backing file larger than >>> its old size, make sure that the backing file data doesn't become >>> visible in the guest, but the added area is properly zeroed out. >>> >>> Consider the following scenario where the overlay is shorter than its >>> backing file: >>> >>> base.qcow2: AAAAAAAA >>> overlay.qcow2: BBBB >>> >>> When resizing (extending) overlay.qcow2, the new blocks should not stay >>> unallocated and make the additional As from base.qcow2 visible like >>> before this patch, but zeros should be read. >>> >>> A similar case happens with the various variants of a commit job when an >>> intermediate file is short (- for unallocated): >>> >>> base.qcow2: A-A-AAAA >>> mid.qcow2: BB-B >>> top.qcow2: C--C--C- >>> >>> After commit top.qcow2 to mid.qcow2, the following happens: >>> >>> mid.qcow2: CB-C00C0 (correct result) >>> mid.qcow2: CB-C--C- (before this fix) >>> >>> Without the fix, blocks that previously read as zeros on top.qcow2 >>> suddenly turn into A. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block/io.c | 32 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >> >> Zeroing the intersection may take some time. So is it right for QMP’s >> block_resize to do this, seeing it is a synchronous operation? > > What else would be right? Returning an error? Going through a deprecation cycle. > Common cases (raw and qcow2 v3 without external data files) are quick > anyway. Well, but quick enough for a fully blocking operation? >> As far as I can tell, jobs actually have the same problem. I don’t >> think mirror or commit have a pause point before truncating, so they >> still block the monitor there, don’t they? > > Do you really need a pause point? They call bdrv_co_truncate() from > inside the job coroutine, so it will yield. I would expect that this > is enough. OK then. > But in fact, all jobs have a pause point before even calling .run(), so > even if that made a difference, it should still be fine. Good. But I believe this is still a problem for block_resize. I don’t see why this needs to be fixed in 4.2-rc3. What’s the problem with going through a proper deprecation cycle other than the fact that we can’t start it in 4.2 because we don’t have a resize job yet? Max
Am 21.11.2019 um 13:21 hat Max Reitz geschrieben: > On 21.11.19 12:34, Kevin Wolf wrote: > > Am 21.11.2019 um 09:59 hat Max Reitz geschrieben: > >> On 20.11.19 19:44, Kevin Wolf wrote: > >>> When extending the size of an image that has a backing file larger than > >>> its old size, make sure that the backing file data doesn't become > >>> visible in the guest, but the added area is properly zeroed out. > >>> > >>> Consider the following scenario where the overlay is shorter than its > >>> backing file: > >>> > >>> base.qcow2: AAAAAAAA > >>> overlay.qcow2: BBBB > >>> > >>> When resizing (extending) overlay.qcow2, the new blocks should not stay > >>> unallocated and make the additional As from base.qcow2 visible like > >>> before this patch, but zeros should be read. > >>> > >>> A similar case happens with the various variants of a commit job when an > >>> intermediate file is short (- for unallocated): > >>> > >>> base.qcow2: A-A-AAAA > >>> mid.qcow2: BB-B > >>> top.qcow2: C--C--C- > >>> > >>> After commit top.qcow2 to mid.qcow2, the following happens: > >>> > >>> mid.qcow2: CB-C00C0 (correct result) > >>> mid.qcow2: CB-C--C- (before this fix) > >>> > >>> Without the fix, blocks that previously read as zeros on top.qcow2 > >>> suddenly turn into A. > >>> > >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >>> --- > >>> block/io.c | 32 ++++++++++++++++++++++++++++++++ > >>> 1 file changed, 32 insertions(+) > >> > >> Zeroing the intersection may take some time. So is it right for QMP’s > >> block_resize to do this, seeing it is a synchronous operation? > > > > What else would be right? Returning an error? > > Going through a deprecation cycle. And keeping the buggy behaviour for two more releases? > > Common cases (raw and qcow2 v3 without external data files) are quick > > anyway. > > Well, but quick enough for a fully blocking operation? For raw definitely yes, because raw doesn't have backing files, so the code will never run. For qcow2, block_resize can already block for a relatively long time while metadata tables are resized, clusters are discarded etc. I just don't really see the difference in quality between that and allocating some zero clusters in a corner case like having a short overlay. Would you feel more comfortable if we set BDRV_REQ_NO_FALLBACK and return an error if we can't zero out the area? We would have to advertise that flag in bs->supported_zero_flags for qcow2 then (but probably we should do that anyway?) > >> As far as I can tell, jobs actually have the same problem. I don’t > >> think mirror or commit have a pause point before truncating, so they > >> still block the monitor there, don’t they? > > > > Do you really need a pause point? They call bdrv_co_truncate() from > > inside the job coroutine, so it will yield. I would expect that this > > is enough. > > OK then. > > > But in fact, all jobs have a pause point before even calling .run(), so > > even if that made a difference, it should still be fine. > > Good. > > But I believe this is still a problem for block_resize. I don’t see why > this needs to be fixed in 4.2-rc3. What’s the problem with going > through a proper deprecation cycle other than the fact that we can’t > start it in 4.2 because we don’t have a resize job yet? That the behaviour is wrong. For commit it's an image corruptor. For resize, I'll admit that it's just wrong behaviour that is probably harmless in most cases, but it's still wrong behaviour. It would be a corruptor in the same way as commit if it allowed resizing intermediate nodes, but I _think_ the old-style op blockers still forbid this. We'd have to double-check this if we leave things broken for block_resize. Anyway, so are you suggesting adding another parameter to bdrv_co_truncate() that enables wrong, but quicker semantics, and that would only be used by block_resize? Kevin
On 21.11.19 15:33, Kevin Wolf wrote: > Am 21.11.2019 um 13:21 hat Max Reitz geschrieben: >> On 21.11.19 12:34, Kevin Wolf wrote: >>> Am 21.11.2019 um 09:59 hat Max Reitz geschrieben: >>>> On 20.11.19 19:44, Kevin Wolf wrote: >>>>> When extending the size of an image that has a backing file larger than >>>>> its old size, make sure that the backing file data doesn't become >>>>> visible in the guest, but the added area is properly zeroed out. >>>>> >>>>> Consider the following scenario where the overlay is shorter than its >>>>> backing file: >>>>> >>>>> base.qcow2: AAAAAAAA >>>>> overlay.qcow2: BBBB >>>>> >>>>> When resizing (extending) overlay.qcow2, the new blocks should not stay >>>>> unallocated and make the additional As from base.qcow2 visible like >>>>> before this patch, but zeros should be read. >>>>> >>>>> A similar case happens with the various variants of a commit job when an >>>>> intermediate file is short (- for unallocated): >>>>> >>>>> base.qcow2: A-A-AAAA >>>>> mid.qcow2: BB-B >>>>> top.qcow2: C--C--C- >>>>> >>>>> After commit top.qcow2 to mid.qcow2, the following happens: >>>>> >>>>> mid.qcow2: CB-C00C0 (correct result) >>>>> mid.qcow2: CB-C--C- (before this fix) >>>>> >>>>> Without the fix, blocks that previously read as zeros on top.qcow2 >>>>> suddenly turn into A. >>>>> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>>> --- >>>>> block/io.c | 32 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 32 insertions(+) >>>> >>>> Zeroing the intersection may take some time. So is it right for QMP’s >>>> block_resize to do this, seeing it is a synchronous operation? >>> >>> What else would be right? Returning an error? >> >> Going through a deprecation cycle. > > And keeping the buggy behaviour for two more releases? This sounds like you don’t care about adding another bug when it means fixing this bug. I do care. And so all I was saying is that it seemed problematic to me to fix the problem in this way, because clearly this would make block_resize block the monitor in some cases and clearly that would be a problem, if not a bug. (And that’s precisely what can be said about the current block_resize behavior of revealing previous backing file data: It is a problem, but I wouldn’t call it a full-on bug. It just seems to me that nobody has ever really thought about it.) Also, I don’t see this as a really pressing issue for block_resize at least, because this isn’t a recent regression or anything. It was just the behavior we had, I believe everyone knew it, we just never thought about whether it really is the best kind of behavior. So, yes, I’m in absolutely no hurry to fix this for block_resize. (Commit is a different story, but then again commit is a job already, so it doesn’t suffer from the blocking issue.) But of course this wasn’t all that you’re saying, you give a very good point next. >>> Common cases (raw and qcow2 v3 without external data files) are quick >>> anyway. >> >> Well, but quick enough for a fully blocking operation?> For qcow2, block_resize can already block for a relatively long time > while metadata tables are resized, clusters are discarded etc. I just > don't really see the difference in quality between that and allocating > some zero clusters in a corner case like having a short overlay. Discarding cropped clusters when shrinking is a good point. I didn’t think of that. So block_resize already has the very problem I was afraid you’d introduce, because of course discarding isn’t very different from quick-zeroing. > Would you feel more comfortable if we set BDRV_REQ_NO_FALLBACK and > return an error if we can't zero out the area? We would have to > advertise that flag in bs->supported_zero_flags for qcow2 then (but > probably we should do that anyway?) Hm. I don’t feel that bad about disallowing this edge case (growing a shrunken overlay) for potentially all non-qcow2v3 formats. I don’t know how bad it would be for users other than block_resize, though. (And I suppose we want a resize job anyway then, and that would work for those cases?) >>>> As far as I can tell, jobs actually have the same problem. I don’t >>>> think mirror or commit have a pause point before truncating, so they >>>> still block the monitor there, don’t they? >>> >>> Do you really need a pause point? They call bdrv_co_truncate() from >>> inside the job coroutine, so it will yield. I would expect that this >>> is enough. >> >> OK then. >> >>> But in fact, all jobs have a pause point before even calling .run(), so >>> even if that made a difference, it should still be fine. >> >> Good. >> >> But I believe this is still a problem for block_resize. I don’t see why >> this needs to be fixed in 4.2-rc3. What’s the problem with going >> through a proper deprecation cycle other than the fact that we can’t >> start it in 4.2 because we don’t have a resize job yet? > > That the behaviour is wrong. I know a couple of wrong behaviors that won’t be fixed in 4.2. > For commit it's an image corruptor. That’s a reason why we need it in 4.2. It’s no reason why we need it for block_resize. > For resize, I'll admit that it's > just wrong behaviour that is probably harmless in most cases, but it's > still wrong behaviour. It would be a corruptor in the same way as commit > if it allowed resizing intermediate nodes, but I _think_ the old-style > op blockers still forbid this. We'd have to double-check this if we > leave things broken for block_resize. >> Anyway, so are you suggesting adding another parameter to > bdrv_co_truncate() that enables wrong, but quicker semantics, and that > would only be used by block_resize? That would certainly be a possibility, yes. I like your suggestion of only allowing it with NO_FALLBACK, but I suppose we’d want the same parameter for that, too, because users other than block_resize probably prefer a slow zeroing over an error. So to me the question then is whether the added complexity is worth it for an rc3. Max
On Thu 21 Nov 2019 03:33:31 PM CET, Kevin Wolf wrote: > For commit it's an image corruptor. For resize, I'll admit that it's > just wrong behaviour that is probably harmless in most cases, but it's > still wrong behaviour. It would be a corruptor in the same way as > commit if it allowed resizing intermediate nodes, but I _think_ the > old-style op blockers still forbid this. We'd have to double-check > this if we leave things broken for block_resize. I tried but cannot make block_resize touch an intermediate image: if (!bdrv_is_first_non_filter(bs)) { error_setg(errp, QERR_FEATURE_DISABLED, "resize"); goto out; } Berto
On Wed 20 Nov 2019 07:44:57 PM CET, Kevin Wolf wrote: > When extending the size of an image that has a backing file larger than > its old size, make sure that the backing file data doesn't become > visible in the guest, but the added area is properly zeroed out. > > Consider the following scenario where the overlay is shorter than its > backing file: > > base.qcow2: AAAAAAAA > overlay.qcow2: BBBB > > When resizing (extending) overlay.qcow2, the new blocks should not stay > unallocated and make the additional As from base.qcow2 visible like > before this patch, but zeros should be read. > > A similar case happens with the various variants of a commit job when an > intermediate file is short (- for unallocated): > > base.qcow2: A-A-AAAA > mid.qcow2: BB-B > top.qcow2: C--C--C- > > After commit top.qcow2 to mid.qcow2, the following happens: > > mid.qcow2: CB-C00C0 (correct result) > mid.qcow2: CB-C--C- (before this fix) > > Without the fix, blocks that previously read as zeros on top.qcow2 > suddenly turn into A. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
diff --git a/block/io.c b/block/io.c index 003f4ea38c..6a5144f8d2 100644 --- a/block/io.c +++ b/block/io.c @@ -3385,12 +3385,44 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); if (ret < 0) { error_setg_errno(errp, -ret, "Could not refresh total sector count"); + goto fail_refresh_total_sectors; } else { offset = bs->total_sectors * BDRV_SECTOR_SIZE; } + + /* + * If the image has a backing file that is large enough that it would + * provide data for the new area, we cannot leave it unallocated because + * then the backing file content would become visible. Instead, zero-fill + * the area where backing file and new area overlap. + * + * Note that if the image has a backing file, but was opened without the + * backing file, taking care of keeping things consistent with that backing + * file is the user's responsibility. + */ + if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) { + int64_t backing_len; + + backing_len = bdrv_getlength(backing_bs(bs)); + if (backing_len < 0) { + ret = backing_len; + goto out; + } + + if (backing_len > old_size) { + ret = bdrv_co_do_pwrite_zeroes( + bs, old_size, MIN(new_bytes, backing_len - old_size), + BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP); + if (ret < 0) { + goto out; + } + } + } + /* It's possible that truncation succeeded but refresh_total_sectors * failed, but the latter doesn't affect how we should finish the request. * Pass 0 as the last parameter so that dirty bitmaps etc. are handled. */ +fail_refresh_total_sectors: bdrv_co_write_req_finish(child, offset - new_bytes, new_bytes, &req, 0); out:
When extending the size of an image that has a backing file larger than its old size, make sure that the backing file data doesn't become visible in the guest, but the added area is properly zeroed out. Consider the following scenario where the overlay is shorter than its backing file: base.qcow2: AAAAAAAA overlay.qcow2: BBBB When resizing (extending) overlay.qcow2, the new blocks should not stay unallocated and make the additional As from base.qcow2 visible like before this patch, but zeros should be read. A similar case happens with the various variants of a commit job when an intermediate file is short (- for unallocated): base.qcow2: A-A-AAAA mid.qcow2: BB-B top.qcow2: C--C--C- After commit top.qcow2 to mid.qcow2, the following happens: mid.qcow2: CB-C00C0 (correct result) mid.qcow2: CB-C--C- (before this fix) Without the fix, blocks that previously read as zeros on top.qcow2 suddenly turn into A. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/io.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)