qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
@ 2011-10-30 10:33 Supriya Kannery
  2011-10-30 10:33 ` [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

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.

This patchset introduces 
a. monitor command 'block_set_hostcache' using which host 
   pagecache setting for a block device can be changed 
   dynamically. 
b. a new option for setting host cache from qemu
   commandline option -drive  "hostcache=on/off".
c. BDRVReopenState, a generic structure which can be 
   extended by each of the block drivers to reopen 
   respective image files.
   Extension of this structure for drivers raw-posix 
   is done here.
d. 'hostcache and 'cache' options when used together,
   cache=xx will override hostcache=yy.

v8:
 1. Mandate implementation of all three reopen 
    related functions by block drivers.
 2. If 'cache=xx' and 'hostcache=yy' specified
    in cmdline, 'cache=' overrides 'hostcache='.
    

v7:
 1. Added structure BDRVReopenState to support safe 
    reopening of image files.
 2. Implemented reopen functions for raw-posix driver

v6:
 1. "block_set_hostcache" to replace "block_set" command

v5:
 1. Defined qerror class for incorrect command syntax.
 2. Changed error_report() calls to qerror_report()

v4:
    Added 'hostcache' option to '-drive' commandline option.

v3:
  1. Command "block_set" for changing various block params
  2. Enhanced info-block to display hostcache setting 
  3. Added qmp interfaces for setting and querying hostcache

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

v1:
     Dynamic cache change through monitor

New block command added:
"block_set_hostcache"
    -- Sets hostcache parameter for block device  while guest is running.

Usage:
 block_set_hostcache  <device> <option>
   <device> = block device
   <option>  = on/off


New 'hostcache' option added to -drive:
 -drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
        ....
"       [,readonly=on|off][,hostcache=on|off]\n"

 qemu/block.c           |   79 +++++++++++++++++++++++++++++++++++++++--------
 qemu/block.h           |    3 ++
 qemu/block/raw-posix.c |   57 +++++++++++++++++++++++++++++++++++++++++
 qemu/block/raw.c       |   23 +++++++++++++++-
 qemu/block_int.h       |   16 +++++++++++
 qemu/blockdev.c        |    7 +++++
 qemu/qemu-common.h     |    1
 qemu/qemu-config.c     |    4 ++
 qemu/qemu-options.hx   |    2 -
 qemu/qerror.c          |    8 +++++
 qemu/qerror.h          |    6 ++++
 qemu/qmp-commands.hx   |    4 ++
 14 files changed, 194 insertions(+), 16 deletions(-)
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
"txt" 13L, 574C

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
@ 2011-10-30 10:33 ` Supriya Kannery
  2011-11-03 13:55   ` Luiz Capitulino
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 2/6]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

Enhance "info block" to display hostcache setting for each
block device.

Example:
(qemu) info block
ide0-hd0: removable=0 file=../sles11-32.raw ro=0 drv=raw encrypted=0

Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../sles11-32.raw ro=0 drv=raw encrypted=0

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

---
 block.c         |   20 ++++++++++++++++----
 qmp-commands.hx |    2 ++
 2 files changed, 18 insertions(+), 4 deletions(-)

Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -1142,6 +1142,7 @@ Each json-object contain the following:
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "tray-open": only present if removable, true if the device has a tray,
                and it is open (json-bool)
+- "hostcache": true if host pagecache enabled, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
    containing the following:
          - "file": device file name (json-string)
@@ -1168,6 +1169,7 @@ Example:
             "io-status": "ok",
             "device":"ide0-hd0",
             "locked":false,
+            "hostcache":false,
             "removable":false,
             "inserted":{
                "ro":false,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1841,6 +1841,11 @@ static void bdrv_print_dict(QObject *obj
                        qdict_get_bool(bs_dict, "tray-open"));
     }
 
+    if (qdict_haskey(bs_dict, "hostcache")) {
+        monitor_printf(mon, " hostcache=%d",
+                       qdict_get_bool(bs_dict, "hostcache"));
+    }
+
     if (qdict_haskey(bs_dict, "io-status")) {
         monitor_printf(mon, " io-status=%s", qdict_get_str(bs_dict, "io-status"));
     }
@@ -1888,10 +1893,12 @@ void bdrv_info(Monitor *mon, QObject **r
         QDict *bs_dict;
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
-                                    "'removable': %i, 'locked': %i }",
+                                    "'removable': %i, 'locked': %i, "
+                                    "'hostcache': %i }",
                                     bs->device_name,
                                     bdrv_dev_has_removable_media(bs),
-                                    bdrv_dev_is_medium_locked(bs));
+                                    bdrv_dev_is_medium_locked(bs),
+                                    !(bs->open_flags & BDRV_O_NOCACHE));
         bs_dict = qobject_to_qdict(bs_obj);
 
         if (bdrv_dev_has_removable_media(bs)) {

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 2/6]Qemu: Error classes for file reopen and data sync failure
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
  2011-10-30 10:33 ` [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2011-10-30 10:34 ` Supriya Kannery
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

New error classes defined for file reopen failure and data
sync error

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

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

Index: qemu/qerror.c
===================================================================
--- qemu.orig/qerror.c
+++ qemu/qerror.c
@@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
         .desc      = "Device '%(device)' is not removable",
     },
     {
+        .error_fmt = QERR_DATA_SYNC_FAILED,
+        .desc      = "Syncing of data failed for device '%(device)'",
+    },
+    {
+        .error_fmt = QERR_REOPEN_FILE_FAILED,
+        .desc      = "Could not reopen '%(filename)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_NO_BUS,
         .desc      = "Device '%(device)' has no child bus",
     },
Index: qemu/qerror.h
===================================================================
--- qemu.orig/qerror.h
+++ qemu/qerror.h
@@ -87,6 +87,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_DEVICE_NOT_FOUND \
     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
+#define QERR_DATA_SYNC_FAILED \
+    "{ 'class': 'DataSyncFailed', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
@@ -144,6 +147,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 } }"
 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
  2011-10-30 10:33 ` [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 2/6]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
@ 2011-10-30 10:34 ` Supriya Kannery
  2011-11-04 10:00   ` Kevin Wolf
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 4/6]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

