[Qemu-devel] [PATCH v2 00/11] block: Fix some things about bdrv_has_zero_init()

[Qemu-devel] [PATCH v2 00/11] block: Fix some things about bdrv_has_zero_init()

[Max Reitz]

Hi,

See the previous cover letter for the reason for patches 6 through 9:
https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg00563.html

But no only some bdrv_has_zero_init() implementations are wrong, some
callers also use it the wrong way.

First, qemu-img and mirror use it for pre-existing images, where it
doesn’t have any meaning.  Both should consider pre-existing images to
always be non-zero and not look at bdrv_has-zero_init() (patches 1, 2,
and the tests in 10 and 11).

Second, vhdx and parallels call bdrv_has_zero_init() when they do not
really care about an image’s post-create state but only about what
happens when you grow an image.  That is a bit ugly, and also overly
safe when growing preallocated images without preallocating the new
areas.  So this series adds a new function bdrv_has_zero_init_truncate()
that is more suited to vhdx's and parallel's needs (patches 3 through
5).


v2:
- Simplified preallocation checks in qcow2 and vhdx [Kevin]
- Added patches 1 – 5, 10, 11


git-backport-diff against v1:

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/11:[down] 'qemu-img: Fix bdrv_has_zero_init() use in convert'
002/11:[down] 'mirror: Fix bdrv_has_zero_init() use'
003/11:[down] 'block: Add bdrv_has_zero_init_truncate()'
004/11:[down] 'block: Implement .bdrv_has_zero_init_truncate()'
005/11:[down] 'block: Use bdrv_has_zero_init_truncate()'
006/11:[0077] [FC] 'qcow2: Fix .bdrv_has_zero_init()'
007/11:[----] [--] 'vdi: Fix .bdrv_has_zero_init()'
008/11:[0021] [FC] 'vhdx: Fix .bdrv_has_zero_init()'
009/11:[----] [--] 'iotests: Convert to preallocated encrypted qcow2'
010/11:[down] 'iotests: Test convert -n to pre-filled image'
011/11:[down] 'iotests: Full mirror to existing non-zero image'


Max Reitz (11):
  qemu-img: Fix bdrv_has_zero_init() use in convert
  mirror: Fix bdrv_has_zero_init() use
  block: Add bdrv_has_zero_init_truncate()
  block: Implement .bdrv_has_zero_init_truncate()
  block: Use bdrv_has_zero_init_truncate()
  qcow2: Fix .bdrv_has_zero_init()
  vdi: Fix .bdrv_has_zero_init()
  vhdx: Fix .bdrv_has_zero_init()
  iotests: Convert to preallocated encrypted qcow2
  iotests: Test convert -n to pre-filled image
  iotests: Full mirror to existing non-zero image

 include/block/block.h       |  1 +
 include/block/block_int.h   |  9 ++++++
 block.c                     | 21 +++++++++++++
 block/file-posix.c          |  1 +
 block/file-win32.c          |  1 +
 block/gluster.c             |  4 +++
 block/mirror.c              | 11 +++++--
 block/nfs.c                 |  1 +
 block/parallels.c           |  2 +-
 block/qcow2.c               | 30 +++++++++++++++++-
 block/qed.c                 |  1 +
 block/raw-format.c          |  6 ++++
 block/rbd.c                 |  1 +
 block/sheepdog.c            |  1 +
 block/ssh.c                 |  1 +
 block/vdi.c                 | 13 +++++++-
 block/vhdx.c                | 28 +++++++++++++++--
 blockdev.c                  | 16 ++++++++--
 qemu-img.c                  | 11 +++++--
 tests/test-block-iothread.c |  2 +-
 tests/qemu-iotests/041      | 62 ++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/041.out  |  4 +--
 tests/qemu-iotests/122      | 17 ++++++++++
 tests/qemu-iotests/122.out  |  8 +++++
 tests/qemu-iotests/188      | 20 +++++++++++-
 tests/qemu-iotests/188.out  |  4 +++
 26 files changed, 254 insertions(+), 22 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Max Reitz]

bdrv_has_zero_init() only has meaning for newly created images or image
areas.  If qemu-img convert did not create the image itself, it cannot
rely on bdrv_has_zero_init()'s result to carry any meaning.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 79983772de..0f4be80c10 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
     bool has_zero_init;
     bool compressed;
     bool unallocated_blocks_are_zero;
+    bool target_is_new;
     bool target_has_backing;
     int64_t target_backing_sectors; /* negative if unknown */
     bool wr_in_order;
@@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
     int64_t sector_num = 0;
 
     /* Check whether we have zero initialisation or can get it efficiently */
