[Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Supriya Kannery]

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


Currently cache setting of a block device cannot be changed without 
restarting a running VM. Following patchset is for enabling dynamic 
change of cache setting for block devices through qemu monitor. Code 
changes are based on patches from Christoph Hellwig and Prerna Saxena.

Monitor command "info block" is extended to display cache setting of 
block devices. New monitor command "cache_set" is added using which 
cache setting can be dynamically changed
Usage:
"cache_set <device> <cache>"
   <device> = block device      
   <cache> = "off"/"none", "on"/"writeback", "writethrough", "unsafe"
      
 1/3 Enhance "info block" to display cache setting
 2/3 New error classes for file reopen and device insertion
 3/3 Add monitor command "cache_set" for dynamic cache change

 qemu/block.c         |   71
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 qemu/block.h         |    2 +
 qemu/blockdev.c      |   20 +++++++++++++++++++
 qemu/blockdev.h      |    1
 qemu/hmp-commands.hx |   14 +++++++++++++
 qemu/qerror.c        |    8 +++++++
 qemu/qerror.h        |    6 +++++
 qemu/qmp-commands.hx |   28 ++++++++++++++++++++++++++
 8 files changed, 148 insertions(+), 2 deletions(-)

~                                                   

[Qemu-devel] [RFC Patch 1/3]Qemu: Enhance "info block" to display cache setting

[Supriya Kannery]

Enhance "info block" to display cache setting

Example:
(qemu) info block
ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 
encrypted=0

Enhanced to include "cache" setting:
(qemu) info block
ide0-hd0: type=hd removable=0 cache=none file=../rhel6-32.qcow2 ro=0 
drv=qcow2 encrypted=0

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>

---
 block.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1713,6 +1713,19 @@ static void bdrv_print_dict(QObject *obj
         monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
     }
 
+    if (qdict_haskey(bs_dict, "open_flags")) {
+        int open_flags = qdict_get_int(bs_dict, "open_flags");
+        if (open_flags & BDRV_O_NOCACHE) {
+            monitor_printf(mon, " cache=none");
+        } else if (open_flags & BDRV_O_CACHE_WB) {
+            if (open_flags & BDRV_O_NO_FLUSH)
+                monitor_printf(mon, " cache=unsafe");
+            else
+                monitor_printf(mon, " cache=writeback");
+        } else
+            monitor_printf(mon, " cache=writethrough");
+    }
+
     if (qdict_haskey(bs_dict, "inserted")) {
         QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
 
@@ -1762,9 +1775,10 @@ void bdrv_info(Monitor *mon, QObject **r
         }
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
-                                    "'removable': %i, 'locked': %i }",
+                                    "'removable': %i, 'locked': %i, "
+                                    "'open_flags': %d }",
                                     bs->device_name, type, bs->removable,
-                                    bs->locked);
+                                    bs->locked, bs->open_flags);
 
         if (bs->drv) {
             QObject *obj;

Re: [Qemu-devel] [RFC Patch 1/3]Qemu: Enhance "info block" to display cache setting

[Kevin Wolf]

Am 16.05.2011 20:10, schrieb Supriya Kannery:
> Enhance "info block" to display cache setting
> 
> Example:
> (qemu) info block
> ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 
> encrypted=0
> 
> Enhanced to include "cache" setting:
> (qemu) info block
> ide0-hd0: type=hd removable=0 cache=none file=../rhel6-32.qcow2 ro=0 
> drv=qcow2 encrypted=0
> 
> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> 
> ---
>  block.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -1713,6 +1713,19 @@ static void bdrv_print_dict(QObject *obj
>          monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
>      }
>  
> +    if (qdict_haskey(bs_dict, "open_flags")) {
> +        int open_flags = qdict_get_int(bs_dict, "open_flags");
> +        if (open_flags & BDRV_O_NOCACHE) {
> +            monitor_printf(mon, " cache=none");
> +        } else if (open_flags & BDRV_O_CACHE_WB) {
> +            if (open_flags & BDRV_O_NO_FLUSH)
> +                monitor_printf(mon, " cache=unsafe");
> +            else
> +                monitor_printf(mon, " cache=writeback");
> +        } else
> +            monitor_printf(mon, " cache=writethrough");
> +    }
> +
>      if (qdict_haskey(bs_dict, "inserted")) {
>          QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
>  
> @@ -1762,9 +1775,10 @@ void bdrv_info(Monitor *mon, QObject **r
>          }
>  
>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
> -                                    "'removable': %i, 'locked': %i }",
> +                                    "'removable': %i, 'locked': %i, "
> +                                    "'open_flags': %d }",
>                                      bs->device_name, type, bs->removable,
> -                                    bs->locked);
> +                                    bs->locked, bs->open_flags);
>  
>          if (bs->drv) {
>              QObject *obj;

bs->open_flags is a purely internal thing and its meaning is not
guaranteed to be stable. Exposing it to the user is wrong.

Kevin

Re: [Qemu-devel] [RFC Patch 1/3]Qemu: Enhance "info block" to display cache setting

[supriya kannery]

Kevin Wolf wrote:
> Am 16.05.2011 20:10, schrieb Supriya Kannery:
>   
>> Enhance "info block" to display cache setting
>>
>> Example:
>> (qemu) info block
>> ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 
>> encrypted=0
>>
>> Enhanced to include "cache" setting:
>> (qemu) info block
>> ide0-hd0: type=hd removable=0 cache=none file=../rhel6-32.qcow2 ro=0 
>> drv=qcow2 encrypted=0
>>
>>     

>>  
>>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
>> -                                    "'removable': %i, 'locked': %i }",
>> +                                    "'removable': %i, 'locked': %i, "
>> +                                    "'open_flags': %d }",
>>                                      bs->device_name, type, bs->removable,
>> -                                    bs->locked);
>> +                                    bs->locked, bs->open_flags);
>>  
>>          if (bs->drv) {
>>              QObject *obj;
>>     
>
> bs->open_flags is a purely internal thing and its meaning is not
> guaranteed to be stable. Exposing it to the user is wrong.
>
>   
ok. Pls suggest what could a better approach to expose the cache setting.
> Kevin
>
>   

[Qemu-devel] [RFC Patch 2/3]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] [RFC Patch 3/3]Qemu: Add command "cache_set" for dynamic cache change

