[Qemu-devel] [PATCH v2 0/4] qemu-img: Fix preallocation with -S 0 for convert

[Qemu-devel] [PATCH v2 0/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

Using -S 0 is supposed to allocate everything in the output image; or at
least it is supposed to always explicitly write zeros even if the area
in question is known to only contain zeros. That doesn't always work
right now, so this series fixes it (patch 1, to be specific).

I only noticed after I had written the test added by patch 4 that we
already had an -S 0 test case which is included in the iotest 122.
However, the test added here works for all image formats and is maybe
more of a direct test (instead of querying the format whether it thinks
it allocated all of the data we directly ask du whether everything has
been allocated) so maybe it reflects better what users expect -S 0 to
do. Maybe.

Patches 2 and 3 are required for the test. I could have written the test
without doing a convert with null-co as the source, but that would have
been boring, so I did not.

If you want to argue that in light of the existence of test 122 the new
test added here is unnecessary and we therefore do not need patches 2, 3
and 4, please go ahead. I won't put up too much of a fight.


**
A note about dependencies: When applied to Kevin's block branch, patch 4
will conflict in the qemu-iotests/group file. I had to base it on master
master because right now about a fourth of all iotests break for qcow2
(for me, at least) on the block branch.
**


v2:
- Patch 1: The additional state BLK_ZERO_DATA actually made things more
  complicated than they are without. Drop it. [Kevin]
- Patch 2: Made reading zeros optional and used "zeroes" throughout the
  patch so the spelling confirms with the "detect-zeroes" option. [Eric]
  (I personally like "zeros" better so it differs from the 3rd p. sg. of
  "to zero".)
- Patch 3: If read-zeroes is false, we shouldn't indicate the BDS to
  only return zero(e)s when read.
- Patch 4: Moved test to 150, and now we need to set the read-zeroes
  option to true.


git-backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[0025] [FC] 'qemu-img: Fix preallocation with -S 0 for convert'
002/4:[down] 'block/null-{co,aio}: Allow reading zeroes'
       ^^^^ Actually just a reworded commit title, but there are
            functional changes here
003/4:[0007] [FC] 'block/null-{co,aio}: Implement get_block_status()'
004/4:[0008] [FC] 'iotests: Test qemu-img convert -S 0 behavior'


Max Reitz (4):
  qemu-img: Fix preallocation with -S 0 for convert
  block/null-{co,aio}: Allow reading zeroes
  block/null-{co,aio}: Implement get_block_status()
  iotests: Test qemu-img convert -S 0 behavior

 block/null.c               |  42 ++++++++++++++++++
 qemu-img.c                 |  26 ++++++-----
 tests/qemu-iotests/122.out |   6 +--
 tests/qemu-iotests/150     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/150.out |  14 ++++++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 179 insertions(+), 15 deletions(-)
 create mode 100755 tests/qemu-iotests/150
 create mode 100644 tests/qemu-iotests/150.out

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Max Reitz]

When passing -S 0 to qemu-img convert, the target image is supposed to
be fully allocated. Right now, this is not the case if the source image
contains areas which bdrv_get_block_status() reports as being zero.

This patch changes a zeroed area's status from BLK_ZERO to BLK_DATA
before invoking convert_write() if -S 0 has been specified. In addition,
the check whether convert_read() actually needs to do anything
(basically only if the current area is a BLK_DATA area) is pulled out of
that function to the caller.

If -S 0 has been specified, zeroed areas need to be written as data to
the output, thus they then have to be accounted when calculating the
progress made.

This patch changes the reference output for iotest 122; contrary to what
it assumed, -S 0 really should allocate everything in the output, not
just areas that are filled with zeros (as opposed to being zeroed).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c                 | 26 +++++++++++++++-----------
 tests/qemu-iotests/122.out |  6 ++----
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 29eae2a..b2e07bb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1507,10 +1507,6 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
     int n;
     int ret;
 
