[Qemu-devel] [v9 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor

[Qemu-devel] [v9 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor

[Supriya Kannery]

Following patchset is for enabling dynamic change of
host pagecache setting of block devices through qemu 
monitor. 

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.
   Note: 'hostcache and 'cache' options when used together,
   cache=xx will override hostcache=yy.

v9:
 1. Rebased patchset to use qapi interfaces
 2. Approach of extending BDRVReopenState changed
    to use container_of()

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           |  111 +++++++++++++++++++++++++++++++++++++++++++++---- 
 qemu/block.h           |    5 +  
 qemu/block/raw-posix.c |   72 ++++++++++++++++++++++++++++++++
 qemu/block/raw.c       |   23 +++++++++-
 qemu/block_int.h       |   13 +++++
 qemu/blockdev.c        |   33 ++++++++++++++
 qemu/blockdev.h        |    2
 qemu/hmp-commands.hx   |   14 ++++++
 qemu/hmp.c             |    2
 qemu/qapi-schema.json  |    4 +
 qemu/qapi-types.h      |    1
 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   |   27 ++++++++++++
 20 files changed, 315 insertions(+), 13 deletions(-)

~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
~                                                                               
"txt" 13L, 574C

[Qemu-devel] [v9 Patch 1/6]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

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

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

Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../rhel6-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/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -409,6 +409,8 @@
 # @locked: True if the guest has locked this device from having its media
 #          removed
 #
+# @hostcache: True if host pagecache is enabled.
+#
 # @tray_open: #optional True if the device has a tray and it is open
 #             (only present if removable is true)
 #
@@ -422,7 +424,7 @@
 ##
 { 'type': 'BlockInfo',
   'data': {'device': 'str', 'type': 'str', 'removable': 'bool',
-           'locked': 'bool', '*inserted': 'BlockDeviceInfo',
+           'locked': 'bool','hostcache': 'bool', '*inserted': 'BlockDeviceInfo',
            '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus'} }
 
 ##
Index: qemu/qapi-types.h
===================================================================
--- qemu.orig/qapi-types.h
+++ qemu/qapi-types.h
@@ -383,6 +383,7 @@ struct BlockInfo
 {
     char * device;
     char * type;
+    bool hostcache;
     bool removable;
     bool locked;
     bool has_inserted;
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1839,6 +1839,7 @@ BlockInfoList *qmp_query_block(Error **e
         info->value->device = g_strdup(bs->device_name);
         info->value->type = g_strdup("unknown");
         info->value->locked = bdrv_dev_is_medium_locked(bs);
+        info->value->hostcache = !(bs->open_flags & BDRV_O_NOCACHE);
         info->value->removable = bdrv_dev_has_removable_media(bs);
 
         if (bdrv_dev_has_removable_media(bs)) {
Index: qemu/hmp.c
===================================================================
--- qemu.orig/hmp.c
+++ qemu/hmp.c
@@ -199,6 +199,8 @@ void hmp_info_block(Monitor *mon)
             monitor_printf(mon, " tray-open=%d", info->value->tray_open);
         }
 
+        monitor_printf(mon, " hostcache=%d", info->value->hostcache);
+
         if (info->value->has_io_status) {
             monitor_printf(mon, " io-status=%s",
                            BlockDeviceIoStatus_lookup[info->value->io_status]);

[Qemu-devel] [v9 Patch 2/6]Qemu: Error classes for file reopen and data sync failure

[Supriya Kannery]

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 } }"
 

[Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[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
@@ -696,6 +696,35 @@ 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();
+    ret = bdrv_flush(bs);
+    if (ret != 0) {
+        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) {
@@ -733,6 +762,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
@@ -104,6 +104,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);
@@ -138,6 +139,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
@@ -716,7 +716,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",

[Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Supriya Kannery]

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.

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 */ }
     },

[Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[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.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -696,10 +696,33 @@ 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);
+    bs->open_flags = 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 *reopen_state = NULL;
 
     /* Quiesce IO for the given block device */
     qemu_aio_flush();
@@ -708,17 +731,31 @@ 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, &reopen_state, bdrv_flags);
+         if (ret < 0) {
+            bdrv_reopen_abort(bs, reopen_state);
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            return ret;
+        }
+
+        bdrv_reopen_commit(bs, reopen_state, 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 with orig and modified flags */
-            abort();
+            /* 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 */
+                bs->drv = NULL;
+            }
         }
     }
 
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -56,6 +56,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 **prs,
+                               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);
@@ -213,6 +221,11 @@ struct BlockDriverState {
     void *private;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+    int reopen_flags;
+};
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -203,6 +203,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
@@ -105,6 +105,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);

[Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[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,23 @@ 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);
+}
+
+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,10 +124,12 @@ static BlockDriver bdrv_raw = {
 
     /* It's really 0, but we need to make g_malloc() happy */
     .instance_size      = 1,
-
     .bdrv_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_co_readv      = raw_co_readv,
     .bdrv_co_writev     = raw_co_writev,
     .bdrv_co_flush      = raw_co_flush,
Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -136,6 +136,11 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    int reopen_fd;
+    BDRVReopenState reopen_state;
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 
@@ -279,6 +284,70 @@ 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)
+{
+    BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
+    BDRVRawState *s = bs->opaque;
+    int ret = 0;
+
+    raw_rs->reopen_state.reopen_flags = s->open_flags;
+    raw_rs->reopen_state.bs = bs;
+    raw_rs->reopen_fd = -1;
+    *prs = &(raw_rs->reopen_state);
+
+    /* Flags that can be set using fcntl */
+    int 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_state.reopen_flags |= O_DIRECT;
+        } else {
+            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
+        }
+        ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_state.reopen_flags);
+    } else {
+
+        /* TBD: Handle O_DSYNC and other flags. For now return error */
+        ret = -1;
+    }
+    return ret;
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs,
+                              int flags)
+{
+    BDRVRawReopenState *raw_rs;
+    BDRVRawState *s = bs->opaque;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    /* Set new flags, so replace old fd with new one */
+    close(s->fd);
+    s->fd = raw_rs->reopen_fd;
+    s->open_flags = rs->reopen_flags;
+    bs->open_flags = flags;
+
+    g_free(raw_rs);
+
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVRawReopenState *raw_rs;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    if (raw_rs->reopen_fd != -1) {
+        close(raw_rs->reopen_fd);
+    }
+    g_free(raw_rs);
+}
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -631,6 +700,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,

Re: [Qemu-devel] [v9 Patch 1/6 - updated]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

