diff mbox

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

Message ID 1433759662-25139-3-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng June 8, 2015, 10:34 a.m. UTC 
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(-)

Comments

Paolo Bonzini June 8, 2015, 10:47 a.m. UTC | #1 
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
Eric Blake June 8, 2015, 2:21 p.m. UTC | #2 
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?
diff mbox

Patch

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