[Qemu-devel] [V2 0/2]Qemu: Enable dynamic hostcache change through monitor

[Qemu-devel] [V2 0/2]Qemu: Enable dynamic hostcache change through monitor

[Supriya Kannery]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

  Currently host page cache setting for a block device cannot be changed
without restarting a running VM. Following patchset [V2] is for enabling 
dynamic change of host cache setting for devices through qemu monitor.

Changes from patchset V1:
1. Support of dynamic cache change only for hostcache.
2. Monitor command "hostcache_get" added to display current cache setting
3. Backed off the changes for display of cache setting in "info block"

Two new monitor commands added.
1. hostcache_get 
    -- Displays hostcache setting of a block device.
Usage:
"hostcache_get <device>
   <device> = block device

2. hostcache_set
    -- Sets hostcache for the device to specified value.
Usage:
"hostcache_set <device> <cache>"
   <device> = block device      
   <cache> = "on"/"off"

1/2 New error classes for file reopen and device insertion
2/2 Add commands "hostcache_set" and "hostcache_get" 
     
 block.c         |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    2 ++
 blockdev.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    2 ++
 hmp-commands.hx |   29 +++++++++++++++++++++++++++++
 qerror.c        |    8 ++++++++
 qerror.h        |    6 ++++++
 qmp-commands.hx |   55 
 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 198 insertions(+)
~                                                                        
~                                                   
~                                                   

[Qemu-devel] [V2 1/2]Qemu: New error classes for file reopen and device insertion

[Supriya Kannery]

New errors defined for device insertion and file reopen

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

---
 qerror.c |    8 ++++++++
 qerror.h |    6 ++++++
 2 files changed, 14 insertions(+)

Index: qemu/qerror.c
===================================================================
--- qemu.orig/qerror.c
+++ qemu/qerror.c
@@ -93,6 +93,10 @@ static const QErrorStringTable qerror_ta
         .desc      = "Device '%(device)' not found",
     },
     {
+        .error_fmt = QERR_DEVICE_NOT_INSERTED,
+        .desc      = "Device '%(device)' has not been inserted",
+    },
+    {
         .error_fmt = QERR_DEVICE_NOT_REMOVABLE,
         .desc      = "Device '%(device)' is not removable",
     },
@@ -161,6 +165,10 @@ static const QErrorStringTable qerror_ta
         .desc      = "Could not open '%(filename)'",
     },
     {
+        .error_fmt = QERR_REOPEN_FILE_FAILED,
+        .desc      = "Could not reopen '%(filename)'",
+    },
+    {
         .error_fmt = QERR_PROPERTY_NOT_FOUND,
         .desc      = "Property '%(device).%(property)' not found",
     },
Index: qemu/qerror.h
===================================================================
--- qemu.orig/qerror.h
+++ qemu/qerror.h
@@ -84,6 +84,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_DEVICE_NOT_FOUND \
     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NOT_INSERTED \
+    "{ 'class': 'DeviceNotInserted', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
@@ -135,6 +138,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
+#define QERR_REOPEN_FILE_FAILED \
+    "{ 'class': 'ReopenFileFailed', 'data': { 'filename': %s } }"
+
 #define QERR_PROPERTY_NOT_FOUND \
     "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
 

[Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Supriya Kannery]

Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
host cache change and display of host cache setting respectively.

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

---
 block.c         |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    2 ++
 blockdev.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    2 ++
 hmp-commands.hx |   29 +++++++++++++++++++++++++++++
 qmp-commands.hx |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 184 insertions(+)

Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,35 @@ but should be used with extreme caution.
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "hostcache_get",
+        .args_type  = "device:B",
+        .params     = "device",
+        .help       = "retrieve host cache settings for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hostcache_get,
+    },
+
+STEXI
+@item hostcache_get
+@findex hostcache_get
+Display host cache settings for a block device while guest is running.
+ETEXI
+
+    {
+        .name       = "hostcache_set",
+        .args_type  = "device:B,hostcache:s",
+        .params     = "device hostcache",
+        .help       = "change host cache setting for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hostcache_set,
+    },
+
+STEXI
+@item hostcache_set
+@findex hostcache_set
+Change host cache options for a block device while guest is running.
+ETEXI
 
     {
         .name       = "eject",
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -657,6 +657,34 @@ unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret = 0;
+
+    /* No need to reopen as no change in flags */
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    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) {
@@ -3049,3 +3077,23 @@ out:
 
     return ret;
 }