On 11/11/2011 12:17 PM, Supriya Kannery wrote:
 > Enhance "info block" to display hostcache setting for each
 > block device.
 >
 >
 >   ##
 > Index: qemu/qapi-types.h
 > ===================================================================
 > --- qemu.orig/qapi-types.h
 > +++ qemu/qapi-types.h
 > @@ -383,6 +383,7 @@ struct BlockInfo
 >   {
 >       char * device;
 >       char * type;
 > +    bool hostcache;
 >       bool removable;
 >       bool locked;
 >       bool has_inserted;
 > Index: qemu/block.c

hostcache gets added to qapi-types.h from
the change done in qapi-schema.json. Hence
above change has to be ignored. Pls find
updated patch.


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

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

Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../rhel6-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/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -409,6 +409,8 @@
  # @locked: True if the guest has locked this device from having its media
  #          removed
  #
+# @hostcache: True if host pagecache is enabled.
+#
  # @tray_open: #optional True if the device has a tray and it is open
  #             (only present if removable is true)
  #
@@ -422,7 +424,7 @@
  ##
  { 'type': 'BlockInfo',
    'data': {'device': 'str', 'type': 'str', 'removable': 'bool',
-           'locked': 'bool', '*inserted': 'BlockDeviceInfo',
+           'locked': 'bool','hostcache': 'bool', '*inserted': 
'BlockDeviceInfo',
             '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus'} }

  ##
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1839,6 +1839,7 @@ BlockInfoList *qmp_query_block(Error **e
          info->value->device = g_strdup(bs->device_name);
          info->value->type = g_strdup("unknown");
          info->value->locked = bdrv_dev_is_medium_locked(bs);
+        info->value->hostcache = !(bs->open_flags & BDRV_O_NOCACHE);
          info->value->removable = bdrv_dev_has_removable_media(bs);

          if (bdrv_dev_has_removable_media(bs)) {
Index: qemu/hmp.c
===================================================================
--- qemu.orig/hmp.c
+++ qemu/hmp.c
@@ -199,6 +199,8 @@ void hmp_info_block(Monitor *mon)
              monitor_printf(mon, " tray-open=%d", info->value->tray_open);
          }

+        monitor_printf(mon, " hostcache=%d", info->value->hostcache);
+
          if (info->value->has_io_status) {
              monitor_printf(mon, " io-status=%s",
 
BlockDeviceIoStatus_lookup[info->value->io_status]);

Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +    {
> +        .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}

@var{option}

> +@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
> @@ -716,7 +716,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)",

It would be more consistent to use "on|off" instead of "true|false".
Or eliminate it entirely by saying "Enable or disable host pagecache
usage".

Stefan

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {

This does not work.  qemu_opt_get_bool() takes a bool default argument
and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
cannot expect it to ever equal -1.

Try this:

if (qemu_opt_get(opts, "hostcache") &&
    !qemu_opt_get_bool(opts, "hostcache", false)) {
    bdrv_flags |= BDRV_O_NOCACHE;
}

Stefan

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Supriya Kannery]

On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
>
> This does not work.  qemu_opt_get_bool() takes a bool default argument
> and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
> cannot expect it to ever equal -1.
>
> Try this:
>
> if (qemu_opt_get(opts, "hostcache")&&
>      !qemu_opt_get_bool(opts, "hostcache", false)) {
>      bdrv_flags |= BDRV_O_NOCACHE;
> }
>
> Stefan
>

Thanks! for pointing this.
Does the following look ok?

  if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
      bdrv_flags |= BDRV_O_NOCACHE;
  }

If either "hostcache" is not at all specified or it is specified
as "on", qemu_opt_get_bool will return 1, which can be ignored
as bdrv_flags is initialized to 0.

Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Supriya Kannery]

On 11/17/2011 12:04 AM, Stefan Hajnoczi wrote:
> On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> +    {
>> +        .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}
>
> @var{option}

Will send updated patch

>
>> +@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
>> @@ -716,7 +716,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)",
>
> It would be more consistent to use "on|off" instead of "true|false".
> Or eliminate it entirely by saying "Enable or disable host pagecache
> usage".
>
> Stefan
>

Followed similar way how set_link is done.
Specified 'true/false' in brackets as 'on' or 'off' are not accepted as
bool parameter in qmp prompt.

Re: [Qemu-devel] [v9 Patch 1/6 - updated]Qemu: Enhance "info block" to display host cache setting

[Luiz Capitulino]

On Fri, 11 Nov 2011 15:39:29 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> On 11/11/2011 12:17 PM, Supriya Kannery wrote:
>  > Enhance "info block" to display hostcache setting for each
>  > block device.
>  >
>  >
>  >   ##
>  > Index: qemu/qapi-types.h
>  > ===================================================================
>  > --- qemu.orig/qapi-types.h
>  > +++ qemu/qapi-types.h
>  > @@ -383,6 +383,7 @@ struct BlockInfo
>  >   {
>  >       char * device;
>  >       char * type;
>  > +    bool hostcache;
>  >       bool removable;
>  >       bool locked;
>  >       bool has_inserted;
>  > Index: qemu/block.c
> 
> hostcache gets added to qapi-types.h from
> the change done in qapi-schema.json. Hence
> above change has to be ignored. Pls find
> updated patch.

git am says this patch is corrupted. Otherwise the QAPI changes look ok
to me.

> 
> 
> *********************************************************************
> Enhance "info block" to display hostcache setting for each
> block device.
> 
> Example:
> (qemu) info block
> ide0-hd0: removable=0 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
> 
> Enhanced to display "hostcache" setting:
> (qemu) info block
> ide0-hd0: removable=0 hostcache=1 file=../rhel6-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/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -409,6 +409,8 @@
>   # @locked: True if the guest has locked this device from having its media
>   #          removed
>   #
> +# @hostcache: True if host pagecache is enabled.
> +#
>   # @tray_open: #optional True if the device has a tray and it is open
>   #             (only present if removable is true)
>   #
> @@ -422,7 +424,7 @@
>   ##
>   { 'type': 'BlockInfo',
>     'data': {'device': 'str', 'type': 'str', 'removable': 'bool',
> -           'locked': 'bool', '*inserted': 'BlockDeviceInfo',
> +           'locked': 'bool','hostcache': 'bool', '*inserted': 
> 'BlockDeviceInfo',
>              '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus'} }
> 
>   ##
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -1839,6 +1839,7 @@ BlockInfoList *qmp_query_block(Error **e
>           info->value->device = g_strdup(bs->device_name);
>           info->value->type = g_strdup("unknown");
>           info->value->locked = bdrv_dev_is_medium_locked(bs);
> +        info->value->hostcache = !(bs->open_flags & BDRV_O_NOCACHE);
>           info->value->removable = bdrv_dev_has_removable_media(bs);
> 
>           if (bdrv_dev_has_removable_media(bs)) {
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -199,6 +199,8 @@ void hmp_info_block(Monitor *mon)
>               monitor_printf(mon, " tray-open=%d", info->value->tray_open);
>           }
> 
> +        monitor_printf(mon, " hostcache=%d", info->value->hostcache);
> +
>           if (info->value->has_io_status) {
>               monitor_printf(mon, " io-status=%s",
>  
> BlockDeviceIoStatus_lookup[info->value->io_status]);
> 
> 

Re: [Qemu-devel] [v9 Patch 2/6]Qemu: Error classes for file reopen and data sync failure

[Luiz Capitulino]

On Fri, 11 Nov 2011 12:17:35 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> 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)'",
> +    },

Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.

> +    {
>          .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 } }"
>  
> 

Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Luiz Capitulino]

On Fri, 11 Nov 2011 12:17:48 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> 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
> @@ -696,6 +696,35 @@ 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();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        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;
> +}

In this thread:

 http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html

