[Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors

[Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

v7: Fix the lost assignment of s->unmap.

v6: Fix pnum in bdrv_get_block_status_above. [Paolo]

v5: Rewrite patch 1.
    Address Eric's comments on patch 3.
    Add Eric's rev-by to patches 2 & 4.
    Check BDRV_BLOCK_DATA in patch 3. [Paolo]

This fixes the mirror assert failure reported by wangxiaolong:

https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html

The direct cause is that hbitmap code couldn't handle unset of bits *after*
iterator's current position. We could fix that, but the bdrv_reset_dirty() call
is more questionable:

Before, if guest discarded some sectors during migration, it could see
different data after moving to dest side, depending on block backends of the
src and the dest. This is IMO worse than mirroring the actual reading as done
in this series, because we don't know what the guest is doing.

For example if a guest first issues WRITE SAME to wipe out the area then issues
UNMAP to discard it, just to get rid of some sensitive data completely, we may
miss both operations and leave stale data on dest image.


Fam Zheng (8):
  block: Add bdrv_get_block_status_above
  qmp: Add optional bool "unmap" to drive-mirror
  mirror: Do zero write on target if sectors not allocated
  block: Fix dirty bitmap in bdrv_co_discard
  block: Remove bdrv_reset_dirty
  qemu-iotests: Make block job methods common
  qemu-iotests: Add test case for mirror with unmap
  iotests: Use event_wait in wait_ready

 block.c                       | 12 --------
 block/io.c                    | 60 ++++++++++++++++++++++++++++++---------
 block/mirror.c                | 28 +++++++++++++++---
 blockdev.c                    |  5 ++++
 hmp.c                         |  2 +-
 include/block/block.h         |  4 +++
 include/block/block_int.h     |  4 +--
 qapi/block-core.json          |  8 +++++-
 qmp-commands.hx               |  3 ++
 tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
 tests/qemu-iotests/132        | 59 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/132.out    |  5 ++++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py | 23 +++++++++++++++
 14 files changed, 196 insertions(+), 84 deletions(-)
 create mode 100644 tests/qemu-iotests/132
 create mode 100644 tests/qemu-iotests/132.out

-- 
2.4.2

[Qemu-devel] [PATCH v7 1/8] block: Add bdrv_get_block_status_above

[Fam Zheng]

Like bdrv_is_allocated_above, this function follows the backing chain until seeing
BDRV_BLOCK_ALLOCATED.  Base is not included.

Reimplement bdrv_is_allocated on top.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c            | 56 +++++++++++++++++++++++++++++++++++++++++----------
 include/block/block.h |  4 ++++
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/block/io.c b/block/io.c
index e394d92..41463a6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1560,28 +1560,54 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     return ret;
 }
 
-/* Coroutine wrapper for bdrv_get_block_status() */
-static void coroutine_fn bdrv_get_block_status_co_entry(void *opaque)
+static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
+        BlockDriverState *base,
+        int64_t sector_num,
+        int nb_sectors,
+        int *pnum)
+{
+    BlockDriverState *p;
+    int64_t ret;
+
+    assert(bs != base);
+    for (p = bs; p != base; p = p->backing_hd) {
+        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
+        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
+            break;
+        }
+        /* [sector_num, pnum] unallocated on this layer, which could be only
+         * the first part of [sector_num, nb_sectors].  */
+        nb_sectors = MIN(nb_sectors, *pnum);
+    }
+    return ret;
+}
+
+/* Coroutine wrapper for bdrv_get_block_status_above() */
+static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
 {
     BdrvCoGetBlockStatusData *data = opaque;
-    BlockDriverState *bs = data->bs;
 
-    data->ret = bdrv_co_get_block_status(bs, data->sector_num, data->nb_sectors,
-                                         data->pnum);
+    data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
+                                               data->sector_num,
+                                               data->nb_sectors,
+                                               data->pnum);
     data->done = true;
 }
 
 /*
- * Synchronous wrapper around bdrv_co_get_block_status().
+ * Synchronous wrapper around bdrv_co_get_block_status_above().
  *
- * See bdrv_co_get_block_status() for details.
+ * See bdrv_co_get_block_status_above() for details.
  */
-int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
-                              int nb_sectors, int *pnum)
+int64_t bdrv_get_block_status_above(BlockDriverState *bs,
+                                    BlockDriverState *base,
+                                    int64_t sector_num,
+                                    int nb_sectors, int *pnum)
 {
     Coroutine *co;
     BdrvCoGetBlockStatusData data = {
         .bs = bs,
+        .base = base,
         .sector_num = sector_num,
         .nb_sectors = nb_sectors,
         .pnum = pnum,
@@ -1590,11 +1616,11 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
 
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
-        bdrv_get_block_status_co_entry(&data);
+        bdrv_get_block_status_above_co_entry(&data);
     } else {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
-        co = qemu_coroutine_create(bdrv_get_block_status_co_entry);
+        co = qemu_coroutine_create(bdrv_get_block_status_above_co_entry);
         qemu_coroutine_enter(co, &data);
         while (!data.done) {
             aio_poll(aio_context, true);
@@ -1603,6 +1629,14 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
     return data.ret;
 }
 
+int64_t bdrv_get_block_status(BlockDriverState *bs,
+                              int64_t sector_num,
+                              int nb_sectors, int *pnum)
+{
+    return bdrv_get_block_status_above(bs, bs->backing_hd,
+                                       sector_num, nb_sectors, pnum);
+}
+
 int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                                    int nb_sectors, int *pnum)
 {
diff --git a/include/block/block.h b/include/block/block.h
index f7680b6..4d9b555 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -361,6 +361,10 @@ bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
                               int nb_sectors, int *pnum);
+int64_t bdrv_get_block_status_above(BlockDriverState *bs,
+                                    BlockDriverState *base,
+                                    int64_t sector_num,
+                                    int nb_sectors, int *pnum);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                       int *pnum);
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
-- 
2.4.2

[Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Fam Zheng]