New command "block_set_hostcache" added for dynamically changing 
host pagecache setting of a block device.

Usage: 
 block_set_hostcache  <device> <option>
   <device> = block device
   <option> = on/off

Example:
 (qemu) block_set_hostcache ide0-hd0 off

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

---
 block.c         |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h         |    2 ++
 blockdev.c      |   26 ++++++++++++++++++++++++++
 blockdev.h      |    2 ++
 hmp-commands.hx |   14 ++++++++++++++
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 6 files changed, 125 insertions(+)

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -693,6 +693,34 @@ unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret = 0, open_flags;
+
+    /* 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;
+    }
+    open_flags = bs->open_flags;
+    bdrv_close(bs);
+
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+    if (ret < 0) {
+        /* Reopen failed. Try to open with original flags */
+        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+        ret = bdrv_open(bs, bs->filename, open_flags, drv);
+        if (ret < 0) {
+            /* Reopen failed with orig and modified flags */
+            abort();
+        }
+    }
+
+    return ret;
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     if (bs->drv) {
@@ -730,6 +758,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
@@ -109,6 +109,7 @@ int bdrv_parse_cache_flags(const char *m
 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_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
@@ -143,6 +144,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
@@ -776,3 +776,29 @@ int do_block_resize(Monitor *mon, const 
 
     return 0;
 }
+
+
+/*
+ * Change host page cache setting while guest is running.
+*/
+int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
+                           QObject **ret_data)
+{
+    BlockDriverState *bs = NULL;
+    int enable;
+    const char *device;
+
+    /* Validate device */
+    device = qdict_get_str(qdict, "device");
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    /* Read hostcache setting */
+    enable = qdict_get_bool(qdict, "option");
+    return bdrv_change_hostcache(bs, enable);
+
+}
+
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -65,5 +65,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_block_set_hostcache(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,20 @@ but should be used with extreme caution.
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set_hostcache,
+    },
+STEXI
+@item block_set_hostcache @var{device} @var{setting}
+@findex block_set_hostcache
+Change host pagecache setting of a block device while guest is running.
+ETEXI
+
 
     {
         .name       = "eject",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -718,7 +718,34 @@ Example:
 
 EQMP
 
+
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache (true|false)",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set_hostcache,
+    },
+
+SQMP
+block_set_hostcache
+-------------------
+
+Change host pagecache setting of a block device (on|off)
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- "option": hostcache setting (json-bool)
+
+Example:
+-> { "execute": "block_set_hostcache", "arguments": { "device": "ide0-hd0", "option": false } }
+<- { "return": {} }
+
+EQMP
+
+	{
         .name       = "balloon",
         .args_type  = "value:M",
         .params     = "target",

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
                   ` (2 preceding siblings ...)
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
@ 2011-10-30 10:34 ` Supriya Kannery
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely Supriya Kannery
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

qemu command option 'hostcache' added to -drive for block devices.
While starting a VM from qemu commandline, this option can be used 
for setting host cache usage for block data access. 'cache=xx'
overrides 'hostcache=yy' when specified simultaneously.

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

---
 blockdev.c      |   13 +++++++++++++
 qemu-config.c   |    4 ++++
 qemu-options.hx |    2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -237,6 +237,7 @@ DriveInfo *drive_init(QemuOpts *opts, in
     DriveInfo *dinfo;
     int snapshot = 0;
     int ret;
+    int hostcache = 0;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
     media = MEDIA_DISK;
@@ -324,6 +325,12 @@ DriveInfo *drive_init(QemuOpts *opts, in
             error_report("invalid cache option");
             return NULL;
         }
+    } else {
+        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
+            if (!hostcache) {
+                bdrv_flags |= BDRV_O_NOCACHE;
+            }
+        }
     }
 
 #ifdef CONFIG_LINUX_AIO
Index: qemu/qemu-options.hx
===================================================================
--- qemu.orig/qemu-options.hx
+++ qemu/qemu-options.hx
@@ -135,7 +135,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off]\n"
+    "       [,readonly=on|off][,hostcache=on|off]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
 STEXI
 @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