Juan uses a similar method (well, at least it looks similar to me)
to fix a problem with migration. However, it was said that that method
can cause problems with -snapshot and encrypted images.

Won't we have the same sort of problems with this series?


> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -733,6 +762,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;
> +    }

I'm wondering if the simplest (and best) thing to do here is to fail
if the drive is not inserted. Just wondering, not exactly asking you
to change it. But it should at least be clearly documented if you keep it.

> +}
> +
>  /* 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
> @@ -104,6 +104,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);
> @@ -138,6 +139,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);
> +
> +}

This is not using the QAPI.

> +
> 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",

"option" is not good enough. I'd call it "enable" or "disable".

> +        .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
> @@ -716,7 +716,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",
> 

Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Luiz Capitulino]

On Thu, 17 Nov 2011 11:15:06 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> On 11/17/2011 12:04 AM, Stefan Hajnoczi wrote:
> > On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery
> > <supriyak@linux.vnet.ibm.com>  wrote:
> >> +    {
> >> +        .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}
> >
> > @var{option}
> 
> Will send updated patch
> 
> >
> >> +@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
> >> @@ -716,7 +716,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)",
> >
> > It would be more consistent to use "on|off" instead of "true|false".
> > Or eliminate it entirely by saying "Enable or disable host pagecache
> > usage".
> >
> > Stefan
> >
> 
> Followed similar way how set_link is done.
> Specified 'true/false' in brackets as 'on' or 'off' are not accepted as
> bool parameter in qmp prompt.

on/off is used in HMP, while true/false is used in QMP.

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Luiz Capitulino]

On Fri, 11 Nov 2011 12:18:18 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> 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.

Shouldn't this patch come before the one introducing the QMP command?

> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -696,10 +696,33 @@ 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);
> +    bs->open_flags = 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 *reopen_state = NULL;
>  
>      /* Quiesce IO for the given block device */
>      qemu_aio_flush();
> @@ -708,17 +731,31 @@ 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, &reopen_state, bdrv_flags);
> +         if (ret < 0) {
> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state, 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 with orig and modified flags */
> -            abort();
> +            /* 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 */
> +                bs->drv = NULL;
> +            }
>          }
>      }
>  
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -56,6 +56,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 **prs,
> +                               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);
> @@ -213,6 +221,11 @@ struct BlockDriverState {
>      void *private;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int reopen_flags;
> +};
> +
>  struct BlockDriverAIOCB {
>      AIOPool *pool;
>      BlockDriverState *bs;
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -203,6 +203,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
> @@ -105,6 +105,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);
> 

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Stefan Hajnoczi]

On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>
>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>
>>> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) !=
>>> -1) {
>>
>> This does not work.  qemu_opt_get_bool() takes a bool default argument
>> and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
>> cannot expect it to ever equal -1.
>>
>> Try this:
>>
>> if (qemu_opt_get(opts, "hostcache")&&
>>     !qemu_opt_get_bool(opts, "hostcache", false)) {
>>     bdrv_flags |= BDRV_O_NOCACHE;
>> }
>>
>> Stefan
>>
>
> Thanks! for pointing this.
> Does the following look ok?
>
>  if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
>     bdrv_flags |= BDRV_O_NOCACHE;
>  }
>
> If either "hostcache" is not at all specified or it is specified
> as "on", qemu_opt_get_bool will return 1, which can be ignored
> as bdrv_flags is initialized to 0.

It depends on the overall way this should work.  I think this captures
all the cases:

1. cache= and hostcache= may not be used together.
2. cache= sets bdrv_flags.
3. hostcache= may |= BDRV_O_NOCACHE.
4. No option defaults to cache=writethrough (bdrv_flags &= ~BDRV_O_CACHE_MASK).

The code you posted will work although I find it a bit weird how it
also includes case #4.  IMO it's cleanest to just do case #3 by
testing whether or not the hostcache= option is set.

BTW, is there a check for case #1 in your patch series.  I thought I
saw one earlier but now I can't find it.

Stefan

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> @@ -708,17 +731,31 @@ 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) {

This seems weird to me because we're saying a driver may have
drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
function doesn't check and return -ENOTSUP.

This check can be moved into bdrv_reopen_prepare().  We can test for
the -ENOTSUP return value here instead.

> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {

Indentation is off here.

Stefan

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[Stefan Hajnoczi]

On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> +                              int flags)
> +{
> +    BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
> +    BDRVRawState *s = bs->opaque;
> +    int ret = 0;
> +
> +    raw_rs->reopen_state.reopen_flags = s->open_flags;
> +    raw_rs->reopen_state.bs = bs;
> +    raw_rs->reopen_fd = -1;
> +    *prs = &(raw_rs->reopen_state);
> +
> +    /* Flags that can be set using fcntl */
> +    int 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;

return -errno;

> +        }
> +        if ((flags & BDRV_O_NOCACHE)) {
> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
> +        } else {
> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
> +        }
> +        ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_state.reopen_flags);

I wonder if this works on Solaris, FreeBSD, etc?

Perhaps there needs to be a fallback to the missing "else" case below...

> +    } else {
> +
> +        /* TBD: Handle O_DSYNC and other flags. For now return error */
> +        ret = -1;

...and this needs to be implemented.

> +    }
> +    return ret;
> +}

Stefan

Re: [Qemu-devel] [v9 Patch 1/6 - updated]Qemu: Enhance "info block" to display host cache setting

[supriya kannery]

Luiz Capitulino wrote:
> On Fri, 11 Nov 2011 15:39:29 +0530
> Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
>
>   
>> On 11/11/2011 12:17 PM, Supriya Kannery wrote:
>> hostcache gets added to qapi-types.h from
>> the change done in qapi-schema.json. Hence
>> above change has to be ignored. Pls find
>> updated patch.
>>     
>
> git am says this patch is corrupted. Otherwise the QAPI changes look ok
> to me.
>
>   

ok, will recheck with git am while sending updated patchset.


- thanks, Supriya

Re: [Qemu-devel] [v9 Patch 2/6]Qemu: Error classes for file reopen and data sync failure

[supriya kannery]

Luiz Capitulino wrote:
> On Fri, 11 Nov 2011 12:17:35 +0530
> Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
>
>   
>> 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)'",
>> +    },
>>     
>
> Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.
>
>   

This could confuse the user, as to what was tried on an
already opened file by qemu. So I feel, QERR_REOPEN_FILE_FAILED can
bring in better clarity to the message passed to the user.

- thanks Supriya

Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[supriya kannery]

Luiz Capitulino wrote:
> On Fri, 11 Nov 2011 12:17:48 +0530
> Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
>
>   
>> +
>> +    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;
>> +}
>>     
>
> In this thread:
>
>  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html
>
> Juan uses a similar method (well, at least it looks similar to me)
> to fix a problem with migration. However, it was said that that method
> can cause problems with -snapshot and encrypted images.
>
> Won't we have the same sort of problems with this series?
>
>
>   

Thanks! for this link. Yes, this series also need to look at such issues.
Will go through the discussions and see what needs to be looked at.

>> +    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;
>> +    }
>>     
>
> I'm wondering if the simplest (and best) thing to do here is to fail
> if the drive is not inserted. Just wondering, not exactly asking you
> to change it. But it should at least be clearly documented if you keep it.
>
>   

