[Qemu-devel] [PATCH v2 0/3] mirror: Allow detection of zeroes on source sectors

[Qemu-devel] [PATCH v2 0/3] mirror: Allow detection of zeroes on source sectors

[Fam Zheng]

[These patches go on top of the "block: Mirror discarded sectors" series]

v2: Rely on block/io.c zero detection. [Paolo]

Some protocols don't have an easy way to query sparseness, (e.g. block/nfs.c,
block/nbd.c), for which block layer always reports block status as "allocated
data".

This will let mirror job do full provisioning even if data is actually sparse
under the hood.

With the new "detect-zeroes" option, we can let mirror job detect zeroes before
sending the data to target, and use zero write when it is more efficient.

Fam

Fam Zheng (3):
  block: Extrace bdrv_parse_detect_zeroes_flags
  qapi: Add "detect-zeroes" option to drive-mirror
  iotests: Add test cases for drive-mirror "detect-zeroes" option

 block.c                       | 26 ++++++++++++++++++++++++++
 blockdev.c                    | 40 +++++++++++++++++++++++++++-------------
 hmp.c                         |  2 +-
 include/block/block.h         |  3 +++
 qapi/block-core.json          |  4 +++-
 qmp-commands.hx               |  4 +++-
 tests/qemu-iotests/132        | 28 +++++++++++++++++++++++++---
 tests/qemu-iotests/132.out    |  4 ++--
 tests/qemu-iotests/iotests.py |  7 +++++++
 9 files changed, 97 insertions(+), 21 deletions(-)

-- 
2.4.2

[Qemu-devel] [PATCH v2 1/3] block: Extrace bdrv_parse_detect_zeroes_flags

[Fam Zheng]

The logic will be shared with qmp_drive_mirror.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 26 ++++++++++++++++++++++++++
 blockdev.c            | 14 ++------------
 include/block/block.h |  3 +++
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 9890707..d9686c5 100644
--- a/block.c
+++ b/block.c
@@ -33,6 +33,7 @@
 #include "qemu/notify.h"
 #include "block/coroutine.h"
 #include "block/qapi.h"
+#include "qapi/util.h"
 #include "qmp-commands.h"
 #include "qemu/timer.h"
 #include "qapi-event.h"
@@ -671,6 +672,31 @@ int bdrv_parse_cache_flags(const char *mode, int *flags)
     return 0;
 }
 
+BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes_flags(const char *mode,
+                                                           int bdrv_flags,
+                                                           Error **errp)
+{
+    Error *local_err = NULL;
+    BlockdevDetectZeroesOptions detect_zeroes;
+    detect_zeroes =
+        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+                        mode,
+                        BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+                        BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+                        &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return detect_zeroes;
+    }
+
+    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+        !(bdrv_flags & BDRV_O_UNMAP)) {
+        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+                         "without setting discard operation to unmap");
+    }
+    return detect_zeroes;
+}
+
 /*
  * Returns the flags that a temporary snapshot should get, based on the
  * originally requested flags (the originally requested image will have flags
diff --git a/blockdev.c b/blockdev.c
index 6a45555..5ad6960 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -483,23 +483,13 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     detect_zeroes =
-        qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
-                        qemu_opt_get(opts, "detect-zeroes"),
-                        BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
-                        BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-                        &error);
+        bdrv_parse_detect_zeroes_flags(qemu_opt_get(opts, "detect-zeroes"),
+                                       bdrv_flags, &error);
     if (error) {
         error_propagate(errp, error);
         goto early_err;
     }
 
-    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-        !(bdrv_flags & BDRV_O_UNMAP)) {
-        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
-                         "without setting discard operation to unmap");
-        goto early_err;
-    }
-
     /* init */
     if ((!file || !*file) && !has_driver_specific_opts) {
         blk = blk_new_with_bs(qemu_opts_id(opts), errp);
diff --git a/include/block/block.h b/include/block/block.h
index 4d9b555..b7905ab 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -194,6 +194,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
+BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes_flags(const char *mode,
+                                                           int bdrv_flags,
+                                                           Error **errp);
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
                     bool allow_none, Error **errp);
-- 
2.4.2

[Qemu-devel] [PATCH v2 2/3] qapi: Add "detect-zeroes" option to drive-mirror

[Fam Zheng]