Index: qemu/qemu-config.c
===================================================================
--- qemu.orig/qemu-config.c
+++ qemu/qemu-config.c
@@ -85,6 +85,10 @@ static QemuOptsList qemu_drive_opts = {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
+        },{
+            .name = "hostcache",
+            .type = QEMU_OPT_BOOL,
+            .help = "set or reset hostcache (on/off)"
         },
         { /* end of list */ }
     },

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
                   ` (3 preceding siblings ...)
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 4/6]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
@ 2011-10-30 10:35 ` Supriya Kannery
  2011-11-04 10:05   ` Kevin Wolf
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions Supriya Kannery
  2011-11-04  2:29 ` [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Zhi Yong Wu
  6 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

Struct BDRVReopenState along with three reopen related functions
introduced for handling reopen state of images safely. This can be
extended by each of the block drivers to reopen respective
image files.

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

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -693,10 +693,32 @@ unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+     BlockDriver *drv = bs->drv;
+
+     return drv->bdrv_reopen_prepare(bs, prs, flags);
+}
+
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs, int flags)
+{
+    BlockDriver *drv = bs->drv;
+
+    drv->bdrv_reopen_commit(bs, rs, flags);
+}
+
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BlockDriver *drv = bs->drv;
+
+    drv->bdrv_reopen_abort(bs, rs);
+}
+
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
 {
     BlockDriver *drv = bs->drv;
     int ret = 0, open_flags;
+    BDRVReopenState *rs = NULL;
 
     /* Quiesce IO for the given block device */
     qemu_aio_flush();
@@ -704,20 +726,35 @@ int bdrv_reopen(BlockDriverState *bs, in
         qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
         return ret;
     }
-    open_flags = bs->open_flags;
-    bdrv_close(bs);
 
-    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
-    if (ret < 0) {
-        /* Reopen failed. Try to open with original flags */
-        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
-        ret = bdrv_open(bs, bs->filename, open_flags, drv);
+    /* Use driver specific reopen() if available */
+    if (drv->bdrv_reopen_prepare) {
+        ret = bdrv_reopen_prepare(bs, &rs, bdrv_flags);
         if (ret < 0) {
-            /* Reopen failed with orig and modified flags */
-            abort();
+            bdrv_reopen_abort(bs, rs);
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            return ret;
         }
-    }
+        bdrv_reopen_commit(bs, rs, bdrv_flags);
+
+    } else {
+        open_flags = bs->open_flags;
+        bdrv_close(bs);
 
+        ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+        if (ret < 0) {
+            /* Reopen failed. Try to open with original flags */
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            ret = bdrv_open(bs, bs->filename, open_flags, drv);
+            if (ret < 0) {
+                /*
+                 * Reopen failed with orig and modified flags
+                 * For next access to fail, set drv to NULL
+                */
+                bs->drv = NULL;
+            }
+        }
+    }
     return ret;
 }
 
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -55,6 +55,14 @@ struct BlockDriver {
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
     int (*bdrv_open)(BlockDriverState *bs, int flags);
+
+    /* For handling image reopen for split or non-split files */
+    int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
+          int flags);
+    void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs,
+          int flags);
+    void (*bdrv_reopen_abort)(BlockDriverState *bs, BDRVReopenState *rs);
+
     int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
@@ -211,6 +219,14 @@ struct BlockDriverState {
     void *private;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+    int reopen_flags;
+
+    /* For raw-posix */
+    int reopen_fd;
+};
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -202,6 +202,7 @@ typedef struct NICInfo NICInfo;
 typedef struct HCIInfo HCIInfo;
 typedef struct AudioState AudioState;
 typedef struct BlockDriverState BlockDriverState;