I initially started the patchset with returning error message on 
detecting uninserted drive.
But later thought that, user should be given the flexibility of changing 
hostcache setting
irrespective of whether drive is full or not. To what I understand, 
hostcache setting is
independent of what user puts in to the drive.

Need to make these saved status flags used when opening a newly inserted 
device.

>> +    /* Read hostcache setting */
>> +    enable = qdict_get_bool(qdict, "option");
>> +    return bdrv_change_hostcache(bs, enable);
>> +
>> +}
>>     
>
> This is not using the QAPI.
>   

will change to use QAPI.
>   
>> +
>> 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",
>>     
>
> "option" is not good enough. I'd call it "enable" or "disable".
>
>   

ok

Re: [Qemu-devel] [v9 Patch 2/6]Qemu: Error classes for file reopen and data sync failure

[Luiz Capitulino]

On Fri, 18 Nov 2011 14:59:40 +0530
supriya kannery <supriyak@in.ibm.com> wrote:

> Luiz Capitulino wrote:
> > On Fri, 11 Nov 2011 12:17:35 +0530
> > Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
> >
> >   
> >> 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)'",
> >> +    },
> >>     
> >
> > Is this really needed? I think you could use QERR_OPEN_FILE_FAILED.
> >
> >   
> 
> This could confuse the user, as to what was tried on an
> already opened file by qemu. So I feel, QERR_REOPEN_FILE_FAILED can
> bring in better clarity to the message passed to the user.

I think it's the contrary.

First, the fact that we reopen the image is an implementation detail. If we
do it differently in the future than the error won't fit anymore.

Second, what did really fail when REOPEN_FILE_FAILED is triggered? Flushing
the buffers? Closing the image? Opening it with different flags? All these
operations are part of the "reopen" algorithm.

On the other hand, if you see a OPEN_FILE_FAILED then there's only one
single operation that might have failed. Also, OPEN_FILE_FAILED is widely
well known as some version of it exists in any operating system.

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[supriya kannery]

Stefan Hajnoczi wrote:
> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com> wrote:
>   
>> @@ -708,17 +731,31 @@ 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) {
>>     
>
> This seems weird to me because we're saying a driver may have
> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
> function doesn't check and return -ENOTSUP.
>   

If drv->bdrv_reopen_prepare == NULL , then we are not calling the
publick bdrv_reopen_prepare at all. Unless we later call  public 
bdrv_reopen_prepare
from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
-ENOTSUP inside the public one is not needed right?

Also, we are handling reopening for even those drivers which don't
have its own bdrv_reopen_prepare defined, by taking the "else"
control path. So condition for reporting "ENOTSUP" shouldn't come
up as of now. Please let me know your thoughts.

> This check can be moved into bdrv_reopen_prepare().  We can test for
> the -ENOTSUP return value here instead.
>
>   
>> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
>> +         if (ret < 0) {
>>     
>
> Indentation is off here.
>   

sure..will take care in next version.
> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[supriya kannery]

Stefan Hajnoczi wrote:
> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com> wrote:
>   
>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>     
>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>       
>>>> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) !=
>>>> -1) {
>>>>         
>>> This does not work.  qemu_opt_get_bool() takes a bool default argument
>>> and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
>>> cannot expect it to ever equal -1.
>>>
>>> Try this:
>>>
>>> if (qemu_opt_get(opts, "hostcache")&&
>>>     !qemu_opt_get_bool(opts, "hostcache", false)) {
>>>     bdrv_flags |= BDRV_O_NOCACHE;
>>> }
>>>
>>> Stefan
>>>
>>>       
>> Thanks! for pointing this.
>> Does the following look ok?
>>
>>  if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
>>     bdrv_flags |= BDRV_O_NOCACHE;
>>  }
>>
>> If either "hostcache" is not at all specified or it is specified
>> as "on", qemu_opt_get_bool will return 1, which can be ignored
>> as bdrv_flags is initialized to 0.
>>     
>
> It depends on the overall way this should work.  I think this captures
> all the cases:
>
> 1. cache= and hostcache= may not be used together.
> 2. cache= sets bdrv_flags.
> 3. hostcache= may |= BDRV_O_NOCACHE.
> 4. No option defaults to cache=writethrough (bdrv_flags &= ~BDRV_O_CACHE_MASK).
>
> The code you posted will work although I find it a bit weird how it
> also includes case #4.  IMO it's cleanest to just do case #3 by
> testing whether or not the hostcache= option is set.
>
> BTW, is there a check for case #1 in your patch series.  I thought I
> saw one earlier but now I can't find it.
>   

Following is what I tried to accomplish:

1. cache= and hostcache= may be used together. Cache= will override hostcache= if specified together
   This condition can be retained till cache-xx can be completely replaced by combinations of separate 
   cmdline options defined for setting hostcache, flush, and WCE
   [I think I can change the current implementation to have Cache=xx ORed with hostcache=yy if 
     they are specified together instead of just ignoring hostcache= ]
2. If only cache= specified sets bdrv_flags.
3. If only hostcache= specified, bdrv_flags |= BDRV_O_NOCACHE
4. No option defaults to cache=writethrough (bdrv_flags &= ~BDRV_O_CACHE_MASK).

So the check I had earlier for case #1 in your list, I changed that to 
allow both the options to co-exist.

-thanks, Supriya
> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[supriya kannery]

Stefan Hajnoczi wrote:
> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com> wrote:
>   
>> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
>> +                              int flags)
>> +{
>> +    BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
>> +    BDRVRawState *s = bs->opaque;
>> +    int ret = 0;
>> +
>> +    raw_rs->reopen_state.reopen_flags = s->open_flags;
>> +    raw_rs->reopen_state.bs = bs;
>> +    raw_rs->reopen_fd = -1;
>> +    *prs = &(raw_rs->reopen_state);
>> +
>> +    /* Flags that can be set using fcntl */
>> +    int 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;
>>     
>
> return -errno;
>
>   
>> +        }
>> +        if ((flags & BDRV_O_NOCACHE)) {
>> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
>> +        } else {
>> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
>> +        }
>> +        ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_state.reopen_flags);
>>     
>
> I wonder if this works on Solaris, FreeBSD, etc?
>
> Perhaps there needs to be a fallback to the missing "else" case below...
>
>   

ok. Will look into whether this will work on Solaris, FreeBSD etc..

>> +    } else {
>> +
>> +        /* TBD: Handle O_DSYNC and other flags. For now return error */
>> +        ret = -1;
>>     
>
> ...and this needs to be implemented.
>
>   

ok

>> +    }
>> +    return ret;
>> +}
>>     
>
> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Stefan Hajnoczi]

