[PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse

[PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse

[Kevin Wolf]

Kevin Wolf (2):
  qcow2: Implement v2 zero writes with discard if possible
  iotests: Test sparseness for qemu-img convert -n

 block/qcow2-cluster.c      |  9 ++++++++-
 tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/122.out | 17 +++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

-- 
2.25.4

[PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Kevin Wolf]

qcow2 version 2 images don't support the zero flag for clusters, so for
write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
writes. If the image doesn't have a backing file, we can do better: Just
discard the respective clusters.

This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
to assume that the existing target image may contain any data, so it has
to write zeroes. Without this patch, this results in a fully allocated
target image, even if the source image was empty.

Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4b5fc8c4a7..a677ba9f5c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
     assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
            end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
 
-    /* The zero flag is only supported by version 3 and newer */
+    /*
+     * The zero flag is only supported by version 3 and newer. However, if we
+     * have no backing file, we can resort to discard in version 2.
+     */
     if (s->qcow_version < 3) {
+        if (!bs->backing) {
+            return qcow2_cluster_discard(bs, offset, bytes,
+                                         QCOW2_DISCARD_REQUEST, false);
+        }
         return -ENOTSUP;
     }
 
-- 
2.25.4

[PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/122.out | 17 +++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index dfd1cd05d6..1112fc0730 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
 
 $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
 
+echo
+echo '=== -n to an empty image ==='
+echo
+
+_make_test_img 64M
+
+# Convert with -n, which should not result in a fully allocated image, not even
+# with compat=0.10 (because the target doesn't have a backing file)
+TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+$QEMU_IMG map --output=json "$TEST_IMG".orig
+
+TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+$QEMU_IMG map --output=json "$TEST_IMG".orig
+
+echo
+echo '=== -n to an empty image with a backing file ==='
+echo
+
+_make_test_img 64M
+TEST_IMG="$TEST_IMG".base _make_test_img 64M
+
+# Convert with -n, which should still not result in a fully allocated image for
+# compat=1.1 (because it can use zero clusters), but it should be fully
+# allocated with compat=0.10
+TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=1.1 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+$QEMU_IMG map --output=json "$TEST_IMG".orig
+
+TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=0.10 64M
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+$QEMU_IMG map --output=json "$TEST_IMG".orig
+
 echo
 echo '=== -n -B to an image without a backing file ==='
 echo
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index f1f195ed77..b8028efb1d 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -229,6 +229,23 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Images are identical.
 
+=== -n to an empty image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+
+=== -n to an empty image with a backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+
 === -n -B to an image without a backing file ===
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
-- 
2.25.4

Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Nir Soffer]

On Mon, Jul 20, 2020 at 4:18 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/122.out | 17 +++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> index dfd1cd05d6..1112fc0730 100755
> --- a/tests/qemu-iotests/122
> +++ b/tests/qemu-iotests/122
> @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
>
>  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
>
> +echo
> +echo '=== -n to an empty image ==='
> +echo
> +
> +_make_test_img 64M

Why make a test image here? We create it again below twice

> +
> +# Convert with -n, which should not result in a fully allocated image, not even
> +# with compat=0.10 (because the target doesn't have a backing file)
> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig

This looks reversed - "$TEST_IMG".orig is the original image, and "$TEST_IMG"
is the target image. So maybe use "$TEST_IMG".target?

> +
> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig

Since the only difference is the compat, why not use a loop?

for compat in 0.10 1.1; do
...

> +
> +echo
> +echo '=== -n to an empty image with a backing file ==='
> +echo
> +
> +_make_test_img 64M
> +TEST_IMG="$TEST_IMG".base _make_test_img 64M
> +
> +# Convert with -n, which should still not result in a fully allocated image for
> +# compat=1.1 (because it can use zero clusters), but it should be fully
> +# allocated with compat=0.10
> +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=1.1 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig

Do we have a real use case for this convert? Doesn't this hide all the
data in the
backing file by data from source?

Assuming source is:

src-top: A0--
dst-bas: --B0

And destination is:

dst-top: ----
dst-bas: CCCC

After the convert we will have:

dst-top: A0B0
dst-bas: CCCC

So entire backing of dst is hidden.

Nir

> +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=0.10 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig
> +
>  echo
>  echo '=== -n -B to an image without a backing file ==='
>  echo
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index f1f195ed77..b8028efb1d 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -229,6 +229,23 @@ wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  Images are identical.
>
> +=== -n to an empty image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +
> +=== -n to an empty image with a backing file ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
> +
>  === -n -B to an image without a backing file ===
>
>  Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> --
> 2.25.4
>

Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Nir Soffer]

On Mon, Jul 20, 2020 at 4:18 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> qcow2 version 2 images don't support the zero flag for clusters, so for
> write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
> writes. If the image doesn't have a backing file, we can do better: Just
> discard the respective clusters.
>
> This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
> to assume that the existing target image may contain any data, so it has
> to write zeroes. Without this patch, this results in a fully allocated
> target image, even if the source image was empty.
>
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4b5fc8c4a7..a677ba9f5c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
>      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
>             end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
>
> -    /* The zero flag is only supported by version 3 and newer */
> +    /*
> +     * The zero flag is only supported by version 3 and newer. However, if we
> +     * have no backing file, we can resort to discard in version 2.
> +     */
>      if (s->qcow_version < 3) {
> +        if (!bs->backing) {
> +            return qcow2_cluster_discard(bs, offset, bytes,
> +                                         QCOW2_DISCARD_REQUEST, false);
> +        }
>          return -ENOTSUP;
>      }

Looks good to me.

>
> --
> 2.25.4
>

Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Max Reitz]

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