-    if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
-        return 0;
-    }
-
     assert(nb_sectors <= s->buf_sectors);
     while (nb_sectors > 0) {
         BlockBackend *blk;
@@ -1648,7 +1644,8 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || (!s->min_sparse && s->status == BLK_ZERO))
+        {
             s->allocated_sectors += n;
         }
         sector_num += n;
@@ -1668,17 +1665,24 @@ static int convert_do_copy(ImgConvertState *s)
             ret = n;
             goto fail;
         }
-        if (s->status == BLK_DATA) {
+        if (s->status == BLK_DATA || (!s->min_sparse && s->status == BLK_ZERO))
+        {
             allocated_done += n;
             qemu_progress_print(100.0 * allocated_done / s->allocated_sectors,
                                 0);
         }
 
-        ret = convert_read(s, sector_num, n, buf);
-        if (ret < 0) {
-            error_report("error while reading sector %" PRId64
-                         ": %s", sector_num, strerror(-ret));
-            goto fail;
+        if (s->status == BLK_DATA) {
+            ret = convert_read(s, sector_num, n, buf);
+            if (ret < 0) {
+                error_report("error while reading sector %" PRId64
+                             ": %s", sector_num, strerror(-ret));
+                goto fail;
+            }
+        } else if (!s->min_sparse && s->status == BLK_ZERO) {
+            n = MIN(n, s->buf_sectors);
+            memset(buf, 0, n * BDRV_SECTOR_SIZE);
+            s->status = BLK_DATA;
         }
 
         ret = convert_write(s, sector_num, n, buf);
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..98814de 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -112,16 +112,14 @@ read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": 327680},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
 
 convert -c -S 0:
 read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
 wrote 33554432/33554432 bytes at offset 0
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] block/null-{co, aio}: Allow reading zeroes

[Max Reitz]

This is optional so that it does not impede the null block driver's
performance unless this behavior is desired.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/null.c b/block/null.c
index d90165d..a7df386 100644
--- a/block/null.c
+++ b/block/null.c
@@ -14,10 +14,12 @@
 #include "block/block_int.h"
 
 #define NULL_OPT_LATENCY "latency-ns"
+#define NULL_OPT_ZEROES  "read-zeroes"
 
 typedef struct {
     int64_t length;
     int64_t latency_ns;
+    bool read_zeroes;
 } BDRVNullState;
 
 static QemuOptsList runtime_opts = {
@@ -40,6 +42,11 @@ static QemuOptsList runtime_opts = {
             .help = "nanoseconds (approximated) to wait "
                     "before completing request",
         },
+        {
+            .name = NULL_OPT_ZEROES,
+            .type = QEMU_OPT_BOOL,
+            .help = "return zeroes when read",
+        },
         { /* end of list */ }
     },
 };
@@ -61,6 +68,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
         error_setg(errp, "latency-ns is invalid");
         ret = -EINVAL;
     }
+    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
     qemu_opts_del(opts);
     return ret;
 }
@@ -90,6 +98,12 @@ static coroutine_fn int null_co_readv(BlockDriverState *bs,
                                       int64_t sector_num, int nb_sectors,
                                       QEMUIOVector *qiov)
 {
+    BDRVNullState *s = bs->opaque;
+
+    if (s->read_zeroes) {
+        qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
+    }
+
     return null_co_common(bs);
 }
 
@@ -159,6 +173,12 @@ static BlockAIOCB *null_aio_readv(BlockDriverState *bs,
                                   BlockCompletionFunc *cb,
                                   void *opaque)
 {
+    BDRVNullState *s = bs->opaque;
+
+    if (s->read_zeroes) {
+        qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
+    }
+
     return null_aio_common(bs, cb, opaque);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] block/null-{co, aio}: Implement get_block_status()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/null.c b/block/null.c
index a7df386..f4b3bba 100644
--- a/block/null.c
+++ b/block/null.c
@@ -204,6 +204,24 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
     return 0;
 }
 
