[for-4.1?,0/4] block: Fix three .bdrv_has_zero_init()s
mbox series

Message ID 20190715104508.7568-1-mreitz@redhat.com
Headers show
Series
  • block: Fix three .bdrv_has_zero_init()s
Related show

Message

Max Reitz July 15, 2019, 10:45 a.m. UTC
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.)


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(-)

Comments

Stefano Garzarella July 15, 2019, 2:49 p.m. UTC | #1
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(-)
>