Bug? qemu-img convert to preallocated image makes it sparse

Bug? qemu-img convert to preallocated image makes it sparse

[Richard W.M. Jones]

I'm not necessarily saying this is a bug, but a change in behaviour in
qemu has caused virt-v2v to fail.  The reproducer is quite simple.

Create sparse and preallocated qcow2 files of the same size:

  $ qemu-img create -f qcow2 sparse.qcow2 50M
  Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16

  $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
  Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16

  $ du -m sparse.qcow2 prealloc.qcow2 
  1 sparse.qcow2
  51	prealloc.qcow2

Now copy the sparse file into the preallocated file using the -n
option so qemu-img doesn't create the target:

  $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
      (100.00/100%)

In new qemu that makes the target file sparse:

  $ du -m sparse.qcow2 prealloc.qcow2 
  1 sparse.qcow2
  1 prealloc.qcow2         <-- should still be 51

In old qemu the target file remained preallocated, which is what
I and virt-v2v are expecting.

I bisected this to the following commit:

4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
Author: Max Reitz <mreitz@redhat.com>
Date:   Wed Jul 24 19:12:29 2019 +0200

    qemu-img: Fix bdrv_has_zero_init() use in convert
    
    bdrv_has_zero_init() only has meaning for newly created images or image
    areas.  If qemu-img convert did not create the image itself, it cannot
    rely on bdrv_has_zero_init()'s result to carry any meaning.
    
    Signed-off-by: Max Reitz <mreitz@redhat.com>
    Message-id: 20190724171239.8764-2-mreitz@redhat.com
    Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
    Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
    Signed-off-by: Max Reitz <mreitz@redhat.com>

 qemu-img.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Reverting this commit on the current master branch restores the
expected behaviour.

Thoughts?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 15:13, Richard W.M. Jones wrote:
> I'm not necessarily saying this is a bug, but a change in behaviour in
> qemu has caused virt-v2v to fail.  The reproducer is quite simple.
> 
> Create sparse and preallocated qcow2 files of the same size:
> 
>   $ qemu-img create -f qcow2 sparse.qcow2 50M
>   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> 
>   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
>   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
> 
>   $ du -m sparse.qcow2 prealloc.qcow2 
>   1 sparse.qcow2
>   51	prealloc.qcow2
> 
> Now copy the sparse file into the preallocated file using the -n
> option so qemu-img doesn't create the target:
> 
>   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
>       (100.00/100%)
> 
> In new qemu that makes the target file sparse:
> 
>   $ du -m sparse.qcow2 prealloc.qcow2 
>   1 sparse.qcow2
>   1 prealloc.qcow2         <-- should still be 51
> 
> In old qemu the target file remained preallocated, which is what
> I and virt-v2v are expecting.
> 
> I bisected this to the following commit:
> 
> 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> Author: Max Reitz <mreitz@redhat.com>
> Date:   Wed Jul 24 19:12:29 2019 +0200
> 
>     qemu-img: Fix bdrv_has_zero_init() use in convert
>     
>     bdrv_has_zero_init() only has meaning for newly created images or image
>     areas.  If qemu-img convert did not create the image itself, it cannot
>     rely on bdrv_has_zero_init()'s result to carry any meaning.
>     
>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>     Message-id: 20190724171239.8764-2-mreitz@redhat.com
>     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>     Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>  qemu-img.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Reverting this commit on the current master branch restores the
> expected behaviour.

The commit changed the behavior because now qemu-img realizes that it
cannot skip writing to areas that are supposed to be zero when it
converts to an existing image (because we have no idea what data that
existing image contains).  So that’s a bug fix, and I don’t think we can
undo it without being wrong.

The problem is that qemu-img will try to be quickthat about making these
areas zero, and so it does zero writes (actually, it even zeroes the
whole image) and in the process it will of course discard all preallocation.

Now, about fixing the problem I’m not so sure.

The problem is that it isn’t easy for qemu-img or the qcow2 driver to
determine whether a preallocated area is zero.  It needs to read the
data from disk and compare it to zero.  That would be slow and not trivial.

So off top of my head, the only thing that comes to my mind would be a
new flag for convert that lets you guarantee the image is zero and qemu
doesn’t need to zero it.