+static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
+                                                     int64_t sector_num,
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file)
+{
+    BDRVNullState *s = bs->opaque;
+    off_t start = sector_num * BDRV_SECTOR_SIZE;
+
+    *pnum = nb_sectors;
+    *file = bs;
+
+    if (s->read_zeroes) {
+        return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
+    } else {
+        return BDRV_BLOCK_OFFSET_VALID | start;
+    }
+}
+
 static BlockDriver bdrv_null_co = {
     .format_name            = "null-co",
     .protocol_name          = "null-co",
@@ -217,6 +235,8 @@ static BlockDriver bdrv_null_co = {
     .bdrv_co_writev         = null_co_writev,
     .bdrv_co_flush_to_disk  = null_co_flush,
     .bdrv_reopen_prepare    = null_reopen_prepare,
+
+    .bdrv_co_get_block_status   = null_co_get_block_status,
 };
 
 static BlockDriver bdrv_null_aio = {
@@ -232,6 +252,8 @@ static BlockDriver bdrv_null_aio = {
     .bdrv_aio_writev        = null_aio_writev,
     .bdrv_aio_flush         = null_aio_flush,
     .bdrv_reopen_prepare    = null_reopen_prepare,
+
+    .bdrv_co_get_block_status   = null_co_get_block_status,
 };
 
 static void bdrv_null_init(void)
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] iotests: Test qemu-img convert -S 0 behavior

[Max Reitz]

Passing -S 0 to qemu-img convert should result in all source data being
copied to the output, even if that source data is known to be 0. The
output image should therefore have exactly the same size on disk as an
image which we explicitly filled with data.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/150     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/150.out |  14 ++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 120 insertions(+)
 create mode 100755 tests/qemu-iotests/150
 create mode 100644 tests/qemu-iotests/150.out

diff --git a/tests/qemu-iotests/150 b/tests/qemu-iotests/150
new file mode 100755
index 0000000..97d2a35
--- /dev/null
+++ b/tests/qemu-iotests/150
@@ -0,0 +1,105 @@
+#!/bin/bash
+#
+# Test that qemu-img convert -S 0 fully allocates the target image
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+
+on_disk_size()
+{
+    du "$@" | sed -e 's/\t\+.*//'
+}
+
+
+img_size=1048576
+
+
+echo
+echo '=== Comparing empty image against sparse conversion ==='
+echo
+
+_make_test_img $img_size
+
+empty_size=$(on_disk_size "$TEST_IMG")
+
+
+$QEMU_IMG_PROG convert -O "$IMGFMT" -S 512 \
+    "json:{ 'driver': 'null-co', 'size': $img_size, 'read-zeroes': true }" \
+    "$TEST_IMG"
+
+sparse_convert_size=$(on_disk_size "$TEST_IMG")
+
+
+if [ "$empty_size" -eq "$sparse_convert_size" ]; then
+    echo 'Equal image size'
+else
+    echo 'Different image size'
+fi
+
+
+echo
+echo '=== Comparing full image against non-sparse conversion ==='
+echo
+
+_make_test_img $img_size
+$QEMU_IO -c "write 0 $img_size" "$TEST_IMG" | _filter_qemu_io
+
+full_size=$(on_disk_size "$TEST_IMG")
+
+
+$QEMU_IMG convert -O "$IMGFMT" -S 0 \
+    "json:{ 'driver': 'null-co', 'size': $img_size, 'read-zeroes': true }" \
+    "$TEST_IMG"
+
+non_sparse_convert_size=$(on_disk_size "$TEST_IMG")
+
+
+if [ "$full_size" -eq "$non_sparse_convert_size" ]; then
+    echo 'Equal image size'
+else
+    echo 'Different image size'
+fi
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/150.out b/tests/qemu-iotests/150.out
new file mode 100644
index 0000000..2d29da1
--- /dev/null
+++ b/tests/qemu-iotests/150.out
@@ -0,0 +1,14 @@
+QA output created by 150
+
+=== Comparing empty image against sparse conversion ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+Equal image size
+
+=== Comparing full image against non-sparse conversion ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Equal image size
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index faf0f21..87e5ea4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -150,3 +150,4 @@
 145 auto quick
 146 auto quick
 148 rw auto quick
+150 rw auto quick
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/4] block/null-{co, aio}: Allow reading zeroes

[Eric Blake]

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