+
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
+{
+    int bdrv_flags = bs->open_flags;
+
+    /* No change in existing hostcache setting */
+    if(!enable_host_cache == (bdrv_flags & BDRV_O_NOCACHE)) {
+        return -1;
+    }
+
+    /* set hostcache flags (without changing WCE/flush bits) */
+    if(!enable_host_cache) {
+        bdrv_flags |= BDRV_O_NOCACHE;
+    } else {
+        bdrv_flags &= ~BDRV_O_NOCACHE;
+    }
+
+    /* Reopen file with changed set of flags */
+    return(bdrv_reopen(bs, bdrv_flags));
+}
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -796,3 +796,51 @@ int do_block_resize(Monitor *mon, const 
 
     return 0;
 }
+
+int do_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    monitor_printf(mon, "%s:", device);
+
+    if (bs->open_flags & BDRV_O_NOCACHE) {
+        monitor_printf(mon, " hostcache=off\n");
+    } else {
+        monitor_printf(mon, " hostcache=on\n");
+    }
+
+    return 0;
+}
+
+int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *hostcache = qdict_get_str(qdict, "hostcache");
+    BlockDriverState *bs;
+    bool enable_host_cache = 0;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    /* cache change applicable only if device inserted */
+    if (bdrv_is_inserted(bs)) {
+        if (!strcmp(hostcache,"on"))
+            enable_host_cache = 1;
+        else
+            enable_host_cache = 0;
+        return(bdrv_change_hostcache(bs, enable_host_cache));
+    } else {
+       qerror_report(QERR_DEVICE_NOT_INSERTED, device);
+       return -1;
+    }
+}
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.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -64,5 +64,7 @@ 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_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -664,6 +664,61 @@ Example:
 -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } }
 <- { "return": {} }
 
+
+EQMP
+
+    {
+        .name       = "hostcache_get",
+        .args_type  = "device:B",
+        .params     = "device",
+        .help       = "retrieve host cache settings for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hostcache_get,
+    },
+SQMP
+cache_get
+---------
+
+Retrieve host page cache setting while a guest is running.
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+
+Example:
+
+-> { "execute": "hostcache_set", "arguments": { "device": "ide0-hd0" } }
+<- { "return": {} }
+
+
+EQMP
+
+    {
+        .name       = "hostcache_set",
+        .args_type  = "device:B,cache:s",
+        .params     = "device cache",
+        .help       = "change hostcache setting for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_hostcache_set,
+    },
+
+SQMP
+hostcache_set
+-------------
+
+Change host page cache setting while a guest is running.
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- "cache": host cache value "off" or "on" (json-string)
+
+Example:
+
+-> { "execute": "hostcache_set", "arguments": { "device": "ide0-hd0", "cache": "on" } }
+<- { "return": {} }
+
+
 EQMP
 
     {

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Stefan Hajnoczi]

On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
> host cache change and display of host cache setting respectively.

A generic command for changing block device options would be nice,
althought I don't see other options where it makes sense to change them
at runtime.

The alternative would be:

block_set hostcache on

"block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}

The hostcache_get information would be part of query-block output:
         {
            "device":"ide0-hd0",
            "locked":false,
            "removable":false,
            "inserted":{
               "ro":false,
               "drv":"qcow2",
               "encrypted":false,
               "file":"disks/test.img"
	       "hostcache":true,
            },
            "type":"hd"
         },

This approach is extensible if more options need to be exposed.

> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
> 
> ---
>  block.c         |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h         |    2 ++
>  blockdev.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    2 ++
>  hmp-commands.hx |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 184 insertions(+)
> 
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -70,6 +70,35 @@ but should be used with extreme caution.
>  resizes image files, it can not resize block devices like LVM volumes.
>  ETEXI
>  
> +    {
> +        .name       = "hostcache_get",
> +        .args_type  = "device:B",
> +        .params     = "device",
> +        .help       = "retrieve host cache settings for device",

Please make it clear these operations affect block devices:
"for block device"

> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_get,
> +    },
> +
> +STEXI
> +@item hostcache_get
> +@findex hostcache_get
> +Display host cache settings for a block device while guest is running.
> +ETEXI
> +
> +    {
> +        .name       = "hostcache_set",
> +        .args_type  = "device:B,hostcache:s",
> +        .params     = "device hostcache",
> +        .help       = "change host cache setting for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_set,
> +    },
> +
> +STEXI
> +@item hostcache_set
> +@findex hostcache_set
> +Change host cache options for a block device while guest is running.
> +ETEXI
>  
>      {
>          .name       = "eject",
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -657,6 +657,34 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +
> +    /* No need to reopen as no change in flags */
> +    if (bdrv_flags == bs->open_flags) {
> +        return 0;
> +    }
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +
> +    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();

The error is never reported on a QMP monitor because qerror_report()
simply stashes away the qerror.  The QMP client doesn't have a chance to
read the error before QEMU terminates.

> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -3049,3 +3077,23 @@ out:
>  
>      return ret;
>  }
> +
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)

Consistently using "hostcache" or "host_cache" would be nice.

> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* No change in existing hostcache setting */
> +    if(!enable_host_cache == (bdrv_flags & BDRV_O_NOCACHE)) {

This expression doesn't work as expected.  bool has a lower rank than
int.  That means !enable_host_cache is converted to an int and compared
against bdrv_flags & BDRV_O_NOCACHE.  This expression is always false
because a bool is 0 or 1 and BDRV_O_NOCACHE is 0x0020.

> +        return -1;

This shouldn't be a failure and please don't use -1 when a negative
errno indicates failure.  -1 == -EPERM.  The return value should be 0
here.

> +    }

Anyway, this whole check is unnecessary since bdrv_reopen() already
performs it.

> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if(!enable_host_cache) {
> +        bdrv_flags |= BDRV_O_NOCACHE;
> +    } else {
> +        bdrv_flags &= ~BDRV_O_NOCACHE;
> +    }
> +
> +    /* Reopen file with changed set of flags */
> +    return(bdrv_reopen(bs, bdrv_flags));

Please run scripts/checkpatch.pl before submitting patches.

> +}
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -796,3 +796,51 @@ int do_block_resize(Monitor *mon, const 
>  
>      return 0;
>  }
> +
> +int do_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    monitor_printf(mon, "%s:", device);
> +
> +    if (bs->open_flags & BDRV_O_NOCACHE) {
> +        monitor_printf(mon, " hostcache=off\n");
> +    } else {
> +        monitor_printf(mon, " hostcache=on\n");
> +    }
> +
> +    return 0;
> +}
> +
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *hostcache = qdict_get_str(qdict, "hostcache");
> +    BlockDriverState *bs;
> +    bool enable_host_cache = 0;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    /* cache change applicable only if device inserted */
> +    if (bdrv_is_inserted(bs)) {
> +        if (!strcmp(hostcache,"on"))
> +            enable_host_cache = 1;
> +        else
> +            enable_host_cache = 0;

Coding style, please use {} or remove the if:
enable_host_cache = !strcmp(hostcache, "on");

> +        return(bdrv_change_hostcache(bs, enable_host_cache));
> +    } else {
> +       qerror_report(QERR_DEVICE_NOT_INSERTED, device);
> +       return -1;
> +    }
> +}
> 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.h
> ===================================================================
> --- qemu.orig/blockdev.h
> +++ qemu/blockdev.h
> @@ -64,5 +64,7 @@ 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_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #endif
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -664,6 +664,61 @@ Example:
>  -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } }
>  <- { "return": {} }
>  
> +
> +EQMP
> +
> +    {
> +        .name       = "hostcache_get",
> +        .args_type  = "device:B",
> +        .params     = "device",
> +        .help       = "retrieve host cache settings for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_get,
> +    },
> +SQMP
> +cache_get

hostcache_get

> +---------
> +
> +Retrieve host page cache setting while a guest is running.

Please make it clear that this is an operation on block devices.

> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +
> +Example:
> +
> +-> { "execute": "hostcache_set", "arguments": { "device": "ide0-hd0" } }
> +<- { "return": {} }
> +
> +
> +EQMP
> +
> +    {
> +        .name       = "hostcache_set",
> +        .args_type  = "device:B,cache:s",
> +        .params     = "device cache",
> +        .help       = "change hostcache setting for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_set,
> +    },
> +
> +SQMP
> +hostcache_set
> +-------------
> +
> +Change host page cache setting while a guest is running.
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +- "cache": host cache value "off" or "on" (json-string)