-    s->has_zero_init = s->min_sparse && !s->target_has_backing
-                     ? bdrv_has_zero_init(blk_bs(s->target))
-                     : false;
+    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+        s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
+    } else {
+        s->has_zero_init = false;
+    }
 
     if (!s->has_zero_init && !s->target_has_backing &&
         bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
@@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    s.target_is_new = !skip_create;
+
     flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
-- 
2.21.0

[Qemu-devel] [PATCH v2 02/11] mirror: Fix bdrv_has_zero_init() use

[Max Reitz]

bdrv_has_zero_init() only has meaning for newly created images or image
areas.  If the mirror job itself did not create the image, it cannot
rely on bdrv_has_zero_init()'s result to carry any meaning.

This is the case for drive-mirror with mode=existing and always for
blockdev-mirror.

Note that we only have to zero-initialize the target with sync=full,
because other modes actually do not promise that the target will contain
the same data as the source after the job -- sync=top only promises to
copy anything allocated in the top layer, and sync=none will only copy
new I/O.  (Which is how mirror has always handled it.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h   |  2 ++
 block/mirror.c              | 11 ++++++++---
 blockdev.c                  | 16 +++++++++++++---
 tests/test-block-iothread.c |  2 +-
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3aa1e832a8..6a0b1b5008 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1116,6 +1116,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
  * @buf_size: The amount of data that can be in flight at one time.
  * @mode: Whether to collapse all images in the chain to the target.
  * @backing_mode: How to establish the target's backing chain after completion.
+ * @zero_target: Whether the target should be explicitly zero-initialized
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @unmap: Whether to unmap target where source sectors only contain zeroes.
@@ -1135,6 +1136,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                   int creation_flags, int64_t speed,
                   uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  bool zero_target,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap, const char *filter_node_name,
diff --git a/block/mirror.c b/block/mirror.c
index 8cb75fb409..50188ce6e9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -51,6 +51,8 @@ typedef struct MirrorBlockJob {
     Error *replace_blocker;
     bool is_none_mode;
     BlockMirrorBackingMode backing_mode;
+    /* Whether the target image requires explicit zero-initialization */
+    bool zero_target;
     MirrorCopyMode copy_mode;
     BlockdevOnError on_source_error, on_target_error;
     bool synced;
@@ -763,7 +765,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
     int ret;
     int64_t count;
 
-    if (base == NULL && !bdrv_has_zero_init(target_bs)) {
+    if (s->zero_target) {
         if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
             return 0;
@@ -1501,6 +1503,7 @@ static BlockJob *mirror_start_job(
                              const char *replaces, int64_t speed,
                              uint32_t granularity, int64_t buf_size,
                              BlockMirrorBackingMode backing_mode,
+                             bool zero_target,
                              BlockdevOnError on_source_error,
                              BlockdevOnError on_target_error,
                              bool unmap,
@@ -1628,6 +1631,7 @@ static BlockJob *mirror_start_job(
     s->on_target_error = on_target_error;
     s->is_none_mode = is_none_mode;
     s->backing_mode = backing_mode;
+    s->zero_target = zero_target;
     s->copy_mode = copy_mode;
     s->base = base;
     s->granularity = granularity;
@@ -1713,6 +1717,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                   int creation_flags, int64_t speed,
                   uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  bool zero_target,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap, const char *filter_node_name,
@@ -1728,7 +1733,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
     mirror_start_job(job_id, bs, creation_flags, target, replaces,
-                     speed, granularity, buf_size, backing_mode,
+                     speed, granularity, buf_size, backing_mode, zero_target,
                      on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
                      filter_node_name, true, copy_mode, errp);
@@ -1755,7 +1760,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
 
     ret = mirror_start_job(
                      job_id, bs, creation_flags, base, NULL, speed, 0, 0,
-                     MIRROR_LEAVE_BACKING_CHAIN,
+                     MIRROR_LEAVE_BACKING_CHAIN, false,
                      on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
                      filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
diff --git a/blockdev.c b/blockdev.c
index 4d141e9a1f..0323f77850 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3705,6 +3705,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    bool has_replaces, const char *replaces,
                                    enum MirrorSyncMode sync,
                                    BlockMirrorBackingMode backing_mode,
+                                   bool zero_target,
                                    bool has_speed, int64_t speed,
                                    bool has_granularity, uint32_t granularity,
                                    bool has_buf_size, int64_t buf_size,
@@ -3813,7 +3814,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
      */
     mirror_start(job_id, bs, target,
                  has_replaces ? replaces : NULL, job_flags,
-                 speed, granularity, buf_size, sync, backing_mode,
+                 speed, granularity, buf_size, sync, backing_mode, zero_target,
                  on_source_error, on_target_error, unmap, filter_node_name,
                  copy_mode, errp);
 }
@@ -3829,6 +3830,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     int flags;
     int64_t size;
     const char *format = arg->format;
+    bool zero_target;
     int ret;
 
     bs = qmp_get_root_bs(arg->device, errp);
@@ -3930,6 +3932,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         goto out;
     }
 
+    zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL &&
+                   (arg->mode == NEW_IMAGE_MODE_EXISTING ||
+                    !bdrv_has_zero_init(target_bs)));
+
     ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
     if (ret < 0) {
         bdrv_unref(target_bs);
@@ -3938,7 +3944,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
-                           backing_mode, arg->has_speed, arg->speed,
+                           backing_mode, zero_target,
+                           arg->has_speed, arg->speed,
                            arg->has_granularity, arg->granularity,
                            arg->has_buf_size, arg->buf_size,
                            arg->has_on_source_error, arg->on_source_error,
@@ -3978,6 +3985,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
     AioContext *aio_context;
     BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
     Error *local_err = NULL;
+    bool zero_target;
     int ret;
 
     bs = qmp_get_root_bs(device, errp);
@@ -3990,6 +3998,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
         return;
     }
 
+    zero_target = (sync == MIRROR_SYNC_MODE_FULL);
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
@@ -4000,7 +4010,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
 
     blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
                            has_replaces, replaces, sync, backing_mode,
-                           has_speed, speed,
+                           zero_target, has_speed, speed,
                            has_granularity, granularity,
                            has_buf_size, buf_size,
                            has_on_source_error, on_source_error,
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index 1949d5e61a..debfb69bfb 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -611,7 +611,7 @@ static void test_propagate_mirror(void)
 
     /* Start a mirror job */
     mirror_start("job0", src, target, NULL, JOB_DEFAULT, 0, 0, 0,
-                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN,
+                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, false,
                  BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
                  false, "filter_node", MIRROR_COPY_MODE_BACKGROUND,
                  &error_abort);
-- 
2.21.0

[Qemu-devel] [PATCH v2 03/11] block: Add bdrv_has_zero_init_truncate()

[Max Reitz]

No .bdrv_has_zero_init() implementation returns 1 if growing the file
would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
in lieu of this new function was always safe.

But on the other hand, it is possible that growing an image that is not
zero-initialized would still add a zero-initialized area, like when
using nonpreallocating truncation on a preallocated image.  For callers
that care only about truncation, not about creation with potential
preallocation, this new function is useful.

Alternatively, we could have added a PreallocMode parameter to
bdrv_has_zero_init().  But the only user would have been qemu-img
convert, which does not have a plain PreallocMode value right now -- it
would have to parse the creation option to obtain it.  Therefore, the
simpler solution is to let bdrv_has_zero_init() inquire the
preallocation status and add the new bdrv_has_zero_init_truncate() that
presupposes PREALLOC_MODE_OFF.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h     |  1 +
 include/block/block_int.h |  7 +++++++
 block.c                   | 21 +++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 50a07c1c33..5321d8afdf 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,6 +438,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
+int bdrv_has_zero_init_truncate(BlockDriverState *bs);
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int bdrv_block_status(BlockDriverState *bs, int64_t offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6a0b1b5008..d7fc6b296b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -420,9 +420,16 @@ struct BlockDriver {
     /*
      * Returns 1 if newly created images are guaranteed to contain only
      * zeros, 0 otherwise.
+     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /*
+     * Returns 1 if new areas added by growing the image with
+     * PREALLOC_MODE_OFF contain only zeros, 0 otherwise.
+     */
+    int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs);
+
     /* Remove fd handlers, timers, and other event loop callbacks so the event
      * loop is no longer in use.  Called with no in-flight requests and in
      * depth-first traversal order with parents before child nodes.
diff --git a/block.c b/block.c
index cbd8da5f3b..81ae44dcf3 100644
--- a/block.c
+++ b/block.c
@@ -5066,6 +5066,27 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+int bdrv_has_zero_init_truncate(BlockDriverState *bs)
+{
+    if (!bs->drv) {
+        return 0;
+    }
+
+    if (bs->backing) {
+        /* Depends on the backing image length, but better safe than sorry */
+        return 0;
+    }
+    if (bs->drv->bdrv_has_zero_init_truncate) {
+        return bs->drv->bdrv_has_zero_init_truncate(bs);
+    }
+    if (bs->file && bs->drv->is_filter) {
+        return bdrv_has_zero_init_truncate(bs->file->bs);
+    }
+
+    /* safe default */
+    return 0;
+}
+
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
 {
     BlockDriverInfo bdi;
-- 
2.21.0

[Qemu-devel] [PATCH v2 04/11] block: Implement .bdrv_has_zero_init_truncate()

[Max Reitz]

We need to implement .bdrv_has_zero_init_truncate() for every block
driver that supports truncation and has a .bdrv_has_zero_init()
implementation.

Implement it the same way each driver implements .bdrv_has_zero_init().
This is at least not any more unsafe than what we had before.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 1 +
 block/file-win32.c | 1 +
 block/gluster.c    | 4 ++++
 block/nfs.c        | 1 +
 block/qcow2.c      | 1 +
 block/qed.c        | 1 +
 block/raw-format.c | 6 ++++++
 block/rbd.c        | 1 +
 block/sheepdog.c   | 1 +
 block/ssh.c        | 1 +
 10 files changed, 18 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 4479cc7ab4..0208006f3c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2924,6 +2924,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_create = raw_co_create,
     .bdrv_co_create_opts = raw_co_create_opts,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
     .bdrv_co_block_status = raw_co_block_status,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
diff --git a/block/file-win32.c b/block/file-win32.c
index 6b2d67b239..41f55dfece 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -635,6 +635,7 @@ BlockDriver bdrv_file = {
     .bdrv_close         = raw_close,
     .bdrv_co_create_opts = raw_co_create_opts,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 
     .bdrv_aio_preadv    = raw_aio_preadv,
     .bdrv_aio_pwritev   = raw_aio_pwritev,
diff --git a/block/gluster.c b/block/gluster.c
index f64dc5b01e..64028b2cba 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1567,6 +1567,7 @@ static BlockDriver bdrv_gluster = {
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
+    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
 #endif
@@ -1598,6 +1599,7 @@ static BlockDriver bdrv_gluster_tcp = {
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
+    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
 #endif
@@ -1629,6 +1631,7 @@ static BlockDriver bdrv_gluster_unix = {
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
+    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
 #endif
@@ -1666,6 +1669,7 @@ static BlockDriver bdrv_gluster_rdma = {
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
+    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
 #endif
diff --git a/block/nfs.c b/block/nfs.c
index d93241b3bb..97c815085f 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -863,6 +863,7 @@ static BlockDriver bdrv_nfs = {
     .create_opts                    = &nfs_create_opts,
 
     .bdrv_has_zero_init             = nfs_has_zero_init,
+    .bdrv_has_zero_init_truncate    = nfs_has_zero_init,
     .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
     .bdrv_co_truncate               = nfs_file_co_truncate,
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 039bdc2f7e..5c40f54d64 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5187,6 +5187,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_co_create_opts  = qcow2_co_create_opts,
     .bdrv_co_create       = qcow2_co_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
     .bdrv_co_block_status = qcow2_co_block_status,
 
     .bdrv_co_preadv         = qcow2_co_preadv,
diff --git a/block/qed.c b/block/qed.c
index 77c7cef175..daaedb6864 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1668,6 +1668,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_co_create           = bdrv_qed_co_create,
     .bdrv_co_create_opts      = bdrv_qed_co_create_opts,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
     .bdrv_co_block_status     = bdrv_qed_co_block_status,
     .bdrv_co_readv            = bdrv_qed_co_readv,
     .bdrv_co_writev           = bdrv_qed_co_writev,
diff --git a/block/raw-format.c b/block/raw-format.c
index bffd424dd0..42c28cc29a 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -413,6 +413,11 @@ static int raw_has_zero_init(BlockDriverState *bs)
     return bdrv_has_zero_init(bs->file->bs);
 }
 
+static int raw_has_zero_init_truncate(BlockDriverState *bs)
+{
+    return bdrv_has_zero_init_truncate(bs->file->bs);
+}
+
 static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
                                            Error **errp)
 {
@@ -572,6 +577,7 @@ BlockDriver bdrv_raw = {
     .bdrv_co_ioctl        = &raw_co_ioctl,
     .create_opts          = &raw_create_opts,
     .bdrv_has_zero_init   = &raw_has_zero_init,
+    .bdrv_has_zero_init_truncate = &raw_has_zero_init_truncate,
     .strong_runtime_opts  = raw_strong_runtime_opts,
     .mutable_opts         = mutable_opts,
 };
diff --git a/block/rbd.c b/block/rbd.c
index 59757b3120..057af43d48 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1288,6 +1288,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_co_create         = qemu_rbd_co_create,
     .bdrv_co_create_opts    = qemu_rbd_co_create_opts,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
     .bdrv_get_info          = qemu_rbd_getinfo,
     .create_opts            = &qemu_rbd_create_opts,
     .bdrv_getlength         = qemu_rbd_getlength,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 6f402e5d4d..a4e111f981 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3228,6 +3228,7 @@ static BlockDriver bdrv_sheepdog = {
     .bdrv_co_create               = sd_co_create,
     .bdrv_co_create_opts          = sd_co_create_opts,
     .bdrv_has_zero_init           = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
     .bdrv_getlength               = sd_getlength,
     .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
     .bdrv_co_truncate             = sd_co_truncate,
diff --git a/block/ssh.c b/block/ssh.c
index 501933b855..84d01e892b 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1390,6 +1390,7 @@ static BlockDriver bdrv_ssh = {
     .bdrv_co_create_opts          = ssh_co_create_opts,
     .bdrv_close                   = ssh_close,
     .bdrv_has_zero_init           = ssh_has_zero_init,
+    .bdrv_has_zero_init_truncate  = ssh_has_zero_init,
     .bdrv_co_readv                = ssh_co_readv,
     .bdrv_co_writev               = ssh_co_writev,
     .bdrv_getlength               = ssh_getlength,
-- 
2.21.0

[Qemu-devel] [PATCH v2 05/11] block: Use bdrv_has_zero_init_truncate()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/parallels.c | 2 +-
 block/vhdx.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 00fae125d1..7cd2714b69 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -835,7 +835,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
 
-    if (!bdrv_has_zero_init(bs->file->bs)) {
+    if (!bdrv_has_zero_init_truncate(bs->file->bs)) {
         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
     }
 
diff --git a/block/vhdx.c b/block/vhdx.c
index d6070b6fa8..a02d1c99a7 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1282,7 +1282,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
                 /* Queue another write of zero buffers if the underlying file
                  * does not zero-fill on file extension */
 
-                if (bdrv_has_zero_init(bs->file->bs) == 0) {
+                if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) {
                     use_zero_buffers = true;
 
                     /* zero fill the front, if any */
-- 
2.21.0

[Qemu-devel] [PATCH v2 06/11] qcow2: Fix .bdrv_has_zero_init()

[Max Reitz]

If a qcow2 file is preallocated, it can no longer guarantee that it
initially appears as filled with zeroes.

So implement .bdrv_has_zero_init() by checking whether the file is
preallocated; if so, forward the call to the underlying storage node,
except for when it is encrypted: Encrypted preallocated images always
return effectively random data, so .bdrv_has_zero_init() must always
return 0 for them.

.bdrv_has_zero_init_truncate() can remain bdrv_has_zero_init_1(),
because it presupposes PREALLOC_MODE_OFF.

Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 5c40f54d64..b4e73aa443 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4631,6 +4631,33 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
     return spec_info;
 }
 
+static int qcow2_has_zero_init(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+    bool preallocated;
+
+    if (qemu_in_coroutine()) {
+        qemu_co_mutex_lock(&s->lock);
+    }
+    /*
+     * Check preallocation status: Preallocated images have all L2
+     * tables allocated, nonpreallocated images have none.  It is
+     * therefore enough to check the first one.
+     */
+    preallocated = s->l1_size > 0 && s->l1_table[0] != 0;
+    if (qemu_in_coroutine()) {
+        qemu_co_mutex_unlock(&s->lock);
+    }
+
+    if (!preallocated) {
+        return 1;
+    } else if (bs->encrypted) {
+        return 0;
+    } else {
+        return bdrv_has_zero_init(s->data_file->bs);
+    }
+}
+
 static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
                               int64_t pos)
 {
@@ -5186,7 +5213,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_child_perm      = bdrv_format_default_perms,
     .bdrv_co_create_opts  = qcow2_co_create_opts,
     .bdrv_co_create       = qcow2_co_create,
-    .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init   = qcow2_has_zero_init,
     .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
     .bdrv_co_block_status = qcow2_co_block_status,
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 07/11] vdi: Fix .bdrv_has_zero_init()

[Max Reitz]

Static VDI images cannot guarantee to be zero-initialized.  If the image
has been statically allocated, forward the call to the underlying
storage node.

Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
---
 block/vdi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index b9845a4cbd..0caa3f281d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -988,6 +988,17 @@ static void vdi_close(BlockDriverState *bs)
     error_free(s->migration_blocker);
 }
 
+static int vdi_has_zero_init(BlockDriverState *bs)
+{
+    BDRVVdiState *s = bs->opaque;
+
+    if (s->header.image_type == VDI_TYPE_STATIC) {
+        return bdrv_has_zero_init(bs->file->bs);
+    } else {
+        return 1;
+    }
+}
+
 static QemuOptsList vdi_create_opts = {
     .name = "vdi-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(vdi_create_opts.head),
@@ -1028,7 +1039,7 @@ static BlockDriver bdrv_vdi = {
     .bdrv_child_perm          = bdrv_format_default_perms,
     .bdrv_co_create      = vdi_co_create,
     .bdrv_co_create_opts = vdi_co_create_opts,
-    .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init  = vdi_has_zero_init,
     .bdrv_co_block_status = vdi_co_block_status,
     .bdrv_make_empty = vdi_make_empty,
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 08/11] vhdx: Fix .bdrv_has_zero_init()

[Max Reitz]

Fixed VHDX images cannot guarantee to be zero-initialized.  If the image
has the "fixed" subformat, forward the call to the underlying storage
node.

Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vhdx.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index a02d1c99a7..6a09d0a55c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2075,6 +2075,30 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
     return 0;
 }
 
+static int vhdx_has_zero_init(BlockDriverState *bs)
+{
+    BDRVVHDXState *s = bs->opaque;
+    int state;
+
+    /*
+     * Check the subformat: Fixed images have all BAT entries present,
+     * dynamic images have none (right after creation).  It is
+     * therefore enough to check the first BAT entry.
+     */
+    if (!s->bat_entries) {
+        return 1;
+    }
+
+    state = s->bat[0] & VHDX_BAT_STATE_BIT_MASK;
+    if (state == PAYLOAD_BLOCK_FULLY_PRESENT) {
+        /* Fixed subformat */
+        return bdrv_has_zero_init(bs->file->bs);
+    }
+
+    /* Dynamic subformat */
+    return 1;
+}
+
 static QemuOptsList vhdx_create_opts = {
     .name = "vhdx-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(vhdx_create_opts.head),
@@ -2128,7 +2152,7 @@ static BlockDriver bdrv_vhdx = {
     .bdrv_co_create_opts    = vhdx_co_create_opts,
     .bdrv_get_info          = vhdx_get_info,
     .bdrv_co_check          = vhdx_co_check,
-    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .bdrv_has_zero_init     = vhdx_has_zero_init,
 
     .create_opts            = &vhdx_create_opts,
 };
-- 
2.21.0

[Qemu-devel] [PATCH v2 09/11] iotests: Convert to preallocated encrypted qcow2

[Max Reitz]

Add a test case for converting an empty image (which only returns zeroes
when read) to a preallocated encrypted qcow2 image.
qcow2_has_zero_init() should return 0 then, thus forcing qemu-img
convert to create zero clusters.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Tested-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tests/qemu-iotests/188     | 20 +++++++++++++++++++-
 tests/qemu-iotests/188.out |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188
index be7278aa65..afca44df54 100755
--- a/tests/qemu-iotests/188
+++ b/tests/qemu-iotests/188
@@ -48,7 +48,7 @@ SECRETALT="secret,id=sec0,data=platypus"
 
 _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
 
-IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
+IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
 
 QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
 
@@ -68,6 +68,24 @@ echo
 echo "== verify open failure with wrong password =="
 $QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
 
+_cleanup_test_img
+
+echo
+echo "== verify that has_zero_init returns false when preallocating =="
+
+# Empty source file
+if [ -n "$TEST_IMG_FILE" ]; then
+    TEST_IMG_FILE="${TEST_IMG_FILE}.orig" _make_test_img $size
+else
+    TEST_IMG="${TEST_IMG}.orig" _make_test_img $size
+fi
+
+$QEMU_IMG convert -O "$IMGFMT" --object $SECRET \
+    -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,preallocation=metadata" \
+    "${TEST_IMG}.orig" "$TEST_IMG"
+
+$QEMU_IMG compare --object $SECRET --image-opts "${IMGSPEC}.orig" "$IMGSPEC"
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
index 97b1402671..c568ef3701 100644
--- a/tests/qemu-iotests/188.out
+++ b/tests/qemu-iotests/188.out
@@ -15,4 +15,8 @@ read 16777216/16777216 bytes at offset 0
 
 == verify open failure with wrong password ==
 qemu-io: can't open: Invalid password, cannot unlock any keyslot
+
+== verify that has_zero_init returns false when preallocating ==
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=16777216
+Images are identical.
 *** done
-- 
2.21.0

[Qemu-devel] [PATCH v2 10/11] iotests: Test convert -n to pre-filled image

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/122     | 17 +++++++++++++++++
 tests/qemu-iotests/122.out |  8 ++++++++
 2 files changed, 25 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index 85c3a8d047..059011ebb1 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -257,6 +257,23 @@ for min_sparse in 4k 8k; do
     $QEMU_IMG map --output=json "$TEST_IMG".orig | _filter_qemu_img_map
 done
 
+
+echo
+echo '=== -n to a non-zero image ==='
+echo
+
+# Keep source zero
+_make_test_img 64M
+
+# Output is not zero, but has bdrv_has_zero_init() == 1
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+$QEMU_IO -c "write -P 42 0 64k" "$TEST_IMG".orig | _filter_qemu_io
+
+# Convert with -n, which should not assume that the target is zeroed
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+
+$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index c576705284..849b6cc2ef 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -220,4 +220,12 @@ convert -c -S 8k
 { "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
 { "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true},
 { "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+
+=== -n to a non-zero image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+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.
 *** done
-- 
2.21.0

[Qemu-devel] [PATCH v2 11/11] iotests: Full mirror to existing non-zero image

[Max Reitz]

The result of a sync=full mirror should always be the equal to the
input.  Therefore, existing images should be treated as potentially
non-zero and thus should be explicitly initialized to be zero
beforehand.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/041     | 62 +++++++++++++++++++++++++++++++++++---
 tests/qemu-iotests/041.out |  4 +--
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 26bf1701eb..8bc8f81db7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -741,8 +741,15 @@ class TestUnbackedSource(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, test_img,
                  str(TestUnbackedSource.image_len))
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM()
         self.vm.launch()
+        result = self.vm.qmp('blockdev-add', node_name='drive0',
+                             driver=iotests.imgfmt,
+                             file={
+                                 'driver': 'file',
+                                 'filename': test_img,
+                             })
+        self.assert_qmp(result, 'return', {})
 
     def tearDown(self):
         self.vm.shutdown()
@@ -751,7 +758,7 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
     def test_absolute_paths_full(self):
         self.assert_no_active_block_jobs()
-        result = self.vm.qmp('drive-mirror', device='drive0',
+        result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
                              sync='full', target=target_img,
                              mode='absolute-paths')
         self.assert_qmp(result, 'return', {})
@@ -760,7 +767,7 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
     def test_absolute_paths_top(self):
         self.assert_no_active_block_jobs()
-        result = self.vm.qmp('drive-mirror', device='drive0',
+        result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
                              sync='top', target=target_img,
                              mode='absolute-paths')
         self.assert_qmp(result, 'return', {})
@@ -769,13 +776,60 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
     def test_absolute_paths_none(self):
         self.assert_no_active_block_jobs()
-        result = self.vm.qmp('drive-mirror', device='drive0',
+        result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
                              sync='none', target=target_img,
                              mode='absolute-paths')
         self.assert_qmp(result, 'return', {})
         self.complete_and_wait()
         self.assert_no_active_block_jobs()
 
+    def test_existing_full(self):
+        qemu_img('create', '-f', iotests.imgfmt, target_img,
+                 str(self.image_len))
+        qemu_io('-c', 'write -P 42 0 64k', target_img)
+
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
+                             sync='full', target=target_img, mode='existing')
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait()
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('blockdev-del', node_name='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
+    def test_blockdev_full(self):
+        qemu_img('create', '-f', iotests.imgfmt, target_img,
+                 str(self.image_len))
+        qemu_io('-c', 'write -P 42 0 64k', target_img)
+
+        result = self.vm.qmp('blockdev-add', node_name='target',
+                             driver=iotests.imgfmt,
+                             file={
+                                 'driver': 'file',
+                                 'filename': target_img,
+                             })
+        self.assert_qmp(result, 'return', {})
+
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('blockdev-mirror', job_id='drive0', device='drive0',
+                             sync='full', target='target')
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait()
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('blockdev-del', node_name='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-del', node_name='target')
+        self.assert_qmp(result, 'return', {})
+
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
 class TestGranularity(iotests.QMPTestCase):
     image_len = 10 * 1024 * 1024 # MB
 
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index e071d0b261..2c448b4239 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-........................................................................................
+..........................................................................................
 ----------------------------------------------------------------------
-Ran 88 tests
+Ran 90 tests
 
 OK
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> bdrv_has_zero_init() only has meaning for newly created images or image
> areas.  If qemu-img convert did not create the image itself, it cannot
> rely on bdrv_has_zero_init()'s result to carry any meaning.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..0f4be80c10 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
>      bool has_zero_init;
>      bool compressed;
>      bool unallocated_blocks_are_zero;
> +    bool target_is_new;
>      bool target_has_backing;
>      int64_t target_backing_sectors; /* negative if unknown */
>      bool wr_in_order;
> @@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
>      int64_t sector_num = 0;
>  
>      /* Check whether we have zero initialisation or can get it efficiently */
> -    s->has_zero_init = s->min_sparse && !s->target_has_backing
> -                     ? bdrv_has_zero_init(blk_bs(s->target))
> -                     : false;
> +    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +        s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> +    } else {
> +        s->has_zero_init = false;
> +    }
>  
>      if (!s->has_zero_init && !s->target_has_backing &&
>          bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
> @@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> +    s.target_is_new = !skip_create;
> +
>      flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 02/11] mirror: Fix bdrv_has_zero_init() use

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> bdrv_has_zero_init() only has meaning for newly created images or image
> areas.  If the mirror job itself did not create the image, it cannot
> rely on bdrv_has_zero_init()'s result to carry any meaning.
> 
> This is the case for drive-mirror with mode=existing and always for
> blockdev-mirror.
> 
> Note that we only have to zero-initialize the target with sync=full,
> because other modes actually do not promise that the target will contain
> the same data as the source after the job -- sync=top only promises to
> copy anything allocated in the top layer, and sync=none will only copy
> new I/O.  (Which is how mirror has always handled it.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block_int.h   |  2 ++
>  block/mirror.c              | 11 ++++++++---
>  blockdev.c                  | 16 +++++++++++++---
>  tests/test-block-iothread.c |  2 +-
>  4 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..6a0b1b5008 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1116,6 +1116,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
>   * @buf_size: The amount of data that can be in flight at one time.
>   * @mode: Whether to collapse all images in the chain to the target.
>   * @backing_mode: How to establish the target's backing chain after completion.
> + * @zero_target: Whether the target should be explicitly zero-initialized
>   * @on_source_error: The action to take upon error reading from the source.
>   * @on_target_error: The action to take upon error writing to the target.
>   * @unmap: Whether to unmap target where source sectors only contain zeroes.
> @@ -1135,6 +1136,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>                    int creation_flags, int64_t speed,
>                    uint32_t granularity, int64_t buf_size,
>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> +                  bool zero_target,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    bool unmap, const char *filter_node_name,
> diff --git a/block/mirror.c b/block/mirror.c
> index 8cb75fb409..50188ce6e9 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -51,6 +51,8 @@ typedef struct MirrorBlockJob {
>      Error *replace_blocker;
>      bool is_none_mode;
>      BlockMirrorBackingMode backing_mode;
> +    /* Whether the target image requires explicit zero-initialization */
> +    bool zero_target;
>      MirrorCopyMode copy_mode;
>      BlockdevOnError on_source_error, on_target_error;
>      bool synced;
> @@ -763,7 +765,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>      int ret;
>      int64_t count;
>  
> -    if (base == NULL && !bdrv_has_zero_init(target_bs)) {
> +    if (s->zero_target) {
The justification for removing base here, is that it can be != NULL only
when MIRROR_SYNC_MODE_TOP. So looks OK


>          if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
>              bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
>              return 0;
> @@ -1501,6 +1503,7 @@ static BlockJob *mirror_start_job(
>                               const char *replaces, int64_t speed,
>                               uint32_t granularity, int64_t buf_size,
>                               BlockMirrorBackingMode backing_mode,
> +                             bool zero_target,
>                               BlockdevOnError on_source_error,
>                               BlockdevOnError on_target_error,
>                               bool unmap,
> @@ -1628,6 +1631,7 @@ static BlockJob *mirror_start_job(
>      s->on_target_error = on_target_error;
>      s->is_none_mode = is_none_mode;
>      s->backing_mode = backing_mode;
> +    s->zero_target = zero_target;
>      s->copy_mode = copy_mode;
>      s->base = base;
>      s->granularity = granularity;
> @@ -1713,6 +1717,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>                    int creation_flags, int64_t speed,
>                    uint32_t granularity, int64_t buf_size,
>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> +                  bool zero_target,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    bool unmap, const char *filter_node_name,
> @@ -1728,7 +1733,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>      is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
>      base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
>      mirror_start_job(job_id, bs, creation_flags, target, replaces,
> -                     speed, granularity, buf_size, backing_mode,
> +                     speed, granularity, buf_size, backing_mode, zero_target,
>                       on_source_error, on_target_error, unmap, NULL, NULL,
>                       &mirror_job_driver, is_none_mode, base, false,
>                       filter_node_name, true, copy_mode, errp);
> @@ -1755,7 +1760,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
>  
>      ret = mirror_start_job(
>                       job_id, bs, creation_flags, base, NULL, speed, 0, 0,
> -                     MIRROR_LEAVE_BACKING_CHAIN,
> +                     MIRROR_LEAVE_BACKING_CHAIN, false,
>                       on_error, on_error, true, cb, opaque,
>                       &commit_active_job_driver, false, base, auto_complete,
>                       filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
> diff --git a/blockdev.c b/blockdev.c
> index 4d141e9a1f..0323f77850 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3705,6 +3705,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>                                     bool has_replaces, const char *replaces,
>                                     enum MirrorSyncMode sync,
>                                     BlockMirrorBackingMode backing_mode,
> +                                   bool zero_target,
>                                     bool has_speed, int64_t speed,
>                                     bool has_granularity, uint32_t granularity,
>                                     bool has_buf_size, int64_t buf_size,
> @@ -3813,7 +3814,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>       */
>      mirror_start(job_id, bs, target,
>                   has_replaces ? replaces : NULL, job_flags,
> -                 speed, granularity, buf_size, sync, backing_mode,
> +                 speed, granularity, buf_size, sync, backing_mode, zero_target,
>                   on_source_error, on_target_error, unmap, filter_node_name,
>                   copy_mode, errp);
>  }
> @@ -3829,6 +3830,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>      int flags;
>      int64_t size;
>      const char *format = arg->format;
> +    bool zero_target;
>      int ret;
>  
>      bs = qmp_get_root_bs(arg->device, errp);
> @@ -3930,6 +3932,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>          goto out;
>      }
>  
> +    zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL &&
> +                   (arg->mode == NEW_IMAGE_MODE_EXISTING ||
> +                    !bdrv_has_zero_init(target_bs)));
> +
>      ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>      if (ret < 0) {
>          bdrv_unref(target_bs);
> @@ -3938,7 +3944,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>  
>      blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
>                             arg->has_replaces, arg->replaces, arg->sync,
> -                           backing_mode, arg->has_speed, arg->speed,
> +                           backing_mode, zero_target,
> +                           arg->has_speed, arg->speed,
>                             arg->has_granularity, arg->granularity,
>                             arg->has_buf_size, arg->buf_size,
>                             arg->has_on_source_error, arg->on_source_error,
> @@ -3978,6 +3985,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>      AioContext *aio_context;
>      BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
>      Error *local_err = NULL;
> +    bool zero_target;
>      int ret;
>  
>      bs = qmp_get_root_bs(device, errp);
> @@ -3990,6 +3998,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>          return;
>      }
>  
> +    zero_target = (sync == MIRROR_SYNC_MODE_FULL);
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> @@ -4000,7 +4010,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>  
>      blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
>                             has_replaces, replaces, sync, backing_mode,
> -                           has_speed, speed,
> +                           zero_target, has_speed, speed,
>                             has_granularity, granularity,
>                             has_buf_size, buf_size,
>                             has_on_source_error, on_source_error,
> diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
> index 1949d5e61a..debfb69bfb 100644
> --- a/tests/test-block-iothread.c
> +++ b/tests/test-block-iothread.c
> @@ -611,7 +611,7 @@ static void test_propagate_mirror(void)
>  
>      /* Start a mirror job */
>      mirror_start("job0", src, target, NULL, JOB_DEFAULT, 0, 0, 0,
> -                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN,
> +                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, false,
>                   BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
>                   false, "filter_node", MIRROR_COPY_MODE_BACKGROUND,
>                   &error_abort);


From my limited understanding of this code, it looks ok to me.

Still to be very sure, I sort of suggest still to check that nobody relies on target zeroing 
when non in full sync mode, to avoid breaking the users

For example, QMP reference states that MIRROR_SYNC_MODE_TOP copies data in the topmost image to the destination.
If there is only the topmost image, I could image the caller assume that target is identical to the source.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 03/11] block: Add bdrv_has_zero_init_truncate()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> No .bdrv_has_zero_init() implementation returns 1 if growing the file
> would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
> in lieu of this new function was always safe.
> 
> But on the other hand, it is possible that growing an image that is not
> zero-initialized would still add a zero-initialized area, like when
> using nonpreallocating truncation on a preallocated image.  For callers
> that care only about truncation, not about creation with potential
> preallocation, this new function is useful.
> 
> Alternatively, we could have added a PreallocMode parameter to
> bdrv_has_zero_init().  But the only user would have been qemu-img
> convert, which does not have a plain PreallocMode value right now -- it
> would have to parse the creation option to obtain it.  Therefore, the
> simpler solution is to let bdrv_has_zero_init() inquire the
> preallocation status and add the new bdrv_has_zero_init_truncate() that
> presupposes PREALLOC_MODE_OFF.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  7 +++++++
>  block.c                   | 21 +++++++++++++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 50a07c1c33..5321d8afdf 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -438,6 +438,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>  int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>  int bdrv_has_zero_init_1(BlockDriverState *bs);
>  int bdrv_has_zero_init(BlockDriverState *bs);
> +int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
>  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>  int bdrv_block_status(BlockDriverState *bs, int64_t offset,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 6a0b1b5008..d7fc6b296b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -420,9 +420,16 @@ struct BlockDriver {
>      /*
>       * Returns 1 if newly created images are guaranteed to contain only
>       * zeros, 0 otherwise.
> +     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /*
> +     * Returns 1 if new areas added by growing the image with
> +     * PREALLOC_MODE_OFF contain only zeros, 0 otherwise.
> +     */
> +    int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs);
> +
>      /* Remove fd handlers, timers, and other event loop callbacks so the event
>       * loop is no longer in use.  Called with no in-flight requests and in
>       * depth-first traversal order with parents before child nodes.
> diff --git a/block.c b/block.c
> index cbd8da5f3b..81ae44dcf3 100644
> --- a/block.c
> +++ b/block.c
> @@ -5066,6 +5066,27 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>      return 0;
>  }
>  
> +int bdrv_has_zero_init_truncate(BlockDriverState *bs)
> +{
> +    if (!bs->drv) {
> +        return 0;
> +    }
> +
> +    if (bs->backing) {
> +        /* Depends on the backing image length, but better safe than sorry */
> +        return 0;
> +    }
> +    if (bs->drv->bdrv_has_zero_init_truncate) {
> +        return bs->drv->bdrv_has_zero_init_truncate(bs);
> +    }
> +    if (bs->file && bs->drv->is_filter) {
> +        return bdrv_has_zero_init_truncate(bs->file->bs);
> +    }
> +
> +    /* safe default */
> +    return 0;
> +}
> +
>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
>  {
>      BlockDriverInfo bdi;


This looks like a very correct change, even for the sake
of clarifying the scope of bdrv_has_zero_init

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 04/11] block: Implement .bdrv_has_zero_init_truncate()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> We need to implement .bdrv_has_zero_init_truncate() for every block
> driver that supports truncation and has a .bdrv_has_zero_init()
> implementation.
> 
> Implement it the same way each driver implements .bdrv_has_zero_init().
> This is at least not any more unsafe than what we had before.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 1 +
>  block/file-win32.c | 1 +
>  block/gluster.c    | 4 ++++
>  block/nfs.c        | 1 +
>  block/qcow2.c      | 1 +
>  block/qed.c        | 1 +
>  block/raw-format.c | 6 ++++++
>  block/rbd.c        | 1 +
>  block/sheepdog.c   | 1 +
>  block/ssh.c        | 1 +
>  10 files changed, 18 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 4479cc7ab4..0208006f3c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2924,6 +2924,7 @@ BlockDriver bdrv_file = {
>      .bdrv_co_create = raw_co_create,
>      .bdrv_co_create_opts = raw_co_create_opts,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status = raw_co_block_status,
>      .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>      .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
> diff --git a/block/file-win32.c b/block/file-win32.c
> index 6b2d67b239..41f55dfece 100644
> --- a/block/file-win32.c
> +++ b/block/file-win32.c
> @@ -635,6 +635,7 @@ BlockDriver bdrv_file = {
>      .bdrv_close         = raw_close,
>      .bdrv_co_create_opts = raw_co_create_opts,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>  
>      .bdrv_aio_preadv    = raw_aio_preadv,
>      .bdrv_aio_pwritev   = raw_aio_pwritev,
> diff --git a/block/gluster.c b/block/gluster.c
> index f64dc5b01e..64028b2cba 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -1567,6 +1567,7 @@ static BlockDriver bdrv_gluster = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1598,6 +1599,7 @@ static BlockDriver bdrv_gluster_tcp = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1629,6 +1631,7 @@ static BlockDriver bdrv_gluster_unix = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1666,6 +1669,7 @@ static BlockDriver bdrv_gluster_rdma = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> diff --git a/block/nfs.c b/block/nfs.c
> index d93241b3bb..97c815085f 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -863,6 +863,7 @@ static BlockDriver bdrv_nfs = {
>      .create_opts                    = &nfs_create_opts,
>  
>      .bdrv_has_zero_init             = nfs_has_zero_init,
> +    .bdrv_has_zero_init_truncate    = nfs_has_zero_init,
>      .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
>      .bdrv_co_truncate               = nfs_file_co_truncate,
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 039bdc2f7e..5c40f54d64 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5187,6 +5187,7 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_co_create_opts  = qcow2_co_create_opts,
>      .bdrv_co_create       = qcow2_co_create,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status = qcow2_co_block_status,
>  
>      .bdrv_co_preadv         = qcow2_co_preadv,
> diff --git a/block/qed.c b/block/qed.c
> index 77c7cef175..daaedb6864 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1668,6 +1668,7 @@ static BlockDriver bdrv_qed = {
>      .bdrv_co_create           = bdrv_qed_co_create,
>      .bdrv_co_create_opts      = bdrv_qed_co_create_opts,
>      .bdrv_has_zero_init       = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status     = bdrv_qed_co_block_status,
>      .bdrv_co_readv            = bdrv_qed_co_readv,
>      .bdrv_co_writev           = bdrv_qed_co_writev,
> diff --git a/block/raw-format.c b/block/raw-format.c
> index bffd424dd0..42c28cc29a 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -413,6 +413,11 @@ static int raw_has_zero_init(BlockDriverState *bs)
>      return bdrv_has_zero_init(bs->file->bs);
>  }
>  
> +static int raw_has_zero_init_truncate(BlockDriverState *bs)
> +{
> +    return bdrv_has_zero_init_truncate(bs->file->bs);
> +}
> +
>  static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
>                                             Error **errp)
>  {
> @@ -572,6 +577,7 @@ BlockDriver bdrv_raw = {
>      .bdrv_co_ioctl        = &raw_co_ioctl,
>      .create_opts          = &raw_create_opts,
>      .bdrv_has_zero_init   = &raw_has_zero_init,
> +    .bdrv_has_zero_init_truncate = &raw_has_zero_init_truncate,
>      .strong_runtime_opts  = raw_strong_runtime_opts,
>      .mutable_opts         = mutable_opts,
>  };
> diff --git a/block/rbd.c b/block/rbd.c
> index 59757b3120..057af43d48 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1288,6 +1288,7 @@ static BlockDriver bdrv_rbd = {
>      .bdrv_co_create         = qemu_rbd_co_create,
>      .bdrv_co_create_opts    = qemu_rbd_co_create_opts,
>      .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_get_info          = qemu_rbd_getinfo,
>      .create_opts            = &qemu_rbd_create_opts,
>      .bdrv_getlength         = qemu_rbd_getlength,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 6f402e5d4d..a4e111f981 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -3228,6 +3228,7 @@ static BlockDriver bdrv_sheepdog = {
>      .bdrv_co_create               = sd_co_create,
>      .bdrv_co_create_opts          = sd_co_create_opts,
>      .bdrv_has_zero_init           = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>      .bdrv_getlength               = sd_getlength,
>      .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>      .bdrv_co_truncate             = sd_co_truncate,
> diff --git a/block/ssh.c b/block/ssh.c
> index 501933b855..84d01e892b 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -1390,6 +1390,7 @@ static BlockDriver bdrv_ssh = {
>      .bdrv_co_create_opts          = ssh_co_create_opts,
>      .bdrv_close                   = ssh_close,
>      .bdrv_has_zero_init           = ssh_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = ssh_has_zero_init,
>      .bdrv_co_readv                = ssh_co_readv,
>      .bdrv_co_writev               = ssh_co_writev,
>      .bdrv_getlength               = ssh_getlength,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 05/11] block: Use bdrv_has_zero_init_truncate()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/parallels.c | 2 +-
>  block/vhdx.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 00fae125d1..7cd2714b69 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -835,7 +835,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail_options;
>      }
>  
> -    if (!bdrv_has_zero_init(bs->file->bs)) {
> +    if (!bdrv_has_zero_init_truncate(bs->file->bs)) {
>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>      }
>  
> diff --git a/block/vhdx.c b/block/vhdx.c
> index d6070b6fa8..a02d1c99a7 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1282,7 +1282,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
>                  /* Queue another write of zero buffers if the underlying file
>                   * does not zero-fill on file extension */
>  
> -                if (bdrv_has_zero_init(bs->file->bs) == 0) {
> +                if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) {
>                      use_zero_buffers = true;
>  
>                      /* zero fill the front, if any */

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 06/11] qcow2: Fix .bdrv_has_zero_init()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> If a qcow2 file is preallocated, it can no longer guarantee that it
> initially appears as filled with zeroes.
> 
> So implement .bdrv_has_zero_init() by checking whether the file is
> preallocated; if so, forward the call to the underlying storage node,
> except for when it is encrypted: Encrypted preallocated images always
> return effectively random data, so .bdrv_has_zero_init() must always
> return 0 for them.
> 
> .bdrv_has_zero_init_truncate() can remain bdrv_has_zero_init_1(),
> because it presupposes PREALLOC_MODE_OFF.
> 
> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 5c40f54d64..b4e73aa443 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4631,6 +4631,33 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
>      return spec_info;
>  }
>  
> +static int qcow2_has_zero_init(BlockDriverState *bs)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    bool preallocated;
> +
> +    if (qemu_in_coroutine()) {
> +        qemu_co_mutex_lock(&s->lock);
> +    }
> +    /*
> +     * Check preallocation status: Preallocated images have all L2
> +     * tables allocated, nonpreallocated images have none.  It is
> +     * therefore enough to check the first one.
> +     */
> +    preallocated = s->l1_size > 0 && s->l1_table[0] != 0;
> +    if (qemu_in_coroutine()) {
> +        qemu_co_mutex_unlock(&s->lock);
> +    }
> +
> +    if (!preallocated) {
> +        return 1;
> +    } else if (bs->encrypted) {
> +        return 0;
> +    } else {
> +        return bdrv_has_zero_init(s->data_file->bs);
> +    }
> +}
> +
>  static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>                                int64_t pos)
>  {
> @@ -5186,7 +5213,7 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_child_perm      = bdrv_format_default_perms,
>      .bdrv_co_create_opts  = qcow2_co_create_opts,
>      .bdrv_co_create       = qcow2_co_create,
> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init   = qcow2_has_zero_init,
>      .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status = qcow2_co_block_status,
>  


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 07/11] vdi: Fix .bdrv_has_zero_init()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> Static VDI images cannot guarantee to be zero-initialized.  If the image
> has been statically allocated, forward the call to the underlying
> storage node.
> 
> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  block/vdi.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index b9845a4cbd..0caa3f281d 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -988,6 +988,17 @@ static void vdi_close(BlockDriverState *bs)
>      error_free(s->migration_blocker);
>  }
>  
> +static int vdi_has_zero_init(BlockDriverState *bs)
> +{
> +    BDRVVdiState *s = bs->opaque;
> +
> +    if (s->header.image_type == VDI_TYPE_STATIC) {
> +        return bdrv_has_zero_init(bs->file->bs);
> +    } else {
> +        return 1;
> +    }
> +}
> +
>  static QemuOptsList vdi_create_opts = {
>      .name = "vdi-create-opts",
>      .head = QTAILQ_HEAD_INITIALIZER(vdi_create_opts.head),
> @@ -1028,7 +1039,7 @@ static BlockDriver bdrv_vdi = {
>      .bdrv_child_perm          = bdrv_format_default_perms,
>      .bdrv_co_create      = vdi_co_create,
>      .bdrv_co_create_opts = vdi_co_create_opts,
> -    .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init  = vdi_has_zero_init,
>      .bdrv_co_block_status = vdi_co_block_status,
>      .bdrv_make_empty = vdi_make_empty,
>  


I am not familiar with VDI format to be honest, but knowing that dynamic format allows for growing
and static are preallocated this makes sense.

I see that the code when it allocates a new block at the end of the file, actually zeroes it out, so most
likely this is right.


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 08/11] vhdx: Fix .bdrv_has_zero_init()

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> Fixed VHDX images cannot guarantee to be zero-initialized.  If the image
> has the "fixed" subformat, forward the call to the underlying storage
> node.
> 
> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vhdx.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index a02d1c99a7..6a09d0a55c 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2075,6 +2075,30 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
>      return 0;
>  }
>  
> +static int vhdx_has_zero_init(BlockDriverState *bs)
> +{
> +    BDRVVHDXState *s = bs->opaque;
> +    int state;
> +
> +    /*
> +     * Check the subformat: Fixed images have all BAT entries present,
> +     * dynamic images have none (right after creation).  It is
> +     * therefore enough to check the first BAT entry.
> +     */
> +    if (!s->bat_entries) {
> +        return 1;
> +    }
> +
> +    state = s->bat[0] & VHDX_BAT_STATE_BIT_MASK;
> +    if (state == PAYLOAD_BLOCK_FULLY_PRESENT) {
> +        /* Fixed subformat */
> +        return bdrv_has_zero_init(bs->file->bs);
> +    }
> +
> +    /* Dynamic subformat */
> +    return 1;
> +}
> +
>  static QemuOptsList vhdx_create_opts = {
>      .name = "vhdx-create-opts",
>      .head = QTAILQ_HEAD_INITIALIZER(vhdx_create_opts.head),
> @@ -2128,7 +2152,7 @@ static BlockDriver bdrv_vhdx = {
>      .bdrv_co_create_opts    = vhdx_co_create_opts,
>      .bdrv_get_info          = vhdx_get_info,
>      .bdrv_co_check          = vhdx_co_check,
> -    .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init     = vhdx_has_zero_init,
>  
>      .create_opts            = &vhdx_create_opts,
>  };

I am not familiar with VHDX format to be honest too, but knowing that dynamic format allows for growing
and static are preallocated this makes sense.

Its a bit amusing and not surprising that the the spec for this format is in .docx. 
I took a quick look to get a rough impression of the file format.


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 09/11] iotests: Convert to preallocated encrypted qcow2

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> Add a test case for converting an empty image (which only returns zeroes
> when read) to a preallocated encrypted qcow2 image.
> qcow2_has_zero_init() should return 0 then, thus forcing qemu-img
> convert to create zero clusters.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/qemu-iotests/188     | 20 +++++++++++++++++++-
>  tests/qemu-iotests/188.out |  4 ++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188
> index be7278aa65..afca44df54 100755
> --- a/tests/qemu-iotests/188
> +++ b/tests/qemu-iotests/188
> @@ -48,7 +48,7 @@ SECRETALT="secret,id=sec0,data=platypus"
>  
>  _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
>  
> -IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
> +IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
This change I think doesn't change anything

>  
>  QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
>  
> @@ -68,6 +68,24 @@ echo
>  echo "== verify open failure with wrong password =="
>  $QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
>  
> +_cleanup_test_img
> +
> +echo
> +echo "== verify that has_zero_init returns false when preallocating =="
> +
> +# Empty source file
> +if [ -n "$TEST_IMG_FILE" ]; then
> +    TEST_IMG_FILE="${TEST_IMG_FILE}.orig" _make_test_img $size
> +else
> +    TEST_IMG="${TEST_IMG}.orig" _make_test_img $size
> +fi

I wonder why do we have TEST_IMG_FILE and TEST_IMG, I don't know iotests well enough
From the quick look at the code, the TEST_IMG_FILE is an actual file, while TEST_IMG can
be various URL like address.

> +
> +$QEMU_IMG convert -O "$IMGFMT" --object $SECRET \
> +    -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,preallocation=metadata" \
> +    "${TEST_IMG}.orig" "$TEST_IMG"
> +
> +$QEMU_IMG compare --object $SECRET --image-opts "${IMGSPEC}.orig" "$IMGSPEC"
> +
>  
>  # success, all done
>  echo "*** done"
> diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
> index 97b1402671..c568ef3701 100644
> --- a/tests/qemu-iotests/188.out
> +++ b/tests/qemu-iotests/188.out
> @@ -15,4 +15,8 @@ read 16777216/16777216 bytes at offset 0
>  
>  == verify open failure with wrong password ==
>  qemu-io: can't open: Invalid password, cannot unlock any keyslot
> +
> +== verify that has_zero_init returns false when preallocating ==
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=16777216
> +Images are identical.
>  *** done

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 10/11] iotests: Test convert -n to pre-filled image

[Maxim Levitsky]

On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/122     | 17 +++++++++++++++++
>  tests/qemu-iotests/122.out |  8 ++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> index 85c3a8d047..059011ebb1 100755
> --- a/tests/qemu-iotests/122
> +++ b/tests/qemu-iotests/122
> @@ -257,6 +257,23 @@ for min_sparse in 4k 8k; do
>      $QEMU_IMG map --output=json "$TEST_IMG".orig | _filter_qemu_img_map
>  done
>  
> +
> +echo
> +echo '=== -n to a non-zero image ==='
> +echo
> +
> +# Keep source zero
> +_make_test_img 64M
> +
> +# Output is not zero, but has bdrv_has_zero_init() == 1
> +TEST_IMG="$TEST_IMG".orig _make_test_img 64M
> +$QEMU_IO -c "write -P 42 0 64k" "$TEST_IMG".orig | _filter_qemu_io
> +
> +# Convert with -n, which should not assume that the target is zeroed
> +$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
> +
> +$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
> +
>  # success, all done
>  echo '*** done'
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> index c576705284..849b6cc2ef 100644
> --- a/tests/qemu-iotests/122.out
> +++ b/tests/qemu-iotests/122.out
> @@ -220,4 +220,12 @@ convert -c -S 8k
>  { "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
>  { "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true},
>  { "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
> +
> +=== -n to a non-zero image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
> +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.
>  *** done


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v2 02/11] mirror: Fix bdrv_has_zero_init() use

[Max Reitz]

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

On 25.07.19 17:28, Maxim Levitsky wrote:
> On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
>> bdrv_has_zero_init() only has meaning for newly created images or image
>> areas.  If the mirror job itself did not create the image, it cannot
>> rely on bdrv_has_zero_init()'s result to carry any meaning.
>>
>> This is the case for drive-mirror with mode=existing and always for
>> blockdev-mirror.
>>
>> Note that we only have to zero-initialize the target with sync=full,
>> because other modes actually do not promise that the target will contain
>> the same data as the source after the job -- sync=top only promises to
>> copy anything allocated in the top layer, and sync=none will only copy
>> new I/O.  (Which is how mirror has always handled it.)
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block_int.h   |  2 ++
>>  block/mirror.c              | 11 ++++++++---
>>  blockdev.c                  | 16 +++++++++++++---
>>  tests/test-block-iothread.c |  2 +-
>>  4 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 3aa1e832a8..6a0b1b5008 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1116,6 +1116,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
>>   * @buf_size: The amount of data that can be in flight at one time.
>>   * @mode: Whether to collapse all images in the chain to the target.
>>   * @backing_mode: How to establish the target's backing chain after completion.
>> + * @zero_target: Whether the target should be explicitly zero-initialized
>>   * @on_source_error: The action to take upon error reading from the source.
>>   * @on_target_error: The action to take upon error writing to the target.
>>   * @unmap: Whether to unmap target where source sectors only contain zeroes.
>> @@ -1135,6 +1136,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>>                    int creation_flags, int64_t speed,
>>                    uint32_t granularity, int64_t buf_size,
>>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>> +                  bool zero_target,
>>                    BlockdevOnError on_source_error,
>>                    BlockdevOnError on_target_error,
>>                    bool unmap, const char *filter_node_name,
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 8cb75fb409..50188ce6e9 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -51,6 +51,8 @@ typedef struct MirrorBlockJob {
>>      Error *replace_blocker;
>>      bool is_none_mode;
>>      BlockMirrorBackingMode backing_mode;
>> +    /* Whether the target image requires explicit zero-initialization */
>> +    bool zero_target;
>>      MirrorCopyMode copy_mode;
>>      BlockdevOnError on_source_error, on_target_error;
>>      bool synced;
>> @@ -763,7 +765,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>      int ret;
>>      int64_t count;
>>  
>> -    if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>> +    if (s->zero_target) {
> The justification for removing base here, is that it can be != NULL only
> when MIRROR_SYNC_MODE_TOP. So looks OK

Or with sync=none, or when doing an active commit,

>>          if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
>>              bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
>>              return 0;
>> @@ -1501,6 +1503,7 @@ static BlockJob *mirror_start_job(
>>                               const char *replaces, int64_t speed,
>>                               uint32_t granularity, int64_t buf_size,
>>                               BlockMirrorBackingMode backing_mode,
>> +                             bool zero_target,
>>                               BlockdevOnError on_source_error,
>>                               BlockdevOnError on_target_error,
>>                               bool unmap,
>> @@ -1628,6 +1631,7 @@ static BlockJob *mirror_start_job(
>>      s->on_target_error = on_target_error;
>>      s->is_none_mode = is_none_mode;
>>      s->backing_mode = backing_mode;
>> +    s->zero_target = zero_target;
>>      s->copy_mode = copy_mode;
>>      s->base = base;
>>      s->granularity = granularity;
>> @@ -1713,6 +1717,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>>                    int creation_flags, int64_t speed,
>>                    uint32_t granularity, int64_t buf_size,
>>                    MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>> +                  bool zero_target,
>>                    BlockdevOnError on_source_error,
>>                    BlockdevOnError on_target_error,
>>                    bool unmap, const char *filter_node_name,
>> @@ -1728,7 +1733,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>>      is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
>>      base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
>>      mirror_start_job(job_id, bs, creation_flags, target, replaces,
>> -                     speed, granularity, buf_size, backing_mode,
>> +                     speed, granularity, buf_size, backing_mode, zero_target,
>>                       on_source_error, on_target_error, unmap, NULL, NULL,
>>                       &mirror_job_driver, is_none_mode, base, false,
>>                       filter_node_name, true, copy_mode, errp);
>> @@ -1755,7 +1760,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
>>  
>>      ret = mirror_start_job(
>>                       job_id, bs, creation_flags, base, NULL, speed, 0, 0,
>> -                     MIRROR_LEAVE_BACKING_CHAIN,
>> +                     MIRROR_LEAVE_BACKING_CHAIN, false,
>>                       on_error, on_error, true, cb, opaque,
>>                       &commit_active_job_driver, false, base, auto_complete,
>>                       filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
>> diff --git a/blockdev.c b/blockdev.c
>> index 4d141e9a1f..0323f77850 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3705,6 +3705,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>>                                     bool has_replaces, const char *replaces,
>>                                     enum MirrorSyncMode sync,
>>                                     BlockMirrorBackingMode backing_mode,
>> +                                   bool zero_target,
>>                                     bool has_speed, int64_t speed,
>>                                     bool has_granularity, uint32_t granularity,
>>                                     bool has_buf_size, int64_t buf_size,
>> @@ -3813,7 +3814,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>>       */
>>      mirror_start(job_id, bs, target,
>>                   has_replaces ? replaces : NULL, job_flags,
>> -                 speed, granularity, buf_size, sync, backing_mode,
>> +                 speed, granularity, buf_size, sync, backing_mode, zero_target,
>>                   on_source_error, on_target_error, unmap, filter_node_name,
>>                   copy_mode, errp);
>>  }
>> @@ -3829,6 +3830,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>      int flags;
>>      int64_t size;
>>      const char *format = arg->format;
>> +    bool zero_target;
>>      int ret;
>>  
>>      bs = qmp_get_root_bs(arg->device, errp);
>> @@ -3930,6 +3932,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>          goto out;
>>      }
>>  
>> +    zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL &&
>> +                   (arg->mode == NEW_IMAGE_MODE_EXISTING ||
>> +                    !bdrv_has_zero_init(target_bs)));
>> +
>>      ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>>      if (ret < 0) {
>>          bdrv_unref(target_bs);
>> @@ -3938,7 +3944,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>  
>>      blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
>>                             arg->has_replaces, arg->replaces, arg->sync,
>> -                           backing_mode, arg->has_speed, arg->speed,
>> +                           backing_mode, zero_target,
>> +                           arg->has_speed, arg->speed,
>>                             arg->has_granularity, arg->granularity,
>>                             arg->has_buf_size, arg->buf_size,
>>                             arg->has_on_source_error, arg->on_source_error,
>> @@ -3978,6 +3985,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>>      AioContext *aio_context;
>>      BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
>>      Error *local_err = NULL;
>> +    bool zero_target;
>>      int ret;
>>  
>>      bs = qmp_get_root_bs(device, errp);
>> @@ -3990,6 +3998,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>>          return;
>>      }
>>  
>> +    zero_target = (sync == MIRROR_SYNC_MODE_FULL);
>> +
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>>  
>> @@ -4000,7 +4010,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
>>  
>>      blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
>>                             has_replaces, replaces, sync, backing_mode,
>> -                           has_speed, speed,
>> +                           zero_target, has_speed, speed,
>>                             has_granularity, granularity,
>>                             has_buf_size, buf_size,
>>                             has_on_source_error, on_source_error,
>> diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
>> index 1949d5e61a..debfb69bfb 100644
>> --- a/tests/test-block-iothread.c
>> +++ b/tests/test-block-iothread.c
>> @@ -611,7 +611,7 @@ static void test_propagate_mirror(void)
>>  
>>      /* Start a mirror job */
>>      mirror_start("job0", src, target, NULL, JOB_DEFAULT, 0, 0, 0,
>> -                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN,
>> +                 MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, false,
>>                   BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
>>                   false, "filter_node", MIRROR_COPY_MODE_BACKGROUND,
>>                   &error_abort);
> 
> 
> From my limited understanding of this code, it looks ok to me.
> 
> Still to be very sure, I sort of suggest still to check that nobody relies on target zeroing 
> when non in full sync mode, to avoid breaking the users

Whenever we zeroed the target before this patch, we will still zero it
afterwards.

We zeroed it only when base == NULL, which translates to sync=full.  We
never zeroed it in any other case.

> For example, QMP reference states that MIRROR_SYNC_MODE_TOP copies data in the topmost image to the destination.
> If there is only the topmost image, I could image the caller assume that target is identical to the source.

It doesn’t say that it copies the data in the topmost image.  It says it
copies the data *allocated* in the topmost image.  It follows that it
will not copy any data that is not allocated.

(So if you preallocate the source, it will indeed copy all data and thus
the target will be identical to the source.)

> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Thanks for reviewing!

Max


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

Re: [Qemu-devel] [PATCH v2 02/11] mirror: Fix bdrv_has_zero_init() use

[Max Reitz]

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

On 25.07.19 18:21, Max Reitz wrote:
> On 25.07.19 17:28, Maxim Levitsky wrote:

[...]

>> For example, QMP reference states that MIRROR_SYNC_MODE_TOP copies data in the topmost image to the destination.
>> If there is only the topmost image, I could image the caller assume that target is identical to the source.
> 
> It doesn’t say that it copies the data in the topmost image.  It says it
> copies the data *allocated* in the topmost image.  It follows that it
> will not copy any data that is not allocated.

(I just saw that MirrorSyncMode indeed just says “data in the topmost
image”.  I was looking at DriveBackup and blockdev-backup, which say
“only the sectors allocated in the topmost image”.  I think what
DriveBackup and blockdev-backup say takes precedence here.)

Max


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

Re: [Qemu-devel] [PATCH v2 09/11] iotests: Convert to preallocated encrypted qcow2

[Max Reitz]

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

On 25.07.19 17:30, Maxim Levitsky wrote:
> On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
>> Add a test case for converting an empty image (which only returns zeroes
>> when read) to a preallocated encrypted qcow2 image.
>> qcow2_has_zero_init() should return 0 then, thus forcing qemu-img
>> convert to create zero clusters.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
>> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  tests/qemu-iotests/188     | 20 +++++++++++++++++++-
>>  tests/qemu-iotests/188.out |  4 ++++
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188
>> index be7278aa65..afca44df54 100755
>> --- a/tests/qemu-iotests/188
>> +++ b/tests/qemu-iotests/188
>> @@ -48,7 +48,7 @@ SECRETALT="secret,id=sec0,data=platypus"
>>  
>>  _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
>>  
>> -IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
>> +IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
> This change I think doesn't change anything
> 
>>  
>>  QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
>>  
>> @@ -68,6 +68,24 @@ echo
>>  echo "== verify open failure with wrong password =="
>>  $QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
>>  
>> +_cleanup_test_img
>> +
>> +echo
>> +echo "== verify that has_zero_init returns false when preallocating =="
>> +
>> +# Empty source file
>> +if [ -n "$TEST_IMG_FILE" ]; then
>> +    TEST_IMG_FILE="${TEST_IMG_FILE}.orig" _make_test_img $size
>> +else
>> +    TEST_IMG="${TEST_IMG}.orig" _make_test_img $size
>> +fi
> 
> I wonder why do we have TEST_IMG_FILE and TEST_IMG, I don't know iotests well enough
> From the quick look at the code, the TEST_IMG_FILE is an actual file, while TEST_IMG can
> be various URL like address.

In theory, $TEST_IMG is what you give to the various qemu commands for
what you want to test.  It can be a URL, a plain path, or even in option
syntax (think file.filename=$TEST_IMG_FILE).  $TEST_IMG_FILE points to
the actual file on the local filesystem.

In practice, $TEST_IMG_FILE can be empty and then you only have
$TEST_IMG to work with.  Also, many tests only support the file protocol
anyway, which is exactly one such case, so they just use $TEST_IMG all
the time.

Max

>> +
>> +$QEMU_IMG convert -O "$IMGFMT" --object $SECRET \
>> +    -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,preallocation=metadata" \
>> +    "${TEST_IMG}.orig" "$TEST_IMG"
>> +
>> +$QEMU_IMG compare --object $SECRET --image-opts "${IMGSPEC}.orig" "$IMGSPEC"
>> +
>>  
>>  # success, all done
>>  echo "*** done"
>> diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
>> index 97b1402671..c568ef3701 100644
>> --- a/tests/qemu-iotests/188.out
>> +++ b/tests/qemu-iotests/188.out
>> @@ -15,4 +15,8 @@ read 16777216/16777216 bytes at offset 0
>>  
>>  == verify open failure with wrong password ==
>>  qemu-io: can't open: Invalid password, cannot unlock any keyslot
>> +
>> +== verify that has_zero_init returns false when preallocating ==
>> +Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=16777216
>> +Images are identical.
>>  *** done
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Best regards,
> 	Maxim Levitsky
> 



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

Re: [Qemu-devel] [PATCH v2 03/11] block: Add bdrv_has_zero_init_truncate()

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 07:12:31PM +0200, Max Reitz wrote:
> No .bdrv_has_zero_init() implementation returns 1 if growing the file
> would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
> in lieu of this new function was always safe.
> 
> But on the other hand, it is possible that growing an image that is not
> zero-initialized would still add a zero-initialized area, like when
> using nonpreallocating truncation on a preallocated image.  For callers
> that care only about truncation, not about creation with potential
> preallocation, this new function is useful.
> 
> Alternatively, we could have added a PreallocMode parameter to
> bdrv_has_zero_init().  But the only user would have been qemu-img
> convert, which does not have a plain PreallocMode value right now -- it
> would have to parse the creation option to obtain it.  Therefore, the
> simpler solution is to let bdrv_has_zero_init() inquire the
> preallocation status and add the new bdrv_has_zero_init_truncate() that
> presupposes PREALLOC_MODE_OFF.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  7 +++++++
>  block.c                   | 21 +++++++++++++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 50a07c1c33..5321d8afdf 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -438,6 +438,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>  int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>  int bdrv_has_zero_init_1(BlockDriverState *bs);
>  int bdrv_has_zero_init(BlockDriverState *bs);
> +int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
>  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>  int bdrv_block_status(BlockDriverState *bs, int64_t offset,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 6a0b1b5008..d7fc6b296b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -420,9 +420,16 @@ struct BlockDriver {
>      /*
>       * Returns 1 if newly created images are guaranteed to contain only
>       * zeros, 0 otherwise.
> +     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
>       */

Does it make sense to make sure of that in the bdrv_has_zero_init()?

I mean something like this:

int bdrv_has_zero_init(BlockDriverState *bs)
{
    ...
    if (bs->drv->bdrv_has_zero_init && bs->drv->bdrv_has_zero_init_truncate) {
        return bs->drv->bdrv_has_zero_init_truncate(bs) &&
               bs->drv->bdrv_has_zero_init(bs);
    } else if (bs->drv->bdrv_has_zero_init)
        return bs->drv->bdrv_has_zero_init(bs);
    }
    ...
}

Thanks,
Stefano

>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /*
> +     * Returns 1 if new areas added by growing the image with
> +     * PREALLOC_MODE_OFF contain only zeros, 0 otherwise.
> +     */
> +    int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs);
> +
>      /* Remove fd handlers, timers, and other event loop callbacks so the event
>       * loop is no longer in use.  Called with no in-flight requests and in
>       * depth-first traversal order with parents before child nodes.
> diff --git a/block.c b/block.c
> index cbd8da5f3b..81ae44dcf3 100644
> --- a/block.c
> +++ b/block.c
> @@ -5066,6 +5066,27 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>      return 0;
>  }
>  
> +int bdrv_has_zero_init_truncate(BlockDriverState *bs)
> +{
> +    if (!bs->drv) {
> +        return 0;
> +    }
> +
> +    if (bs->backing) {
> +        /* Depends on the backing image length, but better safe than sorry */
> +        return 0;
> +    }
> +    if (bs->drv->bdrv_has_zero_init_truncate) {
> +        return bs->drv->bdrv_has_zero_init_truncate(bs);
> +    }
> +    if (bs->file && bs->drv->is_filter) {
> +        return bdrv_has_zero_init_truncate(bs->file->bs);
> +    }
> +
> +    /* safe default */
> +    return 0;
> +}
> +
>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
>  {
>      BlockDriverInfo bdi;
> -- 
> 2.21.0
> 

-- 

Re: [Qemu-devel] [PATCH v2 01/11] qemu-img: Fix bdrv_has_zero_init() use in convert

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 07:12:29PM +0200, Max Reitz wrote:
> bdrv_has_zero_init() only has meaning for newly created images or image
> areas.  If qemu-img convert did not create the image itself, it cannot
> rely on bdrv_has_zero_init()'s result to carry any meaning.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..0f4be80c10 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
>      bool has_zero_init;
>      bool compressed;
>      bool unallocated_blocks_are_zero;
> +    bool target_is_new;
>      bool target_has_backing;
>      int64_t target_backing_sectors; /* negative if unknown */
>      bool wr_in_order;
> @@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
>      int64_t sector_num = 0;
>  
>      /* Check whether we have zero initialisation or can get it efficiently */
> -    s->has_zero_init = s->min_sparse && !s->target_has_backing
> -                     ? bdrv_has_zero_init(blk_bs(s->target))
> -                     : false;
> +    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +        s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> +    } else {
> +        s->has_zero_init = false;
> +    }
>  
>      if (!s->has_zero_init && !s->target_has_backing &&
>          bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
> @@ -2423,6 +2426,8 @@ static int img_convert(int argc, char **argv)
>          }
>      }
>  
> +    s.target_is_new = !skip_create;
> +
>      flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {

Make sense!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 04/11] block: Implement .bdrv_has_zero_init_truncate()

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 07:12:32PM +0200, Max Reitz wrote:
> We need to implement .bdrv_has_zero_init_truncate() for every block
> driver that supports truncation and has a .bdrv_has_zero_init()
> implementation.
> 
> Implement it the same way each driver implements .bdrv_has_zero_init().
> This is at least not any more unsafe than what we had before.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 1 +
>  block/file-win32.c | 1 +
>  block/gluster.c    | 4 ++++
>  block/nfs.c        | 1 +
>  block/qcow2.c      | 1 +
>  block/qed.c        | 1 +
>  block/raw-format.c | 6 ++++++
>  block/rbd.c        | 1 +
>  block/sheepdog.c   | 1 +
>  block/ssh.c        | 1 +
>  10 files changed, 18 insertions(+)
> 

LGTM.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 4479cc7ab4..0208006f3c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2924,6 +2924,7 @@ BlockDriver bdrv_file = {
>      .bdrv_co_create = raw_co_create,
>      .bdrv_co_create_opts = raw_co_create_opts,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status = raw_co_block_status,
>      .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>      .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
> diff --git a/block/file-win32.c b/block/file-win32.c
> index 6b2d67b239..41f55dfece 100644
> --- a/block/file-win32.c
> +++ b/block/file-win32.c
> @@ -635,6 +635,7 @@ BlockDriver bdrv_file = {
>      .bdrv_close         = raw_close,
>      .bdrv_co_create_opts = raw_co_create_opts,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>  
>      .bdrv_aio_preadv    = raw_aio_preadv,
>      .bdrv_aio_pwritev   = raw_aio_pwritev,
> diff --git a/block/gluster.c b/block/gluster.c
> index f64dc5b01e..64028b2cba 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -1567,6 +1567,7 @@ static BlockDriver bdrv_gluster = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1598,6 +1599,7 @@ static BlockDriver bdrv_gluster_tcp = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1629,6 +1631,7 @@ static BlockDriver bdrv_gluster_unix = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> @@ -1666,6 +1669,7 @@ static BlockDriver bdrv_gluster_rdma = {
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_pdiscard             = qemu_gluster_co_pdiscard,
>  #endif
> diff --git a/block/nfs.c b/block/nfs.c
> index d93241b3bb..97c815085f 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -863,6 +863,7 @@ static BlockDriver bdrv_nfs = {
>      .create_opts                    = &nfs_create_opts,
>  
>      .bdrv_has_zero_init             = nfs_has_zero_init,
> +    .bdrv_has_zero_init_truncate    = nfs_has_zero_init,
>      .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
>      .bdrv_co_truncate               = nfs_file_co_truncate,
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 039bdc2f7e..5c40f54d64 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5187,6 +5187,7 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_co_create_opts  = qcow2_co_create_opts,
>      .bdrv_co_create       = qcow2_co_create,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status = qcow2_co_block_status,
>  
>      .bdrv_co_preadv         = qcow2_co_preadv,
> diff --git a/block/qed.c b/block/qed.c
> index 77c7cef175..daaedb6864 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1668,6 +1668,7 @@ static BlockDriver bdrv_qed = {
>      .bdrv_co_create           = bdrv_qed_co_create,
>      .bdrv_co_create_opts      = bdrv_qed_co_create_opts,
>      .bdrv_has_zero_init       = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_co_block_status     = bdrv_qed_co_block_status,
>      .bdrv_co_readv            = bdrv_qed_co_readv,
>      .bdrv_co_writev           = bdrv_qed_co_writev,
> diff --git a/block/raw-format.c b/block/raw-format.c
> index bffd424dd0..42c28cc29a 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -413,6 +413,11 @@ static int raw_has_zero_init(BlockDriverState *bs)
>      return bdrv_has_zero_init(bs->file->bs);
>  }
>  
> +static int raw_has_zero_init_truncate(BlockDriverState *bs)
> +{
> +    return bdrv_has_zero_init_truncate(bs->file->bs);
> +}
> +
>  static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
>                                             Error **errp)
>  {
> @@ -572,6 +577,7 @@ BlockDriver bdrv_raw = {
>      .bdrv_co_ioctl        = &raw_co_ioctl,
>      .create_opts          = &raw_create_opts,
>      .bdrv_has_zero_init   = &raw_has_zero_init,
> +    .bdrv_has_zero_init_truncate = &raw_has_zero_init_truncate,
>      .strong_runtime_opts  = raw_strong_runtime_opts,
>      .mutable_opts         = mutable_opts,
>  };
> diff --git a/block/rbd.c b/block/rbd.c
> index 59757b3120..057af43d48 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1288,6 +1288,7 @@ static BlockDriver bdrv_rbd = {
>      .bdrv_co_create         = qemu_rbd_co_create,
>      .bdrv_co_create_opts    = qemu_rbd_co_create_opts,
>      .bdrv_has_zero_init     = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
>      .bdrv_get_info          = qemu_rbd_getinfo,
>      .create_opts            = &qemu_rbd_create_opts,
>      .bdrv_getlength         = qemu_rbd_getlength,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 6f402e5d4d..a4e111f981 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -3228,6 +3228,7 @@ static BlockDriver bdrv_sheepdog = {
>      .bdrv_co_create               = sd_co_create,
>      .bdrv_co_create_opts          = sd_co_create_opts,
>      .bdrv_has_zero_init           = bdrv_has_zero_init_1,
> +    .bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>      .bdrv_getlength               = sd_getlength,
>      .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>      .bdrv_co_truncate             = sd_co_truncate,
> diff --git a/block/ssh.c b/block/ssh.c
> index 501933b855..84d01e892b 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -1390,6 +1390,7 @@ static BlockDriver bdrv_ssh = {
>      .bdrv_co_create_opts          = ssh_co_create_opts,
>      .bdrv_close                   = ssh_close,
>      .bdrv_has_zero_init           = ssh_has_zero_init,
> +    .bdrv_has_zero_init_truncate  = ssh_has_zero_init,
>      .bdrv_co_readv                = ssh_co_readv,
>      .bdrv_co_writev               = ssh_co_writev,
>      .bdrv_getlength               = ssh_getlength,
> -- 
> 2.21.0
> 

-- 

Re: [Qemu-devel] [PATCH v2 05/11] block: Use bdrv_has_zero_init_truncate()

[Stefano Garzarella]

On Wed, Jul 24, 2019 at 07:12:33PM +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/parallels.c | 2 +-
>  block/vhdx.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 00fae125d1..7cd2714b69 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -835,7 +835,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail_options;
>      }
>  
> -    if (!bdrv_has_zero_init(bs->file->bs)) {
> +    if (!bdrv_has_zero_init_truncate(bs->file->bs)) {
>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>      }
>  
> diff --git a/block/vhdx.c b/block/vhdx.c
> index d6070b6fa8..a02d1c99a7 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1282,7 +1282,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
>                  /* Queue another write of zero buffers if the underlying file
>                   * does not zero-fill on file extension */
>  
> -                if (bdrv_has_zero_init(bs->file->bs) == 0) {
> +                if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) {
>                      use_zero_buffers = true;
>  
>                      /* zero fill the front, if any */
> -- 
> 2.21.0
> 

What about describing in the commit message why we are using
bdrv_has_zero_init_truncate() like in the cover letter?

With or without:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 03/11] block: Add bdrv_has_zero_init_truncate()

[Max Reitz]

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

On 26.07.19 11:04, Stefano Garzarella wrote:
> On Wed, Jul 24, 2019 at 07:12:31PM +0200, Max Reitz wrote:
>> No .bdrv_has_zero_init() implementation returns 1 if growing the file
>> would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
>> in lieu of this new function was always safe.
>>
>> But on the other hand, it is possible that growing an image that is not
>> zero-initialized would still add a zero-initialized area, like when
>> using nonpreallocating truncation on a preallocated image.  For callers
>> that care only about truncation, not about creation with potential
>> preallocation, this new function is useful.
>>
>> Alternatively, we could have added a PreallocMode parameter to
>> bdrv_has_zero_init().  But the only user would have been qemu-img
>> convert, which does not have a plain PreallocMode value right now -- it
>> would have to parse the creation option to obtain it.  Therefore, the
>> simpler solution is to let bdrv_has_zero_init() inquire the
>> preallocation status and add the new bdrv_has_zero_init_truncate() that
>> presupposes PREALLOC_MODE_OFF.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/block/block.h     |  1 +
>>  include/block/block_int.h |  7 +++++++
>>  block.c                   | 21 +++++++++++++++++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 50a07c1c33..5321d8afdf 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -438,6 +438,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>  int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>  int bdrv_has_zero_init_1(BlockDriverState *bs);
>>  int bdrv_has_zero_init(BlockDriverState *bs);
>> +int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
>>  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>>  int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 6a0b1b5008..d7fc6b296b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -420,9 +420,16 @@ struct BlockDriver {
>>      /*
>>       * Returns 1 if newly created images are guaranteed to contain only
>>       * zeros, 0 otherwise.
>> +     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
>>       */
> 
> Does it make sense to make sure of that in the bdrv_has_zero_init()?
> 
> I mean something like this:
> 
> int bdrv_has_zero_init(BlockDriverState *bs)
> {
>     ...
>     if (bs->drv->bdrv_has_zero_init && bs->drv->bdrv_has_zero_init_truncate) {
>         return bs->drv->bdrv_has_zero_init_truncate(bs) &&
>                bs->drv->bdrv_has_zero_init(bs);
>     } else if (bs->drv->bdrv_has_zero_init)
>         return bs->drv->bdrv_has_zero_init(bs);
>     }
>     ...
> }

I thought about it, but I didn’t like it because that would mean that
bdrv_has_zero_init() kind of differs from .bdrv_has_zero_init().

Max

> Thanks,
> Stefano
> 
>>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>>  
>> +    /*
>> +     * Returns 1 if new areas added by growing the image with
>> +     * PREALLOC_MODE_OFF contain only zeros, 0 otherwise.
>> +     */
>> +    int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs);
>> +
>>      /* Remove fd handlers, timers, and other event loop callbacks so the event
>>       * loop is no longer in use.  Called with no in-flight requests and in
>>       * depth-first traversal order with parents before child nodes.
>> diff --git a/block.c b/block.c
>> index cbd8da5f3b..81ae44dcf3 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5066,6 +5066,27 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>>      return 0;
>>  }
>>  
>> +int bdrv_has_zero_init_truncate(BlockDriverState *bs)
>> +{
>> +    if (!bs->drv) {
>> +        return 0;
>> +    }
>> +
>> +    if (bs->backing) {
>> +        /* Depends on the backing image length, but better safe than sorry */
>> +        return 0;
>> +    }
>> +    if (bs->drv->bdrv_has_zero_init_truncate) {
>> +        return bs->drv->bdrv_has_zero_init_truncate(bs);
>> +    }
>> +    if (bs->file && bs->drv->is_filter) {
>> +        return bdrv_has_zero_init_truncate(bs->file->bs);
>> +    }
>> +
>> +    /* safe default */
>> +    return 0;
>> +}
>> +
>>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
>>  {
>>      BlockDriverInfo bdi;
>> -- 
>> 2.21.0
>>
> 



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

Re: [Qemu-devel] [PATCH v2 05/11] block: Use bdrv_has_zero_init_truncate()

[Max Reitz]

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

On 26.07.19 11:43, Stefano Garzarella wrote:
> On Wed, Jul 24, 2019 at 07:12:33PM +0200, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/parallels.c | 2 +-
>>  block/vhdx.c      | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 00fae125d1..7cd2714b69 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -835,7 +835,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>          goto fail_options;
>>      }
>>  
>> -    if (!bdrv_has_zero_init(bs->file->bs)) {
>> +    if (!bdrv_has_zero_init_truncate(bs->file->bs)) {
>>          s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>>      }
>>  
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index d6070b6fa8..a02d1c99a7 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -1282,7 +1282,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t sector_num,
>>                  /* Queue another write of zero buffers if the underlying file
>>                   * does not zero-fill on file extension */
>>  
>> -                if (bdrv_has_zero_init(bs->file->bs) == 0) {
>> +                if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) {
>>                      use_zero_buffers = true;
>>  
>>                      /* zero fill the front, if any */
>> -- 
>> 2.21.0
>>
> 
> What about describing in the commit message why we are using
> bdrv_has_zero_init_truncate() like in the cover letter?

Sure.  (Not sure why I didn’t do it in the first place.)

> With or without:
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks for reviewing!

Max


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

Re: [Qemu-devel] [PATCH v2 03/11] block: Add bdrv_has_zero_init_truncate()

[Stefano Garzarella]

On Fri, Jul 26, 2019 at 12:58:58PM +0200, Max Reitz wrote:
> On 26.07.19 11:04, Stefano Garzarella wrote:
> > On Wed, Jul 24, 2019 at 07:12:31PM +0200, Max Reitz wrote:
> >> No .bdrv_has_zero_init() implementation returns 1 if growing the file
> >> would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
> >> in lieu of this new function was always safe.
> >>
> >> But on the other hand, it is possible that growing an image that is not
> >> zero-initialized would still add a zero-initialized area, like when
> >> using nonpreallocating truncation on a preallocated image.  For callers
> >> that care only about truncation, not about creation with potential
> >> preallocation, this new function is useful.
> >>
> >> Alternatively, we could have added a PreallocMode parameter to
> >> bdrv_has_zero_init().  But the only user would have been qemu-img
> >> convert, which does not have a plain PreallocMode value right now -- it
> >> would have to parse the creation option to obtain it.  Therefore, the
> >> simpler solution is to let bdrv_has_zero_init() inquire the
> >> preallocation status and add the new bdrv_has_zero_init_truncate() that
> >> presupposes PREALLOC_MODE_OFF.
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  include/block/block.h     |  1 +
> >>  include/block/block_int.h |  7 +++++++
> >>  block.c                   | 21 +++++++++++++++++++++
> >>  3 files changed, 29 insertions(+)
> >>
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index 50a07c1c33..5321d8afdf 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -438,6 +438,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
> >>  int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
> >>  int bdrv_has_zero_init_1(BlockDriverState *bs);
> >>  int bdrv_has_zero_init(BlockDriverState *bs);
> >> +int bdrv_has_zero_init_truncate(BlockDriverState *bs);
> >>  bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
> >>  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
> >>  int bdrv_block_status(BlockDriverState *bs, int64_t offset,
> >> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> index 6a0b1b5008..d7fc6b296b 100644
> >> --- a/include/block/block_int.h
> >> +++ b/include/block/block_int.h
> >> @@ -420,9 +420,16 @@ struct BlockDriver {
> >>      /*
> >>       * Returns 1 if newly created images are guaranteed to contain only
> >>       * zeros, 0 otherwise.
> >> +     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
> >>       */
> > 
> > Does it make sense to make sure of that in the bdrv_has_zero_init()?
> > 
> > I mean something like this:
> > 
> > int bdrv_has_zero_init(BlockDriverState *bs)
> > {
> >     ...
> >     if (bs->drv->bdrv_has_zero_init && bs->drv->bdrv_has_zero_init_truncate) {
> >         return bs->drv->bdrv_has_zero_init_truncate(bs) &&
> >                bs->drv->bdrv_has_zero_init(bs);
> >     } else if (bs->drv->bdrv_has_zero_init)
> >         return bs->drv->bdrv_has_zero_init(bs);
> >     }
> >     ...
> > }
> 
> I thought about it, but I didn’t like it because that would mean that
> bdrv_has_zero_init() kind of differs from .bdrv_has_zero_init().

Ah right! And eventually a bug in .bdrv_has_zero_init() would be masked.

So,
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 09/11] iotests: Convert to preallocated encrypted qcow2

[Max Reitz]

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

On 25.07.19 18:27, Max Reitz wrote:
> On 25.07.19 17:30, Maxim Levitsky wrote:
>> On Wed, 2019-07-24 at 19:12 +0200, Max Reitz wrote:
>>> Add a test case for converting an empty image (which only returns zeroes
>>> when read) to a preallocated encrypted qcow2 image.
>>> qcow2_has_zero_init() should return 0 then, thus forcing qemu-img
>>> convert to create zero clusters.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Tested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>>  tests/qemu-iotests/188     | 20 +++++++++++++++++++-
>>>  tests/qemu-iotests/188.out |  4 ++++
>>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188
>>> index be7278aa65..afca44df54 100755
>>> --- a/tests/qemu-iotests/188
>>> +++ b/tests/qemu-iotests/188
>>> @@ -48,7 +48,7 @@ SECRETALT="secret,id=sec0,data=platypus"
>>>  
>>>  _make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
>>>  
>>> -IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
>>> +IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
>> This change I think doesn't change anything

Just noticed now: Yes, it does; it puts the TEST_IMG at end so we can
append to it...

[...]

>>> +
>>> +$QEMU_IMG convert -O "$IMGFMT" --object $SECRET \
>>> +    -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,preallocation=metadata" \
>>> +    "${TEST_IMG}.orig" "$TEST_IMG"
>>> +
>>> +$QEMU_IMG compare --object $SECRET --image-opts "${IMGSPEC}.orig" "$IMGSPEC"

...right here.

Max


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

Re: [Qemu-devel] [PATCH v2 00/11] block: Fix some things about bdrv_has_zero_init()

[Max Reitz]

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

On 24.07.19 19:12, Max Reitz wrote:
> Hi,
> 
> See the previous cover letter for the reason for patches 6 through 9:
> https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg00563.html
> 
> But no only some bdrv_has_zero_init() implementations are wrong, some
> callers also use it the wrong way.
> 
> First, qemu-img and mirror use it for pre-existing images, where it
> doesn’t have any meaning.  Both should consider pre-existing images to
> always be non-zero and not look at bdrv_has-zero_init() (patches 1, 2,
> and the tests in 10 and 11).
> 
> Second, vhdx and parallels call bdrv_has_zero_init() when they do not
> really care about an image’s post-create state but only about what
> happens when you grow an image.  That is a bit ugly, and also overly
> safe when growing preallocated images without preallocating the new
> areas.  So this series adds a new function bdrv_has_zero_init_truncate()
> that is more suited to vhdx's and parallel's needs (patches 3 through
> 5).

Thanks for the reviews, I took a part of this last paragraph, added it
as patch 5’s commit message, and applied the series to my block-next branch.

Max


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