The new optional flag defaults to true, in which case, mirror job would
check the read sectors and use sparse write if they are zero.  Otherwise
data will be fully copied.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c           | 26 +++++++++++++++++++++++++-
 hmp.c                |  2 +-
 qapi/block-core.json |  4 +++-
 qmp-commands.hx      |  4 +++-
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5ad6960..3d008a2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2622,6 +2622,7 @@ void qmp_drive_mirror(const char *device, const char *target,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       bool has_unmap, bool unmap,
+                      bool has_detect_zeroes, bool detect_zeroes,
                       Error **errp)
 {
     BlockBackend *blk;
@@ -2634,6 +2635,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     int flags;
     int64_t size;
     int ret;
+    const char *detect_zeroes_str;
 
     if (!has_speed) {
         speed = 0;
@@ -2656,6 +2658,9 @@ void qmp_drive_mirror(const char *device, const char *target,
     if (!has_unmap) {
         unmap = true;
     }
+    if (!has_detect_zeroes) {
+        detect_zeroes = true;
+    }
 
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -2770,11 +2775,21 @@ void qmp_drive_mirror(const char *device, const char *target,
         goto out;
     }
 
+    options = qdict_new();
     if (has_node_name) {
-        options = qdict_new();
         qdict_put(options, "node-name", qstring_from_str(node_name));
     }
 
+    if (unmap) {
+        flags |= BDRV_O_UNMAP;
+    }
+
+    if (detect_zeroes) {
+        detect_zeroes_str = unmap ? "unmap" : "on";
+    } else {
+        detect_zeroes_str = "off";
+    }
+
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
@@ -2786,6 +2801,15 @@ void qmp_drive_mirror(const char *device, const char *target,
         goto out;
     }
 
+    target_bs->detect_zeroes =
+        bdrv_parse_detect_zeroes_flags(detect_zeroes_str,
+                                       flags,
+                                       &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
     bdrv_set_aio_context(target_bs, aio_context);
 
     /* pass the node name to replace to mirror start since it's loose coupling
diff --git a/hmp.c b/hmp.c
index d5b9ebb..2056b53 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1057,7 +1057,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
                      false, NULL, false, NULL,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, 0, false, 0,
-                     false, 0, false, 0, false, true, &err);
+                     false, 0, false, 0, false, true, false, true, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 71ed838..d157f0f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -978,6 +978,8 @@
 #         target image sectors will be unmapped; otherwise, zeroes will be
 #         written. Both will result in identical contents.
 #         Default is true. (Since 2.4)
+# @detect-zeroes: #optional Whether to detect zero sectors in source and use
+#                 zero write on target. Default is true. (Since 2.4)
 #
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
@@ -991,7 +993,7 @@
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-            '*unmap': 'bool' } }
+            '*unmap': 'bool', '*detect-zeroes': 'bool' } }
 
 ##
 # @BlockDirtyBitmap
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3b33496..efeb77f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1503,7 +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?,"
+                      "unmap:b?,detect-zeroes:b?"
                       "granularity:i?,buf-size:i?",
         .mhandler.cmd_new = qmp_marshal_input_drive_mirror,
     },
@@ -1545,6 +1545,8 @@ Arguments:
   (BlockdevOnError, default 'report')
 - "unmap": whether the target sectors should be discarded where source has only
   zeroes. (json-bool, optional, default true)
+- "detect-zeroes": if true, the source sectors that are zeroes will be written as
+  sparse on target. (json-bool, optional, default true)
 
 The default value of the granularity is the image cluster size clamped
 between 4096 and 65536, if the image format defines one.  If the format
-- 
2.4.2

[Qemu-devel] [PATCH v2 3/3] iotests: Add test cases for drive-mirror "detect-zeroes" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/132        | 28 +++++++++++++++++++++++++---
 tests/qemu-iotests/132.out    |  4 ++--
 tests/qemu-iotests/iotests.py |  7 +++++++
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
index f53ef6e..a4a4f01 100644
--- a/tests/qemu-iotests/132
+++ b/tests/qemu-iotests/132
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Test mirror with unmap
+# Test mirror with unmap and zero source clusters
 #
 # Copyright (C) 2015 Red Hat, Inc.
 #
@@ -21,7 +21,7 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_io, qemu_img_map
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
@@ -55,5 +55,27 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
+    def do_detect_zeroes_test(self, detect_zeroes, unmap):
+        self.vm.hmp_qemu_io('drive0', 'write -P 0 0 2M')
+        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
+                             target=target_img, detect_zeroes=detect_zeroes,
+                             unmap=unmap)
+        self.assert_qmp(result, 'return', {})
+        self.complete_and_wait('drive0')
+        self.vm.shutdown()
+        return qemu_img_map(target_img)
+
+    def test_detect_zeroes(self):
+        m = self.do_detect_zeroes_test(detect_zeroes=True, unmap=False);
+        self.assertTrue(m[0]["zero"])
+
+    def test_detect_zeroes_unmap(self):
+        m = self.do_detect_zeroes_test(detect_zeroes=True, unmap=True);
+        self.assertTrue(m[0]["zero"])
+
+    def test_no_detect_zeroes(self):
+        m = self.do_detect_zeroes_test(detect_zeroes=False, unmap=False);
+        self.assertFalse(m[0]["zero"])
+
 if __name__ == '__main__':
-    iotests.main(supported_fmts=['raw', 'qcow2'])
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/132.out b/tests/qemu-iotests/132.out
index ae1213e..89968f3 100644
--- a/tests/qemu-iotests/132.out
+++ b/tests/qemu-iotests/132.out
@@ -1,5 +1,5 @@
-.
+....
 ----------------------------------------------------------------------
-Ran 1 tests
+Ran 4 tests
 
 OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8615b10..2ddc735 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -27,6 +27,7 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts', '
 import qmp
 import qtest
 import struct
+import json
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
            'VM', 'QMPTestCase', 'notrun', 'main']
@@ -58,6 +59,12 @@ def qemu_img_pipe(*args):
     '''Run qemu-img and return its output'''
     return subprocess.Popen(qemu_img_args + list(args), stdout=subprocess.PIPE).communicate()[0]
 
+def qemu_img_map(*args):
+    '''Run qemu-img map and return the result parsed from the json formated
+    output '''
+    output = qemu_img_pipe(*(['map', '--output=json'] + list(args)))
+    return json.loads(output)
+
 def qemu_io(*args):
     '''Run qemu-io and return the stdout data'''
     args = qemu_io_args + list(args)
-- 
2.4.2

Re: [Qemu-devel] [PATCH v2 2/3] qapi: Add "detect-zeroes" option to drive-mirror

[Paolo Bonzini]

On 08/06/2015 12:34, Fam Zheng wrote:
>  - "unmap": whether the target sectors should be discarded where source has only
>    zeroes. (json-bool, optional, default true)
> +- "detect-zeroes": if true, the source sectors that are zeroes will be written as
> +  sparse on target. (json-bool, optional, default true)

all source sectors that are zeroes will be written using offloaded zero
writes on the target; furthermore, the sectors on the target will be
made sparse if possible if "unmap" is also true.

Perhaps it's simpler to use the enum though.  I'm not sure.  Jeff?

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] block: Extrace bdrv_parse_detect_zeroes_flags

[Eric Blake]

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

On 06/08/2015 04:34 AM, Fam Zheng wrote:
> The logic will be shared with qmp_drive_mirror.

s/Extrace/Extract/ in the subject

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 26 ++++++++++++++++++++++++++
>  blockdev.c            | 14 ++------------
>  include/block/block.h |  3 +++
>  3 files changed, 31 insertions(+), 12 deletions(-)
> 

> +
> +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> +        !(bdrv_flags & BDRV_O_UNMAP)) {
> +        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
> +                         "without setting discard operation to unmap");
> +    }

I think it might be better to have a tri-state enum, than to have two
competing bools where only 3 of the 4 states are valid.  We haven't yet
committed to the 'unmap' bool, so we still have time to get the API right.

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


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

Re: [Qemu-devel] [PATCH v2 2/3] qapi: Add "detect-zeroes" option to drive-mirror

[Eric Blake]

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

On 06/08/2015 04:34 AM, Fam Zheng wrote:
> The new optional flag defaults to true, in which case, mirror job would
> check the read sectors and use sparse write if they are zero.  Otherwise
> data will be fully copied.

Is that a different default than in qemu 2.3?  That's okay, but I need
to figure out how to probe if qemu supports the new flag for libvirt to
know to set it.  I'm hoping Markus' work on introspection might save the
day...

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c           | 26 +++++++++++++++++++++++++-
>  hmp.c                |  2 +-
>  qapi/block-core.json |  4 +++-
>  qmp-commands.hx      |  4 +++-
>  4 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5ad6960..3d008a2 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2622,6 +2622,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>                        bool has_on_source_error, BlockdevOnError on_source_error,
>                        bool has_on_target_error, BlockdevOnError on_target_error,
>                        bool has_unmap, bool unmap,
> +                      bool has_detect_zeroes, bool detect_zeroes,

Again, as I mentioned on 1/3, I think a tri-state enum might be easier
to use than two competing bools.  In fact, it might be more than
tri-state.  What are our possibilities?

1. We want the dest to be fully allocated, regardless of the source
being sparse
2. We want the dest to be as sparse as possible, regardless of the
source being fully allocated (or at least being unable to tell us about
holes)
3. We want the dest to mirror the sparseness of the host, but only where
that is efficient (if the source reads holes, make a hole in the dest)
4. Any other modes?

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


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

Re: [Qemu-devel] [PATCH v2 1/3] block: Extrace bdrv_parse_detect_zeroes_flags

[Paolo Bonzini]

On 08/06/2015 16:17, Eric Blake wrote:
>> > +
>> > +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> > +        !(bdrv_flags & BDRV_O_UNMAP)) {
>> > +        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
>> > +                         "without setting discard operation to unmap");
>> > +    }
> I think it might be better to have a tri-state enum, than to have two
> competing bools where only 3 of the 4 states are valid.

Note that this is not a bool.  We have one bool and one 3-element enum
(off/on/unmap), where only 5 of the 6 states are valid.  Also, at least
detect-zeroes would go away if we had some kind of blockdev-mirror
(where the target is added first with blockdev-add), so I think it's
better to leave it separate.

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] block: Extrace bdrv_parse_detect_zeroes_flags

[Fam Zheng]

On Mon, 06/08 16:53, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 16:17, Eric Blake wrote:
> >> > +
> >> > +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> >> > +        !(bdrv_flags & BDRV_O_UNMAP)) {
> >> > +        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
> >> > +                         "without setting discard operation to unmap");
> >> > +    }
> > I think it might be better to have a tri-state enum, than to have two
> > competing bools where only 3 of the 4 states are valid.
> 
> Note that this is not a bool.  We have one bool and one 3-element enum
> (off/on/unmap), where only 5 of the 6 states are valid.  Also, at least
> detect-zeroes would go away if we had some kind of blockdev-mirror
> (where the target is added first with blockdev-add), so I think it's
> better to leave it separate.

Agreed. But by then "drive-mirror derive=... detect-zeroes=unmap unmap=false"
will be way too confusing.

I say let's save that and just go for blockdev-add :)

https://www.mail-archive.com/qemu-devel@nongnu.org/msg301990.html

Fam

Re: [Qemu-devel] [PATCH v2 1/3] block: Extrace bdrv_parse_detect_zeroes_flags

[Fam Zheng]

On Wed, 06/10 17:11, Fam Zheng wrote:
> On Mon, 06/08 16:53, Paolo Bonzini wrote:
> > 
> > 
> > On 08/06/2015 16:17, Eric Blake wrote:
> > >> > +
> > >> > +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> > >> > +        !(bdrv_flags & BDRV_O_UNMAP)) {
> > >> > +        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
> > >> > +                         "without setting discard operation to unmap");
> > >> > +    }
> > > I think it might be better to have a tri-state enum, than to have two
> > > competing bools where only 3 of the 4 states are valid.
> > 
> > Note that this is not a bool.  We have one bool and one 3-element enum
> > (off/on/unmap), where only 5 of the 6 states are valid.  Also, at least
> > detect-zeroes would go away if we had some kind of blockdev-mirror
> > (where the target is added first with blockdev-add), so I think it's
> > better to leave it separate.
> 
> Agreed. But by then "drive-mirror derive=... detect-zeroes=unmap unmap=false"

s/derive/device/

> will be way too confusing.
> 
> I say let's save that and just go for blockdev-add :)
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg301990.html
> 
> Fam
>