If specified as "true", it allows discarding on target sectors where source is
not allocated.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c            | 8 ++++++--
 blockdev.c                | 5 +++++
 hmp.c                     | 2 +-
 include/block/block_int.h | 2 ++
 qapi/block-core.json      | 8 +++++++-
 qmp-commands.hx           | 3 +++
 6 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..d2515c7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -57,6 +57,7 @@ typedef struct MirrorBlockJob {
     int in_flight;
     int sectors_in_flight;
     int ret;
+    bool unmap;
 } MirrorBlockJob;
 
 typedef struct MirrorOp {
@@ -651,6 +652,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
                              int64_t buf_size,
                              BlockdevOnError on_source_error,
                              BlockdevOnError on_target_error,
+                             bool unmap,
                              BlockCompletionFunc *cb,
                              void *opaque, Error **errp,
                              const BlockJobDriver *driver,
@@ -685,6 +687,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->base = base;
     s->granularity = granularity;
     s->buf_size = MAX(buf_size, granularity);
+    s->unmap = unmap;
 
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
@@ -703,6 +706,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
+                  bool unmap,
                   BlockCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
@@ -717,7 +721,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
     mirror_start_job(bs, target, replaces,
                      speed, granularity, buf_size,
-                     on_source_error, on_target_error, cb, opaque, errp,
+                     on_source_error, on_target_error, unmap, cb, opaque, errp,
                      &mirror_job_driver, is_none_mode, base);
 }
 
@@ -765,7 +769,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
 
     bdrv_ref(base);
     mirror_start_job(bs, base, NULL, speed, 0, 0,
-                     on_error, on_error, cb, opaque, &local_err,
+                     on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/blockdev.c b/blockdev.c
index de94a8b..6a45555 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2631,6 +2631,7 @@ void qmp_drive_mirror(const char *device, const char *target,
                       bool has_buf_size, int64_t buf_size,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
+                      bool has_unmap, bool unmap,
                       Error **errp)
 {
     BlockBackend *blk;
@@ -2662,6 +2663,9 @@ void qmp_drive_mirror(const char *device, const char *target,
     if (!has_buf_size) {
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
+    if (!has_unmap) {
+        unmap = true;
+    }
 
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -2801,6 +2805,7 @@ void qmp_drive_mirror(const char *device, const char *target,
                  has_replaces ? replaces : NULL,
                  speed, granularity, buf_size, sync,
                  on_source_error, on_target_error,
+                 unmap,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
diff --git a/hmp.c b/hmp.c
index 514f22f..d5b9ebb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1057,7 +1057,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
                      false, NULL, false, NULL,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, 0, false, 0,
-                     false, 0, false, 0, &err);
+                     false, 0, false, 0, false, true, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f004378..4e0d700 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -590,6 +590,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
  * @mode: Whether to collapse all images in the chain to the target.
  * @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.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: Error object.
@@ -604,6 +605,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
+                  bool unmap,
                   BlockCompletionFunc *cb,
                   void *opaque, Error **errp);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8411d4f..71ed838 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -973,6 +973,11 @@
 # @on-target-error: #optional the action to take on an error on the target,
 #                   default 'report' (no limitations, since this applies to
 #                   a different block device than @device).
+# @unmap: #optional Whether to try to unmap target sectors where source has
+#         only zero. If true, and target unallocated sectors will read as zero,
+#         target image sectors will be unmapped; otherwise, zeroes will be
+#         written. Both will result in identical contents.
+#         Default is true. (Since 2.4)
 #
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
@@ -985,7 +990,8 @@
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
-            '*on-target-error': 'BlockdevOnError' } }
+            '*on-target-error': 'BlockdevOnError',
+            '*unmap': 'bool' } }
 
 ##
 # @BlockDirtyBitmap
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 867a21f..3b33496 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1503,6 +1503,7 @@ EQMP
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "node-name:s?,replaces:s?,"
                       "on-source-error:s?,on-target-error:s?,"
+                      "unmap:b?,"
                       "granularity:i?,buf-size:i?",
         .mhandler.cmd_new = qmp_marshal_input_drive_mirror,
     },
@@ -1542,6 +1543,8 @@ Arguments:
   (BlockdevOnError, default 'report')
 - "on-target-error": the action to take on an error on the target
   (BlockdevOnError, default 'report')
+- "unmap": whether the target sectors should be discarded where source has only
+  zeroes. (json-bool, optional, default true)
 
 The default value of the granularity is the image cluster size clamped
 between 4096 and 65536, if the image format defines one.  If the format
-- 
2.4.2

[Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated

[Fam Zheng]

If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
Some protocols do zero upon discard, where it's best to use
bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index d2515c7..3c38695 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
     uint64_t delay_ns = 0;
     MirrorOp *op;
+    int pnum;
+    int64_t ret;
 
     s->sector_num = hbitmap_iter_next(&s->hbi);
     if (s->sector_num < 0) {
@@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     s->in_flight++;
     s->sectors_in_flight += nb_sectors;
     trace_mirror_one_iteration(s, sector_num, nb_sectors);
-    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
-                   mirror_read_complete, op);
+
+    ret = bdrv_get_block_status_above(source, NULL, sector_num,
+                                      nb_sectors, &pnum);
+    if (ret < 0 || pnum < nb_sectors ||
+            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
+        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
+                       mirror_read_complete, op);
+    } else if (ret & BDRV_BLOCK_ZERO) {
+        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
+                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
+                              mirror_write_complete, op);
+    } else {
+        assert(!(ret & BDRV_BLOCK_DATA));
+        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
+                         mirror_write_complete, op);
+    }
     return delay_ns;
 }
 
-- 
2.4.2

[Qemu-devel] [PATCH v7 4/8] block: Fix dirty bitmap in bdrv_co_discard

[Fam Zheng]

Unsetting dirty globally with discard is not very correct. The discard may zero
out sectors (depending on can_write_zeroes_with_unmap), we should replicate
this change to destination side to make sure that the guest sees the same data.

Calling bdrv_reset_dirty also troubles mirror job because the hbitmap iterator
doesn't expect unsetting of bits after current position.

So let's do it the opposite way which fixes both problems: set the dirty bits
if we are to discard it.

Reported-by: wangxiaolong@ucloud.cn
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 41463a6..a71346c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2440,8 +2440,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return -EPERM;
     }
 
-    bdrv_reset_dirty(bs, sector_num, nb_sectors);
-
     /* Do nothing if disabled.  */
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
         return 0;
@@ -2451,6 +2449,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return 0;
     }
 
