[Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Kevin Wolf]

Like for most other image formats, vhdx images read as all zero in qemu
after their creation (we're taking advantage from the fact that qemu has
just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
undefined rather than zeroed according to the spec).

This fixes that 'qemu-img convert' to vhdx fully populates the image.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vhdx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index 12bfe75..8a54da0 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1951,6 +1951,7 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_co_readv          = vhdx_co_readv,
     .bdrv_co_writev         = vhdx_co_writev,
     .bdrv_create            = vhdx_create,
+    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .bdrv_get_info          = vhdx_get_info,
     .bdrv_check             = vhdx_check,
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Max Reitz]

On 2014-12-05 at 11:26, Kevin Wolf wrote:
> Like for most other image formats, vhdx images read as all zero in qemu
> after their creation (we're taking advantage from the fact that qemu has
> just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> undefined rather than zeroed according to the spec).

I think whether qemu created the image doesn't matter. If it's 
undefined, it's completely valid for qemu to interpret it as zeroed.

So, as we discussed, the problem comes in when another program opens the 
image and actually interprets the data as something else than zero, 
which will be an issue if we used convert which then will have converted 
zeros from the input to basically random data (for qcow2 it will be zero 
again, but for other programs it might not be).

We could make block_state_zero=on non-optional, but that would mean we 
always would have to write the full BAT. Not so nice either. So what 
could probably solve the problem is a BDS-specific has_zero_init, which 
would be equal to the block_state_zero value here. But I'm not sure 
whether we want that either.

Max

> This fixes that 'qemu-img convert' to vhdx fully populates the image.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vhdx.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 12bfe75..8a54da0 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1951,6 +1951,7 @@ static BlockDriver bdrv_vhdx = {
>       .bdrv_co_readv          = vhdx_co_readv,
>       .bdrv_co_writev         = vhdx_co_writev,
>       .bdrv_create            = vhdx_create,
> +    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
>       .bdrv_get_info          = vhdx_get_info,
>       .bdrv_check             = vhdx_check,

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Kevin Wolf]

Am 05.12.2014 um 11:26 hat Kevin Wolf geschrieben:
> Like for most other image formats, vhdx images read as all zero in qemu
> after their creation (we're taking advantage from the fact that qemu has
> just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> undefined rather than zeroed according to the spec).

Brain fart, self-NACK.

This obviously means that 'qemu-img convert' still can't leave zeros in
the source file unwritten in the destination, otherwise we're corrupting
data. qemu would still read them correctly, but other implementations
might not.

We'd have to write the BAT for dynamic images with PAYLOAD_BLOCK_ZERO
for all clusters before we can make this change.

Jeff, do you want to fix this?

Kevin

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Jeff Cody]

On Fri, Dec 05, 2014 at 12:07:19PM +0100, Kevin Wolf wrote:
> Am 05.12.2014 um 11:26 hat Kevin Wolf geschrieben:
> > Like for most other image formats, vhdx images read as all zero in qemu
> > after their creation (we're taking advantage from the fact that qemu has
> > just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> > undefined rather than zeroed according to the spec).
> 
> Brain fart, self-NACK.
> 
> This obviously means that 'qemu-img convert' still can't leave zeros in
> the source file unwritten in the destination, otherwise we're corrupting
> data. qemu would still read them correctly, but other implementations
> might not.
> 
> We'd have to write the BAT for dynamic images with PAYLOAD_BLOCK_ZERO
> for all clusters before we can make this change.
> 
> Jeff, do you want to fix this?
>

Yes, I'll go ahead and put together a patch(es) for that.

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Kevin Wolf]

Am 05.12.2014 um 11:26 hat Kevin Wolf geschrieben:
> Like for most other image formats, vhdx images read as all zero in qemu
> after their creation (we're taking advantage from the fact that qemu has
> just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> undefined rather than zeroed according to the spec).
> 
> This fixes that 'qemu-img convert' to vhdx fully populates the image.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Jeff, another thing that Max found while we looked at the spec is that
the 1.0 spec defines PAYLOAD_BLOCK_UNMAPPED as 3, but we define it as 5.
It appears that the 0.95 spec actually had it that way.

Should we handle both 3 and 5 as unmapped?

Kevin

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Jeff Cody]

On Fri, Dec 05, 2014 at 12:49:22PM +0100, Kevin Wolf wrote:
> Am 05.12.2014 um 11:26 hat Kevin Wolf geschrieben:
> > Like for most other image formats, vhdx images read as all zero in qemu
> > after their creation (we're taking advantage from the fact that qemu has
> > just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> > undefined rather than zeroed according to the spec).
> > 
> > This fixes that 'qemu-img convert' to vhdx fully populates the image.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Jeff, another thing that Max found while we looked at the spec is that
> the 1.0 spec defines PAYLOAD_BLOCK_UNMAPPED as 3, but we define it as 5.
> It appears that the 0.95 spec actually had it that way.
> 
> Should we handle both 3 and 5 as unmapped?
> 

Wow - you are right, I checked the 1.0 spec change history. Since one
of the valid actions is to read zeros for PAYLOAD_BLOCK_UNMAPPED, it
would seem safe to do that for a technically undefined state of '5',
especially since that will maintain backwards compatibility of
0.95-spec images.  So I think 'yes'.

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Stefan Hajnoczi]

On Fri, Dec 05, 2014 at 11:26:49AM +0100, Kevin Wolf wrote:
> Like for most other image formats, vhdx images read as all zero in qemu
> after their creation (we're taking advantage from the fact that qemu has
> just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> undefined rather than zeroed according to the spec).
> 
> This fixes that 'qemu-img convert' to vhdx fully populates the image.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vhdx.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Kevin Wolf]

Am 23.10.2015 um 17:32 hat Stefan Hajnoczi geschrieben:
> On Fri, Dec 05, 2014 at 11:26:49AM +0100, Kevin Wolf wrote:
> > Like for most other image formats, vhdx images read as all zero in qemu
> > after their creation (we're taking advantage from the fact that qemu has
> > just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> > undefined rather than zeroed according to the spec).
> > 
> > This fixes that 'qemu-img convert' to vhdx fully populates the image.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/vhdx.c | 1 +
> >  1 file changed, 1 insertion(+)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Hasn't this been implemented in commit 85b712c9 almost a year ago?

Kevin

Re: [Qemu-devel] [PATCH] vhdx: Return true for bdrv_has_zero_init

[Stefan Hajnoczi]

On Fri, Oct 23, 2015 at 06:00:52PM +0200, Kevin Wolf wrote:
> Am 23.10.2015 um 17:32 hat Stefan Hajnoczi geschrieben:
> > On Fri, Dec 05, 2014 at 11:26:49AM +0100, Kevin Wolf wrote:
> > > Like for most other image formats, vhdx images read as all zero in qemu
> > > after their creation (we're taking advantage from the fact that qemu has
> > > just created the image, because PAYLOAD_BLOCK_NOT_PRESENT actually means
> > > undefined rather than zeroed according to the spec).
> > > 
> > > This fixes that 'qemu-img convert' to vhdx fully populates the image.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/vhdx.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Hasn't this been implemented in commit 85b712c9 almost a year ago?

Yes :).

I was cleaning out my inbox and didn't notice the date until after
replying.