Patchwork [V4,3/4] Qemu: Command "block_set" for dynamic block params change

login
register
mail settings
Submitter Supriya Kannery
Date July 4, 2011, 10:43 a.m.
Message ID <20110704104320.28170.29248.sendpatchset@skannery>
Download mbox | patch
Permalink /patch/103086/
State New
Headers show

Comments

Supriya Kannery - July 4, 2011, 10:43 a.m.
New command "block_set" added for dynamically changing any of the block
device parameters. For now, dynamic setting of hostcache params using this
command is implemented. Other block device parameters, can be integrated
in similar lines.

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

---
 block.c         |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    2 ++
 blockdev.c      |   26 ++++++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   15 +++++++++++++++
 qmp-commands.hx |   28 ++++++++++++++++++++++++++++
 6 files changed, 124 insertions(+)
Stefan Hajnoczi - July 4, 2011, 12:29 p.m.
On Mon, Jul 4, 2011 at 11:43 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +/*
> + * Handle changes to block device settings, like hostcache,
> + * while guest is running.
> +*/
> +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *name = qdict_get_str(qdict, "name");
> +    int enable = qdict_get_bool(qdict, "enable");
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    if (!strcmp(name, "hostcache")) {
> +        return bdrv_change_hostcache(bs, enable);
> +    }
> +
> +    return 0;

No error for unknown "name" argument?

>  #endif
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -70,6 +70,21 @@ but should be used with extreme caution.
>  resizes image files, it can not resize block devices like LVM volumes.
>  ETEXI
>
> +    {
> +        .name       = "block_set",
> +        .args_type  = "device:B,name:s,enable:b",
> +        .params     = "device name enable",
> +        .help       = "On/Off block device parameters like hostcache",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_block_set,
> +    },
> +
> +STEXI
> +@item block_set
> +@findex block_set
> +Change block device params (eg:"hostcache"=on/off) while guest is running.
> +ETEXI

See QMP comments below.