[Supriya Kannery]

Add monitor command "cache_set" for dynamic cache change

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

---
 block.c         |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    2 ++
 blockdev.c      |   20 ++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   14 ++++++++++++++
 qmp-commands.hx |   28 ++++++++++++++++++++++++++++
 6 files changed, 118 insertions(+)

Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,20 @@ but should be used with extreme caution.
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "cache_set",
+        .args_type  = "device:B,cache:s",
+        .params     = "device cache",
+        .help       = "change cache setting for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_cache_set,
+    },
+
+STEXI
+@item cache_set
+@findex cache_set
+Change 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) {
@@ -3063,3 +3091,28 @@ out:
 
     return ret;
 }
+
+int bdrv_change_cache(BlockDriverState *bs, const char *cache)
+{
+    int bdrv_flags = 0;
+
+    /* Clear cache flags */
+    bdrv_flags = bs->open_flags & ~BDRV_O_CACHE_MASK;
+
+    /* Set flags for requested cache setting */
+    if (strcmp(cache, "writethrough")) {
+      if (!strcmp(cache, "off") || !strcmp(cache, "none")) {
+            bdrv_flags |= BDRV_O_NOCACHE;
+        } else if (!strcmp(cache, "writeback") || !strcmp(cache, "on")) {
+            bdrv_flags |= BDRV_O_CACHE_WB;
+        } else if (!strcmp(cache, "unsafe")) {
+            bdrv_flags |= BDRV_O_CACHE_WB;
+            bdrv_flags |= BDRV_O_NO_FLUSH;
+        } else {
+           error_report("invalid cache option");
+           return -1;
+        }
+    }
+
+    return(bdrv_reopen(bs, bdrv_flags));
+}
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -796,3 +796,23 @@ int do_block_resize(Monitor *mon, const 
 
     return 0;
 }
+
+int do_cache_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *cache = qdict_get_str(qdict, "cache");
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    if(bdrv_is_inserted(bs)) {
+       return(bdrv_change_cache(bs, 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_cache(BlockDriverState *bs, const char *cache);
 
 
 typedef struct BdrvCheckResult {
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -64,5 +64,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_cache_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,34 @@ Example:
 -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } }
 <- { "return": {} }
 
+
+EQMP
+
+    {
+        .name       = "cache_set",
+        .args_type  = "device:B,cache:s",
+        .params     = "device cache",
+        .help       = "change cache setting for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_cache_set,
+    },
+
+SQMP
+cache_set
+---------
+
+Change cache setting while a guest is running.
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- "cache": new cache string like none, writethrough etc.. (json-string)
+
+Example:
+
+-> { "execute": "cache_set", "arguments": { "device": "ide0-hd0", "cache": "writeback" } }
+<- { "return": {} }
+
 EQMP
 
     {

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Christoph Hellwig]

Why are you even trying this again?  As explained very clearly last time you
can't change from a writeback-style to a write-through style I/O from
the monitor without creating massive data integrity problems.  See my
patchset that allows changing this from the guest for how it should be
done - I just need to get back and revisit the virtio protocol support
for it.

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Anthony Liguori]

On 05/16/2011 03:23 PM, Christoph Hellwig wrote:
> Why are you even trying this again?  As explained very clearly last time you
> can't change from a writeback-style to a write-through style I/O from
> the monitor without creating massive data integrity problems.

To further clarify:

Today cache=none|writethrough|writeback does two things.  It:

1) Changes the WCE flag that's visible to the guest

