On Mon, Jul 15, 2019 at 12:45:04PM +0200, Max Reitz wrote:
> Hi,
>
> .bdrv_has_zero_init() must not return 1 if the (newly created[1]) image
> may not return zeroes when read.
>
> [1] This is guaranteed by the caller.
>
> If the image is preallocated, this generally depends on the protocol
> layer, because the format layer will just allocate the necessary
> metadata, make it point to data blocks and leave their initialization to
> the protocol layer. For example, qcow2:
>
> - leaves the blocks uninitialized with preallocation=metadata,
> - and passes preallocation=falloc and =full on to the protocol node.
>
> In either case, the data then stored in these blocks fully depends on
> the protocol level.
>
> Therefore, format drivers have to pass through .bdrv_has_zero_init() to
> the data storage node when dealing with preallocated images.
>
> Protocol drivers OTOH have to be accurate in what they return from
> .bdrv_has_zero_init(). They are free to return 0 even for preallocated
> images.
>
>
> So let’s look at the existing .bdrv_has_zero_init() implementations:
>
> - file-posix: Always returns 1 (for regular files). Correct, because it
> makes sure the image always reads as 0, preallocated or not.
>
> - file-win32: Same. (But doesn’t even support preallocation.)
>
> - gluster: Always returns 0. Safe.
>
> - nfs: Only returns 1 for regular files, similarly to file-posix. Seems
> reasonable.
>
> - parallels: Always returns 1. This format does not support
> preallocation, but apparently ensures that it always writes out data
> that reads back as 0 (where necessary), because if the protocol node
> does not have_zero_init, it explicitly writes zeroes to it instead of
> just truncating it.
> So this driver seems OK, too.
>
> - qcow2: Always returns 1. This is wrong for preallocated images, and
> really wrong for preallocated encrypted images. Addressed by patch 1.
>
> - qcow: Always returns 1. Has no preallocation support, so that seems
> OK.
>
> - qed: Same as qcow.
>
> - raw: Always forwards the value from the filtered node. OK.
>
> - rbd: Always returns 1. This is a protocol driver, so I’ll just trust
> it knows what it’s doing.
>
> - sheepdog: Always returns 1. From the fact that its preallocation code
> simply reads the image and writes it back, this seems correct to me.
>
> - ssh: Same as nfs.
>
> - vdi: Always returns 1. It does support preallocation=metadata, in
> which case this may be wrong. Addressed by patch 2.
>
> - vhdx: Similar to vdi, except it doesn’t support @preallocation, but
> has its own option “subformat=fixed”. Addressed by patch 3.
>
> - vmdk: Hey, this one is already exactly what we want. If any of the
> extents is flat, it goes to the respective protocol node, and if that
> does not have_zero_init, it returns 0. Great.
> (Added in commit da7a50f9385.)
>
> - vpc: Hey, this one, too. With subformat=fixed, it returns what the
> protocol node has to say about has_zero_init.
> (Added in commit 72c6cc94daa.)
>
> So that leaves three cases to fix, which are the first three patches in
> this series. The final patch adds a test case for qcow2. (It’s
> difficult to test the other drivers, because that would require a
> protocol driver with image creation support and has_zero_init=0, which
> is not so easily available.)
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
Thanks,
Stefano
>
>
> Max Reitz (4):
> qcow2: Fix .bdrv_has_zero_init()
> vdi: Fix .bdrv_has_zero_init()
> vhdx: Fix .bdrv_has_zero_init()
> iotests: Convert to preallocated encrypted qcow2
>
> block/qcow2.c | 90 +++++++++++++++++++++++++++++++++++++-
> block/vdi.c | 13 +++++-
> block/vhdx.c | 21 ++++++++-
> tests/qemu-iotests/188 | 20 ++++++++-
> tests/qemu-iotests/188.out | 4 ++
> 5 files changed, 144 insertions(+), 4 deletions(-)
>