+typedef struct BDRVReopenState BDRVReopenState;
 typedef struct DriveInfo DriveInfo;
 typedef struct DisplayState DisplayState;
 typedef struct DisplayChangeListener DisplayChangeListener;
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -110,6 +110,9 @@ int bdrv_file_open(BlockDriverState **pb
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags);
+void bdrv_reopen_commit(BlockDriverState *bs,  BDRVReopenState *rs, int flags);
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_attach_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
                   ` (4 preceding siblings ...)
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely Supriya Kannery
@ 2011-10-30 10:35 ` Supriya Kannery
  2011-11-04 10:19   ` Kevin Wolf
  2011-11-04  2:29 ` [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Zhi Yong Wu
  6 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-10-30 10:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig

raw-posix driver changes for bdrv_reopen_xx functions to
safely reopen image files. Reopening of image files while 
changing hostcache dynamically, is handled here.

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

Index: qemu/block/raw.c
===================================================================
--- qemu.orig/block/raw.c
+++ qemu/block/raw.c
@@ -9,6 +9,24 @@ static int raw_open(BlockDriverState *bs
     return 0;
 }
 
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                              int flags)
+{
+    return bdrv_reopen_prepare(bs->file, prs, flags);
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs,
+                              int flags)
+{
+    bdrv_reopen_commit(bs->file, rs, flags);
+    bs->open_flags = bs->file->open_flags;
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    bdrv_reopen_abort(bs->file, rs);
+}
+
 static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *qiov)
 {
@@ -107,7 +125,10 @@ static BlockDriver bdrv_raw = {
 
     /* It's really 0, but we need to make g_malloc() happy */
     .instance_size      = 1,
-
+    .bdrv_reopen_prepare
+                        = raw_reopen_prepare,
+    .bdrv_reopen_commit = raw_reopen_commit,
+    .bdrv_reopen_abort  = raw_reopen_abort,
     .bdrv_open          = raw_open,
     .bdrv_close         = raw_close,
 
Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -279,6 +279,60 @@ static int raw_open(BlockDriverState *bs
     return raw_open_common(bs, filename, flags, 0);
 }
 
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                              int flags)
+{
+    BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
+    BDRVRawState *s = bs->opaque;
+
+    raw_rs->bs = bs;
+    raw_rs->reopen_flags = s->open_flags;
+    raw_rs->reopen_fd = -1;
+    *prs = raw_rs;
+    int ret = 0;
+
+    /* If only O_DIRECT to be toggled, use fcntl */
+    if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
+            (flags & ~BDRV_O_NOCACHE))) {
+        raw_rs->reopen_fd = dup(s->fd);
+        if (raw_rs->reopen_fd <= 0) {
+            return -1;
+        }
+        if ((flags & BDRV_O_NOCACHE)) {
+            raw_rs->reopen_flags |= O_DIRECT;
+        } else {
+            raw_rs->reopen_flags &= ~O_DIRECT;
+        }
+        ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_flags);
+    }
+
+    /* TBD: Handle O_DSYNC and other flags */
+
+    return ret;
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs,
+                              int flags)
+{
+    BDRVRawState *s = bs->opaque;
+
+    /* Set new flags, so replace old fd with new one */
+    close(s->fd);
+    s->fd = rs->reopen_fd;
+    s->open_flags = rs->reopen_flags;
+    bs->open_flags = flags;
+    g_free(rs);
+
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    if (rs->reopen_fd != -1) {
+        close(rs->reopen_fd);
+    }
+    g_free(rs);
+}
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -631,6 +685,9 @@ static BlockDriver bdrv_file = {
     .instance_size = sizeof(BDRVRawState),
     .bdrv_probe = NULL, /* no probe for protocols */
     .bdrv_file_open = raw_open,
+    .bdrv_reopen_prepare = raw_reopen_prepare,
+    .bdrv_reopen_commit = raw_reopen_commit,
+    .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
     .bdrv_co_discard = raw_co_discard,

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting
  2011-10-30 10:33 ` [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2011-11-03 13:55   ` Luiz Capitulino
  2011-11-04 10:55     ` Supriya Kannery
  0 siblings, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-11-03 13:55 UTC (permalink / raw)
  To: Supriya Kannery
  Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On Sun, 30 Oct 2011 16:03:54 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> Enhance "info block" to display hostcache setting for each
> block device.
> 
> Example:
> (qemu) info block
> ide0-hd0: removable=0 file=../sles11-32.raw ro=0 drv=raw encrypted=0
> 
> Enhanced to display "hostcache" setting:
> (qemu) info block
> ide0-hd0: removable=0 hostcache=1 file=../sles11-32.raw ro=0 drv=raw encrypted=0
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> ---
>  block.c         |   20 ++++++++++++++++----
>  qmp-commands.hx |    2 ++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -1142,6 +1142,7 @@ Each json-object contain the following:
>  - "locked": true if the device is locked, false otherwise (json-bool)
>  - "tray-open": only present if removable, true if the device has a tray,
>                 and it is open (json-bool)
> +- "hostcache": true if host pagecache enabled, false otherwise (json-bool)
>  - "inserted": only present if the device is inserted, it is a json-object
>     containing the following:
>           - "file": device file name (json-string)
> @@ -1168,6 +1169,7 @@ Example:
>              "io-status": "ok",
>              "device":"ide0-hd0",
>              "locked":false,
> +            "hostcache":false,
>              "removable":false,
>              "inserted":{
>                 "ro":false,
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -1841,6 +1841,11 @@ static void bdrv_print_dict(QObject *obj
>                         qdict_get_bool(bs_dict, "tray-open"));
>      }
>  
> +    if (qdict_haskey(bs_dict, "hostcache")) {
> +        monitor_printf(mon, " hostcache=%d",
> +                       qdict_get_bool(bs_dict, "hostcache"));
> +    }

This series needs to be rebased, as the info block command has been
converted to the QAPI. Please, see the following commit for details:
b202381800d81fbff9978abbdea95760dd363bb6.

Also note that if you're adding new commands (I haven't reviewed the
series) you should use the QAPI. A document on how to use it is coming soon.

> 
>      if (qdict_haskey(bs_dict, "io-status")) {
>          monitor_printf(mon, " io-status=%s", qdict_get_str(bs_dict, "io-status"));
>      }
> @@ -1888,10 +1893,12 @@ void bdrv_info(Monitor *mon, QObject **r
>          QDict *bs_dict;
>  
>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> -                                    "'removable': %i, 'locked': %i }",
> +                                    "'removable': %i, 'locked': %i, "
> +                                    "'hostcache': %i }",
>                                      bs->device_name,
>                                      bdrv_dev_has_removable_media(bs),
> -                                    bdrv_dev_is_medium_locked(bs));
> +                                    bdrv_dev_is_medium_locked(bs),
> +                                    !(bs->open_flags & BDRV_O_NOCACHE));
>          bs_dict = qobject_to_qdict(bs_obj);
>  
>          if (bdrv_dev_has_removable_media(bs)) {
> 
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
  2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
                   ` (5 preceding siblings ...)
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions Supriya Kannery
@ 2011-11-04  2:29 ` Zhi Yong Wu
  2011-11-07  8:38   ` Supriya Kannery
  6 siblings, 1 reply; 19+ messages in thread
From: Zhi Yong Wu @ 2011-11-04  2:29 UTC (permalink / raw)
  To: Supriya Kannery
  Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On Sun, Oct 30, 2011 at 6:33 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> 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.
>
> This patchset introduces
> a. monitor command 'block_set_hostcache' using which host
>   pagecache setting for a block device can be changed
>   dynamically.
I got a bit confusion. Is it used to change host pagecache setting on
hyperviser or on guest?
This block device said by you is for guest, right?

> b. a new option for setting host cache from qemu
>   commandline option -drive  "hostcache=on/off".
> c. BDRVReopenState, a generic structure which can be
>   extended by each of the block drivers to reopen
>   respective image files.
>   Extension of this structure for drivers raw-posix
>   is done here.
> d. 'hostcache and 'cache' options when used together,
>   cache=xx will override hostcache=yy.
>
> v8:
>  1. Mandate implementation of all three reopen
>    related functions by block drivers.
>  2. If 'cache=xx' and 'hostcache=yy' specified
>    in cmdline, 'cache=' overrides 'hostcache='.
>
>
> v7:
>  1. Added structure BDRVReopenState to support safe
>    reopening of image files.
>  2. Implemented reopen functions for raw-posix driver
>
> v6:
>  1. "block_set_hostcache" to replace "block_set" command
>
> v5:
>  1. Defined qerror class for incorrect command syntax.
>  2. Changed error_report() calls to qerror_report()
>
> v4:
>    Added 'hostcache' option to '-drive' commandline option.
>
> v3:
>  1. Command "block_set" for changing various block params
>  2. Enhanced info-block to display hostcache setting
>  3. Added qmp interfaces for setting and querying hostcache
>
> v2:
>  1. Support of dynamic cache change only for hostcache.
>  2. Monitor command "hostcache_get" added to display cache setting
>  3. Backed off the changes for display of cache setting in "info block"
>
> v1:
>     Dynamic cache change through monitor
>
> New block command added:
> "block_set_hostcache"
>    -- Sets hostcache parameter for block device  while guest is running.
>
> Usage:
>  block_set_hostcache  <device> <option>
>   <device> = block device
>   <option>  = on/off
>
>
> New 'hostcache' option added to -drive:
>  -drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
>        ....
> "       [,readonly=on|off][,hostcache=on|off]\n"
>
>  qemu/block.c           |   79 +++++++++++++++++++++++++++++++++++++++--------
>  qemu/block.h           |    3 ++
>  qemu/block/raw-posix.c |   57 +++++++++++++++++++++++++++++++++++++++++
>  qemu/block/raw.c       |   23 +++++++++++++++-
>  qemu/block_int.h       |   16 +++++++++++
>  qemu/blockdev.c        |    7 +++++
>  qemu/qemu-common.h     |    1
>  qemu/qemu-config.c     |    4 ++
>  qemu/qemu-options.hx   |    2 -
>  qemu/qerror.c          |    8 +++++
>  qemu/qerror.h          |    6 ++++
>  qemu/qmp-commands.hx   |    4 ++
>  14 files changed, 194 insertions(+), 16 deletions(-)
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> "txt" 13L, 574C
>
>
>
>
>
>
>



-- 
Regards,

Zhi Yong Wu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change
  2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
@ 2011-11-04 10:00   ` Kevin Wolf
  2011-11-04 11:03     ` Supriya Kannery
  0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2011-11-04 10:00 UTC (permalink / raw)
  To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig

Am 30.10.2011 11:34, schrieb Supriya Kannery:
> New command "block_set_hostcache" added for dynamically changing 
> host pagecache setting of a block device.
> 
> Usage: 
>  block_set_hostcache  <device> <option>
>    <device> = block device
>    <option> = on/off
> 
> Example:
>  (qemu) block_set_hostcache ide0-hd0 off
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> ---
>  block.c         |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h         |    2 ++
>  blockdev.c      |   26 ++++++++++++++++++++++++++
>  blockdev.h      |    2 ++
>  hmp-commands.hx |   14 ++++++++++++++
>  qmp-commands.hx |   27 +++++++++++++++++++++++++++
>  6 files changed, 125 insertions(+)
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -693,6 +693,34 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0, open_flags;
> +
> +    /* 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;

ret is always 0. I think you wanted to return the return value of
bdrv_flush?

> +    }
> +    open_flags = bs->open_flags;
> +    bdrv_close(bs);
> +
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +    if (ret < 0) {
> +        /* Reopen failed. Try to open with original flags */
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +        if (ret < 0) {
> +            /* Reopen failed with orig and modified flags */
> +            abort();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -730,6 +758,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;
> +    }

Maybe the !bdrv_is_inserted() case should be handled in bdrv_reopen(). I
think it would be the same for changing other flags.

Kevin

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely Supriya Kannery
@ 2011-11-04 10:05   ` Kevin Wolf
  2011-11-04 11:10     ` Supriya Kannery
  0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2011-11-04 10:05 UTC (permalink / raw)
  To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig

Am 30.10.2011 11:35, schrieb Supriya Kannery:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopen state of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -55,6 +55,14 @@ struct BlockDriver {
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
>      int (*bdrv_open)(BlockDriverState *bs, int flags);
> +
> +    /* For handling image reopen for split or non-split files */
> +    int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
> +          int flags);
> +    void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs,
> +          int flags);
> +    void (*bdrv_reopen_abort)(BlockDriverState *bs, BDRVReopenState *rs);
> +
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
>                       uint8_t *buf, int nb_sectors);
> @@ -211,6 +219,14 @@ struct BlockDriverState {
>      void *private;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int reopen_flags;
> +
> +    /* For raw-posix */
> +    int reopen_fd;
> +};

