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

From: Eric Blake <eblake@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: vsementsov@virtuozzo.com, qemu-devel@nongnu.org,
	stefanha@redhat.com, mreitz@redhat.com
Subject: Re: [PATCH v2 2/6] block: truncate: Don't make backing file data visible
Date: Wed, 20 Nov 2019 15:15:57 -0600	[thread overview]
Message-ID: <96c2c681-851d-4b83-2acd-3952fa850e5f@redhat.com> (raw)
In-Reply-To: <20191120184501.28159-3-kwolf@redhat.com>

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org