+    bdrv_set_dirty(bs, sector_num, nb_sectors);
+
     max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
     while (nb_sectors > 0) {
         int ret;
-- 
2.4.2

[Qemu-devel] [PATCH v7 5/8] block: Remove bdrv_reset_dirty

[Fam Zheng]

Using this function would always be wrong because a dirty bitmap must
have a specific owner that consumes the dirty bits and calls
bdrv_reset_dirty_bitmap().

Remove the unused function to avoid future misuse.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block.c                   | 12 ------------
 include/block/block_int.h |  2 --
 2 files changed, 14 deletions(-)

diff --git a/block.c b/block.c
index 2b9ceae..9890707 100644
--- a/block.c
+++ b/block.c
@@ -3347,18 +3347,6 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
     }
 }
 
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
-                      int nr_sectors)
-{
-    BdrvDirtyBitmap *bitmap;
-    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
-        if (!bdrv_dirty_bitmap_enabled(bitmap)) {
-            continue;
-        }
-        hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
-    }
-}
-
 /**
  * Advance an HBitmapIter to an arbitrary offset.
  */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4e0d700..459fe1c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -640,7 +640,5 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);
 void blk_dev_resize_cb(BlockBackend *blk);
 
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
-                      int nr_sectors);
 
 #endif /* BLOCK_INT_H */
-- 
2.4.2

[Qemu-devel] [PATCH v7 6/8] qemu-iotests: Make block job methods common

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
 tests/qemu-iotests/iotests.py | 28 ++++++++++++++++++
 2 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59a8f73..3d46ed7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -34,38 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
 quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
 quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
 
-class ImageMirroringTestCase(iotests.QMPTestCase):
-    '''Abstract base class for image mirroring test cases'''
 