I think I commented the same on the previous version: BDRVReopenState
shouldn't contain any format specific fields. raw-posix must extend the
struct like this and use container_of() to get it from a BDRVReopenState
pointer:

struct BDRVRawReopenState {
    BDRVReopenState common;
    int reopen_fd;
};

Kevin

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions
  2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions Supriya Kannery
@ 2011-11-04 10:19   ` Kevin Wolf
  0 siblings, 0 replies; 19+ messages in thread
From: Kevin Wolf @ 2011-11-04 10:19 UTC (permalink / raw)
  To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig

Am 30.10.2011 11:35, schrieb Supriya Kannery:
> raw-posix driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while 
> changing hostcache dynamically, is handled here.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,24 @@ static int raw_open(BlockDriverState *bs
>      return 0;
>  }
>  
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> +                              int flags)
> +{
> +    return bdrv_reopen_prepare(bs->file, prs, flags);
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs,
> +                              int flags)
> +{
> +    bdrv_reopen_commit(bs->file, rs, flags);
> +    bs->open_flags = bs->file->open_flags;

I think it should be bs->open_flags = flags, even if the underlying
driver masks something away (which it shouldn't do in the first place,
but using flags here is clearer).

Also I'm wondering if updating bs->open_flags isn't common to all
formats, so could we move it into the block.c function?

> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    bdrv_reopen_abort(bs->file, rs);
> +}
> +
>  static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
>                                       int nb_sectors, QEMUIOVector *qiov)
>  {
> @@ -107,7 +125,10 @@ static BlockDriver bdrv_raw = {
>  
>      /* It's really 0, but we need to make g_malloc() happy */
>      .instance_size      = 1,
> -
> +    .bdrv_reopen_prepare
> +                        = raw_reopen_prepare,
> +    .bdrv_reopen_commit = raw_reopen_commit,
> +    .bdrv_reopen_abort  = raw_reopen_abort,
>      .bdrv_open          = raw_open,
>      .bdrv_close         = raw_close,

Mostly a matter of taste, but I would prefer open/close to stay first
and having bdrv_reopen_* only after them.

>  
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -279,6 +279,60 @@ static int raw_open(BlockDriverState *bs
>      return raw_open_common(bs, filename, flags, 0);
>  }
>  
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> +                              int flags)
> +{
> +    BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
> +    BDRVRawState *s = bs->opaque;
> +
> +    raw_rs->bs = bs;
> +    raw_rs->reopen_flags = s->open_flags;
> +    raw_rs->reopen_fd = -1;
> +    *prs = raw_rs;
> +    int ret = 0;
> +
> +    /* If only O_DIRECT to be toggled, use fcntl */
> +    if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
> +            (flags & ~BDRV_O_NOCACHE))) {

Wouldn't it be more readable like this?

/* Use fcntl if all affected flags can be changes this way */
fcntl_flags = BDRV_O_NOCACHE;
if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
    ...


> +        raw_rs->reopen_fd = dup(s->fd);
> +        if (raw_rs->reopen_fd <= 0) {
> +            return -1;
> +        }
> +        if ((flags & BDRV_O_NOCACHE)) {
> +            raw_rs->reopen_flags |= O_DIRECT;
> +        } else {
> +            raw_rs->reopen_flags &= ~O_DIRECT;
> +        }
> +        ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_flags);
> +    }

I think there should be an else branch that returns an error. Currently
requests involving anything but BDRV_O_NOCACHE are completely ignored,
but still success is returned.

> +
> +    /* TBD: Handle O_DSYNC and other flags */
> +
> +    return ret;
> +}

Kevin

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting
  2011-11-03 13:55   ` Luiz Capitulino