On 20.07.20 15:18, Kevin Wolf wrote:
> qcow2 version 2 images don't support the zero flag for clusters, so for
> write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
> writes. If the image doesn't have a backing file, we can do better: Just
> discard the respective clusters.
> 
> This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
> to assume that the existing target image may contain any data, so it has
> to write zeroes. Without this patch, this results in a fully allocated
> target image, even if the source image was empty.
> 
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Max Reitz]

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

On 20.07.20 15:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/122.out | 17 +++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> index dfd1cd05d6..1112fc0730 100755
> --- a/tests/qemu-iotests/122
> +++ b/tests/qemu-iotests/122
> @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
>  
>  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
>  
> +echo
> +echo '=== -n to an empty image ==='
> +echo
> +
> +_make_test_img 64M
> +
> +# Convert with -n, which should not result in a fully allocated image, not even
> +# with compat=0.10 (because the target doesn't have a backing file)
> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig
> +
> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M

It’s a shame that with this, the test will no longer pass with
refcount_bits=1.  (Or an external data file.)

But, well.  Maybe we don’t care and then should just put both options
into _unsupported_imgopts.

Apart from that, the test looks good to me.

Max

> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig
> +
> +echo
> +echo '=== -n to an empty image with a backing file ==='
> +echo
> +
> +_make_test_img 64M
> +TEST_IMG="$TEST_IMG".base _make_test_img 64M
> +
> +# Convert with -n, which should still not result in a fully allocated image for
> +# compat=1.1 (because it can use zero clusters), but it should be fully
> +# allocated with compat=0.10
> +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=1.1 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig
> +
> +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=0.10 64M
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +$QEMU_IMG map --output=json "$TEST_IMG".orig
> +
>  echo
>  echo '=== -n -B to an image without a backing file ==='
>  echo
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index f1f195ed77..b8028efb1d 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -229,6 +229,23 @@ wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  Images are identical.
>  
> +=== -n to an empty image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +
> +=== -n to an empty image with a backing file ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": true, "data": false}]
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
> +
>  === -n -B to an image without a backing file ===
>  
>  Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> 



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

Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Kevin Wolf]

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

Am 21.07.2020 um 12:19 hat Max Reitz geschrieben:
> On 20.07.20 15:18, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/122.out | 17 +++++++++++++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> > index dfd1cd05d6..1112fc0730 100755
> > --- a/tests/qemu-iotests/122
> > +++ b/tests/qemu-iotests/122
> > @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> >  
> >  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
> >  
> > +echo
> > +echo '=== -n to an empty image ==='
> > +echo
> > +
> > +_make_test_img 64M
> > +
> > +# Convert with -n, which should not result in a fully allocated image, not even
> > +# with compat=0.10 (because the target doesn't have a backing file)
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> > +
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
> 
> It’s a shame that with this, the test will no longer pass with
> refcount_bits=1.  (Or an external data file.)

You mean because of the compat=0.10? We already use that in this test
case, however just with "$QEMU_IMG convert" so that $IMGOPTS doesn't
apply.

I guess I could just override $IMGOPTS for this line to get the same
behaviour here and make sure that none of these options are used.

Kevin

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

Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Max Reitz]

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

