Patchwork [RFC,V1,11/12] qmp: Add block-pause-dedup.

login
register
mail settings
Submitter Benoît Canet
Date Jan. 16, 2013, 4:25 p.m.
Message ID <1358353518-5421-12-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/212881/
State New
Headers show

Comments

Benoît Canet - Jan. 16, 2013, 4:25 p.m.
---
 blockdev.c       |   18 ++++++++++++++++++
 qapi-schema.json |   18 ++++++++++++++++++
 qmp-commands.hx  |   23 +++++++++++++++++++++++
 3 files changed, 59 insertions(+)
Eric Blake - Jan. 16, 2013, 11:32 p.m.
On 01/16/2013 09:25 AM, Benoît Canet wrote:
> ---
>  blockdev.c       |   18 ++++++++++++++++++
>  qapi-schema.json |   18 ++++++++++++++++++
>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
> 
>  
>  ##
> +# @block-pause-dedup:
> +#
> +# This command pause the deduplication on a device that support it.

s/support/supports/

> +#
> +# @device:   the name of the device to pause the deduplication on
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @device is not deduplicated, DeviceNotDeduplicated

I don't think you need this second error.  A generic error is good
enough unless we can prove that having a dedicated error class makes
algorithmic sense for a given client, and I can't come up with such a
scenario off the top of my head for libvirt.

> +SQMP
> +block-pause-dedup
> +------------
> +
> +Pause the deduplication on a device that support it.

s/support/supports/

I notice that between this and patch 12, you are adding two very similar
commands (block-pause-dedup, block-resume-dedup); would it be any
simpler to add a single command instead:

{ 'command': 'block-dedup-control',
  'data': { 'device': 'str', 'enable': 'bool' } }

where the user calls:

{ "execute": "block-dedup-control",
  "arguments": { "device": "ide0-hd0", "enable": false } }

to pause, and "enable":true to resume?
Benoît Canet - Jan. 17, 2013, 11:20 a.m.
> > +#
> > +# @device:   the name of the device to pause the deduplication on
> > +#
> > +# Returns: nothing on success
> > +#          If @device is not a valid block device, DeviceNotFound
> > +#          If @device is not deduplicated, DeviceNotDeduplicated
> 
> I don't think you need this second error.  A generic error is good
> enough unless we can prove that having a dedicated error class makes
> algorithmic sense for a given client, and I can't come up with such a
> scenario off the top of my head for libvirt.

Ok i'll remove it.

> 
> > +SQMP
> > +block-pause-dedup
> > +------------
> > +
> > +Pause the deduplication on a device that support it.
> 
> s/support/supports/
> 
> I notice that between this and patch 12, you are adding two very similar
> commands (block-pause-dedup, block-resume-dedup); would it be any
> simpler to add a single command instead:
> 
> { 'command': 'block-dedup-control',
>   'data': { 'device': 'str', 'enable': 'bool' } }
> 
> where the user calls:
> 
> { "execute": "block-dedup-control",
>   "arguments": { "device": "ide0-hd0", "enable": false } }
> 
> to pause, and "enable":true to resume?

Ok I'll merge these.

Regards

Benoît

Patch

diff --git a/blockdev.c b/blockdev.c
index d724e2d..4c5f954 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -896,6 +896,24 @@  void qmp_block_passwd(const char *device, const char *password, Error **errp)
     }
 }
 
+void qmp_block_pause_dedup(const char *device, Error **errp)
+{
+    BlockDriverState *bs;
+    int err;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    err = bdrv_pause_dedup(bs);
+    if (err == -EINVAL) {
+        error_set(errp, QERR_DEVICE_NOT_DEDUPLICATED, bdrv_get_device_name(bs));
+        return;
+    }
+}
+
 static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
                                     int bdrv_flags, BlockDriver *drv,
                                     const char *password, Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 1a5014c..d8c8348 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -720,6 +720,24 @@ 
 { 'command': 'query-block', 'returns': ['BlockInfo'] }
 
 ##
+# @block-pause-dedup:
+#
+# This command pause the deduplication on a device that support it.
+#
+# @device:   the name of the device to pause the deduplication on
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @device is not deduplicated, DeviceNotDeduplicated
+#
+# Notes:  Not all block formats support deduplication one must use
+#         query-blockstats before and look at the optional deduplication field.
+#
+# Since: 1.5
+##
+{ 'command': 'block-pause-dedup', 'data': {'device': 'str' } }
+
+##
 # @BlockDeviceDedupInfo
 #
 # Statistics of the deduplication on a virtual block device implementing it
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..acc9fd0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1236,6 +1236,29 @@  Example:
 EQMP
 
     {
+        .name       = "block-pause-dedup",
+        .args_type  = "device:B",
+        .mhandler.cmd_new = qmp_marshal_input_block_pause_dedup,
+    },
+
+SQMP
+block-pause-dedup
+------------
+
+Pause the deduplication on a device that support it.
+
+Arguments:
+
+- "device": device name (json-string)
+
+Example:
+
+-> { "execute": "block-pause-dedup", "arguments": { "device": "ide0-hd0" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "block_set_io_throttle",
         .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
         .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,