Am 23.04.2020 um 13:14 hat Max Reitz geschrieben: > On 22.04.20 17:21, 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 <[email protected]> > > Reviewed-by: Alberto Garcia <[email protected]> > > --- > > block/io.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/block/io.c b/block/io.c > > index 795075954e..8fbb607515 100644 > > --- a/block/io.c > > +++ b/block/io.c > > @@ -3394,6 +3394,20 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, > > int64_t offset, bool exact, > > goto out; > > } > > > > + /* > > + * 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 new area. > > + * > > + * 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) { > > + flags |= BDRV_REQ_ZERO_WRITE; > > + } > > This breaks growing any non-qcow2 image with any backing file. Do we > care about that? > > The comment says something about “a backing file that is large enough > that it would provide data for the new area”, but that condition doesn’t > appear in the code. Should it? (If it did, I think the number of cases > this change broke would be much smaller.) > > If it was deliberate to not have that condition here, and if we decide > that we don’t care about non-qcow2 formats here, then I think at least > the error message deserves some improvement over “qemu-img: Block driver > does not support requested flags”.
This was not deliberate. v3 had the check and I'm not sure why I removed it. Probably because the new approach felt so much simpler and I was glad that I could throw away complicated code that I threw away more than I should have... Kevin
signature.asc
Description: PGP signature