The problem with this is that I don’t think we ever guaranteed that
preallocated images stay preallocated when written to, and so even if we
assume the image is fully zero and thus restore the old behavior for
your case, we might break it in the future again.


So are there any ways to safely convert an image to an existing one and
keeping the destinations preallocation intact?  Sadly, I don’t think
there is.  Well, you can always use -S 0, but that will change your
falloc preallocation into a full one.

So I suppose the best idea I can come up with is indeed a
--target-is-zero flag for qemu-img convert -n.  Would that work for you?

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 15:13, Richard W.M. Jones wrote:
> I'm not necessarily saying this is a bug, but a change in behaviour in
> qemu has caused virt-v2v to fail.  The reproducer is quite simple.
> 
> Create sparse and preallocated qcow2 files of the same size:
> 
>   $ qemu-img create -f qcow2 sparse.qcow2 50M
>   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> 
>   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
>   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
> 
>   $ du -m sparse.qcow2 prealloc.qcow2 
>   1 sparse.qcow2
>   51	prealloc.qcow2
> 
> Now copy the sparse file into the preallocated file using the -n
> option so qemu-img doesn't create the target:
> 
>   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
>       (100.00/100%)
> 
> In new qemu that makes the target file sparse:
> 
>   $ du -m sparse.qcow2 prealloc.qcow2 
>   1 sparse.qcow2
>   1 prealloc.qcow2         <-- should still be 51
> 
> In old qemu the target file remained preallocated, which is what
> I and virt-v2v are expecting.
> 
> I bisected this to the following commit:
> 
> 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> Author: Max Reitz <mreitz@redhat.com>
> Date:   Wed Jul 24 19:12:29 2019 +0200
> 
>     qemu-img: Fix bdrv_has_zero_init() use in convert
>     
>     bdrv_has_zero_init() only has meaning for newly created images or image
>     areas.  If qemu-img convert did not create the image itself, it cannot
>     rely on bdrv_has_zero_init()'s result to carry any meaning.
>     
>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>     Message-id: 20190724171239.8764-2-mreitz@redhat.com
>     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>     Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>  qemu-img.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Reverting this commit on the current master branch restores the
> expected behaviour.

So what this commit changed was that when you take an existing image as
the destination, you can’t assume anything about its contents.  Before
this commit, we assumed it’s zero.  That’s clearly wrong, because it can
be anything.

So when you convert to the target image, you have to make sure all areas
that are zero in the source are zero in the target, too.  The way we do
that is to write zeroes to the target.  The problem is that this
operation disregards the previous preallocation and discards the
preallocated space.

As for fixing the bug...  Can we fix it in qemu(-img)?

We could try to detect whether areas that are zero in the source are
zero in the (preallocated) target image, too.  But doing so what require
reading the data from those areas and comparing it to zero.  That would
take time and it isn’t trivial.  So that’s something I’d rather avoid.

Off the top of my head, the only thing that comes to my mind would be to
add a flag to qemu-img convert with which you can let it know that you
guarantee the target image is zero.  I suppose we could document it also
to imply that given this flag, areas that are zero in the source will
then not be changed in the target image; i.e. that preallocation stays
intact in those areas.


OTOH, can it be fixed in virt-v2v?  Is there already a safe way to call
qemu-img convert -n and keeping the target’s preallocation intact?
Unfortunately, I don’t think so.  I don’t think we ever guaranteed it
would, and well, now it broke.


So would you be OK with a --target-is-zero flag?  (I think we could let
this flag guarantee that your use case works, so it should be future-safe.)

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