@ 2011-11-04 10:55     ` Supriya Kannery
  0 siblings, 0 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-11-04 10:55 UTC (permalink / raw)
  To: Luiz Capitulino
  Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On 11/03/2011 07:25 PM, Luiz Capitulino wrote:
> On Sun, 30 Oct 2011 16:03:54 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com>  wrote:
>> +    if (qdict_haskey(bs_dict, "hostcache")) {
>> +        monitor_printf(mon, " hostcache=%d",
>> +                       qdict_get_bool(bs_dict, "hostcache"));
>> +    }
>
> This series needs to be rebased, as the info block command has been
> converted to the QAPI. Please, see the following commit for details:
> b202381800d81fbff9978abbdea95760dd363bb6.
>
> Also note that if you're adding new commands (I haven't reviewed the
> series) you should use the QAPI. A document on how to use it is coming soon.
>

yes, will rebase and use QAPI

>>
>>       if (qdict_haskey(bs_dict, "io-status")) {
>>           monitor_printf(mon, " io-status=%s", qdict_get_str(bs_dict, "io-status"));
>>       }

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change
  2011-11-04 10:00   ` Kevin Wolf
@ 2011-11-04 11:03     ` Supriya Kannery
  0 siblings, 0 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-11-04 11:03 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On 11/04/2011 03:30 PM, Kevin Wolf wrote:
> Am 30.10.2011 11:34, schrieb Supriya Kannery:
>> +
>> +    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;
>> +    }
>
> Maybe the !bdrv_is_inserted() case should be handled in bdrv_reopen(). I
> think it would be the same for changing other flags.
>

I am yet to look at the conditions specific to other flags. So I can
move this check to bdrv_reopen when implementing one more flag I guess.

> Kevin
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely
  2011-11-04 10:05   ` Kevin Wolf