On 21.07.20 13:20, Kevin Wolf wrote:
> Am 21.07.2020 um 12:19 hat Max Reitz geschrieben:
>> On 20.07.20 15:18, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/122.out | 17 +++++++++++++++++
>>>  2 files changed, 51 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
>>> index dfd1cd05d6..1112fc0730 100755
>>> --- a/tests/qemu-iotests/122
>>> +++ b/tests/qemu-iotests/122
>>> @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
>>>  
>>>  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
>>>  
>>> +echo
>>> +echo '=== -n to an empty image ==='
>>> +echo
>>> +
>>> +_make_test_img 64M
>>> +
>>> +# Convert with -n, which should not result in a fully allocated image, not even
>>> +# with compat=0.10 (because the target doesn't have a backing file)
>>> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
>>> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
>>> +$QEMU_IMG map --output=json "$TEST_IMG".orig
>>> +
>>> +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
>>
>> It’s a shame that with this, the test will no longer pass with
>> refcount_bits=1.  (Or an external data file.)
> 
> You mean because of the compat=0.10? We already use that in this test
> case, however just with "$QEMU_IMG convert" so that $IMGOPTS doesn't
> apply.
> 
> I guess I could just override $IMGOPTS for this line to get the same
> behaviour here and make sure that none of these options are used.

Well... Not my favorite, but probably because I just never thought of that.

I suppose it works, so why not, actually.

Max


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

Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n

[Kevin Wolf]

Am 20.07.2020 um 16:47 hat Nir Soffer geschrieben:
> On Mon, Jul 20, 2020 at 4:18 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/122     | 34 ++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/122.out | 17 +++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> > index dfd1cd05d6..1112fc0730 100755
> > --- a/tests/qemu-iotests/122
> > +++ b/tests/qemu-iotests/122
> > @@ -281,6 +281,40 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> >
> >  $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
> >
> > +echo
> > +echo '=== -n to an empty image ==='
> > +echo
> > +
> > +_make_test_img 64M
> 
> Why make a test image here? We create it again below twice

This is a different image because the invocations below change the
TEST_IMG variable.

> > +
> > +# Convert with -n, which should not result in a fully allocated image, not even
> > +# with compat=0.10 (because the target doesn't have a backing file)
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=1.1 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> This looks reversed - "$TEST_IMG".orig is the original image, and
> "$TEST_IMG" is the target image. So maybe use "$TEST_IMG".target?

I'll use .orig for the source and without a suffix for the target (which
are filenames that _cleanup_test_img covers automatically).

> > +
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -o compat=0.10 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> Since the only difference is the compat, why not use a loop?
> 
> for compat in 0.10 1.1; do
> ...

Makes sense.

> > +
> > +echo
> > +echo '=== -n to an empty image with a backing file ==='
> > +echo
> > +
> > +_make_test_img 64M
> > +TEST_IMG="$TEST_IMG".base _make_test_img 64M
> > +
> > +# Convert with -n, which should still not result in a fully allocated image for
> > +# compat=1.1 (because it can use zero clusters), but it should be fully
> > +# allocated with compat=0.10
> > +TEST_IMG="$TEST_IMG".orig _make_test_img -b "$TEST_IMG".base -F $IMGFMT -o compat=1.1 64M
> > +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> > +$QEMU_IMG map --output=json "$TEST_IMG".orig
> 
> Do we have a real use case for this convert? Doesn't this hide all the
> data in the backing file by data from source?

There is probably no real use case for this. But it has a defined
behaviour and it's always good to cover corner cases with tests so that
unintentional changes can be found (which may potentially affect more
relevant cases, too).

Kevin

Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Maxim Levitsky]

On Mon, 2020-07-20 at 15:18 +0200, Kevin Wolf wrote:
> qcow2 version 2 images don't support the zero flag for clusters, so for
> write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
> writes. If the image doesn't have a backing file, we can do better: Just
> discard the respective clusters.
> 
> This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
> to assume that the existing target image may contain any data, so it has
> to write zeroes. Without this patch, this results in a fully allocated
> target image, even if the source image was empty.
> 
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4b5fc8c4a7..a677ba9f5c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
>      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
>             end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
>  
> -    /* The zero flag is only supported by version 3 and newer */
> +    /*
> +     * The zero flag is only supported by version 3 and newer. However, if we
> +     * have no backing file, we can resort to discard in version 2.
> +     */
>      if (s->qcow_version < 3) {
> +        if (!bs->backing) {
> +            return qcow2_cluster_discard(bs, offset, bytes,
> +                                         QCOW2_DISCARD_REQUEST, false);
> +        }
>          return -ENOTSUP;
>      }
>  

From my knowelege of nvme, I remember that discard doesn't have to zero the blocks.
There is special namespace capability the indicates the contents of the discarded block.
(Deallocate Logical Block Features)

If and only if the discard behavier flag indicates that discarded areas are zero,
then the write-zero command can have special 'deallocate' flag that hints the controller
to discard the sectors.