-    def wait_ready(self, drive='drive0'):
-        '''Wait until a block job BLOCK_JOB_READY event'''
-        ready = False
-        while not ready:
-            for event in self.vm.get_qmp_events(wait=True):
-                if event['event'] == 'BLOCK_JOB_READY':
-                    self.assert_qmp(event, 'data/type', 'mirror')
-                    self.assert_qmp(event, 'data/device', drive)
-                    ready = True
-
-    def wait_ready_and_cancel(self, drive='drive0'):
-        self.wait_ready(drive=drive)
-        event = self.cancel_and_wait(drive=drive)
-        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
-        self.assert_qmp(event, 'data/type', 'mirror')
-        self.assert_qmp(event, 'data/offset', event['data']['len'])
-
-    def complete_and_wait(self, drive='drive0', wait_ready=True):
-        '''Complete a block job and wait for it to finish'''
-        if wait_ready:
-            self.wait_ready(drive=drive)
-
-        result = self.vm.qmp('block-job-complete', device=drive)
-        self.assert_qmp(result, 'return', {})
-
-        event = self.wait_until_completed(drive=drive)
-        self.assert_qmp(event, 'data/type', 'mirror')
-
-class TestSingleDrive(ImageMirroringTestCase):
+class TestSingleDrive(iotests.QMPTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
     def setUp(self):
@@ -221,17 +191,9 @@ class TestSingleDriveUnalignedLength(TestSingleDrive):
     test_small_buffer2 = None
     test_large_cluster = None
 
-class TestMirrorNoBacking(ImageMirroringTestCase):
+class TestMirrorNoBacking(iotests.QMPTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
-    def complete_and_wait(self, drive='drive0', wait_ready=True):
-        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
-        return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
-
-    def compare_images(self, img1, img2):
-        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
-        return iotests.compare_images(img1, img2)
-
     def setUp(self):
         iotests.create_image(backing_img, TestMirrorNoBacking.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
@@ -242,7 +204,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
         self.vm.shutdown()
         os.remove(test_img)
         os.remove(backing_img)
-        os.remove(target_backing_img)
+        try:
+            os.remove(target_backing_img)
+        except:
+            pass
         os.remove(target_img)
 
     def test_complete(self):
@@ -257,7 +222,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', target_img)
         self.vm.shutdown()
-        self.assertTrue(self.compare_images(test_img, target_img),
+        self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
     def test_cancel(self):
@@ -272,7 +237,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', test_img)
         self.vm.shutdown()
-        self.assertTrue(self.compare_images(test_img, target_img),
+        self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
     def test_large_cluster(self):
@@ -283,7 +248,6 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
                         %(TestMirrorNoBacking.image_len), target_backing_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
                         % (TestMirrorNoBacking.image_len, target_backing_img), target_img)
-        os.remove(target_backing_img)
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              mode='existing', target=target_img)
@@ -293,10 +257,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', target_img)
         self.vm.shutdown()
-        self.assertTrue(self.compare_images(test_img, target_img),
+        self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
-class TestMirrorResized(ImageMirroringTestCase):
+class TestMirrorResized(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * 1024 * 1024 # MB
 
@@ -344,7 +308,7 @@ class TestMirrorResized(ImageMirroringTestCase):
         self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
-class TestReadErrors(ImageMirroringTestCase):
+class TestReadErrors(iotests.QMPTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     # this should be a multiple of twice the default granularity
@@ -498,7 +462,7 @@ new_state = "1"
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-class TestWriteErrors(ImageMirroringTestCase):
+class TestWriteErrors(iotests.QMPTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     # this should be a multiple of twice the default granularity
@@ -624,7 +588,7 @@ new_state = "1"
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-class TestSetSpeed(ImageMirroringTestCase):
+class TestSetSpeed(iotests.QMPTestCase):
     image_len = 80 * 1024 * 1024 # MB
 
     def setUp(self):
@@ -690,7 +654,7 @@ class TestSetSpeed(ImageMirroringTestCase):
 
         self.wait_ready_and_cancel()
 
-class TestUnbackedSource(ImageMirroringTestCase):
+class TestUnbackedSource(iotests.QMPTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     def setUp(self):
@@ -731,7 +695,7 @@ class TestUnbackedSource(ImageMirroringTestCase):
         self.complete_and_wait()
         self.assert_no_active_block_jobs()
 
-class TestRepairQuorum(ImageMirroringTestCase):
+class TestRepairQuorum(iotests.QMPTestCase):
     """ This class test quorum file repair using drive-mirror.
         It's mostly a fork of TestSingleDrive """
     image_len = 1 * 1024 * 1024 # MB
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 04a294d..63de726 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -326,6 +326,34 @@ class QMPTestCase(unittest.TestCase):
         self.assert_no_active_block_jobs()
         return event
 
+    def wait_ready(self, drive='drive0'):
+        '''Wait until a block job BLOCK_JOB_READY event'''
+        ready = False
+        while not ready:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_READY':
+                    self.assert_qmp(event, 'data/type', 'mirror')
+                    self.assert_qmp(event, 'data/device', drive)
+                    ready = True
+
+    def wait_ready_and_cancel(self, drive='drive0'):
+        self.wait_ready(drive=drive)
+        event = self.cancel_and_wait(drive=drive)
+        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
+        self.assert_qmp(event, 'data/type', 'mirror')
+        self.assert_qmp(event, 'data/offset', event['data']['len'])
+
+    def complete_and_wait(self, drive='drive0', wait_ready=True):
+        '''Complete a block job and wait for it to finish'''
+        if wait_ready:
+            self.wait_ready(drive=drive)
+
+        result = self.vm.qmp('block-job-complete', device=drive)
+        self.assert_qmp(result, 'return', {})
+
+        event = self.wait_until_completed(drive=drive)
+        self.assert_qmp(event, 'data/type', 'mirror')
+
 def notrun(reason):
     '''Skip this test suite'''
     # Each test in qemu-iotests has a number ("seq")
-- 
2.4.2

[Qemu-devel] [PATCH v7 7/8] qemu-iotests: Add test case for mirror with unmap

[Fam Zheng]

This checks that the discard on mirror source that effectively zeroes
data is also reflected by the data of target.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/132     | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/132.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 65 insertions(+)
 create mode 100644 tests/qemu-iotests/132
 create mode 100644 tests/qemu-iotests/132.out

diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
new file mode 100644
index 0000000..f53ef6e
--- /dev/null
+++ b/tests/qemu-iotests/132
@@ -0,0 +1,59 @@
+#!/usr/bin/env python
+#
+# Test mirror with unmap
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+target_img = os.path.join(iotests.test_dir, 'target.img')
+
+class TestSingleDrive(iotests.QMPTestCase):
+    image_len = 2 * 1024 * 1024 # MB
+
+    def setUp(self):
+        # Write data to the image so we can compare later
+        qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 0 2M', test_img)
+
+        self.vm = iotests.VM().add_drive(test_img, 'discard=unmap')
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def test_mirror_discard(self):
+        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
+                             target=target_img)
+        self.assert_qmp(result, 'return', {})
+        self.vm.hmp_qemu_io('drive0', 'discard 0 64k')
+        self.complete_and_wait('drive0')
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/132.out b/tests/qemu-iotests/132.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/132.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0b817ca..757ac1b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -129,4 +129,5 @@
 129 rw auto quick
 130 rw auto quick
 131 rw auto quick
+132 rw auto quick
 134 rw auto quick
-- 
2.4.2

[Qemu-devel] [PATCH v7 8/8] iotests: Use event_wait in wait_ready

[Fam Zheng]

Only poll the specific type of event we are interested in, to avoid
stealing events that should be consumed by someone else.

Suggested-by: John Snow <jsnow@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 63de726..8615b10 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -328,13 +328,8 @@ class QMPTestCase(unittest.TestCase):
 
     def wait_ready(self, drive='drive0'):
         '''Wait until a block job BLOCK_JOB_READY event'''
-        ready = False
-        while not ready:
-            for event in self.vm.get_qmp_events(wait=True):
-                if event['event'] == 'BLOCK_JOB_READY':
-                    self.assert_qmp(event, 'data/type', 'mirror')
-                    self.assert_qmp(event, 'data/device', drive)
-                    ready = True
+        f = {'data': {'type': 'mirror', 'device': drive } }
+        event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
 
     def wait_ready_and_cancel(self, drive='drive0'):
         self.wait_ready(drive=drive)
-- 
2.4.2

Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors

[Stefan Hajnoczi]

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

On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> v7: Fix the lost assignment of s->unmap.
> 
> v6: Fix pnum in bdrv_get_block_status_above. [Paolo]
> 
> v5: Rewrite patch 1.
>     Address Eric's comments on patch 3.
>     Add Eric's rev-by to patches 2 & 4.
>     Check BDRV_BLOCK_DATA in patch 3. [Paolo]
> 
> This fixes the mirror assert failure reported by wangxiaolong:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
> 
> The direct cause is that hbitmap code couldn't handle unset of bits *after*
> iterator's current position. We could fix that, but the bdrv_reset_dirty() call
> is more questionable:
> 
> Before, if guest discarded some sectors during migration, it could see
> different data after moving to dest side, depending on block backends of the
> src and the dest. This is IMO worse than mirroring the actual reading as done
> in this series, because we don't know what the guest is doing.
> 
> For example if a guest first issues WRITE SAME to wipe out the area then issues
> UNMAP to discard it, just to get rid of some sensitive data completely, we may
> miss both operations and leave stale data on dest image.
> 
> 
> Fam Zheng (8):
>   block: Add bdrv_get_block_status_above
>   qmp: Add optional bool "unmap" to drive-mirror
>   mirror: Do zero write on target if sectors not allocated
>   block: Fix dirty bitmap in bdrv_co_discard
>   block: Remove bdrv_reset_dirty
>   qemu-iotests: Make block job methods common
>   qemu-iotests: Add test case for mirror with unmap
>   iotests: Use event_wait in wait_ready
> 
>  block.c                       | 12 --------
>  block/io.c                    | 60 ++++++++++++++++++++++++++++++---------
>  block/mirror.c                | 28 +++++++++++++++---
>  blockdev.c                    |  5 ++++
>  hmp.c                         |  2 +-
>  include/block/block.h         |  4 +++
>  include/block/block_int.h     |  4 +--
>  qapi/block-core.json          |  8 +++++-
>  qmp-commands.hx               |  3 ++
>  tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
>  tests/qemu-iotests/132        | 59 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/132.out    |  5 ++++
>  tests/qemu-iotests/group      |  1 +
>  tests/qemu-iotests/iotests.py | 23 +++++++++++++++
>  14 files changed, 196 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qemu-iotests/132
>  create mode 100644 tests/qemu-iotests/132.out
> 
> -- 
> 2.4.2
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Eric Blake]

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

On 06/07/2015 11:56 PM, Fam Zheng wrote:
> If specified as "true", it allows discarding on target sectors where source is
> not allocated.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c            | 8 ++++++--
>  blockdev.c                | 5 +++++
>  hmp.c                     | 2 +-
>  include/block/block_int.h | 2 ++
>  qapi/block-core.json      | 8 +++++++-
>  qmp-commands.hx           | 3 +++
>  6 files changed, 24 insertions(+), 4 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -973,6 +973,11 @@
>  # @on-target-error: #optional the action to take on an error on the target,
>  #                   default 'report' (no limitations, since this applies to
>  #                   a different block device than @device).
> +# @unmap: #optional Whether to try to unmap target sectors where source has
> +#         only zero. If true, and target unallocated sectors will read as zero,
> +#         target image sectors will be unmapped; otherwise, zeroes will be
> +#         written. Both will result in identical contents.
> +#         Default is true. (Since 2.4)
>  #

Per the other thread on adding 'detect-zeroes', should we instead be
using an enum here that describes one of several behaviors without an
explosion into multiple knobs?

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


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

Re: [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Paolo Bonzini]

On 08/06/2015 16:51, Eric Blake wrote:
>> > +# @unmap: #optional Whether to try to unmap target sectors where source has
>> > +#         only zero. If true, and target unallocated sectors will read as zero,
>> > +#         target image sectors will be unmapped; otherwise, zeroes will be
>> > +#         written. Both will result in identical contents.
>> > +#         Default is true. (Since 2.4)
>> >  #
> Per the other thread on adding 'detect-zeroes', should we instead be
> using an enum here that describes one of several behaviors without an
> explosion into multiple knobs?

See my answer there. :)

Paolo

Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

On Mon, 06/08 14:02, Stefan Hajnoczi wrote:
> On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> > v7: Fix the lost assignment of s->unmap.
> > 
> > v6: Fix pnum in bdrv_get_block_status_above. [Paolo]
> > 
> > v5: Rewrite patch 1.
> >     Address Eric's comments on patch 3.
> >     Add Eric's rev-by to patches 2 & 4.
> >     Check BDRV_BLOCK_DATA in patch 3. [Paolo]
> > 
> > This fixes the mirror assert failure reported by wangxiaolong:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
> > 
> > The direct cause is that hbitmap code couldn't handle unset of bits *after*
> > iterator's current position. We could fix that, but the bdrv_reset_dirty() call
> > is more questionable:
> > 
> > Before, if guest discarded some sectors during migration, it could see
> > different data after moving to dest side, depending on block backends of the
> > src and the dest. This is IMO worse than mirroring the actual reading as done
> > in this series, because we don't know what the guest is doing.
> > 
> > For example if a guest first issues WRITE SAME to wipe out the area then issues
> > UNMAP to discard it, just to get rid of some sensitive data completely, we may
> > miss both operations and leave stale data on dest image.
> > 
> > 
> > Fam Zheng (8):
> >   block: Add bdrv_get_block_status_above
> >   qmp: Add optional bool "unmap" to drive-mirror
> >   mirror: Do zero write on target if sectors not allocated
> >   block: Fix dirty bitmap in bdrv_co_discard
> >   block: Remove bdrv_reset_dirty
> >   qemu-iotests: Make block job methods common
> >   qemu-iotests: Add test case for mirror with unmap
> >   iotests: Use event_wait in wait_ready
> > 
> >  block.c                       | 12 --------
> >  block/io.c                    | 60 ++++++++++++++++++++++++++++++---------
> >  block/mirror.c                | 28 +++++++++++++++---
> >  blockdev.c                    |  5 ++++
> >  hmp.c                         |  2 +-
> >  include/block/block.h         |  4 +++
> >  include/block/block_int.h     |  4 +--
> >  qapi/block-core.json          |  8 +++++-
> >  qmp-commands.hx               |  3 ++
> >  tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
> >  tests/qemu-iotests/132        | 59 ++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/132.out    |  5 ++++
> >  tests/qemu-iotests/group      |  1 +
> >  tests/qemu-iotests/iotests.py | 23 +++++++++++++++
> >  14 files changed, 196 insertions(+), 84 deletions(-)
> >  create mode 100644 tests/qemu-iotests/132
> >  create mode 100644 tests/qemu-iotests/132.out
> > 
> > -- 
> > 2.4.2
> > 
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Stefan,

The only controversial patches are the qmp/drive-mirror ones (1-3), while
patches 4-8 are still useful on their own: they fix the mentioned crash and
improve iotests.

Shall we merge the second half (of course none of them depend on 1-3) now that
softfreeze is approaching?

Fam

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

On Thu, 06/11 16:29, Fam Zheng wrote:
> On Mon, 06/08 14:02, Stefan Hajnoczi wrote:
> > On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> > > v7: Fix the lost assignment of s->unmap.
> > > 
> > > v6: Fix pnum in bdrv_get_block_status_above. [Paolo]
> > > 
> > > v5: Rewrite patch 1.
> > >     Address Eric's comments on patch 3.
> > >     Add Eric's rev-by to patches 2 & 4.
> > >     Check BDRV_BLOCK_DATA in patch 3. [Paolo]
> > > 
> > > This fixes the mirror assert failure reported by wangxiaolong:
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
> > > 
> > > The direct cause is that hbitmap code couldn't handle unset of bits *after*
> > > iterator's current position. We could fix that, but the bdrv_reset_dirty() call
> > > is more questionable:
> > > 
> > > Before, if guest discarded some sectors during migration, it could see
> > > different data after moving to dest side, depending on block backends of the
> > > src and the dest. This is IMO worse than mirroring the actual reading as done
> > > in this series, because we don't know what the guest is doing.
> > > 
> > > For example if a guest first issues WRITE SAME to wipe out the area then issues
> > > UNMAP to discard it, just to get rid of some sensitive data completely, we may
> > > miss both operations and leave stale data on dest image.
> > > 
> > > 
> > > Fam Zheng (8):
> > >   block: Add bdrv_get_block_status_above
> > >   qmp: Add optional bool "unmap" to drive-mirror
> > >   mirror: Do zero write on target if sectors not allocated
> > >   block: Fix dirty bitmap in bdrv_co_discard
> > >   block: Remove bdrv_reset_dirty
> > >   qemu-iotests: Make block job methods common
> > >   qemu-iotests: Add test case for mirror with unmap
> > >   iotests: Use event_wait in wait_ready
> > > 
> > >  block.c                       | 12 --------
> > >  block/io.c                    | 60 ++++++++++++++++++++++++++++++---------
> > >  block/mirror.c                | 28 +++++++++++++++---
> > >  blockdev.c                    |  5 ++++
> > >  hmp.c                         |  2 +-
> > >  include/block/block.h         |  4 +++
> > >  include/block/block_int.h     |  4 +--
> > >  qapi/block-core.json          |  8 +++++-
> > >  qmp-commands.hx               |  3 ++
> > >  tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
> > >  tests/qemu-iotests/132        | 59 ++++++++++++++++++++++++++++++++++++++
> > >  tests/qemu-iotests/132.out    |  5 ++++
> > >  tests/qemu-iotests/group      |  1 +
> > >  tests/qemu-iotests/iotests.py | 23 +++++++++++++++
> > >  14 files changed, 196 insertions(+), 84 deletions(-)
> > >  create mode 100644 tests/qemu-iotests/132
> > >  create mode 100644 tests/qemu-iotests/132.out
> > > 
> > > -- 
> > > 2.4.2
> > > 
> > 
> > Thanks, applied to my block tree:
> > https://github.com/stefanha/qemu/commits/block
> 
> Stefan,
> 
> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> patches 4-8 are still useful on their own: they fix the mentioned crash and
> improve iotests.
> 
> Shall we merge the second half (of course none of them depend on 1-3) now that
> softfreeze is approaching?

Stefan, would you consider applying patches 4-8?

Fam

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Paolo Bonzini]

On 24/06/2015 11:08, Fam Zheng wrote:
>> Stefan,
>>
>> The only controversial patches are the qmp/drive-mirror ones (1-3), while
>> patches 4-8 are still useful on their own: they fix the mentioned crash and
>> improve iotests.
>>
>> Shall we merge the second half (of course none of them depend on 1-3) now that
>> softfreeze is approaching?
> 
> Stefan, would you consider applying patches 4-8?

Actually why not apply all of them?  Even if blockdev-mirror is a
superior interface in the long run, the current behavior of drive-mirror
can cause images to balloon up to the full size, which is bad.
Extending drive-mirror is okay IMHO for 2.4.

Paolo

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

On Wed, 06/24 19:01, Paolo Bonzini wrote:
> 
> 
> On 24/06/2015 11:08, Fam Zheng wrote:
> >> Stefan,
> >>
> >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> >> improve iotests.
> >>
> >> Shall we merge the second half (of course none of them depend on 1-3) now that
> >> softfreeze is approaching?
> > 
> > Stefan, would you consider applying patches 4-8?
> 
> Actually why not apply all of them?  Even if blockdev-mirror is a
> superior interface in the long run, the current behavior of drive-mirror
> can cause images to balloon up to the full size, which is bad.
> Extending drive-mirror is okay IMHO for 2.4.
> 

Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
I'll take a look at it today.

Fam

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

On Thu, 06/25 09:02, Fam Zheng wrote:
> On Wed, 06/24 19:01, Paolo Bonzini wrote:
> > 
> > 
> > On 24/06/2015 11:08, Fam Zheng wrote:
> > >> Stefan,
> > >>
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> > >> improve iotests.
> > >>
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that
> > >> softfreeze is approaching?
> > > 
> > > Stefan, would you consider applying patches 4-8?
> > 
> > Actually why not apply all of them?  Even if blockdev-mirror is a
> > superior interface in the long run, the current behavior of drive-mirror
> > can cause images to balloon up to the full size, which is bad.
> > Extending drive-mirror is okay IMHO for 2.4.
> > 
> 
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
> I'll take a look at it today.

There is no problem, the observasion by Andrey was just that qmp command takes
a few minutes before returning, because he didn't apply

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html

Fam

Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors

[Stefan Hajnoczi]

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

On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> v7: Fix the lost assignment of s->unmap.
> 
> v6: Fix pnum in bdrv_get_block_status_above. [Paolo]
> 
> v5: Rewrite patch 1.
>     Address Eric's comments on patch 3.
>     Add Eric's rev-by to patches 2 & 4.
>     Check BDRV_BLOCK_DATA in patch 3. [Paolo]
> 
> This fixes the mirror assert failure reported by wangxiaolong:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
> 
> The direct cause is that hbitmap code couldn't handle unset of bits *after*
> iterator's current position. We could fix that, but the bdrv_reset_dirty() call
> is more questionable:
> 
> Before, if guest discarded some sectors during migration, it could see
> different data after moving to dest side, depending on block backends of the
> src and the dest. This is IMO worse than mirroring the actual reading as done
> in this series, because we don't know what the guest is doing.
> 
> For example if a guest first issues WRITE SAME to wipe out the area then issues
> UNMAP to discard it, just to get rid of some sensitive data completely, we may
> miss both operations and leave stale data on dest image.
> 
> 
> Fam Zheng (8):
>   block: Add bdrv_get_block_status_above
>   qmp: Add optional bool "unmap" to drive-mirror
>   mirror: Do zero write on target if sectors not allocated
>   block: Fix dirty bitmap in bdrv_co_discard
>   block: Remove bdrv_reset_dirty
>   qemu-iotests: Make block job methods common
>   qemu-iotests: Add test case for mirror with unmap
>   iotests: Use event_wait in wait_ready
> 
>  block.c                       | 12 --------
>  block/io.c                    | 60 ++++++++++++++++++++++++++++++---------
>  block/mirror.c                | 28 +++++++++++++++---
>  blockdev.c                    |  5 ++++
>  hmp.c                         |  2 +-
>  include/block/block.h         |  4 +++
>  include/block/block_int.h     |  4 +--
>  qapi/block-core.json          |  8 +++++-
>  qmp-commands.hx               |  3 ++
>  tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
>  tests/qemu-iotests/132        | 59 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/132.out    |  5 ++++
>  tests/qemu-iotests/group      |  1 +
>  tests/qemu-iotests/iotests.py | 23 +++++++++++++++
>  14 files changed, 196 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qemu-iotests/132
>  create mode 100644 tests/qemu-iotests/132.out
> 
> -- 
> 2.4.2

Thanks, applied to my block tree again now that the bool/enum question
has been resolved:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Alexandre DERUMIER]

Hi,

>>There is no problem, the observasion by Andrey was just that qmp command takes 
>>a few minutes before returning, because he didn't apply 
>>
>>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 

Is this patch already apply on the block tree ?

With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).



----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "pbonzini" <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
Envoyé: Jeudi 25 Juin 2015 12:45:38
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

On Thu, 06/25 09:02, Fam Zheng wrote: 
> On Wed, 06/24 19:01, Paolo Bonzini wrote: 
> > 
> > 
> > On 24/06/2015 11:08, Fam Zheng wrote: 
> > >> Stefan, 
> > >> 
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while 
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and 
> > >> improve iotests. 
> > >> 
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that 
> > >> softfreeze is approaching? 
> > > 
> > > Stefan, would you consider applying patches 4-8? 
> > 
> > Actually why not apply all of them? Even if blockdev-mirror is a 
> > superior interface in the long run, the current behavior of drive-mirror 
> > can cause images to balloon up to the full size, which is bad. 
> > Extending drive-mirror is okay IMHO for 2.4. 
> > 
> 
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true, 
> I'll take a look at it today. 

There is no problem, the observasion by Andrey was just that qmp command takes 
a few minutes before returning, because he didn't apply 

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 

Fam 

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Alexandre DERUMIER]

>>With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).