> +
>
>     {
>         .name       = "eject",
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -694,6 +694,34 @@ Example:
>  EQMP
>
>     {
> +        .name       = "block_set",
> +        .args_type  = "device:B,name:s,enable:b",
> +        .params     = "device name enable",

Perhaps:

.args_type  = "device:B,name:s,enable:b?",
.params     = "device name [enable]",

But I am not sure what the best way to express this is.

> +        .help       = "Enable/Disable block device params like hostcache",

Arguments (like "enable") should depend on the block parameter that is
being set:

.help = "Set block device parameter"

If there is no good way to support different optional arguments and
types then a json-string "value" argument would be best.

> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_block_set,
> +    },
> +
> +SQMP
> +block_set
> +---------
> +
> +Change various block device parameters like hostcache.
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +- "name": name of the parameter to be changed" (json-string)

Trailing '"'.

> +- "enable": value to be set for the parameter, 'true' or 'false' (json-bool)

The relevant arguments depend on which block parameter you are
changing.  I think "enable" should be documented specifically for
"hostcache":

Block parameters:

- "hostcache": Enable/disable host buffer cache
    - "enable": 'true' or 'false' (json-bool)

Stefan
Stefan Hajnoczi - July 4, 2011, 12:35 p.m.
On Mon, Jul 4, 2011 at 11:43 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    if (bdrv_flush(bs)) {
> +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
> +        return ret;
> +    }
> +    bdrv_close(bs);
> +
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +
> +    /*
> +     * A failed attempt to reopen the image file must lead to 'abort()'
> +    */
> +    if (ret != 0) {
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +        abort();

I think it is still worth trying to reopen with the old flags.  If you
specialy hostcache=off for a file on tmpfs then the open will fail
(tmpfs doesn't support O_DIRECT), but we can still save ourselves by
reopening with the old flags.

Ideally we'd probably treat this condition just like ENOSPC and keep
the VM paused until the administrator has taken action, and then
retry.

BTW qerror_report() followed by abort(3) does not actually report the
error because we exit before QMP has a chance to send out the error.

Stefan
supriya kannery - July 5, 2011, 10:55 a.m.
On 07/04/2011 05:59 PM, Stefan Hajnoczi wrote:
> On Mon, Jul 4, 2011 at 11:43 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>>
>>      {
>> +        .name       = "block_set",
>> +        .args_type  = "device:B,name:s,enable:b",
>> +        .params     = "device name enable",
>
> Perhaps:
>
> .args_type  = "device:B,name:s,enable:b?",
> .params     = "device name [enable]",
>
> But I am not sure what the best way to express this is.
>
>> +        .help       = "Enable/Disable block device params like hostcache",
>
> Arguments (like "enable") should depend on the block parameter that is
> being set:
>
> .help = "Set block device parameter"
>
> If there is no good way to support different optional arguments and
> types then a json-string "value" argument would be best.
>

"device_add" is defined and implemented to handle multiple types of
optional arguments. Will work on to see whether that approach
can be adopted for block_set
     {
         .name       = "device_add",
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = do_device_add,
     },



>
> Stefan

Patch

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,32 @@  unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret = 0;
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    if (bdrv_flush(bs)) {
+        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
+        return ret;
+    }
+    bdrv_close(bs);
+
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+    */
+    if (ret != 0) {
+        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+        abort();
+    }
+
+    return ret;
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     if (bs->drv) {
@@ -691,6 +717,32 @@  void bdrv_close_all(void)
     }
 }
 
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
+{
+    int bdrv_flags = bs->open_flags;
+
+    /* set hostcache flags (without changing WCE/flush bits) */
+    if (enable_host_cache) {
+        bdrv_flags &= ~BDRV_O_NOCACHE;
+    } else {
+        bdrv_flags |= BDRV_O_NOCACHE;
+    }
+
+    /* If no change in flags, no need to reopen */
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    if (bdrv_is_inserted(bs)) {
+        /* Reopen file with changed set of flags */
+        return bdrv_reopen(bs, bdrv_flags);
+    } else {
+        /* Save hostcache change for future use */
+        bs->open_flags = bdrv_flags;
+        return 0;
+    }
+}
+
 /* make a BlockDriverState anonymous by removing from bdrv_state list.
    Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -71,6 +71,7 @@  void bdrv_delete(BlockDriverState *bs);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_attach(BlockDriverState *bs, DeviceState *qdev);
 void bdrv_detach(BlockDriverState *bs, DeviceState *qdev);
@@ -96,6 +97,7 @@  void bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache);
 
 
 typedef struct BdrvCheckResult {
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -797,3 +797,29 @@  int do_block_resize(Monitor *mon, const 
 
     return 0;
 }
+
+
+/*
+ * Handle changes to block device settings, like hostcache,
+ * while guest is running.
+*/
+int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    int enable = qdict_get_bool(qdict, "enable");
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    if (!strcmp(name, "hostcache")) {
+        return bdrv_change_hostcache(bs, enable);
+    }
+
+    return 0;
+}
+
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -65,5 +65,6 @@  int do_change_block(Monitor *mon, const 
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,21 @@  but should be used with extreme caution.
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "block_set",
+        .args_type  = "device:B,name:s,enable:b",
+        .params     = "device name enable",
+        .help       = "On/Off block device parameters like hostcache",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set,
+    },
+
+STEXI
+@item block_set
+@findex block_set
+Change block device params (eg:"hostcache"=on/off) while guest is running.
+ETEXI
+
 
     {
         .name       = "eject",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -694,6 +694,34 @@  Example:
 EQMP
 
     {
+        .name       = "block_set",
+        .args_type  = "device:B,name:s,enable:b",
+        .params     = "device name enable",
+        .help       = "Enable/Disable block device params like hostcache",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set,
+    },
+
+SQMP
+block_set
+---------
+
+Change various block device parameters like hostcache.
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- "name": name of the parameter to be changed" (json-string)
+- "enable": value to be set for the parameter, 'true' or 'false' (json-bool)
+
+Example:
+
+-> { "execute": "block_set", "arguments": { "device": "ide0-hd0", "name": "hostcache", "enable": true } }
+<- { "return": {} }
+
+EQMP
+
+	{
         .name       = "balloon",
         .args_type  = "value:M",
         .params     = "target",