Re: [PATCH v2 2/6] block: truncate: Don't make backing file data visible

[Kevin Wolf]

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 <address@hidden>
> > ---
> >   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