it's blocked around 30min for 300GB, with a raw file on a netapp san array through nfs.


----- Mail original -----
De: "aderumier" <aderumier@odiso.com>
À: "Fam Zheng" <famz@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, "pbonzini" <pbonzini@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
Envoyé: Vendredi 26 Juin 2015 15:36:10
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

Hi, 

>>There is no problem, the observasion by Andrey was just that qmp command takes 
>>a few minutes before returning, because he didn't apply 
>> 
>>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 

Is this patch already apply on the block tree ? 

With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops). 



----- Mail original ----- 
De: "Fam Zheng" <famz@redhat.com> 
À: "pbonzini" <pbonzini@redhat.com> 
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn 
Envoyé: Jeudi 25 Juin 2015 12:45:38 
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors 

On Thu, 06/25 09:02, Fam Zheng wrote: 
> On Wed, 06/24 19:01, Paolo Bonzini wrote: 
> > 
> > 
> > On 24/06/2015 11:08, Fam Zheng wrote: 
> > >> Stefan, 
> > >> 
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while 
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and 
> > >> improve iotests. 
> > >> 
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that 
> > >> softfreeze is approaching? 
> > > 
> > > Stefan, would you consider applying patches 4-8? 
> > 
> > Actually why not apply all of them? Even if blockdev-mirror is a 
> > superior interface in the long run, the current behavior of drive-mirror 
> > can cause images to balloon up to the full size, which is bad. 
> > Extending drive-mirror is okay IMHO for 2.4. 
> > 
> 
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true, 
> I'll take a look at it today. 