Am 16.01.2020 um 15:37 hat Max Reitz geschrieben:
> On 16.01.20 15:13, Richard W.M. Jones wrote:
> > I'm not necessarily saying this is a bug, but a change in behaviour in
> > qemu has caused virt-v2v to fail.  The reproducer is quite simple.
> > 
> > Create sparse and preallocated qcow2 files of the same size:
> > 
> >   $ qemu-img create -f qcow2 sparse.qcow2 50M
> >   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > 
> >   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
> >   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
> > 
> >   $ du -m sparse.qcow2 prealloc.qcow2 
> >   1 sparse.qcow2
> >   51	prealloc.qcow2
> > 
> > Now copy the sparse file into the preallocated file using the -n
> > option so qemu-img doesn't create the target:
> > 
> >   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
> >       (100.00/100%)
> > 
> > In new qemu that makes the target file sparse:
> > 
> >   $ du -m sparse.qcow2 prealloc.qcow2 
> >   1 sparse.qcow2
> >   1 prealloc.qcow2         <-- should still be 51
> > 
> > In old qemu the target file remained preallocated, which is what
> > I and virt-v2v are expecting.
> > 
> > I bisected this to the following commit:
> > 
> > 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> > commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> > Author: Max Reitz <mreitz@redhat.com>
> > Date:   Wed Jul 24 19:12:29 2019 +0200
> > 
> >     qemu-img: Fix bdrv_has_zero_init() use in convert
> >     
> >     bdrv_has_zero_init() only has meaning for newly created images or image
> >     areas.  If qemu-img convert did not create the image itself, it cannot
> >     rely on bdrv_has_zero_init()'s result to carry any meaning.
> >     
> >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> >     Message-id: 20190724171239.8764-2-mreitz@redhat.com
> >     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> >  qemu-img.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > Reverting this commit on the current master branch restores the
> > expected behaviour.
> 
> The commit changed the behavior because now qemu-img realizes that it
> cannot skip writing to areas that are supposed to be zero when it
> converts to an existing image (because we have no idea what data that
> existing image contains).  So that’s a bug fix, and I don’t think we can
> undo it without being wrong.
> 
> The problem is that qemu-img will try to be quickthat about making these
> areas zero, and so it does zero writes (actually, it even zeroes the
> whole image) and in the process it will of course discard all preallocation.
> 
> Now, about fixing the problem I’m not so sure.

Wouldn't just passing -S 0 solve the problem? It should tell qemu-img
convert that you don't want it to sparsify anything.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Richard W.M. Jones]

On Thu, Jan 16, 2020 at 03:47:30PM +0100, Max Reitz wrote:
> On 16.01.20 15:13, Richard W.M. Jones wrote:
> > I'm not necessarily saying this is a bug, but a change in behaviour in
> > qemu has caused virt-v2v to fail.  The reproducer is quite simple.
> > 
> > Create sparse and preallocated qcow2 files of the same size:
> > 
> >   $ qemu-img create -f qcow2 sparse.qcow2 50M
> >   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > 
> >   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
> >   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
> > 
> >   $ du -m sparse.qcow2 prealloc.qcow2 
> >   1 sparse.qcow2
> >   51	prealloc.qcow2
> > 
> > Now copy the sparse file into the preallocated file using the -n
> > option so qemu-img doesn't create the target:
> > 
> >   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
> >       (100.00/100%)
> > 
> > In new qemu that makes the target file sparse:
> > 
> >   $ du -m sparse.qcow2 prealloc.qcow2 
> >   1 sparse.qcow2
> >   1 prealloc.qcow2         <-- should still be 51
> > 
> > In old qemu the target file remained preallocated, which is what
> > I and virt-v2v are expecting.
> > 
> > I bisected this to the following commit:
> > 
> > 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> > commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> > Author: Max Reitz <mreitz@redhat.com>
> > Date:   Wed Jul 24 19:12:29 2019 +0200
> > 
> >     qemu-img: Fix bdrv_has_zero_init() use in convert
> >     
> >     bdrv_has_zero_init() only has meaning for newly created images or image
> >     areas.  If qemu-img convert did not create the image itself, it cannot
> >     rely on bdrv_has_zero_init()'s result to carry any meaning.
> >     
> >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> >     Message-id: 20190724171239.8764-2-mreitz@redhat.com
> >     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> > 
> >  qemu-img.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > Reverting this commit on the current master branch restores the
> > expected behaviour.
> 
> So what this commit changed was that when you take an existing image as
> the destination, you can’t assume anything about its contents.  Before
> this commit, we assumed it’s zero.  That’s clearly wrong, because it can
> be anything.
> 
> So when you convert to the target image, you have to make sure all areas
> that are zero in the source are zero in the target, too.  The way we do
> that is to write zeroes to the target.  The problem is that this
> operation disregards the previous preallocation and discards the
> preallocated space.
> 
> As for fixing the bug...  Can we fix it in qemu(-img)?
> 
> We could try to detect whether areas that are zero in the source are
> zero in the (preallocated) target image, too.  But doing so what require
> reading the data from those areas and comparing it to zero.  That would
> take time and it isn’t trivial.  So that’s something I’d rather avoid.
> 
> Off the top of my head, the only thing that comes to my mind would be to
> add a flag to qemu-img convert with which you can let it know that you
> guarantee the target image is zero.  I suppose we could document it also
> to imply that given this flag, areas that are zero in the source will
> then not be changed in the target image; i.e. that preallocation stays
> intact in those areas.
> 
> 
> OTOH, can it be fixed in virt-v2v?  Is there already a safe way to call
> qemu-img convert -n and keeping the target’s preallocation intact?
> Unfortunately, I don’t think so.  I don’t think we ever guaranteed it
> would, and well, now it broke.

