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 15:33:31 +0100	[thread overview]
Message-ID: <20191121143331.GG6007@linux.fritz.box> (raw)
In-Reply-To: <38b48cd4-a7b6-c2c0-db38-99c2192b6d05@redhat.com>

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

Am 21.11.2019 um 13:21 hat Max Reitz geschrieben:
> On 21.11.19 12:34, Kevin Wolf wrote:
> > 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?
> 
> Going through a deprecation cycle.

And keeping the buggy behaviour for two more releases?

> > Common cases (raw and qcow2 v3 without external data files) are quick
> > anyway.
> 
> Well, but quick enough for a fully blocking operation?

For raw definitely yes, because raw doesn't have backing files, so the
code will never run.

For qcow2, block_resize can already block for a relatively long time
while metadata tables are resized, clusters are discarded etc. I just
don't really see the difference in quality between that and allocating
some zero clusters in a corner case like having a short overlay.

Would you feel more comfortable if we set BDRV_REQ_NO_FALLBACK and
return an error if we can't zero out the area? We would have to
advertise that flag in bs->supported_zero_flags for qcow2 then (but
probably we should do that 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.
> 
> OK then.
> 
> > 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.
> 
> Good.
> 
> But I believe this is still a problem for block_resize.  I don’t see why
> this needs to be fixed in 4.2-rc3.  What’s the problem with going
> through a proper deprecation cycle other than the fact that we can’t
> start it in 4.2 because we don’t have a resize job yet?

That the behaviour is wrong.

For commit it's an image corruptor. For resize, I'll admit that it's
just wrong behaviour that is probably harmless in most cases, but it's
still wrong behaviour. It would be a corruptor in the same way as commit
if it allowed resizing intermediate nodes, but I _think_ the old-style
op blockers still forbid this. We'd have to double-check this if we
leave things broken for block_resize.

Anyway, so are you suggesting adding another parameter to
bdrv_co_truncate() that enables wrong, but quicker semantics, and that
would only be used by block_resize?

Kevin

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