Re: [PATCH v5 7/9] block: truncate: Don't make backing file data visible

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

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 <kwolf@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]