From the fixing virt-v2v point of view, it's a bit tricky since the
code has to deal with all kinds of output targets.  (For example we
sometimes qemu-img convert into an NBD target.)

However we do know when the target contains zeroes - in fact it always
contains zeroes, so:

> So would you be OK with a --target-is-zero flag?  (I think we could let
> this flag guarantee that your use case works, so it should be future-safe.)

this one should work.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 15:50, Kevin Wolf wrote:
> Am 16.01.2020 um 15:37 hat Max Reitz geschrieben:
>> On 16.01.20 15:13, Richard W.M. Jones wrote:
>>> I'm not necessarily saying this is a bug, but a change in behaviour in
>>> qemu has caused virt-v2v to fail.  The reproducer is quite simple.
>>>
>>> Create sparse and preallocated qcow2 files of the same size:
>>>
>>>   $ qemu-img create -f qcow2 sparse.qcow2 50M
>>>   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>>
>>>   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
>>>   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
>>>
>>>   $ du -m sparse.qcow2 prealloc.qcow2 
>>>   1 sparse.qcow2
>>>   51	prealloc.qcow2
>>>
>>> Now copy the sparse file into the preallocated file using the -n
>>> option so qemu-img doesn't create the target:
>>>
>>>   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
>>>       (100.00/100%)
>>>
>>> In new qemu that makes the target file sparse:
>>>
>>>   $ du -m sparse.qcow2 prealloc.qcow2 
>>>   1 sparse.qcow2
>>>   1 prealloc.qcow2         <-- should still be 51
>>>
>>> In old qemu the target file remained preallocated, which is what
>>> I and virt-v2v are expecting.
>>>
>>> I bisected this to the following commit:
>>>
>>> 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
>>> commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
>>> Author: Max Reitz <mreitz@redhat.com>
>>> Date:   Wed Jul 24 19:12:29 2019 +0200
>>>
>>>     qemu-img: Fix bdrv_has_zero_init() use in convert
>>>     
>>>     bdrv_has_zero_init() only has meaning for newly created images or image
>>>     areas.  If qemu-img convert did not create the image itself, it cannot
>>>     rely on bdrv_has_zero_init()'s result to carry any meaning.
>>>     
>>>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>     Message-id: 20190724171239.8764-2-mreitz@redhat.com
>>>     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>>>     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>>  qemu-img.c | 11 ++++++++---
>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> Reverting this commit on the current master branch restores the
>>> expected behaviour.
>>
>> The commit changed the behavior because now qemu-img realizes that it
>> cannot skip writing to areas that are supposed to be zero when it
>> converts to an existing image (because we have no idea what data that
>> existing image contains).  So that’s a bug fix, and I don’t think we can
>> undo it without being wrong.
>>
>> The problem is that qemu-img will try to be quickthat about making these
>> areas zero, and so it does zero writes (actually, it even zeroes the
>> whole image) and in the process it will of course discard all preallocation.
>>
>> Now, about fixing the problem I’m not so sure.
> 
> Wouldn't just passing -S 0 solve the problem? It should tell qemu-img
> convert that you don't want it to sparsify anything.

But it would also convert the falloc preallocation to a full one.

(I had a section to this effect in my first draft, but then I
accidentally deleted it and forgot it in my second version...)

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Eric Blake]

On 1/16/20 8:47 AM, Max Reitz wrote:

> So when you convert to the target image, you have to make sure all areas
> that are zero in the source are zero in the target, too.  The way we do
> that is to write zeroes to the target.  The problem is that this
> operation disregards the previous preallocation and discards the
> preallocated space.
> 
> As for fixing the bug...  Can we fix it in qemu(-img)?
> 
> We could try to detect whether areas that are zero in the source are
> zero in the (preallocated) target image, too.  But doing so what require
> reading the data from those areas and comparing it to zero.  That would
> take time and it isn’t trivial.  So that’s something I’d rather avoid.

Can't we also use block status queries on the destination, as that is 
likely to be faster than actual reads and comparison to zero?


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 15:57, Eric Blake wrote:
> On 1/16/20 8:47 AM, Max Reitz wrote:
> 
>> So when you convert to the target image, you have to make sure all areas
>> that are zero in the source are zero in the target, too.  The way we do
>> that is to write zeroes to the target.  The problem is that this
>> operation disregards the previous preallocation and discards the
>> preallocated space.
>>
>> As for fixing the bug...  Can we fix it in qemu(-img)?
>>
>> We could try to detect whether areas that are zero in the source are
>> zero in the (preallocated) target image, too.  But doing so what require
>> reading the data from those areas and comparing it to zero.  That would
>> take time and it isn’t trivial.  So that’s something I’d rather avoid.
> 
> Can't we also use block status queries on the destination, as that is
> likely to be faster than actual reads and comparison to zero?

It might work for falloc preallocation, but I don’t know whether we are
really guaranteed that fallocated() areas return holes through lseek().

It won’t work for full preallocation, though.  Yes, you might get around
that with -S 0 then, but I think overall the simplest thing would be a
--target-is-zero flag.  (I don’t know, it seems useful to me in general.
 For example, when you convert to a new block device.  The biggest
drawback I see is that people might use it blindly and then complain
that their image contains garbage...)

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Maxim Levitsky]

On Thu, 2020-01-16 at 15:55 +0100, Max Reitz wrote:
> On 16.01.20 15:50, Kevin Wolf wrote:
> > Am 16.01.2020 um 15:37 hat Max Reitz geschrieben:
> > > On 16.01.20 15:13, Richard W.M. Jones wrote:
> > > > I'm not necessarily saying this is a bug, but a change in behaviour in
> > > > qemu has caused virt-v2v to fail.  The reproducer is quite simple.
> > > > 
> > > > Create sparse and preallocated qcow2 files of the same size:
> > > > 
> > > >   $ qemu-img create -f qcow2 sparse.qcow2 50M
> > > >   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > > > 
> > > >   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
> > > >   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
> > > > 
> > > >   $ du -m sparse.qcow2 prealloc.qcow2 
> > > >   1 sparse.qcow2
> > > >   51	prealloc.qcow2
> > > > 
> > > > Now copy the sparse file into the preallocated file using the -n
> > > > option so qemu-img doesn't create the target:
> > > > 
> > > >   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
> > > >       (100.00/100%)
> > > > 
> > > > In new qemu that makes the target file sparse:
> > > > 
> > > >   $ du -m sparse.qcow2 prealloc.qcow2 
> > > >   1 sparse.qcow2
> > > >   1 prealloc.qcow2         <-- should still be 51
> > > > 
> > > > In old qemu the target file remained preallocated, which is what
> > > > I and virt-v2v are expecting.
> > > > 
> > > > I bisected this to the following commit:
> > > > 
> > > > 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
> > > > commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
> > > > Author: Max Reitz <mreitz@redhat.com>
> > > > Date:   Wed Jul 24 19:12:29 2019 +0200
> > > > 
> > > >     qemu-img: Fix bdrv_has_zero_init() use in convert
> > > >     
> > > >     bdrv_has_zero_init() only has meaning for newly created images or image
> > > >     areas.  If qemu-img convert did not create the image itself, it cannot
> > > >     rely on bdrv_has_zero_init()'s result to carry any meaning.
> > > >     
> > > >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > >     Message-id: 20190724171239.8764-2-mreitz@redhat.com
> > > >     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >     Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > > 
> > > >  qemu-img.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > Reverting this commit on the current master branch restores the
> > > > expected behaviour.
> > > 
> > > The commit changed the behavior because now qemu-img realizes that it
> > > cannot skip writing to areas that are supposed to be zero when it
> > > converts to an existing image (because we have no idea what data that
> > > existing image contains).  So that’s a bug fix, and I don’t think we can
> > > undo it without being wrong.
> > > 
> > > The problem is that qemu-img will try to be quickthat about making these
> > > areas zero, and so it does zero writes (actually, it even zeroes the
> > > whole image) and in the process it will of course discard all preallocation.
> > > 
> > > Now, about fixing the problem I’m not so sure.
> > 
> > Wouldn't just passing -S 0 solve the problem? It should tell qemu-img
> > convert that you don't want it to sparsify anything.
> 
> But it would also convert the falloc preallocation to a full one.
> 
> (I had a section to this effect in my first draft, but then I
> accidentally deleted it and forgot it in my second version...)
> 
> Max
> 