@ 2011-11-04 11:10     ` Supriya Kannery
  0 siblings, 0 replies; 19+ messages in thread
From: Supriya Kannery @ 2011-11-04 11:10 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On 11/04/2011 03:35 PM, Kevin Wolf wrote:
> Am 30.10.2011 11:35, schrieb Supriya Kannery:
>>
>> +struct BDRVReopenState {
>> +    BlockDriverState *bs;
>> +    int reopen_flags;
>> +
>> +    /* For raw-posix */
>> +    int reopen_fd;
>> +};
>
> I think I commented the same on the previous version: BDRVReopenState
> shouldn't contain any format specific fields. raw-posix must extend the
> struct like this and use container_of() to get it from a BDRVReopenState
> pointer:
>
> struct BDRVRawReopenState {
>      BDRVReopenState common;
>      int reopen_fd;
> };
>

I don't recall this was suggested in prev version or may be I missed to 
notice..
ok, will have raw extending common BDRVReopenState struct.


> Kevin
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
  2011-11-04  2:29 ` [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Zhi Yong Wu
@ 2011-11-07  8:38   ` Supriya Kannery
  2011-11-07  8:49     ` Zhi Yong Wu
  0 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-11-07  8:38 UTC (permalink / raw)
  To: Zhi Yong Wu; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On 11/04/2011 07:59 AM, Zhi Yong Wu wrote:
> On Sun, Oct 30, 2011 at 6:33 PM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> 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.
>>
>> This patchset introduces
>> a. monitor command 'block_set_hostcache' using which host
>>    pagecache setting for a block device can be changed
>>    dynamically.
> I got a bit confusion. Is it used to change host pagecache setting on
> hyperviser or on guest?
> This block device said by you is for guest, right?
>

   This command is used for changing pagecache setting of
the image files in host which are acting as block devices
for guest.

-thanks, Supriya

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
  2011-11-07  8:38   ` Supriya Kannery
@ 2011-11-07  8:49     ` Zhi Yong Wu
  2011-11-07  9:35       ` Supriya Kannery
  0 siblings, 1 reply; 19+ messages in thread
From: Zhi Yong Wu @ 2011-11-07  8:49 UTC (permalink / raw)
  To: supriyak; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On Mon, Nov 7, 2011 at 4:38 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> On 11/04/2011 07:59 AM, Zhi Yong Wu wrote:
>>
>> On Sun, Oct 30, 2011 at 6:33 PM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>
>>> 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.
>>>
>>> This patchset introduces
>>> a. monitor command 'block_set_hostcache' using which host
>>>   pagecache setting for a block device can be changed
>>>   dynamically.
>>
>> I got a bit confusion. Is it used to change host pagecache setting on
>> hyperviser or on guest?
>> This block device said by you is for guest, right?
>>
>
>  This command is used for changing pagecache setting of
> the image files in host which are acting as block devices
> for guest.
So what is the difference between it and cache option?

>
> -thanks, Supriya
>
>



-- 
Regards,

Zhi Yong Wu

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
  2011-11-07  8:49     ` Zhi Yong Wu
@ 2011-11-07  9:35       ` Supriya Kannery
  2011-11-07 10:24         ` Zhi Yong Wu
  0 siblings, 1 reply; 19+ messages in thread
From: Supriya Kannery @ 2011-11-07  9:35 UTC (permalink / raw)
  To: Zhi Yong Wu; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On 11/07/2011 02:19 PM, Zhi Yong Wu wrote:
> On Mon, Nov 7, 2011 at 4:38 PM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> On 11/04/2011 07:59 AM, Zhi Yong Wu wrote:
>>>
>>> On Sun, Oct 30, 2011 at 6:33 PM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com>    wrote:
>>>>
>>>> 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.
>>>>
>>>> This patchset introduces
>>>> a. monitor command 'block_set_hostcache' using which host
>>>>    pagecache setting for a block device can be changed
>>>>    dynamically.
>>>
>>> I got a bit confusion. Is it used to change host pagecache setting on
>>> hyperviser or on guest?
>>> This block device said by you is for guest, right?
>>>
>>
>>   This command is used for changing pagecache setting of
>> the image files in host which are acting as block devices
>> for guest.
> So what is the difference between it and cache option?
>

Cache=xx sets a combination of status flags for image files
among which host pagecache is just one. Intention here is
to gradually replace cache=xx with more explicit settings
like hostcache, flush, WCE.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor
  2011-11-07  9:35       ` Supriya Kannery
@ 2011-11-07 10:24         ` Zhi Yong Wu
  0 siblings, 0 replies; 19+ messages in thread
From: Zhi Yong Wu @ 2011-11-07 10:24 UTC (permalink / raw)
  To: supriyak; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig

On Mon, Nov 7, 2011 at 5:35 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> On 11/07/2011 02:19 PM, Zhi Yong Wu wrote:
>>
>> On Mon, Nov 7, 2011 at 4:38 PM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>
>>> On 11/04/2011 07:59 AM, Zhi Yong Wu wrote:
>>>>
>>>> On Sun, Oct 30, 2011 at 6:33 PM, Supriya Kannery
>>>> <supriyak@linux.vnet.ibm.com>    wrote:
>>>>>
>>>>> 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.
>>>>>
>>>>> This patchset introduces
>>>>> a. monitor command 'block_set_hostcache' using which host
>>>>>   pagecache setting for a block device can be changed
>>>>>   dynamically.
>>>>
>>>> I got a bit confusion. Is it used to change host pagecache setting on
>>>> hyperviser or on guest?
>>>> This block device said by you is for guest, right?
>>>>
>>>
>>>  This command is used for changing pagecache setting of
>>> the image files in host which are acting as block devices
>>> for guest.
>>
>> So what is the difference between it and cache option?
>>
>
> Cache=xx sets a combination of status flags for image files
> among which host pagecache is just one. Intention here is
> to gradually replace cache=xx with more explicit settings
> like hostcache, flush, WCE.
Great, thanks.
>
>



-- 
Regards,

Zhi Yong Wu

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2011-11-07 10:24 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-30 10:33 [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
2011-10-30 10:33 ` [Qemu-devel] [v8 Patch 1/6]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
2011-11-03 13:55   ` Luiz Capitulino
2011-11-04 10:55     ` Supriya Kannery
2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 2/6]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
2011-11-04 10:00   ` Kevin Wolf
2011-11-04 11:03     ` Supriya Kannery
2011-10-30 10:34 ` [Qemu-devel] [v8 Patch 4/6]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 5/6]Qemu: Framework for reopening images safely Supriya Kannery
2011-11-04 10:05   ` Kevin Wolf
2011-11-04 11:10     ` Supriya Kannery
2011-10-30 10:35 ` [Qemu-devel] [v8 Patch 6/6]Qemu: raw posix implementation of reopen functions Supriya Kannery
2011-11-04 10:19   ` Kevin Wolf
2011-11-04  2:29 ` [Qemu-devel] [v8 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor Zhi Yong Wu
2011-11-07  8:38   ` Supriya Kannery
2011-11-07  8:49     ` Zhi Yong Wu
2011-11-07  9:35       ` Supriya Kannery
2011-11-07 10:24         ` Zhi Yong Wu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).