There is a boolean value that can be used instead of string on|off.  See
the set_link command.

Stefan

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Supriya Kannery]

On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>> host cache change and display of host cache setting respectively.
>
> A generic command for changing block device options would be nice,
> althought I don't see other options where it makes sense to change them
> at runtime.
>
> The alternative would be:
>
> block_set hostcache on
>
> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>
> The hostcache_get information would be part of query-block output:
>           {
>              "device":"ide0-hd0",
>              "locked":false,
>              "removable":false,
>              "inserted":{
>                 "ro":false,
>                 "drv":"qcow2",
>                 "encrypted":false,
>                 "file":"disks/test.img"
> 	       "hostcache":true,
>              },
>              "type":"hd"
>           },
>
> This approach is extensible if more options need to be exposed.

Sure, I will resubmit this patchset, after making this feature more 
generic. Can you pls help finding atleast one or two options (other than 
hostcache) which can be changed dynamically. This will help me evaluate 
the generic approach.

>> +        .params     = "device",
>> +        .help       = "retrieve host cache settings for device",
>
> Please make it clear these operations affect block devices:
> "for block device"

ok

>
>> +    /*
>> +     * A failed attempt to reopen the image file must lead to 'abort()'
>> +     */
>> +    if (ret != 0) {
>> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +        abort();
>
> The error is never reported on a QMP monitor because qerror_report()
> simply stashes away the qerror.  The QMP client doesn't have a chance to
> read the error before QEMU terminates.
>

Verified this from qemu command line and the error got displayed before 
aborting (when the image file was renamed while VM was running).

>> +
>> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
>
> Consistently using "hostcache" or "host_cache" would be nice.
>

ok

>
>> +        return -1;
>
> This shouldn't be a failure and please don't use -1 when a negative
> errno indicates failure.  -1 == -EPERM.  The return value should be 0
> here.
>
>> +    }
>
> Anyway, this whole check is unnecessary since bdrv_reopen() already
> performs it.
>

will take this off

> Please run scripts/checkpatch.pl before submitting patches.
>

ok..will do

>> +
>> +Arguments:
>> +
>> +- "device": the device's ID, must be unique (json-string)
>> +- "cache": host cache value "off" or "on" (json-string)
>
> There is a boolean value that can be used instead of string on|off.  See
> the set_link command.
>

ok

> Stefan
>

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Stefan Hajnoczi]

On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>
>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>
>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>> host cache change and display of host cache setting respectively.
>>
>> A generic command for changing block device options would be nice,
>> althought I don't see other options where it makes sense to change them
>> at runtime.
>>
>> The alternative would be:
>>
>> block_set hostcache on
>>
>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>
>> The hostcache_get information would be part of query-block output:
>>          {
>>             "device":"ide0-hd0",
>>             "locked":false,
>>             "removable":false,
>>             "inserted":{
>>                "ro":false,
>>                "drv":"qcow2",
>>                "encrypted":false,
>>                "file":"disks/test.img"
>>               "hostcache":true,
>>             },
>>             "type":"hd"
>>          },
>>
>> This approach is extensible if more options need to be exposed.
>
> Sure, I will resubmit this patchset, after making this feature more generic.
> Can you pls help finding atleast one or two options (other than hostcache)
> which can be changed dynamically. This will help me evaluate the generic
> approach.

Hang on, let's see if we can get agreement from Kevin and others
before taking this approach.  Like I said, I don't see other options
that should be changed at runtime.

