[Qemu-devel] [PATCH v3 0/6] block: Mirror discarded sectors

[Qemu-devel] [PATCH v3 0/6] block: Mirror discarded sectors

[Fam Zheng]

v3: Add John's rev-by in patches 3~6.
    Rewrite patch 1: discard may not suffice, use write zeroes in that case.

v2: Fix typo and add Eric's rev-by in patch 3.
    Add patch 1 to discard target in mirror job. (Paolo)
    Add patch 6 to improve iotests.wait_ready. (John)

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 (6):
  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                    |  4 +--
 block/mirror.c                | 53 ++++++++++++++++++++++++++++++----
 include/block/block_int.h     |  2 --
 tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
 tests/qemu-iotests/131        | 59 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/131.out    |  5 ++++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py | 23 +++++++++++++++
 9 files changed, 152 insertions(+), 73 deletions(-)
 create mode 100644 tests/qemu-iotests/131
 create mode 100644 tests/qemu-iotests/131.out

-- 
2.4.0

[Qemu-devel] [PATCH v3 1/6] mirror: Do zero write on target if sectors not allocated

[Fam Zheng]

If guest discards a source cluster during mirror, we would want to
skip the read-write steps.

Source and target may have different "can_write_zeroes_with_unmap"
values and different discard granularity. It's important to replicate
the exact zeroing on the destination, so bdrv_aio_discard is only safe
if source has can_write_zeroes_with_unmap=false.

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

diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..1a1d997 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -57,6 +57,10 @@ typedef struct MirrorBlockJob {
     int in_flight;
     int sectors_in_flight;
     int ret;
+    /* Source driver can_write_zeroes_with_unmap. */
+    bool source_may_unmap;
+    /* Target driver can_write_zeroes_with_unmap. */
+    bool target_may_unmap;
 } MirrorBlockJob;
 
 typedef struct MirrorOp {
@@ -163,6 +167,7 @@ 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;
 
     s->sector_num = hbitmap_iter_next(&s->hbi);
     if (s->sector_num < 0) {
@@ -289,8 +294,32 @@ 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);
+
+    if (!bdrv_is_allocated_above(source, NULL, sector_num,
+                                 nb_sectors, &pnum)) {
+        op->nb_sectors = pnum;
+        if (s->source_may_unmap) {
+            /*
+             * Source unallocated sectors have zero data. We can't discard
+             * target even if s->target_may_unmap, because the discard
+             * granularity may be different.
+             */
+            bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
+                                  s->target_may_unmap ? BDRV_REQ_MAY_UNMAP : 0,
+                                  mirror_write_complete,
+                                  op);
+        } else {
+            /*
+             * Source has irrelevant data in unmapped sectors, it's safe to
+             * discard target.
+             * */
+            bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
+                             mirror_write_complete, op);
+        }
+    } else {
+        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
+                       mirror_read_complete, op);
+    }
     return delay_ns;
 }
 
@@ -399,6 +428,22 @@ static void coroutine_fn mirror_run(void *opaque)
     length = DIV_ROUND_UP(s->bdev_length, s->granularity);
     s->in_flight_bitmap = bitmap_new(length);
 