There is no problem, the observasion by Andrey was just that qmp command takes 
a few minutes before returning, because he didn't apply 

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 

Fam 

Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors

[Fam Zheng]

On Fri, 06/26 15:36, Alexandre DERUMIER wrote:
> Hi,
> 
> >>There is no problem, the observasion by Andrey was just that qmp command takes 
> >>a few minutes before returning, because he didn't apply 
> >>
> >>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 
> 
> Is this patch already apply on the block tree ?
> 
> With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).

The patch is waiting for the next PULL in Jeff Cody's tree I think.

Fam

> 
> 
> 
> ----- Mail original -----
> De: "Fam Zheng" <famz@redhat.com>
> À: "pbonzini" <pbonzini@redhat.com>
> Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
> Envoyé: Jeudi 25 Juin 2015 12:45:38
> Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
> 
> On Thu, 06/25 09:02, Fam Zheng wrote: 
> > On Wed, 06/24 19:01, Paolo Bonzini wrote: 
> > > 
> > > 
> > > On 24/06/2015 11:08, Fam Zheng wrote: 
> > > >> Stefan, 
> > > >> 
> > > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while 
> > > >> patches 4-8 are still useful on their own: they fix the mentioned crash and 
> > > >> improve iotests. 
> > > >> 
> > > >> Shall we merge the second half (of course none of them depend on 1-3) now that 
> > > >> softfreeze is approaching? 
> > > > 
> > > > Stefan, would you consider applying patches 4-8? 
> > > 
> > > Actually why not apply all of them? Even if blockdev-mirror is a 
> > > superior interface in the long run, the current behavior of drive-mirror 
> > > can cause images to balloon up to the full size, which is bad. 
> > > Extending drive-mirror is okay IMHO for 2.4. 
> > > 
> > 
> > Before we do that, Andrey Korolyov has reported a hang issue with unmap=true, 
> > I'll take a look at it today. 
> 
> There is no problem, the observasion by Andrey was just that qmp command takes 
> a few minutes before returning, because he didn't apply 
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html 
> 
> Fam 
> 

Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated

[Kevin Wolf]

Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> Some protocols do zero upon discard, where it's best to use
> bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index d2515c7..3c38695 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>      int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
>      uint64_t delay_ns = 0;
>      MirrorOp *op;
> +    int pnum;
> +    int64_t ret;
>  
>      s->sector_num = hbitmap_iter_next(&s->hbi);
>      if (s->sector_num < 0) {
> @@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>      s->in_flight++;
>      s->sectors_in_flight += nb_sectors;
>      trace_mirror_one_iteration(s, sector_num, nb_sectors);
> -    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> -                   mirror_read_complete, op);
> +
> +    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> +                                      nb_sectors, &pnum);
> +    if (ret < 0 || pnum < nb_sectors ||

Earlier today I told Richard Jones that qemu-img commit should really
be using zero cluster support in the backing file since 2.4 because I
remembered this commit. Turns out it doesn't actually use it but writes
explicit zeros instead.

The reason is the condition 'pnum < nb_sectors' here, which makes mirror
fall back to explicit writes if bdrv_get_block_status_above() doesn't
return enough sectors (enough being relatively large here, I think in
qemu-img commit it's always the full 10 MB buffer).

In other words, we are ignoring any zero areas smaller than 10 MB!

(What made this worse is that qcow2 had a bug that reports only a single
zero cluster at a time, so it would never report more than 10 MB, even
if the image was completely zeroed. I've sent a fix for that one.)

In order to fix this, we'll probably need to move the call to
bdrv_get_block_status_above() before actually allocating memory and
all that for the full nb_chunks. We should detect zeros on the usual
block job granularity (64k by default, I think).

> +            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> +                       mirror_read_complete, op);
> +    } else if (ret & BDRV_BLOCK_ZERO) {
> +        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> +                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> +                              mirror_write_complete, op);
> +    } else {
> +        assert(!(ret & BDRV_BLOCK_DATA));
> +        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> +                         mirror_write_complete, op);
> +    }
>      return delay_ns;
>  }