2) Determines whether the host page cache is used for doing guest I/O

As Christoph is very correct in pointing out, we cannot change (1) at 
run time because this is guest visible.  You will break a guest if you 
do this.

But it's still desirable to be able to change (2) at run time.  Before 
we can do this properly though, we need to separate out the logic for 
setting (1) vs. (2).

And ideally, we would allow (1) to be changed by the guest itself at run 
time which allows for full dynamic control.  This is what he's referring 
to below.

Regards,

Anthony Liguori

   See my
> patchset that allows changing this from the guest for how it should be
> done - I just need to get back and revisit the virtio protocol support
> for it.
>
>

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[supriya kannery]

Anthony Liguori wrote:
> On 05/16/2011 03:23 PM, Christoph Hellwig wrote:
>> Why are you even trying this again?  As explained very clearly last 
>> time you
>> can't change from a writeback-style to a write-through style I/O from
>> the monitor without creating massive data integrity problems.
>
> To further clarify:
>
> Today cache=none|writethrough|writeback does two things.  It:
>
> 1) Changes the WCE flag that's visible to the guest
>
> 2) Determines whether the host page cache is used for doing guest I/O
>
> As Christoph is very correct in pointing out, we cannot change (1) at 
> run time because this is guest visible.  You will break a guest if you 
> do this.
>
ok
> But it's still desirable to be able to change (2) at run time.  Before 
> we can do this properly though, we need to separate out the logic for 
> setting (1) vs. (2).
>
Will go through the code in detail to understand handling of (1) and (2).
> And ideally, we would allow (1) to be changed by the guest itself at 
> run time which allows for full dynamic control.  This is what he's 
> referring to below.
>
> Regards,
>
> Anthony Liguori
>
>   See my
>> patchset that allows changing this from the guest for how it should be
>> done - I just need to get back and revisit the virtio protocol support
>> for it.
>>
>>
>
>

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Christoph Hellwig]

On Mon, May 16, 2011 at 04:10:21PM -0500, Anthony Liguori wrote:
> To further clarify:
>
> Today cache=none|writethrough|writeback does two things.  It:
>
> 1) Changes the WCE flag that's visible to the guest
>
> 2) Determines whether the host page cache is used for doing guest I/O
>
> As Christoph is very correct in pointing out, we cannot change (1) at run 
> time because this is guest visible.  You will break a guest if you do this.
>
> But it's still desirable to be able to change (2) at run time.  Before we 
> can do this properly though, we need to separate out the logic for setting 
> (1) vs. (2).
>
> And ideally, we would allow (1) to be changed by the guest itself at run 
> time which allows for full dynamic control.  This is what he's referring to 
> below.

Exactly.  Setting/clearing the BDRV_O_NO_FLUSH also seems useful, maybe
in addition to also allowing an equivalent for the writethrough modes.

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[Supriya Kannery]

On 05/17/2011 09:11 PM, Christoph Hellwig wrote:
> On Mon, May 16, 2011 at 04:10:21PM -0500, Anthony Liguori wrote:
>> To further clarify:
>>
>> Today cache=none|writethrough|writeback does two things.  It:
>>
>> 1) Changes the WCE flag that's visible to the guest
>>
>> 2) Determines whether the host page cache is used for doing guest I/O
>>
>> As Christoph is very correct in pointing out, we cannot change (1) at run
>> time because this is guest visible.  You will break a guest if you do this.
>>
>> But it's still desirable to be able to change (2) at run time.  Before we
>> can do this properly though, we need to separate out the logic for setting
>> (1) vs. (2).
>>
>> And ideally, we would allow (1) to be changed by the guest itself at run
>> time which allows for full dynamic control.  This is what he's referring to
>> below.
>
> Exactly.  Setting/clearing the BDRV_O_NO_FLUSH also seems useful, maybe
> in addition to also allowing an equivalent for the writethrough modes.
>

Posted second version of the patchset (RFC) which supports only 
hostcache setting/clearing from qemu monitor.
http://www.mail-archive.com/qemu-devel@nongnu.org/msg64658.html
Please comment.

Re: [Qemu-devel] [RFC Patch 0/3]Qemu: Enable dynamic cache change through qemu monitor

[supriya kannery]

Christoph Hellwig wrote:
> Why are you even trying this again?  
Enabling control of cache setting from qemu monitor will help 
users/admins to
accomplish cache value change without depending on the guest.
> As explained very clearly last time you
> can't change from a writeback-style to a write-through style I/O from
> the monitor without creating massive data integrity problems.  See my
> patchset that allows changing this from the guest for how it should be
> done - I just need to get back and revisit the virtio protocol support
> for it.
>
>
>   
ok, sure, I will go through your related patches and work further on this.