>
>>> +        .params     = "device",
>>> +        .help       = "retrieve host cache settings for device",
>>
>> Please make it clear these operations affect block devices:
>> "for block device"
>
> ok
>
>>
>>> +    /*
>>> +     * A failed attempt to reopen the image file must lead to 'abort()'
>>> +     */
>>> +    if (ret != 0) {
>>> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>>> +        abort();
>>
>> The error is never reported on a QMP monitor because qerror_report()
>> simply stashes away the qerror.  The QMP client doesn't have a chance to
>> read the error before QEMU terminates.
>>
>
> Verified this from qemu command line and the error got displayed before
> aborting (when the image file was renamed while VM was running).

QMP takes a different code path from the human monitor (HMP) that you
used; see monitor_cur_is_qmp() and how it affects the qerror_report()
path.  CCing Luiz to see whether there is a strategy that we should
use here to ensure the error message does get delivered to the QMP
client before exiting.

Stefan

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Kevin Wolf]

Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>
>>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>>
>>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>>> host cache change and display of host cache setting respectively.
>>>
>>> A generic command for changing block device options would be nice,
>>> althought I don't see other options where it makes sense to change them
>>> at runtime.
>>>
>>> The alternative would be:
>>>
>>> block_set hostcache on
>>>
>>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>>
>>> The hostcache_get information would be part of query-block output:
>>>          {
>>>             "device":"ide0-hd0",
>>>             "locked":false,
>>>             "removable":false,
>>>             "inserted":{
>>>                "ro":false,
>>>                "drv":"qcow2",
>>>                "encrypted":false,
>>>                "file":"disks/test.img"
>>>               "hostcache":true,
>>>             },
>>>             "type":"hd"
>>>          },
>>>
>>> This approach is extensible if more options need to be exposed.
>>
>> Sure, I will resubmit this patchset, after making this feature more generic.
>> Can you pls help finding atleast one or two options (other than hostcache)
>> which can be changed dynamically. This will help me evaluate the generic
>> approach.
> 
> Hang on, let's see if we can get agreement from Kevin and others
> before taking this approach.  Like I said, I don't see other options
> that should be changed at runtime.

Things like enabling copy on read could fit here.

Generally I'm in favour of having a generic command. We just need to pay
attention not to include things that we don't want to maintain long
term, i.e. just putting the current cache=... parameter into the
argument isn't going to work. Maybe two booleans 'o_direct' and
'ignore_flushes' is what we want to have. The same structure should be
used for blkdev_add then, even though it will allow some more options.

I'm also not completely sure how you would enable cache=writethrough
from the command line in a fully converted world. Would this be one of
the arguments that must be specified on BlockDriverState creation and
can't be changed later, and the device will pick it up from there? Or is
it a qdev property and somehow makes it way to the block layer?

Kevin

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Stefan Hajnoczi]

On Mon, May 23, 2011 at 1:58 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
>>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>>
>>>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>>>
>>>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>>>> host cache change and display of host cache setting respectively.
>>>>
>>>> A generic command for changing block device options would be nice,
>>>> althought I don't see other options where it makes sense to change them
>>>> at runtime.
>>>>
>>>> The alternative would be:
>>>>
>>>> block_set hostcache on
>>>>
>>>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>>>
>>>> The hostcache_get information would be part of query-block output:
>>>>          {
>>>>             "device":"ide0-hd0",
>>>>             "locked":false,
>>>>             "removable":false,
>>>>             "inserted":{
>>>>                "ro":false,
>>>>                "drv":"qcow2",
>>>>                "encrypted":false,
>>>>                "file":"disks/test.img"
>>>>               "hostcache":true,
>>>>             },
>>>>             "type":"hd"
>>>>          },
>>>>
>>>> This approach is extensible if more options need to be exposed.
>>>
>>> Sure, I will resubmit this patchset, after making this feature more generic.
>>> Can you pls help finding atleast one or two options (other than hostcache)
>>> which can be changed dynamically. This will help me evaluate the generic
>>> approach.
>>
>> Hang on, let's see if we can get agreement from Kevin and others
>> before taking this approach.  Like I said, I don't see other options
>> that should be changed at runtime.
>
> Things like enabling copy on read could fit here.
>
> Generally I'm in favour of having a generic command. We just need to pay
> attention not to include things that we don't want to maintain long
> term, i.e. just putting the current cache=... parameter into the
> argument isn't going to work. Maybe two booleans 'o_direct' and
> 'ignore_flushes' is what we want to have. The same structure should be
> used for blkdev_add then, even though it will allow some more options.

Supriya, it sounds to me like the generic block_set command and
query-block integration is an acceptable approach.