On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <supriyak@in.ibm.com> wrote:
> Stefan Hajnoczi wrote:
>>
>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com> wrote:
>>
>>>
>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>>
>>>>
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>>
>>>>>
>>>>> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) !=
>>>>> -1) {
>>>>>
>>>>
>>>> This does not work.  qemu_opt_get_bool() takes a bool default argument
>>>> and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
>>>> cannot expect it to ever equal -1.
>>>>
>>>> Try this:
>>>>
>>>> if (qemu_opt_get(opts, "hostcache")&&
>>>>    !qemu_opt_get_bool(opts, "hostcache", false)) {
>>>>    bdrv_flags |= BDRV_O_NOCACHE;
>>>> }
>>>>
>>>> Stefan
>>>>
>>>>
>>>
>>> Thanks! for pointing this.
>>> Does the following look ok?
>>>
>>>  if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
>>>    bdrv_flags |= BDRV_O_NOCACHE;
>>>  }
>>>
>>> If either "hostcache" is not at all specified or it is specified
>>> as "on", qemu_opt_get_bool will return 1, which can be ignored
>>> as bdrv_flags is initialized to 0.
>>>
>>
>> It depends on the overall way this should work.  I think this captures
>> all the cases:
>>
>> 1. cache= and hostcache= may not be used together.
>> 2. cache= sets bdrv_flags.
>> 3. hostcache= may |= BDRV_O_NOCACHE.
>> 4. No option defaults to cache=writethrough (bdrv_flags &=
>> ~BDRV_O_CACHE_MASK).
>>
>> The code you posted will work although I find it a bit weird how it
>> also includes case #4.  IMO it's cleanest to just do case #3 by
>> testing whether or not the hostcache= option is set.
>>
>> BTW, is there a check for case #1 in your patch series.  I thought I
>> saw one earlier but now I can't find it.
>>
>
> Following is what I tried to accomplish:
>
> 1. cache= and hostcache= may be used together. Cache= will override
> hostcache= if specified together
>  This condition can be retained till cache-xx can be completely replaced by
> combinations of separate   cmdline options defined for setting hostcache,
> flush, and WCE
>  [I think I can change the current implementation to have Cache=xx ORed with
> hostcache=yy if     they are specified together instead of just ignoring
> hostcache= ]

Okay, I can see that line of reasoning but then hostcache= does not
provide much value on the command-line.  Perhaps it's better to drop
this patch and not merge a new -drive option until the guest-visible
write cache enable support is also merged?

The interesting feature that this series adds is changing of hostcache
at run-time.

Stefan

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Stefan Hajnoczi]

On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
> Stefan Hajnoczi wrote:
>>
>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com> wrote:
>>
>>>
>>> @@ -708,17 +731,31 @@ 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) {
>>>
>>
>> This seems weird to me because we're saying a driver may have
>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>> function doesn't check and return -ENOTSUP.
>>
>
> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
> publick bdrv_reopen_prepare at all. Unless we later call  public
> bdrv_reopen_prepare
> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
> -ENOTSUP inside the public one is not needed right?
>
> Also, we are handling reopening for even those drivers which don't
> have its own bdrv_reopen_prepare defined, by taking the "else"
> control path. So condition for reporting "ENOTSUP" shouldn't come
> up as of now. Please let me know your thoughts.

How does VMDK implement its prepare/commit/abort?  It needs to use the
"public" bdrv_reopen_prepare() function on its image files.

BTW I think the bdrv_reopen_*() functions should go in block_int.h and
not block.h.  They are visible to the block layer but not public to
the rest of QEMU, which must use the bdrv_reopen() interface only.

I think what's really missing is a way to tie this all together.  You
have posted raw format and raw-posix protocol patches.  But we need to
cover image formats, where VMDK is the multi-file special case and
qcow2/qed/etc are simpler but also need to be supported.

Right now anything but raw-posix is still closing and reopening.  By
adding support for image formats I think you'll find the right way to
structure this code.

Stefan

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[supriya kannery]

Stefan Hajnoczi wrote:
> On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>
>>>       
>>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>>>
>>>>         
>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>> <supriyak@linux.vnet.ibm.com>  wrote:
>>>>>
>>>>>           
>>>>>> +        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) !=
>>>>>> -1) {
>>>>>>
>>>>>>             
>>>>> This does not work.  qemu_opt_get_bool() takes a bool default argument
>>>>> and returns a bool.  (bool)-1 == true.  But (int)true == 1 and you
>>>>> cannot expect it to ever equal -1.
>>>>>
>>>>> Try this:
>>>>>
>>>>> if (qemu_opt_get(opts, "hostcache")&&
>>>>>    !qemu_opt_get_bool(opts, "hostcache", false)) {
>>>>>    bdrv_flags |= BDRV_O_NOCACHE;
>>>>> }
>>>>>
>>>>> Stefan
>>>>>
>>>>>
>>>>>           
>>>> Thanks! for pointing this.
>>>> Does the following look ok?
>>>>
>>>>  if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
>>>>    bdrv_flags |= BDRV_O_NOCACHE;
>>>>  }
>>>>
>>>> If either "hostcache" is not at all specified or it is specified
>>>> as "on", qemu_opt_get_bool will return 1, which can be ignored
>>>> as bdrv_flags is initialized to 0.
>>>>
>>>>         
>>> It depends on the overall way this should work.  I think this captures
>>> all the cases:
>>>
>>> 1. cache= and hostcache= may not be used together.
>>> 2. cache= sets bdrv_flags.
>>> 3. hostcache= may |= BDRV_O_NOCACHE.
>>> 4. No option defaults to cache=writethrough (bdrv_flags &=
>>> ~BDRV_O_CACHE_MASK).
>>>
>>> The code you posted will work although I find it a bit weird how it
>>> also includes case #4.  IMO it's cleanest to just do case #3 by
>>> testing whether or not the hostcache= option is set.
>>>
>>> BTW, is there a check for case #1 in your patch series.  I thought I
>>> saw one earlier but now I can't find it.
>>>
>>>       
>> Following is what I tried to accomplish:
>>
>> 1. cache= and hostcache= may be used together. Cache= will override
>> hostcache= if specified together
>>  This condition can be retained till cache-xx can be completely replaced by
>> combinations of separate   cmdline options defined for setting hostcache,
>> flush, and WCE
>>  [I think I can change the current implementation to have Cache=xx ORed with
>> hostcache=yy if     they are specified together instead of just ignoring
>> hostcache= ]
>>     
>
> Okay, I can see that line of reasoning but then hostcache= does not
> provide much value on the command-line.  Perhaps it's better to drop
> this patch and not merge a new -drive option until the guest-visible
> write cache enable support is also merged?
>
>   

Let us have the implementation for hostcache= as command line option, with
the condition that if both cache= and hostcache= are specified together,
then depending upon enable/disable value specified for hostcache, 
corresponding
bit in cache flags can be set/reset. That way, there is a value add on 
specifying
hostcache in cmdline as well as user will be able to control hostcache 
value from cmdline
itself.

> The interesting feature that this series adds is changing of hostcache
> at run-time.
>
>
>   
Yes.. will resume with dynamic hostcache change part  to make it
usable by qemu

- thanks, Supriya
> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[supriya kannery]