On 03/24/2016 04:33 PM, Max Reitz wrote:
> This is optional so that it does not impede the null block driver's
> performance unless this behavior is desired.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/null.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2/4] block/null-{co, aio}: Allow reading zeroes

[Fam Zheng]

On Thu, 03/24 23:33, Max Reitz wrote:
> This is optional so that it does not impede the null block driver's
> performance unless this behavior is desired.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/null.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/null.c b/block/null.c
> index d90165d..a7df386 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -14,10 +14,12 @@
>  #include "block/block_int.h"
>  
>  #define NULL_OPT_LATENCY "latency-ns"
> +#define NULL_OPT_ZEROES  "read-zeroes"
>  
>  typedef struct {
>      int64_t length;
>      int64_t latency_ns;
> +    bool read_zeroes;
>  } BDRVNullState;
>  
>  static QemuOptsList runtime_opts = {
> @@ -40,6 +42,11 @@ static QemuOptsList runtime_opts = {
>              .help = "nanoseconds (approximated) to wait "
>                      "before completing request",
>          },
> +        {
> +            .name = NULL_OPT_ZEROES,
> +            .type = QEMU_OPT_BOOL,
> +            .help = "return zeroes when read",
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -61,6 +68,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
>          error_setg(errp, "latency-ns is invalid");
>          ret = -EINVAL;
>      }
> +    s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
>      qemu_opts_del(opts);
>      return ret;
>  }
> @@ -90,6 +98,12 @@ static coroutine_fn int null_co_readv(BlockDriverState *bs,
>                                        int64_t sector_num, int nb_sectors,
>                                        QEMUIOVector *qiov)
>  {
> +    BDRVNullState *s = bs->opaque;
> +
> +    if (s->read_zeroes) {
> +        qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
> +    }
> +
>      return null_co_common(bs);
>  }
>  
> @@ -159,6 +173,12 @@ static BlockAIOCB *null_aio_readv(BlockDriverState *bs,
>                                    BlockCompletionFunc *cb,
>                                    void *opaque)
>  {
> +    BDRVNullState *s = bs->opaque;
> +
> +    if (s->read_zeroes) {
> +        qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
> +    }
> +
>      return null_aio_common(bs, cb, opaque);
>  }
>  
> -- 
> 2.7.4
> 

Acked-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] block/null-{co, aio}: Implement get_block_status()

[Fam Zheng]