Stefan

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
>>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>>
>>>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>>>
>>>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>>>> host cache change and display of host cache setting respectively.
>>>>
>>>> A generic command for changing block device options would be nice,
>>>> althought I don't see other options where it makes sense to change them
>>>> at runtime.
>>>>
>>>> The alternative would be:
>>>>
>>>> block_set hostcache on
>>>>
>>>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>>>
>>>> The hostcache_get information would be part of query-block output:
>>>>          {
>>>>             "device":"ide0-hd0",
>>>>             "locked":false,
>>>>             "removable":false,
>>>>             "inserted":{
>>>>                "ro":false,
>>>>                "drv":"qcow2",
>>>>                "encrypted":false,
>>>>                "file":"disks/test.img"
>>>>               "hostcache":true,
>>>>             },
>>>>             "type":"hd"
>>>>          },
>>>>
>>>> This approach is extensible if more options need to be exposed.
>>>
>>> Sure, I will resubmit this patchset, after making this feature more generic.
>>> Can you pls help finding atleast one or two options (other than hostcache)
>>> which can be changed dynamically. This will help me evaluate the generic
>>> approach.
>> 
>> Hang on, let's see if we can get agreement from Kevin and others
>> before taking this approach.  Like I said, I don't see other options
>> that should be changed at runtime.
>
> Things like enabling copy on read could fit here.
>
> Generally I'm in favour of having a generic command. We just need to pay
> attention not to include things that we don't want to maintain long
> term, i.e. just putting the current cache=... parameter into the
> argument isn't going to work. Maybe two booleans 'o_direct' and
> 'ignore_flushes' is what we want to have. The same structure should be
> used for blkdev_add then, even though it will allow some more options.
>
> I'm also not completely sure how you would enable cache=writethrough
> from the command line in a fully converted world. Would this be one of
> the arguments that must be specified on BlockDriverState creation and
> can't be changed later, and the device will pick it up from there? Or is
> it a qdev property and somehow makes it way to the block layer?

qdev properties are the configurable bits of the device's guest part.
For something to be a qdev property, it must belong to the guest part,
and it must be configurable.

Example: a drive serial number belongs to the guest part.  It's
guest-visible.  The host part doesn't care about it; in fact, if you
unplug the device, you could reuse the same host part with another guest
part with different serial number.

Example: the image format doesn't belong to the guest part.  It's not
guest-visible.  Even the device model code is (should be!) ignorant of
it.

Device guest part reconfiguration at run-time is done by the guest OS,
just like for non-virtual devices.  This may make the device model call
out to the block layer to adjust settings there.

Except for configuration knobs that match physical knobs (media eject
and insert, for instance).  Those belong into the monitor.

So, if your configuration knob can be manipulated at run time from the
monitor, and it doesn't correspond to a physical knob, then it
*probably* belongs to the host part, and its setting isn't
guest-visible.

Let's assume we're indeed talking about reconfiguring the host part.  A
generic command for that makes sense to me.  Could look like
"blockdev_set ID NAME=VAL,..." in the human monitor.  The NAME=VAL
should be consistent with blockdev_add whenever possible.  Requires a
crystal ball until we actually get blockdev_add.  If we're afraid of
getting it wrong, we could do a drive_set now.

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Kevin Wolf]