So woudn't discarding the clusters have theoretical risk of introducing garbage there?

Best regards,
	Maxim Levitsky

Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Kevin Wolf]

Am 22.07.2020 um 19:01 hat Maxim Levitsky geschrieben:
> On Mon, 2020-07-20 at 15:18 +0200, Kevin Wolf wrote:
> > qcow2 version 2 images don't support the zero flag for clusters, so for
> > write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
> > writes. If the image doesn't have a backing file, we can do better: Just
> > discard the respective clusters.
> > 
> > This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
> > to assume that the existing target image may contain any data, so it has
> > to write zeroes. Without this patch, this results in a fully allocated
> > target image, even if the source image was empty.
> > 
> > Reported-by: Nir Soffer <nsoffer@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2-cluster.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 4b5fc8c4a7..a677ba9f5c 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> >      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
> >             end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
> >  
> > -    /* The zero flag is only supported by version 3 and newer */
> > +    /*
> > +     * The zero flag is only supported by version 3 and newer. However, if we
> > +     * have no backing file, we can resort to discard in version 2.
> > +     */
> >      if (s->qcow_version < 3) {
> > +        if (!bs->backing) {
> > +            return qcow2_cluster_discard(bs, offset, bytes,
> > +                                         QCOW2_DISCARD_REQUEST, false);
> > +        }
> >          return -ENOTSUP;
> >      }
> >  
> 
> From my knowelege of nvme, I remember that discard doesn't have to zero the blocks.
> There is special namespace capability the indicates the contents of the discarded block.
> (Deallocate Logical Block Features)
> 
> If and only if the discard behavier flag indicates that discarded areas are zero,
> then the write-zero command can have special 'deallocate' flag that hints the controller
> to discard the sectors.
> 
> So woudn't discarding the clusters have theoretical risk of introducing garbage there?

No, qcow2_cluster_discard() has a defined behaviour. For v2 images, it
unallocates the cluster in the L2 table (this is only safe without a
backing file), for v3 images it converts them to zero clusters.

Kevin

Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible

[Maxim Levitsky]

On Wed, 2020-07-22 at 19:14 +0200, Kevin Wolf wrote:
> Am 22.07.2020 um 19:01 hat Maxim Levitsky geschrieben:
> > On Mon, 2020-07-20 at 15:18 +0200, Kevin Wolf wrote:
> > > qcow2 version 2 images don't support the zero flag for clusters, so for
> > > write_zeroes requests, we return -ENOTSUP and get explicit zero buffer
> > > writes. If the image doesn't have a backing file, we can do better: Just
> > > discard the respective clusters.
> > > 
> > > This is relevant for 'qemu-img convert -O qcow2 -n', where qemu-img has
> > > to assume that the existing target image may contain any data, so it has
> > > to write zeroes. Without this patch, this results in a fully allocated
> > > target image, even if the source image was empty.
> > > 
> > > Reported-by: Nir Soffer <nsoffer@redhat.com>
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/qcow2-cluster.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > index 4b5fc8c4a7..a677ba9f5c 100644
> > > --- a/block/qcow2-cluster.c
> > > +++ b/block/qcow2-cluster.c
> > > @@ -1797,8 +1797,15 @@ int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> > >      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
> > >             end_offset >= bs->total_sectors << BDRV_SECTOR_BITS);
> > >  
> > > -    /* The zero flag is only supported by version 3 and newer */
> > > +    /*
> > > +     * The zero flag is only supported by version 3 and newer. However, if we
> > > +     * have no backing file, we can resort to discard in version 2.
> > > +     */
> > >      if (s->qcow_version < 3) {
> > > +        if (!bs->backing) {
> > > +            return qcow2_cluster_discard(bs, offset, bytes,
> > > +                                         QCOW2_DISCARD_REQUEST, false);
> > > +        }
> > >          return -ENOTSUP;
> > >      }
> > >  
> > 
> > From my knowelege of nvme, I remember that discard doesn't have to zero the blocks.
> > There is special namespace capability the indicates the contents of the discarded block.
> > (Deallocate Logical Block Features)
> > 
> > If and only if the discard behavier flag indicates that discarded areas are zero,
> > then the write-zero command can have special 'deallocate' flag that hints the controller
> > to discard the sectors.
> > 
> > So woudn't discarding the clusters have theoretical risk of introducing garbage there?
> 
> No, qcow2_cluster_discard() has a defined behaviour. For v2 images, it
> unallocates the cluster in the L2 table (this is only safe without a
> backing file), for v3 images it converts them to zero clusters.

All right then!

Best regards,
	Maxim Levitsky
> 
> Kevin