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

[Max Reitz]

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

> Common cases (raw and qcow2 v3 without external data files) are quick
> anyway.

Well, but quick enough for a fully blocking operation?

>> 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?

Max

signature.asc
Description: OpenPGP digital signature