Paolo also noticed that there's no reason at all to allocate buffers
and a qiov for the write_zeroes and discard cases.

Kevin

Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated

[Fam Zheng]

On Wed, 11/04 19:35, Kevin Wolf wrote:
> Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> > If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> > Some protocols do zero upon discard, where it's best to use
> > bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/mirror.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index d2515c7..3c38695 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> >      int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> >      uint64_t delay_ns = 0;
> >      MirrorOp *op;
> > +    int pnum;
> > +    int64_t ret;
> >  
> >      s->sector_num = hbitmap_iter_next(&s->hbi);
> >      if (s->sector_num < 0) {
> > @@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> >      s->in_flight++;
> >      s->sectors_in_flight += nb_sectors;
> >      trace_mirror_one_iteration(s, sector_num, nb_sectors);
> > -    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> > -                   mirror_read_complete, op);
> > +
> > +    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> > +                                      nb_sectors, &pnum);
> > +    if (ret < 0 || pnum < nb_sectors ||
> 
> Earlier today I told Richard Jones that qemu-img commit should really
> be using zero cluster support in the backing file since 2.4 because I
> remembered this commit. Turns out it doesn't actually use it but writes
> explicit zeros instead.
> 
> The reason is the condition 'pnum < nb_sectors' here, which makes mirror
> fall back to explicit writes if bdrv_get_block_status_above() doesn't
> return enough sectors (enough being relatively large here, I think in
> qemu-img commit it's always the full 10 MB buffer).
> 
> In other words, we are ignoring any zero areas smaller than 10 MB!
> 
> (What made this worse is that qcow2 had a bug that reports only a single
> zero cluster at a time, so it would never report more than 10 MB, even
> if the image was completely zeroed. I've sent a fix for that one.)
> 
> In order to fix this, we'll probably need to move the call to
> bdrv_get_block_status_above() before actually allocating memory and
> all that for the full nb_chunks. We should detect zeros on the usual
> block job granularity (64k by default, I think).
> 
> > +            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> > +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> > +                       mirror_read_complete, op);
> > +    } else if (ret & BDRV_BLOCK_ZERO) {
> > +        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> > +                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> > +                              mirror_write_complete, op);
> > +    } else {
> > +        assert(!(ret & BDRV_BLOCK_DATA));
> > +        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> > +                         mirror_write_complete, op);
> > +    }
> >      return delay_ns;
> >  }
> 
> Paolo also noticed that there's no reason at all to allocate buffers
> and a qiov for the write_zeroes and discard cases.

I'll write a patch to address these. Thanks!

Fam

Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated

[Kevin Wolf]

Am 05.11.2015 um 06:42 hat Fam Zheng geschrieben:
> On Wed, 11/04 19:35, Kevin Wolf wrote:
> > Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> > > If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> > > Some protocols do zero upon discard, where it's best to use
> > > bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  block/mirror.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/mirror.c b/block/mirror.c
> > > index d2515c7..3c38695 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> > >      int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> > >      uint64_t delay_ns = 0;
> > >      MirrorOp *op;
> > > +    int pnum;
> > > +    int64_t ret;
> > >  
> > >      s->sector_num = hbitmap_iter_next(&s->hbi);
> > >      if (s->sector_num < 0) {
> > > @@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> > >      s->in_flight++;
> > >      s->sectors_in_flight += nb_sectors;
> > >      trace_mirror_one_iteration(s, sector_num, nb_sectors);
> > > -    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> > > -                   mirror_read_complete, op);
> > > +
> > > +    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> > > +                                      nb_sectors, &pnum);
> > > +    if (ret < 0 || pnum < nb_sectors ||
> > 
> > Earlier today I told Richard Jones that qemu-img commit should really
> > be using zero cluster support in the backing file since 2.4 because I
> > remembered this commit. Turns out it doesn't actually use it but writes
> > explicit zeros instead.
> > 
> > The reason is the condition 'pnum < nb_sectors' here, which makes mirror
> > fall back to explicit writes if bdrv_get_block_status_above() doesn't
> > return enough sectors (enough being relatively large here, I think in
> > qemu-img commit it's always the full 10 MB buffer).
> > 
> > In other words, we are ignoring any zero areas smaller than 10 MB!
> > 
> > (What made this worse is that qcow2 had a bug that reports only a single
> > zero cluster at a time, so it would never report more than 10 MB, even
> > if the image was completely zeroed. I've sent a fix for that one.)
> > 
> > In order to fix this, we'll probably need to move the call to
> > bdrv_get_block_status_above() before actually allocating memory and
> > all that for the full nb_chunks. We should detect zeros on the usual
> > block job granularity (64k by default, I think).
> > 
> > > +            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> > > +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> > > +                       mirror_read_complete, op);
> > > +    } else if (ret & BDRV_BLOCK_ZERO) {
> > > +        bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> > > +                              s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> > > +                              mirror_write_complete, op);
> > > +    } else {
> > > +        assert(!(ret & BDRV_BLOCK_DATA));
> > > +        bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> > > +                         mirror_write_complete, op);
> > > +    }
> > >      return delay_ns;
> > >  }
> > 
> > Paolo also noticed that there's no reason at all to allocate buffers
> > and a qiov for the write_zeroes and discard cases.
> 
> I'll write a patch to address these. Thanks!

Thanks, Fam!

Kevin