Am 23.05.2011 18:01, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
>>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
>>>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>>>
>>>>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>>>>
>>>>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>>>>> host cache change and display of host cache setting respectively.
>>>>>
>>>>> A generic command for changing block device options would be nice,
>>>>> althought I don't see other options where it makes sense to change them
>>>>> at runtime.
>>>>>
>>>>> The alternative would be:
>>>>>
>>>>> block_set hostcache on
>>>>>
>>>>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>>>>
>>>>> The hostcache_get information would be part of query-block output:
>>>>>          {
>>>>>             "device":"ide0-hd0",
>>>>>             "locked":false,
>>>>>             "removable":false,
>>>>>             "inserted":{
>>>>>                "ro":false,
>>>>>                "drv":"qcow2",
>>>>>                "encrypted":false,
>>>>>                "file":"disks/test.img"
>>>>>               "hostcache":true,
>>>>>             },
>>>>>             "type":"hd"
>>>>>          },
>>>>>
>>>>> This approach is extensible if more options need to be exposed.
>>>>
>>>> Sure, I will resubmit this patchset, after making this feature more generic.
>>>> Can you pls help finding atleast one or two options (other than hostcache)
>>>> which can be changed dynamically. This will help me evaluate the generic
>>>> approach.
>>>
>>> Hang on, let's see if we can get agreement from Kevin and others
>>> before taking this approach.  Like I said, I don't see other options
>>> that should be changed at runtime.
>>
>> Things like enabling copy on read could fit here.
>>
>> Generally I'm in favour of having a generic command. We just need to pay
>> attention not to include things that we don't want to maintain long
>> term, i.e. just putting the current cache=... parameter into the
>> argument isn't going to work. Maybe two booleans 'o_direct' and
>> 'ignore_flushes' is what we want to have. The same structure should be
>> used for blkdev_add then, even though it will allow some more options.
>>
>> I'm also not completely sure how you would enable cache=writethrough
>> from the command line in a fully converted world. Would this be one of
>> the arguments that must be specified on BlockDriverState creation and
>> can't be changed later, and the device will pick it up from there? Or is
>> it a qdev property and somehow makes it way to the block layer?
> 
> qdev properties are the configurable bits of the device's guest part.
> For something to be a qdev property, it must belong to the guest part,
> and it must be configurable.
> 
> Example: a drive serial number belongs to the guest part.  It's
> guest-visible.  The host part doesn't care about it; in fact, if you
> unplug the device, you could reuse the same host part with another guest
> part with different serial number.
> 
> Example: the image format doesn't belong to the guest part.  It's not
> guest-visible.  Even the device model code is (should be!) ignorant of
> it.
> 
> Device guest part reconfiguration at run-time is done by the guest OS,
> just like for non-virtual devices.  This may make the device model call
> out to the block layer to adjust settings there.
> 
> Except for configuration knobs that match physical knobs (media eject
> and insert, for instance).  Those belong into the monitor.

Thanks Markus. I think I'm well aware what a guest and what a host is.
The trouble begins when there's a connection between both...

Let me try to list the facts with cache=writethrough:

* It is currently considered host state (part of bs->open_flags). It
  results in the image file being opened with O_SYNC.

* It is guest visible as something like a WCE bit (Write Cache Enable).
  The guest is allowed to toggle this bit on real hardware and we want
  to add this functionality, too.

* We need a way to configure it from the start. Currently done by
  cache=writethrough, but that is host state even though it's guest
  visible. That would be an argument for moving it into qdev.

* You're supposed to use blockdev_add first and then device_add. This
  means that we open the image file before the device and its qdev
  config exists. So the first thing the device must do is to reopen the
  image? What to do with the fd: protocol?

So, treating it as host state is wrong. Treating it as guest state
doesn't work really well either.

> So, if your configuration knob can be manipulated at run time from the
> monitor, and it doesn't correspond to a physical knob, then it
> *probably* belongs to the host part, and its setting isn't
> guest-visible.
> 
> Let's assume we're indeed talking about reconfiguring the host part.  A
> generic command for that makes sense to me.  Could look like
> "blockdev_set ID NAME=VAL,..." in the human monitor.  The NAME=VAL
> should be consistent with blockdev_add whenever possible.  Requires a
> crystal ball until we actually get blockdev_add.  If we're afraid of
> getting it wrong, we could do a drive_set now.

If we're always afraid of getting it wrong and try to evade, we'll never
get blockdev_add.