How about doing write zeros without discard only in this particular case (convert to existing image)
Basically omitting the BDRV_REQ_MAY_UNMAP flag to blk_co_pwrite_zeroes.
It will be slow, but maybe for this particular case, it is acceptable?

Best regards,
	Maxim Levitsky

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 16:38, Maxim Levitsky wrote:
> On Thu, 2020-01-16 at 15:55 +0100, Max Reitz wrote:
>> On 16.01.20 15:50, Kevin Wolf wrote:
>>> Am 16.01.2020 um 15:37 hat Max Reitz geschrieben:
>>>> On 16.01.20 15:13, Richard W.M. Jones wrote:
>>>>> I'm not necessarily saying this is a bug, but a change in behaviour in
>>>>> qemu has caused virt-v2v to fail.  The reproducer is quite simple.
>>>>>
>>>>> Create sparse and preallocated qcow2 files of the same size:
>>>>>
>>>>>   $ qemu-img create -f qcow2 sparse.qcow2 50M
>>>>>   Formatting 'sparse.qcow2', fmt=qcow2 size=52428800 cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>>>>
>>>>>   $ qemu-img create -f qcow2 prealloc.qcow2 50M -o preallocation=falloc,compat=1.1
>>>>>   Formatting 'prealloc.qcow2', fmt=qcow2 size=52428800 compat=1.1 cluster_size=65536 preallocation=falloc lazy_refcounts=off refcount_bits=16
>>>>>
>>>>>   $ du -m sparse.qcow2 prealloc.qcow2 
>>>>>   1 sparse.qcow2
>>>>>   51	prealloc.qcow2
>>>>>
>>>>> Now copy the sparse file into the preallocated file using the -n
>>>>> option so qemu-img doesn't create the target:
>>>>>
>>>>>   $ qemu-img convert -p -n -f qcow2 -O qcow2 sparse.qcow2 prealloc.qcow2
>>>>>       (100.00/100%)
>>>>>
>>>>> In new qemu that makes the target file sparse:
>>>>>
>>>>>   $ du -m sparse.qcow2 prealloc.qcow2 
>>>>>   1 sparse.qcow2
>>>>>   1 prealloc.qcow2         <-- should still be 51
>>>>>
>>>>> In old qemu the target file remained preallocated, which is what
>>>>> I and virt-v2v are expecting.
>>>>>
>>>>> I bisected this to the following commit:
>>>>>
>>>>> 4d7c487eac1652dfe4498fe84f32900ad461d61b is the first bad commit
>>>>> commit 4d7c487eac1652dfe4498fe84f32900ad461d61b
>>>>> Author: Max Reitz <mreitz@redhat.com>
>>>>> Date:   Wed Jul 24 19:12:29 2019 +0200
>>>>>
>>>>>     qemu-img: Fix bdrv_has_zero_init() use in convert
>>>>>     
>>>>>     bdrv_has_zero_init() only has meaning for newly created images or image
>>>>>     areas.  If qemu-img convert did not create the image itself, it cannot
>>>>>     rely on bdrv_has_zero_init()'s result to carry any meaning.
>>>>>     
>>>>>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>     Message-id: 20190724171239.8764-2-mreitz@redhat.com
>>>>>     Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>>>>>     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>>     Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>
>>>>>  qemu-img.c | 11 ++++++++---
>>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> Reverting this commit on the current master branch restores the
>>>>> expected behaviour.
>>>>
>>>> The commit changed the behavior because now qemu-img realizes that it
>>>> cannot skip writing to areas that are supposed to be zero when it
>>>> converts to an existing image (because we have no idea what data that
>>>> existing image contains).  So that’s a bug fix, and I don’t think we can
>>>> undo it without being wrong.
>>>>
>>>> The problem is that qemu-img will try to be quickthat about making these
>>>> areas zero, and so it does zero writes (actually, it even zeroes the
>>>> whole image) and in the process it will of course discard all preallocation.
>>>>
>>>> Now, about fixing the problem I’m not so sure.
>>>
>>> Wouldn't just passing -S 0 solve the problem? It should tell qemu-img
>>> convert that you don't want it to sparsify anything.
>>
>> But it would also convert the falloc preallocation to a full one.
>>
>> (I had a section to this effect in my first draft, but then I
>> accidentally deleted it and forgot it in my second version...)
>>
>> Max
>>
> 
> How about doing write zeros without discard only in this particular case (convert to existing image)
> Basically omitting the BDRV_REQ_MAY_UNMAP flag to blk_co_pwrite_zeroes.
> It will be slow, but maybe for this particular case, it is acceptable?

