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

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

[Fam Zheng]

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                | 27 +++++++++++++++---
 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, 195 insertions(+), 84 deletions(-)
 create mode 100644 tests/qemu-iotests/132
 create mode 100644 tests/qemu-iotests/132.out

-- 
2.4.1

[Qemu-devel] [PATCH v6 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 c1c963e..8a13bed 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.1

[Qemu-devel] [PATCH v6 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            | 7 +++++--
 blockdev.c                | 5 +++++
 hmp.c                     | 2 +-
 include/block/block_int.h | 2 ++
 qapi/block-core.json      | 8 +++++++-
 qmp-commands.hx           | 3 +++
 6 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..85995b2 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,
@@ -703,6 +705,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 +720,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 +768,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 5eaf77e..4387e14 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2632,6 +2632,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;
@@ -2663,6 +2664,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",
@@ -2802,6 +2806,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 e17852d..62c53e0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1056,7 +1056,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 863ffea..a59063d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -954,6 +954,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
@@ -966,7 +971,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 14e109e..63c86fc 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.1

[Qemu-devel] [PATCH v6 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 85995b2..ba33254 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.1

[Qemu-devel] [PATCH v6 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.1

[Qemu-devel] [PATCH v6 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 f42d70e..f6ee9db 100644
--- a/block.c
+++ b/block.c
@@ -3336,18 +3336,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.1

[Qemu-devel] [PATCH v6 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.1

[Qemu-devel] [PATCH v6 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.1

[Qemu-devel] [PATCH v6 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.1

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

[Paolo Bonzini]

On 28/05/2015 07:29, Fam Zheng wrote:
> 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.

"Reimplement bdrv_get_block_status on top".

Can be fixed by whoever commits this, so

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

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

[Paolo Bonzini]

On 28/05/2015 07:29, 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>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  block/mirror.c            | 7 +++++--
>  blockdev.c                | 5 +++++
>  hmp.c                     | 2 +-
>  include/block/block_int.h | 2 ++
>  qapi/block-core.json      | 8 +++++++-
>  qmp-commands.hx           | 3 +++
>  6 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 58f391a..85995b2 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,
> @@ -703,6 +705,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 +720,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 +768,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 5eaf77e..4387e14 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2632,6 +2632,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;
> @@ -2663,6 +2664,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",
> @@ -2802,6 +2806,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 e17852d..62c53e0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1056,7 +1056,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 863ffea..a59063d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -954,6 +954,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
> @@ -966,7 +971,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 14e109e..63c86fc 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
> 

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

[Paolo Bonzini]

On 28/05/2015 07:29, Fam Zheng wrote:
> 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>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  block/mirror.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 85995b2..ba33254 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;
>  }
>  
> 

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

[Paolo Bonzini]

On 28/05/2015 07:29, Fam Zheng wrote:
> 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
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Paolo Bonzini]

On 28/05/2015 07:29, Fam Zheng wrote:
> 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)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Stefan Hajnoczi]

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

On Thu, May 28, 2015 at 01:29:42PM +0800, Fam Zheng wrote:
> 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                | 27 +++++++++++++++---
>  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, 195 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qemu-iotests/132
>  create mode 100644 tests/qemu-iotests/132.out
> 
> -- 
> 2.4.1
> 
> 

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 v6 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Eric Blake]

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

On 05/27/2015 11:29 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>
> ---

> +++ b/qapi/block-core.json
> @@ -954,6 +954,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)

Just making sure I understand:

The guest sees identical contents, but with "unmap":true, the host file
is potentially sparse, while with "unmap":false, the host file is
fully-allocated.

Also, while the default is now true, this doesn't tell me what the
behavior was in 2.3.  Is this a new default behavior (where in 2.3 you
could not preserve sparseness), or a new knob (previously you always got
a sparse copy, and could not request full allocation)?  I'm okay either
way, but I'm trying to understand whether libvirt should advertise this
knob to higher-level apps, and if so, what libvirt should do when it
detects qemu 2.3 (that is, should it tell upper-level apps that
sparseness cannot be preserved, or that full allocation cannot be
guaranteed, when the "unmap" parameter is not detected).

-- 
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 v6 2/8] qmp: Add optional bool "unmap" to drive-mirror

[Fam Zheng]

On Tue, 06/02 11:28, Eric Blake wrote:
> On 05/27/2015 11:29 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>
> > ---
> 
> > +++ b/qapi/block-core.json
> > @@ -954,6 +954,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)
> 
> Just making sure I understand:
> 
> The guest sees identical contents, but with "unmap":true, the host file
> is potentially sparse, while with "unmap":false, the host file is
> fully-allocated.
> 
> Also, while the default is now true, this doesn't tell me what the
> behavior was in 2.3.  Is this a new default behavior (where in 2.3 you
> could not preserve sparseness), or a new knob (previously you always got
> a sparse copy, and could not request full allocation)?  I'm okay either
> way, but I'm trying to understand whether libvirt should advertise this
> knob to higher-level apps, and if so, what libvirt should do when it
> detects qemu 2.3 (that is, should it tell upper-level apps that
> sparseness cannot be preserved, or that full allocation cannot be
> guaranteed, when the "unmap" parameter is not detected).

We always skip sectors which are initially unallocated (at the time of mirror
job starting), actually this even stays true for both unmap=true and
unmap=false now.

The difference is how we handle sectors discarded *after* mirror job starts:

Before, we ignore the *discard*, which means the target remains populated, with
old data before discard.

After, we honor the discard, depending on two factors:

    source read as zero    unmap                 RESULT
==========================================================================

           Y               true       write zero with BDRV_REQ_MAY_UNMAP

           Y               false      write zero without BDRV_REQ_MAY_UNMAP

           N               both       discard (note that on some protocols
                                      this may be nop)

In other words, the unmap option only affects sector X if:

1) in the beginning, sector X was allocated
2) drive-mirror starts
3) sector X got discarded at source side

All in all, this is quite advisory.

Fam