+    ret = bdrv_get_info(bs, &bdi);
+    if (ret < 0) {
+        /* Safe side. */
+        s->source_may_unmap = true;
+    } else {
+        s->source_may_unmap = bdi.can_write_zeroes_with_unmap;
+    }
+
+    ret = bdrv_get_info(s->target, &bdi);
+    if (ret < 0) {
+        /* Safe side. */
+        s->target_may_unmap = false;
+    } else {
+        s->target_may_unmap = bdi.can_write_zeroes_with_unmap;
+    }
+
     /* If we have no backing file yet in the destination, we cannot let
      * the destination do COW.  Instead, we copy sectors around the
      * dirty data if needed.  We need a bitmap to do that.
@@ -406,10 +451,6 @@ static void coroutine_fn mirror_run(void *opaque)
     bdrv_get_backing_filename(s->target, backing_filename,
                               sizeof(backing_filename));
     if (backing_filename[0] && !s->target->backing_hd) {
-        ret = bdrv_get_info(s->target, &bdi);
-        if (ret < 0) {
-            goto immediate_exit;
-        }
         if (s->granularity < bdi.cluster_size) {
             s->buf_size = MAX(s->buf_size, bdi.cluster_size);
             s->cow_bitmap = bitmap_new(length);
-- 
2.4.0

[Qemu-devel] [PATCH v3 2/6] 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 destinition 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>
---
 block/io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..809688b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2343,8 +2343,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
         return -EROFS;
     }
 
-    bdrv_reset_dirty(bs, sector_num, nb_sectors);
-
     /* Do nothing if disabled.  */
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
         return 0;
@@ -2354,6 +2352,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.0

[Qemu-devel] [PATCH v3 3/6] 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 7904098..511e13c 100644
--- a/block.c
+++ b/block.c
@@ -3324,18 +3324,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 db29b74..aaed2fa 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -635,7 +635,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.0

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

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@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 e93e623..2e07cc4 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.0

[Qemu-devel] [PATCH v3 5/6] 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/131     | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/131.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 65 insertions(+)
 create mode 100644 tests/qemu-iotests/131
 create mode 100644 tests/qemu-iotests/131.out

diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
new file mode 100644
index 0000000..f53ef6e
--- /dev/null
+++ b/tests/qemu-iotests/131
@@ -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/131.out b/tests/qemu-iotests/131.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/131.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6ca3466..34b16cb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -128,3 +128,4 @@
 128 rw auto quick
 129 rw auto quick
 130 rw auto quick
+131 rw auto quick
-- 
2.4.0

[Qemu-devel] [PATCH v3 6/6] 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 2e07cc4..0ddc513 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.0

Re: [Qemu-devel] [PATCH v3 0/6] block: Mirror discarded sectors

[Fam Zheng]

On Tue, 05/12 19:48, Fam Zheng wrote:
> This fixes the mirror assert failure reported by wangxiaolong:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
> 

Cc: qemu-stable@nongnu.org

Re: [Qemu-devel] [PATCH v3 1/6] mirror: Do zero write on target if sectors not allocated

[Paolo Bonzini]

On 12/05/2015 13:48, Fam Zheng wrote:
> +    if (!bdrv_is_allocated_above(source, NULL, sector_num,
> +                                 nb_sectors, &pnum)) {
> +        op->nb_sectors = pnum;
> +        if (s->source_may_unmap) {

Can you avoid this check by introducing bdrv_get_block_status_above?  Then:

- if BDRV_ZERO, you use bdrv_aio_write_zeroes

- if BDRV_ALLOCATED, you use bdrv_aio_readv

- else you use bdrv_aio_discard

> +            /*
> +             * Source unallocated sectors have zero data. We can't discard
> +             * target even if s->target_may_unmap, because the discard
> +             * granularity may be different.
> +             */
> +            bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> +                                  s->target_may_unmap ? BDRV_REQ_MAY_UNMAP : 0,

You can set the flag unconditionally.  But it's probably better to make
this a drive-mirror argument instead of checking the target's
BlockDriverInfo.

Paolo

> +                                  mirror_write_complete,
> +                                  op);
> +        } else {
> +            /*
> +             * Source has irrelevant data in unmapped sectors, it's safe to
> +             * discard target.
> +             * */
> +            bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> +                             mirror_write_complete, op);
> +        }
> +    } else {
> +        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> +                       mirror_read_complete, op);
> +    }
>      return delay_ns;
>  }
>  
> @@ -399,6 +428,22 @@ static void coroutine_fn mirror_run(void *opaque)
>      length = DIV_ROUND_UP(s->bdev_length, s->granularity);
>      s->in_flight_bitmap = bitmap_new(length);
>  
> +    ret = bdrv_get_info(bs, &bdi);
> +    if (ret < 0) {
> +        /* Safe side. */
> +        s->source_may_unmap = true;
> +    } else {
> +        s->source_may_unmap = bdi.can_write_zeroes_with_unmap;
> +    }
> +
> +    ret = bdrv_get_info(s->target, &bdi);
> +    if (ret < 0) {
> +        /* Safe side. */
> +        s->target_may_unmap = false;
> +    } else {
> +        s->target_may_unmap = bdi.can_write_zeroes_with_unmap;
> +    }
> +