I don’t think so, because AFAIA one relatively common case is to convert
to an existing block device, and we want to zero that quickly.

Or if the target image actually stores some data, I can imagine that
people actually want a discarding unmap so the image doesn’t use more
space than necessary.

(Also, if we resolve this without any new kind of flag, I’m not sure
whether we can guarantee to keep the desired behavior in the future,
because maybe something else will force us to forego existing
preallocation unless we know that the user really wants to keep it.  I
think there are just different use cases that convert -n is used for and
it makes sense to add a flag to distinguish between them.)

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Richard W.M. Jones]

On Thu, Jan 16, 2020 at 05:38:03PM +0200, Maxim Levitsky wrote:
> How about doing write zeros without discard only in this particular case (convert to existing image)
> Basically omitting the BDRV_REQ_MAY_UNMAP flag to blk_co_pwrite_zeroes.
> It will be slow, but maybe for this particular case, it is acceptable?

I should probably say that we don't want to break the other case
(which is likely more important) where we write a sparse source to a
sparse target and want the target to contain only the union of the two
sparse maps, not fully allocated :-)

It would be fine, I think, to have a new "make this disk fully
allocated" operation.  qemu-img resize could almost do it with a
request to add 0 extra bytes, but the --preallocation flag only
applies to the new space.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: Bug? qemu-img convert to preallocated image makes it sparse

[Max Reitz]

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

On 16.01.20 17:00, Richard W.M. Jones wrote:
> On Thu, Jan 16, 2020 at 05:38:03PM +0200, Maxim Levitsky wrote:
>> How about doing write zeros without discard only in this particular case (convert to existing image)
>> Basically omitting the BDRV_REQ_MAY_UNMAP flag to blk_co_pwrite_zeroes.
>> It will be slow, but maybe for this particular case, it is acceptable?
> 
> I should probably say that we don't want to break the other case
> (which is likely more important) where we write a sparse source to a
> sparse target and want the target to contain only the union of the two
> sparse maps, not fully allocated :-)
> 
> It would be fine, I think, to have a new "make this disk fully
> allocated" operation.  qemu-img resize could almost do it with a
> request to add 0 extra bytes, but the --preallocation flag only
> applies to the new space.

Well, that works with -S 0, as Kevin has said.  But that will take time
because it will write actual zeroes wherever the source is zero.

Max


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

Re: Bug? qemu-img convert to preallocated image makes it sparse

[David Edmondson]

On Thursday, 2020-01-16 at 15:37:22 +01, Max Reitz wrote:

> So I suppose the best idea I can come up with is indeed a
> --target-is-zero flag for qemu-img convert -n.  Would that work for you?

I was looking at adding this for a slightly different reason (converting
sparse images to newly provisioned LUNs).

Followup is a naive patch (I'm new to hacking on qemu and the block
layer seems complex due to maturity) that works in my tests. Feedback
much appreciated.

The patch specifically targets the initial blanking of the image rather
than any other attempt to write zeroes, as I couldn't convince myself
that there was no control flow where qemu-img convert might need to
overwrite (with zeroes) data that it itself had previously written.

dme.
-- 
Stop the music and go home.