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

From: Kevin Wolf <kwolf@redhat.com>
To: Max Reitz <mreitz@redhat.com>
Cc: vsementsov@virtuozzo.com, stefanha@redhat.com,
	qemu-block@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 2/6] block: truncate: Don't make backing file data visible
Date: Thu, 21 Nov 2019 12:34:05 +0100	[thread overview]
Message-ID: <20191121113405.GE6007@linux.fritz.box> (raw)
In-Reply-To: <c53f6f2d-2451-2fb3-cbf0-2a1a686cccf5@redhat.com>

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

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

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