On Thu, 03/24 23:33, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/null.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block/null.c b/block/null.c
> index a7df386..f4b3bba 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -204,6 +204,24 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state,
>      return 0;
>  }
>  
> +static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int *pnum,
> +                                                     BlockDriverState **file)
> +{
> +    BDRVNullState *s = bs->opaque;
> +    off_t start = sector_num * BDRV_SECTOR_SIZE;
> +
> +    *pnum = nb_sectors;
> +    *file = bs;
> +
> +    if (s->read_zeroes) {
> +        return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
> +    } else {
> +        return BDRV_BLOCK_OFFSET_VALID | start;
> +    }
> +}
> +
>  static BlockDriver bdrv_null_co = {
>      .format_name            = "null-co",
>      .protocol_name          = "null-co",
> @@ -217,6 +235,8 @@ static BlockDriver bdrv_null_co = {
>      .bdrv_co_writev         = null_co_writev,
>      .bdrv_co_flush_to_disk  = null_co_flush,
>      .bdrv_reopen_prepare    = null_reopen_prepare,
> +
> +    .bdrv_co_get_block_status   = null_co_get_block_status,
>  };
>  
>  static BlockDriver bdrv_null_aio = {
> @@ -232,6 +252,8 @@ static BlockDriver bdrv_null_aio = {
>      .bdrv_aio_writev        = null_aio_writev,
>      .bdrv_aio_flush         = null_aio_flush,
>      .bdrv_reopen_prepare    = null_reopen_prepare,
> +
> +    .bdrv_co_get_block_status   = null_co_get_block_status,
>  };
>  
>  static void bdrv_null_init(void)
> -- 
> 2.7.4
> 

Acked-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Fam Zheng]

On Thu, 03/24 23:33, Max Reitz wrote:
> When passing -S 0 to qemu-img convert, the target image is supposed to
> be fully allocated. Right now, this is not the case if the source image
> contains areas which bdrv_get_block_status() reports as being zero.
> 
> This patch changes a zeroed area's status from BLK_ZERO to BLK_DATA
> before invoking convert_write() if -S 0 has been specified. In addition,
> the check whether convert_read() actually needs to do anything
> (basically only if the current area is a BLK_DATA area) is pulled out of
> that function to the caller.
> 
> If -S 0 has been specified, zeroed areas need to be written as data to
> the output, thus they then have to be accounted when calculating the
> progress made.
> 
> This patch changes the reference output for iotest 122; contrary to what
> it assumed, -S 0 really should allocate everything in the output, not
> just areas that are filled with zeros (as opposed to being zeroed).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c                 | 26 +++++++++++++++-----------
>  tests/qemu-iotests/122.out |  6 ++----
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 29eae2a..b2e07bb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1507,10 +1507,6 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors,
>      int n;
>      int ret;
>  
> -    if (s->status == BLK_ZERO || s->status == BLK_BACKING_FILE) {
> -        return 0;
> -    }
> -
>      assert(nb_sectors <= s->buf_sectors);
>      while (nb_sectors > 0) {
>          BlockBackend *blk;
> @@ -1648,7 +1644,8 @@ static int convert_do_copy(ImgConvertState *s)
>              ret = n;
>              goto fail;
>          }
> -        if (s->status == BLK_DATA) {
> +        if (s->status == BLK_DATA || (!s->min_sparse && s->status == BLK_ZERO))
> +        {
>              s->allocated_sectors += n;
>          }
>          sector_num += n;
> @@ -1668,17 +1665,24 @@ static int convert_do_copy(ImgConvertState *s)
>              ret = n;
>              goto fail;
>          }
> -        if (s->status == BLK_DATA) {
> +        if (s->status == BLK_DATA || (!s->min_sparse && s->status == BLK_ZERO))
> +        {
>              allocated_done += n;
>              qemu_progress_print(100.0 * allocated_done / s->allocated_sectors,
>                                  0);
>          }
>  
> -        ret = convert_read(s, sector_num, n, buf);
> -        if (ret < 0) {
> -            error_report("error while reading sector %" PRId64
> -                         ": %s", sector_num, strerror(-ret));
> -            goto fail;
> +        if (s->status == BLK_DATA) {
> +            ret = convert_read(s, sector_num, n, buf);
> +            if (ret < 0) {
> +                error_report("error while reading sector %" PRId64
> +                             ": %s", sector_num, strerror(-ret));
> +                goto fail;
> +            }
> +        } else if (!s->min_sparse && s->status == BLK_ZERO) {
> +            n = MIN(n, s->buf_sectors);
> +            memset(buf, 0, n * BDRV_SECTOR_SIZE);
> +            s->status = BLK_DATA;
>          }
>  
>          ret = convert_write(s, sector_num, n, buf);
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index 0068e96..98814de 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -112,16 +112,14 @@ read 3145728/3145728 bytes at offset 0
>  3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 63963136/63963136 bytes at offset 3145728
>  61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": 327680},
> -{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
>  
>  convert -c -S 0:
>  read 3145728/3145728 bytes at offset 0
>  3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 63963136/63963136 bytes at offset 3145728
>  61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true},
> -{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
> +[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
>  Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
>  wrote 33554432/33554432 bytes at offset 0
>  32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -- 
> 2.7.4
> 

Looks sane to me,

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] iotests: Test qemu-img convert -S 0 behavior

[Fam Zheng]

On Thu, 03/24 23:34, Max Reitz wrote:
> Passing -S 0 to qemu-img convert should result in all source data being
> copied to the output, even if that source data is known to be 0. The
> output image should therefore have exactly the same size on disk as an
> image which we explicitly filled with data.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/150     | 105 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/150.out |  14 ++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 120 insertions(+)
>  create mode 100755 tests/qemu-iotests/150
>  create mode 100644 tests/qemu-iotests/150.out
> 
> diff --git a/tests/qemu-iotests/150 b/tests/qemu-iotests/150
> new file mode 100755
> index 0000000..97d2a35
> --- /dev/null
> +++ b/tests/qemu-iotests/150
> @@ -0,0 +1,105 @@
> +#!/bin/bash
> +#
> +# Test that qemu-img convert -S 0 fully allocates the target image
> +#
> +# Copyright (C) 2016 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=mreitz@redhat.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt generic
> +_supported_proto file
> +_supported_os Linux
> +
> +
> +on_disk_size()
> +{
> +    du "$@" | sed -e 's/\t\+.*//'
> +}
> +
> +
> +img_size=1048576
> +
> +
> +echo
> +echo '=== Comparing empty image against sparse conversion ==='
> +echo
> +
> +_make_test_img $img_size
> +
> +empty_size=$(on_disk_size "$TEST_IMG")
> +
> +
> +$QEMU_IMG_PROG convert -O "$IMGFMT" -S 512 \
> +    "json:{ 'driver': 'null-co', 'size': $img_size, 'read-zeroes': true }" \
> +    "$TEST_IMG"
> +
> +sparse_convert_size=$(on_disk_size "$TEST_IMG")
> +
> +
> +if [ "$empty_size" -eq "$sparse_convert_size" ]; then
> +    echo 'Equal image size'
> +else
> +    echo 'Different image size'
> +fi
> +
> +
> +echo
> +echo '=== Comparing full image against non-sparse conversion ==='
> +echo
> +
> +_make_test_img $img_size
> +$QEMU_IO -c "write 0 $img_size" "$TEST_IMG" | _filter_qemu_io
> +
> +full_size=$(on_disk_size "$TEST_IMG")
> +
> +
> +$QEMU_IMG convert -O "$IMGFMT" -S 0 \
> +    "json:{ 'driver': 'null-co', 'size': $img_size, 'read-zeroes': true }" \
> +    "$TEST_IMG"
> +
> +non_sparse_convert_size=$(on_disk_size "$TEST_IMG")
> +
> +
> +if [ "$full_size" -eq "$non_sparse_convert_size" ]; then
> +    echo 'Equal image size'
> +else
> +    echo 'Different image size'
> +fi
> +
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/150.out b/tests/qemu-iotests/150.out
> new file mode 100644
> index 0000000..2d29da1
> --- /dev/null
> +++ b/tests/qemu-iotests/150.out
> @@ -0,0 +1,14 @@
> +QA output created by 150
> +
> +=== Comparing empty image against sparse conversion ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +Equal image size
> +
> +=== Comparing full image against non-sparse conversion ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +wrote 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Equal image size
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index faf0f21..87e5ea4 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -150,3 +150,4 @@
>  145 auto quick
>  146 auto quick
>  148 rw auto quick
> +150 rw auto quick
> -- 
> 2.7.4
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/4] qemu-img: Fix preallocation with -S 0 for convert

[Kevin Wolf]

Am 24.03.2016 um 23:33 hat Max Reitz geschrieben:
> Using -S 0 is supposed to allocate everything in the output image; or at
> least it is supposed to always explicitly write zeros even if the area
> in question is known to only contain zeros. That doesn't always work
> right now, so this series fixes it (patch 1, to be specific).
> 
> I only noticed after I had written the test added by patch 4 that we
> already had an -S 0 test case which is included in the iotest 122.
> However, the test added here works for all image formats and is maybe
> more of a direct test (instead of querying the format whether it thinks
> it allocated all of the data we directly ask du whether everything has
> been allocated) so maybe it reflects better what users expect -S 0 to
> do. Maybe.
> 
> Patches 2 and 3 are required for the test. I could have written the test
> without doing a convert with null-co as the source, but that would have
> been boring, so I did not.
> 
> If you want to argue that in light of the existence of test 122 the new
> test added here is unnecessary and we therefore do not need patches 2, 3
> and 4, please go ahead. I won't put up too much of a fight.

Thanks, applied to the block branch.

Kevin