Re: [Qemu-devel] [PATCH for-4.1? 1/4] qcow2: Fix .bdrv_has_zero_init()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2516 bytes --]

On 16.07.19 18:54, Kevin Wolf wrote:
> Am 15.07.2019 um 12:45 hat Max Reitz geschrieben:
>> If a qcow2 file is preallocated, it can no longer guarantee that it
>> initially appears as filled with zeroes.
>>
>> So implement .bdrv_has_zero_init() by checking whether the file is
>> preallocated; if so, forward the call to the underlying storage node,
>> except for when it is encrypted: Encrypted preallocated images always
>> return effectively random data, so .bdrv_has_zero_init() must always
>> return 0 for them.
>>
>> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Hm... This patch only really works directly after image creation (which
> is indeed where .bdrv_has_zero_init is used). Why do we have to have a
> full qcow2_is_zero() that loops over the whole image just to find out
> whether it's preallocated? Wouldn't looking at a single data cluster be
> enough?

Hm.  I would like to agree (because you’re right), but now I see that
the callers of bdrv_has_zero_init() don’t necessarily hold to that
convention.

For example, qemu-img convert has the -n flag, but that doesn’t stop it
from invoking bdrv_has_zero_init().

Which is a bug, of course.

$ ./qemu-img create -f qcow2 src.qcow2 64M
$ ./qemu-img create -f qcow2 dest.qcow2 64M
$ ./qemu-io -c 'write -P 42 0 64M' dest.qcow2
$ ./qemu-img convert -n src.qcow2 dest.qcow2
$ ./qemu-img compare src.qcow2 dest.qcow2
Content mismatch at offset 0!

Aw, man, why does this keep happening... :-/

OK, so qemu-img convert -n is easy to fix.

But there are more callers:

mirror: Uses this function to inquire whether it needs to zero the
target before actually doing something useful.  There is no guarantee
that the target is a new image.  Well, it just isn’t with mode=existing
or blockdev-mirror.

parallels: Whether to write zeroes to newly added image areas.  That
actually sounds correct, because those new areas cannot point to any
data yet.
Well, maybe not correct, because bdrv_has_zero_init() is not the same as
“when this image grows, new areas will be zero”, but at least
bdrv_hsa_zero_init() will return false if the the latter is false.

vhdx: Similarly to parallels, it uses this information to check whether
it needs to zero new areas when growing an image file.

raw/vmdk/vpc: Just passing through info from their storage child.


Hm, OK.  So mirror and qemu-img need fixing.  That sounds possible.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]