Kevin

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.05.2011 18:01, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
>>>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supriyak@in.ibm.com> wrote:
>>>>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>>>>
>>>>>> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
>>>>>>>
>>>>>>> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
>>>>>>> host cache change and display of host cache setting respectively.
>>>>>>
>>>>>> A generic command for changing block device options would be nice,
>>>>>> althought I don't see other options where it makes sense to change them
>>>>>> at runtime.
>>>>>>
>>>>>> The alternative would be:
>>>>>>
>>>>>> block_set hostcache on
>>>>>>
>>>>>> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>>>>>>
>>>>>> The hostcache_get information would be part of query-block output:
>>>>>>          {
>>>>>>             "device":"ide0-hd0",
>>>>>>             "locked":false,
>>>>>>             "removable":false,
>>>>>>             "inserted":{
>>>>>>                "ro":false,
>>>>>>                "drv":"qcow2",
>>>>>>                "encrypted":false,
>>>>>>                "file":"disks/test.img"
>>>>>>               "hostcache":true,
>>>>>>             },
>>>>>>             "type":"hd"
>>>>>>          },
>>>>>>
>>>>>> This approach is extensible if more options need to be exposed.
>>>>>
>>>>> Sure, I will resubmit this patchset, after making this feature more generic.
>>>>> Can you pls help finding atleast one or two options (other than hostcache)
>>>>> which can be changed dynamically. This will help me evaluate the generic
>>>>> approach.
>>>>
>>>> Hang on, let's see if we can get agreement from Kevin and others
>>>> before taking this approach.  Like I said, I don't see other options
>>>> that should be changed at runtime.
>>>
>>> Things like enabling copy on read could fit here.
>>>
>>> Generally I'm in favour of having a generic command. We just need to pay
>>> attention not to include things that we don't want to maintain long
>>> term, i.e. just putting the current cache=... parameter into the
>>> argument isn't going to work. Maybe two booleans 'o_direct' and
>>> 'ignore_flushes' is what we want to have. The same structure should be
>>> used for blkdev_add then, even though it will allow some more options.
>>>
>>> I'm also not completely sure how you would enable cache=writethrough
>>> from the command line in a fully converted world. Would this be one of
>>> the arguments that must be specified on BlockDriverState creation and
>>> can't be changed later, and the device will pick it up from there? Or is
>>> it a qdev property and somehow makes it way to the block layer?
>> 
>> qdev properties are the configurable bits of the device's guest part.
>> For something to be a qdev property, it must belong to the guest part,
>> and it must be configurable.
>> 
>> Example: a drive serial number belongs to the guest part.  It's
>> guest-visible.  The host part doesn't care about it; in fact, if you
>> unplug the device, you could reuse the same host part with another guest
>> part with different serial number.
>> 
>> Example: the image format doesn't belong to the guest part.  It's not
>> guest-visible.  Even the device model code is (should be!) ignorant of
>> it.
>> 
>> Device guest part reconfiguration at run-time is done by the guest OS,
>> just like for non-virtual devices.  This may make the device model call
>> out to the block layer to adjust settings there.
>> 
>> Except for configuration knobs that match physical knobs (media eject
>> and insert, for instance).  Those belong into the monitor.
>
> Thanks Markus. I think I'm well aware what a guest and what a host is.
> The trouble begins when there's a connection between both...
>
> Let me try to list the facts with cache=writethrough:
>
> * It is currently considered host state (part of bs->open_flags). It
>   results in the image file being opened with O_SYNC.
>
> * It is guest visible as something like a WCE bit (Write Cache Enable).
>   The guest is allowed to toggle this bit on real hardware and we want
>   to add this functionality, too.
>
> * We need a way to configure it from the start. Currently done by
>   cache=writethrough, but that is host state even though it's guest
>   visible. That would be an argument for moving it into qdev.
>
> * You're supposed to use blockdev_add first and then device_add. This
>   means that we open the image file before the device and its qdev
>   config exists. So the first thing the device must do is to reopen the
>   image? What to do with the fd: protocol?
>
> So, treating it as host state is wrong. Treating it as guest state
> doesn't work really well either.

Similar to read-only, except for the "guest can change it" bit.

For read-only, we concluded that it belongs to both guest and host part.
Connecting a read/write guest part to a read-only host part is not
allowed.  For convenience, the guest part's read-only bit could default
to match the host part's.

Can we solve the WCE bit problem similarly?

>> So, if your configuration knob can be manipulated at run time from the
>> monitor, and it doesn't correspond to a physical knob, then it
>> *probably* belongs to the host part, and its setting isn't
>> guest-visible.
>> 
>> Let's assume we're indeed talking about reconfiguring the host part.  A
>> generic command for that makes sense to me.  Could look like
>> "blockdev_set ID NAME=VAL,..." in the human monitor.  The NAME=VAL
>> should be consistent with blockdev_add whenever possible.  Requires a
>> crystal ball until we actually get blockdev_add.  If we're afraid of
>> getting it wrong, we could do a drive_set now.
>
> If we're always afraid of getting it wrong and try to evade, we'll never
> get blockdev_add.

Yes.  But it's advisable to start with blockdev_add rather than with
blockdev_set.