supriya kannery wrote:
> Stefan Hajnoczi wrote:
>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>> <supriyak@linux.vnet.ibm.com> wrote:
>>   
>>> +        }
>>> +        if ((flags & BDRV_O_NOCACHE)) {
>>> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
>>> +        } else {
>>> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
>>> +        }
>>> +        ret = fcntl_setfl(raw_rs->reopen_fd, 
>>> raw_rs->reopen_state.reopen_flags);
>>>     
>>
>> I wonder if this works on Solaris, FreeBSD, etc?
>>
>> Perhaps there needs to be a fallback to the missing "else" case below...
>>
>>   
>
> ok. Will look into whether this will work on Solaris, FreeBSD etc..
>

This should work for all non-win Oses.
I have tested only in x86.

#ifndef _WIN32
/* Sets a specific flag */
int fcntl_setfl(int fd, int flag)
{
    int flags;

    flags = fcntl(fd, F_GETFL);

- thanks, Supriya

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Kevin Wolf]

Am 22.11.2011 09:10, schrieb supriya kannery:
> Let us have the implementation for hostcache= as command line option, with
> the condition that if both cache= and hostcache= are specified together,
> then depending upon enable/disable value specified for hostcache, 
> corresponding
> bit in cache flags can be set/reset. That way, there is a value add on 
> specifying
> hostcache in cmdline as well as user will be able to control hostcache 
> value from cmdline
> itself.

The additional thing this would introduce is cache=unsafe hostcache=off.
Probably not the most common thing to have, but why not allow it now.

Kevin

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[supriya kannery]

Stefan Hajnoczi wrote:
> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>
>>>       
>>>> @@ -708,17 +731,31 @@ 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) {
>>>>
>>>>         
>>> This seems weird to me because we're saying a driver may have
>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>>> function doesn't check and return -ENOTSUP.
>>>
>>>       
>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
>> publick bdrv_reopen_prepare at all. Unless we later call  public
>> bdrv_reopen_prepare
>> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
>> -ENOTSUP inside the public one is not needed right?
>>
>> Also, we are handling reopening for even those drivers which don't
>> have its own bdrv_reopen_prepare defined, by taking the "else"
>> control path. So condition for reporting "ENOTSUP" shouldn't come
>> up as of now. Please let me know your thoughts.
>>     
>
> How does VMDK implement its prepare/commit/abort?  It needs to use the
> "public" bdrv_reopen_prepare() function on its image files.
>   

bdrv_reopen() is the public interface which gets called by any of the 
image formats.
So VMDK or any image format has to call bdrv_reopen which decides to call
driver specific prepare/commit/abort or simply close and reopen the file.

> BTW I think the bdrv_reopen_*() functions should go in block_int.h and
> not block.h.  They are visible to the block layer but not public to
> the rest of QEMU, which must use the bdrv_reopen() interface only.
>
> I think what's really missing is a way to tie this all together.  You
> have posted raw format and raw-posix protocol patches.  But we need to
> cover image formats, where VMDK is the multi-file special case and
> qcow2/qed/etc are simpler but also need to be supported.
>
> Right now anything but raw-posix is still closing and reopening.  By
> adding support for image formats I think you'll find the right way to
> structure this code.
>
>   

Since only bdrv_reopen() is public, it is declared in block.h and 
structure of
code done in similar way how bdrv_open() is  done.

The else part in bdrv_reopen() will handle reopen requests
for images other than raw for now (simply close and reopen).

-thanks, Supriya
> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Kevin Wolf]