Re: [Qemu-devel] [PATCH v3 2/6] block: Fix dirty bitmap in bdrv_co_discard

[Paolo Bonzini]

On 12/05/2015 13:48, Fam Zheng wrote:
> 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 destinition 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>

> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 1ce62c4..809688b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2343,8 +2343,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>          return -EROFS;
>      }
>  
> -    bdrv_reset_dirty(bs, sector_num, nb_sectors);
> -
>      /* Do nothing if disabled.  */
>      if (!(bs->open_flags & BDRV_O_UNMAP)) {
>          return 0;
> @@ -2354,6 +2352,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;
> 

Re: [Qemu-devel] [PATCH v3 4/6] qemu-iotests: Make block job methods common

[Paolo Bonzini]

On 12/05/2015 13:48, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@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 e93e623..2e07cc4 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")
> 

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

Re: [Qemu-devel] [PATCH v3 5/6] qemu-iotests: Add test case for mirror with unmap

[Paolo Bonzini]

On 12/05/2015 13:48, 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/131     | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/131.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100644 tests/qemu-iotests/131
>  create mode 100644 tests/qemu-iotests/131.out
> 
> diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
> new file mode 100644
> index 0000000..f53ef6e
> --- /dev/null
> +++ b/tests/qemu-iotests/131
> @@ -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/131.out b/tests/qemu-iotests/131.out
> new file mode 100644
> index 0000000..ae1213e
> --- /dev/null
> +++ b/tests/qemu-iotests/131.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 6ca3466..34b16cb 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -128,3 +128,4 @@
>  128 rw auto quick
>  129 rw auto quick
>  130 rw auto quick
> +131 rw auto quick
> 

Do other "quick" tests need a QEMU binary?

Paolo

Re: [Qemu-devel] [PATCH v3 5/6] qemu-iotests: Add test case for mirror with unmap

[Fam Zheng]

On Thu, 05/14 12:37, Paolo Bonzini wrote:
> Do other "quick" tests need a QEMU binary?

102

Re: [Qemu-devel] [PATCH v3 1/6] mirror: Do zero write on target if sectors not allocated

[Fam Zheng]

On Thu, 05/14 12:30, Paolo Bonzini wrote:
> 
> 
> On 12/05/2015 13:48, Fam Zheng wrote:
> > +    if (!bdrv_is_allocated_above(source, NULL, sector_num,
> > +                                 nb_sectors, &pnum)) {
> > +        op->nb_sectors = pnum;
> > +        if (s->source_may_unmap) {
> 
> Can you avoid this check by introducing bdrv_get_block_status_above?  Then:
> 
> - if BDRV_ZERO, you use bdrv_aio_write_zeroes
> 
> - if BDRV_ALLOCATED, you use bdrv_aio_readv
> 
> - else you use bdrv_aio_discard

OK, that's better.

> 
> > +            /*
> > +             * Source unallocated sectors have zero data. We can't discard
> > +             * target even if s->target_may_unmap, because the discard
> > +             * granularity may be different.
> > +             */
> > +            bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> > +                                  s->target_may_unmap ? BDRV_REQ_MAY_UNMAP : 0,
> 
> You can set the flag unconditionally.  But it's probably better to make
> this a drive-mirror argument instead of checking the target's
> BlockDriverInfo.

OK, I'll add a flag.

Fam