* [PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse @ 2020-07-20 13:18 Kevin Wolf 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf 2020-07-20 13:18 ` [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n Kevin Wolf 0 siblings, 2 replies; 13+ messages in thread From: Kevin Wolf @ 2020-07-20 13:18 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, nsoffer, qemu-devel, mreitz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-20 13:18 [PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse Kevin Wolf @ 2020-07-20 13:18 ` Kevin Wolf 2020-07-20 14:50 ` Nir Soffer ` (2 more replies) 2020-07-20 13:18 ` [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n Kevin Wolf 1 sibling, 3 replies; 13+ messages in thread From: Kevin Wolf @ 2020-07-20 13:18 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, nsoffer, qemu-devel, mreitz 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf @ 2020-07-20 14:50 ` Nir Soffer 2020-07-21 10:07 ` Max Reitz 2020-07-22 17:01 ` Maxim Levitsky 2 siblings, 0 replies; 13+ messages in thread From: Nir Soffer @ 2020-07-20 14:50 UTC (permalink / raw) To: Kevin Wolf; +Cc: QEMU Developers, qemu-block, Max Reitz 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf 2020-07-20 14:50 ` Nir Soffer @ 2020-07-21 10:07 ` Max Reitz 2020-07-22 17:01 ` Maxim Levitsky 2 siblings, 0 replies; 13+ messages in thread From: Max Reitz @ 2020-07-21 10:07 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: nsoffer, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf 2020-07-20 14:50 ` Nir Soffer 2020-07-21 10:07 ` Max Reitz @ 2020-07-22 17:01 ` Maxim Levitsky 2020-07-22 17:14 ` Kevin Wolf 2 siblings, 1 reply; 13+ messages in thread From: Maxim Levitsky @ 2020-07-22 17:01 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel, mreitz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-22 17:01 ` Maxim Levitsky @ 2020-07-22 17:14 ` Kevin Wolf 2020-07-22 17:15 ` Maxim Levitsky 0 siblings, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2020-07-22 17:14 UTC (permalink / raw) To: Maxim Levitsky; +Cc: qemu-devel, qemu-block, mreitz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible 2020-07-22 17:14 ` Kevin Wolf @ 2020-07-22 17:15 ` Maxim Levitsky 0 siblings, 0 replies; 13+ messages in thread From: Maxim Levitsky @ 2020-07-22 17:15 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-20 13:18 [PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse Kevin Wolf 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf @ 2020-07-20 13:18 ` Kevin Wolf 2020-07-20 14:47 ` Nir Soffer 2020-07-21 10:19 ` Max Reitz 1 sibling, 2 replies; 13+ messages in thread From: Kevin Wolf @ 2020-07-20 13:18 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, nsoffer, qemu-devel, mreitz 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-20 13:18 ` [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n Kevin Wolf @ 2020-07-20 14:47 ` Nir Soffer 2020-07-21 13:49 ` Kevin Wolf 2020-07-21 10:19 ` Max Reitz 1 sibling, 1 reply; 13+ messages in thread From: Nir Soffer @ 2020-07-20 14:47 UTC (permalink / raw) To: Kevin Wolf; +Cc: QEMU Developers, qemu-block, Max Reitz 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-20 14:47 ` Nir Soffer @ 2020-07-21 13:49 ` Kevin Wolf 0 siblings, 0 replies; 13+ messages in thread From: Kevin Wolf @ 2020-07-21 13:49 UTC (permalink / raw) To: Nir Soffer; +Cc: QEMU Developers, qemu-block, Max Reitz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-20 13:18 ` [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n Kevin Wolf 2020-07-20 14:47 ` Nir Soffer @ 2020-07-21 10:19 ` Max Reitz 2020-07-21 11:20 ` Kevin Wolf 1 sibling, 1 reply; 13+ messages in thread From: Max Reitz @ 2020-07-21 10:19 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: nsoffer, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-21 10:19 ` Max Reitz @ 2020-07-21 11:20 ` Kevin Wolf 2020-07-21 11:25 ` Max Reitz 0 siblings, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2020-07-21 11:20 UTC (permalink / raw) To: Max Reitz; +Cc: nsoffer, qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n 2020-07-21 11:20 ` Kevin Wolf @ 2020-07-21 11:25 ` Max Reitz 0 siblings, 0 replies; 13+ messages in thread From: Max Reitz @ 2020-07-21 11:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: nsoffer, qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-22 17:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-20 13:18 [PATCH for-5.1 0/2] qemu-img convert -n: Keep qcow2 v2 target sparse Kevin Wolf 2020-07-20 13:18 ` [PATCH for-5.1 1/2] qcow2: Implement v2 zero writes with discard if possible Kevin Wolf 2020-07-20 14:50 ` Nir Soffer 2020-07-21 10:07 ` Max Reitz 2020-07-22 17:01 ` Maxim Levitsky 2020-07-22 17:14 ` Kevin Wolf 2020-07-22 17:15 ` Maxim Levitsky 2020-07-20 13:18 ` [PATCH for-5.1 2/2] iotests: Test sparseness for qemu-img convert -n Kevin Wolf 2020-07-20 14:47 ` Nir Soffer 2020-07-21 13:49 ` Kevin Wolf 2020-07-21 10:19 ` Max Reitz 2020-07-21 11:20 ` Kevin Wolf 2020-07-21 11:25 ` Max Reitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).