Am 22.11.2011 11:24, schrieb supriya kannery:
> Stefan Hajnoczi wrote:
>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>>   
>>> Stefan Hajnoczi wrote:
>>>     
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>
>>>>       
>>>>> @@ -708,17 +731,31 @@ 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) {
>>>>>
>>>>>         
>>>> This seems weird to me because we're saying a driver may have
>>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>>>> function doesn't check and return -ENOTSUP.
>>>>
>>>>       
>>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
>>> publick bdrv_reopen_prepare at all. Unless we later call  public
>>> bdrv_reopen_prepare
>>> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
>>> -ENOTSUP inside the public one is not needed right?
>>>
>>> Also, we are handling reopening for even those drivers which don't
>>> have its own bdrv_reopen_prepare defined, by taking the "else"
>>> control path. So condition for reporting "ENOTSUP" shouldn't come
>>> up as of now. Please let me know your thoughts.
>>>     
>>
>> How does VMDK implement its prepare/commit/abort?  It needs to use the
>> "public" bdrv_reopen_prepare() function on its image files.
>>   
> 
> bdrv_reopen() is the public interface which gets called by any of the 
> image formats.
> So VMDK or any image format has to call bdrv_reopen which decides to call
> driver specific prepare/commit/abort or simply close and reopen the file.

No, that doesn't work. In order to get all-or-nothing semantics, you
need to explicitly prepare all child images and only when you know the
results of all preparations, you can decide whether to commit or abort all.

Kevin

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[supriya kannery]

Kevin Wolf wrote:
> Am 22.11.2011 11:24, schrieb supriya kannery:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>>>   
>>>       
>>>> Stefan Hajnoczi wrote:
>>>>     
>>>>         
>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>       
>>>>>           
>>>>>> @@ -708,17 +731,31 @@ 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) {
>>>>>>
>>>>>>         
>>>>>>             
>>>>> This seems weird to me because we're saying a driver may have
>>>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>>>>> function doesn't check and return -ENOTSUP.
>>>>>
>>>>>       
>>>>>           
>>>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
>>>> publick bdrv_reopen_prepare at all. Unless we later call  public
>>>> bdrv_reopen_prepare
>>>> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
>>>> -ENOTSUP inside the public one is not needed right?
>>>>
>>>> Also, we are handling reopening for even those drivers which don't
>>>> have its own bdrv_reopen_prepare defined, by taking the "else"
>>>> control path. So condition for reporting "ENOTSUP" shouldn't come
>>>> up as of now. Please let me know your thoughts.
>>>>     
>>>>         
>>> How does VMDK implement its prepare/commit/abort?  It needs to use the
>>> "public" bdrv_reopen_prepare() function on its image files.
>>>   
>>>       
>> bdrv_reopen() is the public interface which gets called by any of the 
>> image formats.
>> So VMDK or any image format has to call bdrv_reopen which decides to call
>> driver specific prepare/commit/abort or simply close and reopen the file.
>>     
>
> No, that doesn't work. In order to get all-or-nothing semantics, you
> need to explicitly prepare all child images and only when you know the
> results of all preparations, you can decide whether to commit or abort all.
>   
bdrv_reopen_prepare/commit/abort will be implemented specific to VMDK in 
vmdk.c. Then for vmdk,
drv->bdrv_reopen_prepare() will handle  preparing child images and 
return success to bdrv_reopen ()
only if all of them get prepared successfully.  The prepare/commit/abort 
concept we took up considering
vmdk's special case of multiple files.

So it is bdrv_reopen() which is public and called by hostcache change 
request for any of the image formats.
It then routes the processing to respective prepare/commit/abort 
implemented by the drivers, including VMDK.
In cases where drivers don't have their own implementation, default 
route is taken which is simply
closing and opening the file.

- thanks, Supriya
> Kevin
>
>   

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 9:55 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 22.11.2011 09:10, schrieb supriya kannery:
>> Let us have the implementation for hostcache= as command line option, with
>> the condition that if both cache= and hostcache= are specified together,
>> then depending upon enable/disable value specified for hostcache,
>> corresponding
>> bit in cache flags can be set/reset. That way, there is a value add on
>> specifying
>> hostcache in cmdline as well as user will be able to control hostcache
>> value from cmdline
>> itself.
>
> The additional thing this would introduce is cache=unsafe hostcache=off.
> Probably not the most common thing to have, but why not allow it now.

QEMU's command-line is hairy enough already.  I don't think this
option is helpful until we can replace cache= completely.

This is a subjective issue though so if you want to take this option,
Kevin, then please do.

Stefan

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[supriya kannery]

Stefan Hajnoczi wrote:
> On Tue, Nov 22, 2011 at 9:45 AM, supriya kannery <supriyak@in.ibm.com> wrote:
>   
>> supriya kannery wrote:
>>     
>>> Stefan Hajnoczi wrote:
>>>       
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>
>>>>         
>>>>> +        }
>>>>> +        if ((flags & BDRV_O_NOCACHE)) {
>>>>> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
>>>>> +        } else {
>>>>> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
>>>>> +        }
>>>>> +        ret = fcntl_setfl(raw_rs->reopen_fd,
>>>>> raw_rs->reopen_state.reopen_flags);
>>>>>
>>>>>           
>>>> I wonder if this works on Solaris, FreeBSD, etc?
>>>>
>>>> Perhaps there needs to be a fallback to the missing "else" case below...
>>>>
>>>>
>>>>         
>>> ok. Will look into whether this will work on Solaris, FreeBSD etc..
>>>
>>>       
>> This should work for all non-win Oses.
>> I have tested only in x86.
>>
>> #ifndef _WIN32
>> /* Sets a specific flag */
>> int fcntl_setfl(int fd, int flag)
>> {
>>   int flags;
>>
>>   flags = fcntl(fd, F_GETFL);
>>     
>
> Are you sure POSIX guarantees that O_DIRECT can be changed with
> F_SETFL?  I didn't find any statement in the specification.  It is
> possible that this code compiles but does not actually work on
> non-Linux OSes.  Did you run tests?
>   
I don't have FreeBSD and Solaris systems to use.
Referred the following man page links to verify that O_DIRECT in these
OSes can be changed using fcntl.
http://fuse4bsd.creo.hu/localcgi/man-cgi.cgi?fcntl+2
http://man-wiki.net/index.php/2:fcntl
If anybody with these systems can confirm, that would be very helpful.

-thanks, Supriya


> Stefan
>
>   

Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[Kevin Wolf]

Am 22.11.2011 12:17, schrieb Stefan Hajnoczi:
> On Tue, Nov 22, 2011 at 9:55 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 22.11.2011 09:10, schrieb supriya kannery:
>>> Let us have the implementation for hostcache= as command line option, with
>>> the condition that if both cache= and hostcache= are specified together,
>>> then depending upon enable/disable value specified for hostcache,
>>> corresponding
>>> bit in cache flags can be set/reset. That way, there is a value add on
>>> specifying
>>> hostcache in cmdline as well as user will be able to control hostcache
>>> value from cmdline
>>> itself.
>>
>> The additional thing this would introduce is cache=unsafe hostcache=off.
>> Probably not the most common thing to have, but why not allow it now.
> 
> QEMU's command-line is hairy enough already.  I don't think this
> option is helpful until we can replace cache= completely.
> 
> This is a subjective issue though so if you want to take this option,
> Kevin, then please do.

In fact, I think I suggested removing it in an earlier version of the
series because I felt the same. I didn't have a strong enough opinion on
it to really insist on it, though. Discuss it with Supriya and I'll be
happy with whatever you decide.

Kevin

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 9:45 AM, supriya kannery <supriyak@in.ibm.com> wrote:
> supriya kannery wrote:
>>
>> Stefan Hajnoczi wrote:
>>>
>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>
>>>>
>>>> +        }
>>>> +        if ((flags & BDRV_O_NOCACHE)) {
>>>> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
>>>> +        } else {
>>>> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
>>>> +        }
>>>> +        ret = fcntl_setfl(raw_rs->reopen_fd,
>>>> raw_rs->reopen_state.reopen_flags);
>>>>
>>>
>>> I wonder if this works on Solaris, FreeBSD, etc?
>>>
>>> Perhaps there needs to be a fallback to the missing "else" case below...
>>>
>>>
>>
>> ok. Will look into whether this will work on Solaris, FreeBSD etc..
>>
>
> This should work for all non-win Oses.
> I have tested only in x86.
>
> #ifndef _WIN32
> /* Sets a specific flag */
> int fcntl_setfl(int fd, int flag)
> {
>   int flags;
>
>   flags = fcntl(fd, F_GETFL);

Are you sure POSIX guarantees that O_DIRECT can be changed with
F_SETFL?  I didn't find any statement in the specification.  It is
possible that this code compiles but does not actually work on
non-Linux OSes.  Did you run tests?

Stefan

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[Christoph Hellwig]

On Tue, Nov 22, 2011 at 11:32:42AM +0000, Stefan Hajnoczi wrote:
> Are you sure POSIX guarantees that O_DIRECT can be changed with
> F_SETFL?  I didn't find any statement in the specification.  It is
> possible that this code compiles but does not actually work on
> non-Linux OSes.  Did you run tests?

O_DIRECT is a non-posix extention, and semantics of it will differ
greatly between the operating systems implementing it.

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 11:16 AM, supriya kannery <supriyak@in.ibm.com> wrote:
> Kevin Wolf wrote:
>>
>> Am 22.11.2011 11:24, schrieb supriya kannery:
>>
>>>
>>> Stefan Hajnoczi wrote:
>>>
>>>>
>>>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <supriyak@in.ibm.com>
>>>> wrote:
>>>>
>>>>>
>>>>> Stefan Hajnoczi wrote:
>>>>>
>>>>>>
>>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> @@ -708,17 +731,31 @@ 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) {
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> This seems weird to me because we're saying a driver may have
>>>>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare()
>>>>>> function doesn't check and return -ENOTSUP.
>>>>>>
>>>>>>
>>>>>
>>>>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the
>>>>> publick bdrv_reopen_prepare at all. Unless we later call  public
>>>>> bdrv_reopen_prepare
>>>>> from elsewhere without checking drv->bdrv_reopen_prepare,  a check for
>>>>> -ENOTSUP inside the public one is not needed right?
>>>>>
>>>>> Also, we are handling reopening for even those drivers which don't
>>>>> have its own bdrv_reopen_prepare defined, by taking the "else"
>>>>> control path. So condition for reporting "ENOTSUP" shouldn't come
>>>>> up as of now. Please let me know your thoughts.
>>>>>
>>>>
>>>> How does VMDK implement its prepare/commit/abort?  It needs to use the
>>>> "public" bdrv_reopen_prepare() function on its image files.
>>>>
>>>
>>> bdrv_reopen() is the public interface which gets called by any of the
>>> image formats.
>>> So VMDK or any image format has to call bdrv_reopen which decides to call
>>> driver specific prepare/commit/abort or simply close and reopen the file.
>>>
>>
>> No, that doesn't work. In order to get all-or-nothing semantics, you
>> need to explicitly prepare all child images and only when you know the
>> results of all preparations, you can decide whether to commit or abort
>> all.
>>
>
> bdrv_reopen_prepare/commit/abort will be implemented specific to VMDK in
> vmdk.c. Then for vmdk,
> drv->bdrv_reopen_prepare() will handle  preparing child images and return
> success to bdrv_reopen ()
> only if all of them get prepared successfully.  The prepare/commit/abort
> concept we took up considering
> vmdk's special case of multiple files.
>
> So it is bdrv_reopen() which is public and called by hostcache change
> request for any of the image formats.
> It then routes the processing to respective prepare/commit/abort implemented
> by the drivers, including VMDK.
> In cases where drivers don't have their own implementation, default route is
> taken which is simply
> closing and opening the file.

VMDK must call bdrv_reopen_prepare()/bdrv_reopen_commit()/bdrv_reopen_abort()
on its child images in order to support aborting when there is a
failure half-way through.  If it used bdrv_reopen() on its child
images then it could not roll back later when there is a failure on
the next child.

My bigger picture comment was that safe reopen support for raw-posix
is great but we should be able to take advantage of that for image
formats.  I'd rather see all image formats except VMDK have safe
reopen in this series than only raw-posix and vmdk.  How about the
generic prepare/commit/abort implementation that Kevin suggested in a
previous thread - something that qcow2, qed, etc can use in order to
get the safe reopen ability?

(If we don't get safe reopen support for qcow2, qed, etc then dynamic
hostcache changing will take the unsafe reopen path in some of the
common usecases with image file.)

Stefan

Re: [Qemu-devel] [v9 Patch 6/6]Qemu: raw posix implementation of reopen functions

[Stefan Hajnoczi]

On Tue, Nov 22, 2011 at 11:30 AM, supriya kannery <supriyak@in.ibm.com> wrote:
> Stefan Hajnoczi wrote:
>>
>> On Tue, Nov 22, 2011 at 9:45 AM, supriya kannery <supriyak@in.ibm.com>
>> wrote:
>>
>>>
>>> supriya kannery wrote:
>>>
>>>>
>>>> Stefan Hajnoczi wrote:
>>>>
>>>>>
>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>
>>>>>>
>>>>>> +        }
>>>>>> +        if ((flags & BDRV_O_NOCACHE)) {
>>>>>> +            raw_rs->reopen_state.reopen_flags |= O_DIRECT;
>>>>>> +        } else {
>>>>>> +            raw_rs->reopen_state.reopen_flags &= ~O_DIRECT;
>>>>>> +        }
>>>>>> +        ret = fcntl_setfl(raw_rs->reopen_fd,
>>>>>> raw_rs->reopen_state.reopen_flags);
>>>>>>
>>>>>>
>>>>>
>>>>> I wonder if this works on Solaris, FreeBSD, etc?
>>>>>
>>>>> Perhaps there needs to be a fallback to the missing "else" case
>>>>> below...
>>>>>
>>>>>
>>>>>
>>>>
>>>> ok. Will look into whether this will work on Solaris, FreeBSD etc..
>>>>
>>>>
>>>
>>> This should work for all non-win Oses.
>>> I have tested only in x86.
>>>
>>> #ifndef _WIN32
>>> /* Sets a specific flag */
>>> int fcntl_setfl(int fd, int flag)
>>> {
>>>  int flags;
>>>
>>>  flags = fcntl(fd, F_GETFL);
>>>
>>
>> Are you sure POSIX guarantees that O_DIRECT can be changed with
>> F_SETFL?  I didn't find any statement in the specification.  It is
>> possible that this code compiles but does not actually work on
>> non-Linux OSes.  Did you run tests?
>>
>
> I don't have FreeBSD and Solaris systems to use.
> Referred the following man page links to verify that O_DIRECT in these
> OSes can be changed using fcntl.
> http://fuse4bsd.creo.hu/localcgi/man-cgi.cgi?fcntl+2
> http://man-wiki.net/index.php/2:fcntl
> If anybody with these systems can confirm, that would be very helpful.

Cool, thanks for sharing.  The safest would be to do a F_GETFL
afterwards and fall back to unsafe reopen if it didn't work.

Stefan

Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely

[Supriya Kannery]

On 11/22/2011 05:19 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 22, 2011 at 11:16 AM, supriya kannery<supriyak@in.ibm.com>  wrote:
>> Kevin Wolf wrote:
>>>
>>> Am 22.11.2011 11:24, schrieb supriya kannery:
>>>
>>>>
>>>>>
>>>>> How does VMDK implement its prepare/commit/abort?  It needs to use the
>>>>> "public" bdrv_reopen_prepare() function on its image files.
>>>>>
>>>>
>>>> bdrv_reopen() is the public interface which gets called by any of the
>>>> image formats.
>>>> So VMDK or any image format has to call bdrv_reopen which decides to call
>>>> driver specific prepare/commit/abort or simply close and reopen the file.
>>>>
>>>
>>> No, that doesn't work. In order to get all-or-nothing semantics, you
>>> need to explicitly prepare all child images and only when you know the
>>> results of all preparations, you can decide whether to commit or abort
>>> all.
>>>
>>
>> bdrv_reopen_prepare/commit/abort will be implemented specific to VMDK in
>> vmdk.c. Then for vmdk,
>> drv->bdrv_reopen_prepare() will handle  preparing child images and return
>> success to bdrv_reopen ()
>> only if all of them get prepared successfully.  The prepare/commit/abort
>> concept we took up considering
>> vmdk's special case of multiple files.
>>
>> So it is bdrv_reopen() which is public and called by hostcache change
>> request for any of the image formats.
>> It then routes the processing to respective prepare/commit/abort implemented
>> by the drivers, including VMDK.
>> In cases where drivers don't have their own implementation, default route is
>> taken which is simply
>> closing and opening the file.
>
> VMDK must call bdrv_reopen_prepare()/bdrv_reopen_commit()/bdrv_reopen_abort()
> on its child images in order to support aborting when there is a
> failure half-way through.  If it used bdrv_reopen() on its child
> images then it could not roll back later when there is a failure on
> the next child.
>

I got both your's and Kevin's point now, after looking into vmdk.c code
related to extents.
My initial understanding was bdrv_reopen_prepare() implemented inside 
vmdk.c can be called for handling reopening of multiple child
images. But observing how vmdk_open() is implemented for child images, I 
should follow that method. So will make 
bdrv_reopen_prepare()/commit/abort in block.c to be used by vmdk as well 
as other required image formats (like how raw.c is using them).

- thanks, Supriya

Re: [Qemu-devel] [v9 Patch 0/6]Qemu: Host pagecache setting from cmdline and monitor

[Kevin Wolf]

Am 11.11.2011 07:47, schrieb Supriya Kannery:
> Following patchset is for enabling dynamic change of
> host pagecache setting of block devices through qemu 
> monitor. 
> 
> 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.
>    Note: 'hostcache and 'cache' options when used together,
>    cache=xx will override hostcache=yy.

Are you going to submit a v10 of the series? It would be great to have
this functionality in qemu 1.1.

Kevin