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

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

[Supriya Kannery]

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

This patchset introduces 
a. monitor command 'block_set_hostcache' using which host 
   pagecache setting for a block device can be changed 
   dynamically while guest VM is running.
b. 'hostcache' - a new option for setting host cache 
   from qemu command -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 raw-posix is done
   for now. I am working on to extend the same for 
   raw-win32  image files as well.

Note: 'Hostcache and 'cache' options cannot be used 
simultaneously from commandline.

v7:
 1. Added structure BDRVReopenState to support safe 
    reopening of image files.

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           |   119 ++++++++++++++++++++++++++++++++++++++-------
 qemu/block.h           |    2 +
 qemu/block/raw-posix.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++
 qemu/block/raw.c       |   11 +++++++
 qemu/block_int.h       |   15 ++++++++++
 qemu/blockdev.c        |   39 +++++++++++++++++++++++++
 qemu/blockdev.h        |    2 +
 qemu/hmp-commands.hx   |   14 +++++++++
 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   |   29 +++++++++++++++++++
 17 files changed, 306 insertions(+), 13 deletions(-)

[Qemu-devel] [v7 Patch 1/5]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=../sles11-32.raw ro=0 drv=raw encrypted=0

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

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

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

Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -1142,6 +1142,7 @@ Each json-object contain the following:
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "tray-open": only present if removable, true if the device has a tray,
                and it is open (json-bool)
+- "hostcache": true if host pagecache enabled, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
    containing the following:
          - "file": device file name (json-string)
@@ -1163,6 +1164,7 @@ Example:
          {
             "device":"ide0-hd0",
             "locked":false,
+            "hostcache":false,
             "removable":false,
             "inserted":{
                "ro":false,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1866,6 +1866,15 @@ static void bdrv_print_dict(QObject *obj
         monitor_printf(mon, " tray-open=%d",
                        qdict_get_bool(bs_dict, "tray-open"));
     }
+
+    if (qdict_haskey(bs_dict, "open_flags")) {
+        int open_flags = qdict_get_int(bs_dict, "open_flags");
+        if (open_flags & BDRV_O_NOCACHE)
+            monitor_printf(mon, " hostcache=0");
+        else
+            monitor_printf(mon, " hostcache=1");
+    }
+
     if (qdict_haskey(bs_dict, "inserted")) {
         QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
 
@@ -1903,11 +1912,14 @@ void bdrv_info(Monitor *mon, QObject **r
         QDict *bs_dict;
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
-                                    "'removable': %i, 'locked': %i }",
+                                    "'removable': %i, 'locked': %i, "
+                                    "'hostcache': %i }",
                                     bs->device_name,
                                     bdrv_dev_has_removable_media(bs),
-                                    bdrv_dev_is_medium_locked(bs));
+                                    bdrv_dev_is_medium_locked(bs),
+                                    !(bs->open_flags & BDRV_O_NOCACHE));
         bs_dict = qobject_to_qdict(bs_obj);
+        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
 
         if (bdrv_dev_has_removable_media(bs)) {
             qdict_put(bs_dict, "tray-open",

[Qemu-devel] [v7 Patch 2/5]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] [v7 Patch 3/5]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
@@ -702,6 +702,34 @@ unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret = 0, open_flags;
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    if (bdrv_flush(bs)) {
+        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
+        return ret;
+    }
+    open_flags = bs->open_flags;
+    bdrv_close(bs);
+
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+    if (ret < 0) {
+        /* Reopen failed. Try to open with original flags */
+        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+        ret = bdrv_open(bs, bs->filename, open_flags, drv);
+        if (ret < 0) {
+            /* Reopen failed with orig and modified flags */
+            abort();
+        }
+    }
+
+    return ret;
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     if (bs->drv) {
@@ -739,6 +767,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
@@ -99,6 +99,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);
@@ -133,6 +134,7 @@ void bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache);
 
 
 typedef struct BdrvCheckResult {
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -776,3 +776,29 @@ int do_block_resize(Monitor *mon, const 
 
     return 0;
 }
+
+
+/*
+ * Change host page cache setting while guest is running.
+*/
+int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
+                           QObject **ret_data)
+{
+    BlockDriverState *bs = NULL;
+    int enable;
+    const char *device;
+
+    /* Validate device */
+    device = qdict_get_str(qdict, "device");
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    /* Read hostcache setting */
+    enable = qdict_get_bool(qdict, "option");
+    return bdrv_change_hostcache(bs, enable);
+
+}
+
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const 
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
+                           QObject **ret_data);
 
 #endif
Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,20 @@ but should be used with extreme caution.
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set_hostcache,
+    },
+STEXI
+@item block_set_hostcache @var{device} @var{setting}
+@findex block_set_hostcache
+Change host pagecache setting of a block device while guest is running.
+ETEXI
+
 
     {
         .name       = "eject",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -718,7 +718,34 @@ Example:
 
 EQMP
 
+
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache (true|false)",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_block_set_hostcache,
+    },
+
+SQMP
+block_set_hostcache
+-------------------
+
+Change host pagecache setting of a block device (on|off)
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- "option": hostcache setting (json-bool)
+
+Example:
+-> { "execute": "block_set_hostcache", "arguments": { "device": "ide0-hd0", "option": false } }
+<- { "return": {} }
+
+EQMP
+
+	{
         .name       = "balloon",
         .args_type  = "value:M",
         .params     = "target",

[Qemu-devel] [v7 Patch 4/5]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. Simultaneous
use of 'hostcache' and 'cache' options not allowed.

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;
@@ -319,7 +320,19 @@ DriveInfo *drive_init(QemuOpts *opts, in
 	}
     }
 
+    if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
+        if (!hostcache) {
+            bdrv_flags |= BDRV_O_NOCACHE;
+        } else {
+            bdrv_flags &= ~BDRV_O_NOCACHE;
+        }
+    }
+
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
+        if (hostcache != -1) {
+            error_report("'hostcache' and 'cache' cannot co-exist");
+            return NULL;
+        }
         if (bdrv_parse_cache_flags(buf, &bdrv_flags) != 0) {
             error_report("invalid cache option");
             return NULL;
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] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Supriya Kannery]

Struct BDRVReopenState introduced for handling reopen state of images. 
This can be  extended by each of the block drivers to reopen respective
image files. Implementation for raw-posix is done here.

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

---
 block.c           |   46 ++++++++++++++++++++++++++++++++--------
 block/raw-posix.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h       |   15 +++++++++++++
 3 files changed, 114 insertions(+), 9 deletions(-)

Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs
     return raw_open_common(bs, filename, flags, 0);
 }
 
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs,
+                              int flags)
+{
+    BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
+    BDRVRawState *s = bs->opaque;
+
+    raw_rs->bs = bs;
+    raw_rs->reopen_flags = flags;
+    raw_rs->reopen_fd = -1;
+
+    /* If only O_DIRECT to be toggled, use fcntl */
+    if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
+            (flags & ~BDRV_O_NOCACHE))) {
+        raw_rs->reopen_fd = dup(s->fd);
+        if (raw_rs->reopen_fd <= 0) {
+            return -1;
+        }
+    }
+
+    /* TBD: Handle O_DSYNC and other flags */
+    *rs = raw_rs;
+    return 0;
+}
+
+static int raw_reopen_commit(BDRVReopenState *rs)
+{
+    BlockDriverState *bs = rs->bs;
+    BDRVRawState *s = bs->opaque;
+
+    if (!rs->reopen_fd) {
+        return -1;
+    }
+
+    int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* Set new flags, so replace old fd with new one */
+    close(s->fd);
+    s->fd = rs->reopen_fd;
+    s->open_flags = bs->open_flags = rs->reopen_flags;
+    g_free(rs);
+    rs = NULL;
+
+    return 0;
+}
+
+static int raw_reopen_abort(BDRVReopenState *rs)
+{
+    if (rs->reopen_fd != -1) {
+        close(rs->reopen_fd);
+        rs->reopen_fd = -1;
+        rs->reopen_flags = 0;
+    }
+    g_free(rs);
+    rs = NULL;
+    return 0;
+}
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -896,6 +956,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_read = raw_read,
     .bdrv_write = raw_write,
     .bdrv_close = raw_close,
@@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = {
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_probe_device  = hdev_probe_device,
     .bdrv_file_open     = hdev_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        = hdev_create,
     .create_options     = raw_create_options,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
 {
     BlockDriver *drv = bs->drv;
     int ret = 0, open_flags;
+    BDRVReopenState *rs;
 
     /* Quiesce IO for the given block device */
     qemu_aio_flush();
@@ -713,20 +714,48 @@ 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 = drv->bdrv_reopen_prepare(bs, &rs, bdrv_flags);
         if (ret < 0) {
-            /* Reopen failed with orig and modified flags */
-            abort();
+            goto fail;
         }
-    }
+        if (drv->bdrv_reopen_commit) {
+            ret = drv->bdrv_reopen_commit(rs);
+            if (ret < 0) {
+                goto fail;
+            }
+            return 0;
+        }
+fail:
+        if (drv->bdrv_reopen_abort) {
+            ret = drv->bdrv_reopen_abort(rs);
+            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+            return ret;
+        }
+
+    } else {
+
+        /* Use reopen procedure common for all drivers */
+        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 with orig and modified flags failed
+                 */
+                abort();
+            }
+        }
+    }
     return ret;
 }
 
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -55,6 +55,13 @@ struct BlockDriver {
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
     int (*bdrv_open)(BlockDriverState *bs, int flags);
+
+    /* For handling image reopen for split or non-split files */
+    int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
+          int flags);
+    int (*bdrv_reopen_commit)(BDRVReopenState *rs);
+    int (*bdrv_reopen_abort)(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);
@@ -207,6 +214,14 @@ struct BlockDriverState {
     void *private;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+    int reopen_flags;
+
+    /* For raw-posix */
+    int reopen_fd;
+};
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -228,6 +228,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/raw.c
===================================================================
--- qemu.orig/block/raw.c
+++ qemu/block/raw.c
@@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs
     return 0;
 }
 
+static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+    int ret = bdrv_reopen(bs->file, flags);
+    if (!ret) {
+        bs->open_flags = bs->file->open_flags;
+    }
+    return ret;
+}
+
 static int raw_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
@@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
     .instance_size      = 1,
 
     .bdrv_open          = raw_open,
+    .bdrv_reopen_prepare
+                        = raw_reopen,
     .bdrv_close         = raw_close,
     .bdrv_read          = raw_read,
     .bdrv_write         = raw_write,

Re: [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting

[Kevin Wolf]

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

Coding style requires braces.

> +    }
> +
>      if (qdict_haskey(bs_dict, "inserted")) {
>          QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
>  
> @@ -1903,11 +1912,14 @@ void bdrv_info(Monitor *mon, QObject **r
>          QDict *bs_dict;
>  
>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> -                                    "'removable': %i, 'locked': %i }",
> +                                    "'removable': %i, 'locked': %i, "
> +                                    "'hostcache': %i }",
>                                      bs->device_name,
>                                      bdrv_dev_has_removable_media(bs),
> -                                    bdrv_dev_is_medium_locked(bs));
> +                                    bdrv_dev_is_medium_locked(bs),
> +                                    !(bs->open_flags & BDRV_O_NOCACHE));
>          bs_dict = qobject_to_qdict(bs_obj);
> +        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));

No. This adds a open_flags field to the QMP structure that is
transferred to clients. This is wrong, open_flags is an internal thing
that should never be visible on an interface.

In bdrv_print_dict, access the hostcache field that you introduced, it
provides the same information.

Kevin

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

[Kevin Wolf]

Am 11.10.2011 05:11, schrieb 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. Simultaneous
> use of 'hostcache' and 'cache' options not allowed.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

I'm not sure if introducing this alone makes sense. I think I would only
do it when we introduce more options that allow replacing all cache=...
options by other parameters.

> 
> ---
>  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;
> @@ -319,7 +320,19 @@ DriveInfo *drive_init(QemuOpts *opts, in
>  	}
>      }
>  
> +    if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
> +        if (!hostcache) {
> +            bdrv_flags |= BDRV_O_NOCACHE;
> +        } else {
> +            bdrv_flags &= ~BDRV_O_NOCACHE;
> +        }

bdrv_flags is initialised to 0, so the else branch is unnecessary.

Kevin

Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Kevin Wolf]

Am 11.10.2011 05:11, schrieb Supriya Kannery:
> Struct BDRVReopenState introduced for handling reopen state of images. 
> This can be  extended by each of the block drivers to reopen respective
> image files. Implementation for raw-posix is done here.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

Maybe it would make sense to split this into two patches, one for the
block.c infrastructure and another one for adding the callbacks in drivers.

> ---
>  block.c           |   46 ++++++++++++++++++++++++++++++++--------
>  block/raw-posix.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block_int.h       |   15 +++++++++++++
>  3 files changed, 114 insertions(+), 9 deletions(-)
> 
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs
>      return raw_open_common(bs, filename, flags, 0);
>  }
>  
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs,
> +                              int flags)
> +{
> +    BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
> +    BDRVRawState *s = bs->opaque;
> +
> +    raw_rs->bs = bs;
> +    raw_rs->reopen_flags = flags;
> +    raw_rs->reopen_fd = -1;
> +
> +    /* If only O_DIRECT to be toggled, use fcntl */
> +    if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
> +            (flags & ~BDRV_O_NOCACHE))) {
> +        raw_rs->reopen_fd = dup(s->fd);
> +        if (raw_rs->reopen_fd <= 0) {
> +            return -1;

This leaks raw_rs.

> +        }
> +    }
> +
> +    /* TBD: Handle O_DSYNC and other flags */
> +    *rs = raw_rs;
> +    return 0;
> +}
> +
> +static int raw_reopen_commit(BDRVReopenState *rs)

bdrv_reopen_commit must never fail. Any error that can happen must
already be handled in bdrv_reopen_prepare.

The commit function should really only do s->fd = rs->reopen_fd (besides
cleanup), everything else should already be prepared.

> +{
> +    BlockDriverState *bs = rs->bs;
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (!rs->reopen_fd) {
> +        return -1;
> +    }
> +
> +    int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);

reopen_flags is BDRV_O_*, not O_*, so it needs to be translated.

> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* Set new flags, so replace old fd with new one */
> +    close(s->fd);
> +    s->fd = rs->reopen_fd;
> +    s->open_flags = bs->open_flags = rs->reopen_flags;
> +    g_free(rs);
> +    rs = NULL;

Setting to NULL has no effect, rs isn't read afterwards.

> +
> +    return 0;
> +}
> +
> +static int raw_reopen_abort(BDRVReopenState *rs)

This must not fail either, so it can be void, too.

> +{
> +    if (rs->reopen_fd != -1) {
> +        close(rs->reopen_fd);
> +        rs->reopen_fd = -1;
> +        rs->reopen_flags = 0;
> +    }
> +    g_free(rs);
> +    rs = NULL;
> +    return 0;
> +}
> +
>  /* XXX: use host sector size if necessary with:
>  #ifdef DIOCGSECTORSIZE
>          {
> @@ -896,6 +956,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_read = raw_read,
>      .bdrv_write = raw_write,
>      .bdrv_close = raw_close,
> @@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = {
>      .instance_size      = sizeof(BDRVRawState),
>      .bdrv_probe_device  = hdev_probe_device,
>      .bdrv_file_open     = hdev_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        = hdev_create,
>      .create_options     = raw_create_options,
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
>  {
>      BlockDriver *drv = bs->drv;
>      int ret = 0, open_flags;
> +    BDRVReopenState *rs;
>  
>      /* Quiesce IO for the given block device */
>      qemu_aio_flush();
> @@ -713,20 +714,48 @@ 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 = drv->bdrv_reopen_prepare(bs, &rs, bdrv_flags);
>          if (ret < 0) {
> -            /* Reopen failed with orig and modified flags */
> -            abort();
> +            goto fail;
>          }
> -    }
> +        if (drv->bdrv_reopen_commit) {
> +            ret = drv->bdrv_reopen_commit(rs);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            return 0;
> +        }

Pull the return 0; out one level. It would be really strange if we
turned a successful prepare into reopen_abort just because the driver
doesn't need a commit function.

(The other consistent way would be to require that if a driver
implements one reopen function, it has to implement all three of them.
I'm fine either way.)

> +fail:
> +        if (drv->bdrv_reopen_abort) {
> +            ret = drv->bdrv_reopen_abort(rs);
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +    } else {
> +
> +        /* Use reopen procedure common for all drivers */
> +        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 with orig and modified flags failed
> +                 */
> +                abort();

Make this bs->drv = NULL, so that trying to access to image will fail,
but at least not the whole VM crashes.

> +            }
> +        }
> +    }
>      return ret;
>  }
>  
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -55,6 +55,13 @@ struct BlockDriver {
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
>      int (*bdrv_open)(BlockDriverState *bs, int flags);
> +
> +    /* For handling image reopen for split or non-split files */
> +    int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
> +          int flags);
> +    int (*bdrv_reopen_commit)(BDRVReopenState *rs);
> +    int (*bdrv_reopen_abort)(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);
> @@ -207,6 +214,14 @@ struct BlockDriverState {
>      void *private;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +    int reopen_flags;
> +
> +    /* For raw-posix */
> +    int reopen_fd;
> +};
> +
>  struct BlockDriverAIOCB {
>      AIOPool *pool;
>      BlockDriverState *bs;
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -228,6 +228,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/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs
>      return 0;
>  }
>  
> +static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> +    int ret = bdrv_reopen(bs->file, flags);
> +    if (!ret) {
> +        bs->open_flags = bs->file->open_flags;
> +    }
> +    return ret;
> +}
> +
>  static int raw_read(BlockDriverState *bs, int64_t sector_num,
>                      uint8_t *buf, int nb_sectors)
>  {
> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
>      .instance_size      = 1,
>  
>      .bdrv_open          = raw_open,
> +    .bdrv_reopen_prepare
> +                        = raw_reopen,
>      .bdrv_close         = raw_close,
>      .bdrv_read          = raw_read,
>      .bdrv_write         = raw_write,

I think raw must pass through all three functions. Otherwise it could
happen that we need to abort, but the image has already been reopened.

Kevin

Re: [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

On 10/12/2011 07:47 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:10, schrieb Supriya Kannery:
>> Enhance "info block" to display hostcache setting for each
>> block device.

>> +    if (qdict_haskey(bs_dict, "open_flags")) {
>> +        int open_flags = qdict_get_int(bs_dict, "open_flags");
>> +        if (open_flags&  BDRV_O_NOCACHE)
>> +            monitor_printf(mon, " hostcache=0");
>> +        else
>> +            monitor_printf(mon, " hostcache=1");
>
> Coding style requires braces.
>

ok..will add. checkpatch.pl didn't catch this!

>>
>>           bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
>> -                                    "'removable': %i, 'locked': %i }",
>> +                                    "'removable': %i, 'locked': %i, "
>> +                                    "'hostcache': %i }",
>>                                       bs->device_name,
>>                                       bdrv_dev_has_removable_media(bs),
>> -                                    bdrv_dev_is_medium_locked(bs));
>> +                                    bdrv_dev_is_medium_locked(bs),
>> +                                    !(bs->open_flags&  BDRV_O_NOCACHE));
>>           bs_dict = qobject_to_qdict(bs_obj);
>> +        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
>
> No. This adds a open_flags field to the QMP structure that is
> transferred to clients. This is wrong, open_flags is an internal thing
> that should never be visible on an interface.
>
> In bdrv_print_dict, access the hostcache field that you introduced, it
> provides the same information.
>

Will replace "open_flags" with "hostcache" field.

thanks, Supriya

Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Stefan Hajnoczi]

On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote:
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
>  {
>      BlockDriver *drv = bs->drv;
>      int ret = 0, open_flags;
> +    BDRVReopenState *rs;

BDRVReopenState *rs = NULL;

If the abort path is taken we need to make sure rs has a defined value.
Note that the abort path currently doesn't handle rs == NULL and will
segfault in raw_reopen_abort().

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

[Supriya Kannery]

On 10/12/2011 08:00 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:11, schrieb 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. Simultaneous
>> use of 'hostcache' and 'cache' options not allowed.
>>
>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> I'm not sure if introducing this alone makes sense. I think I would only
> do it when we introduce more options that allow replacing all cache=...
> options by other parameters.
>

Can we do transition to alternatives for 'cache=' in a phased manner?
Until all other params are ready, we can allow hostcache (as well
as other params as and when they are ready) in cmdline with the
condition that 'cache=x', if specified, overrides these params.
Once we have all other params ready, 'cache=' can be replaced completely.

thanks, Supriya

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

[Kevin Wolf]

Am 14.10.2011 13:19, schrieb Supriya Kannery:
> On 10/12/2011 08:00 PM, Kevin Wolf wrote:
>> Am 11.10.2011 05:11, schrieb 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. Simultaneous
>>> use of 'hostcache' and 'cache' options not allowed.
>>>
>>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>>
>> I'm not sure if introducing this alone makes sense. I think I would only
>> do it when we introduce more options that allow replacing all cache=...
>> options by other parameters.
>>
> 
> Can we do transition to alternatives for 'cache=' in a phased manner?
> Until all other params are ready, we can allow hostcache (as well
> as other params as and when they are ready) in cmdline with the
> condition that 'cache=x', if specified, overrides these params.
> Once we have all other params ready, 'cache=' can be replaced completely.

I guess that would be good enough. There's still not much use in
specifying hostcache=... at the same time as cache=... but at least it
doesn't take away other options then.

Kevin

Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Supriya Kannery]

On 10/12/2011 08:25 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:11, schrieb Supriya Kannery:
>> Struct BDRVReopenState introduced for handling reopen state of images.
>> This can be  extended by each of the block drivers to reopen respective
>> image files. Implementation for raw-posix is done here.
>>
>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> Maybe it would make sense to split this into two patches, one for the
> block.c infrastructure and another one for adding the callbacks in drivers.
>

ok..will split in v8.

>> +
>> +    /* If only O_DIRECT to be toggled, use fcntl */
>> +    if (!((bs->open_flags&  ~BDRV_O_NOCACHE) ^
>> +            (flags&  ~BDRV_O_NOCACHE))) {
>> +        raw_rs->reopen_fd = dup(s->fd);
>> +        if (raw_rs->reopen_fd<= 0) {
>> +            return -1;
>
> This leaks raw_rs.
>

will handle

>> +        }
>> +    }
>> +
>> +    /* TBD: Handle O_DSYNC and other flags */
>> +    *rs = raw_rs;
>> +    return 0;
>> +}
>> +
>> +static int raw_reopen_commit(BDRVReopenState *rs)
>
> bdrv_reopen_commit must never fail. Any error that can happen must
> already be handled in bdrv_reopen_prepare.
>
> The commit function should really only do s->fd = rs->reopen_fd (besides
> cleanup), everything else should already be prepared.
>

will move call to fcntl to bdrv_reopen_prepare.

>> +{
>> +    BlockDriverState *bs = rs->bs;
>> +    BDRVRawState *s = bs->opaque;
>> +
>> +    if (!rs->reopen_fd) {
>> +        return -1;
>> +    }
>> +
>> +    int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
>
> reopen_flags is BDRV_O_*, not O_*, so it needs to be translated.
>

ok

>> +    /* Use driver specific reopen() if available */
>> +    if (drv->bdrv_reopen_prepare) {
>> +        ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags);
>>           if (ret<  0) {
>> -            /* Reopen failed with orig and modified flags */
>> -            abort();
>> +            goto fail;
>>           }
>> -    }
>> +        if (drv->bdrv_reopen_commit) {
>> +            ret = drv->bdrv_reopen_commit(rs);
>> +            if (ret<  0) {
>> +                goto fail;
>> +            }
>> +            return 0;
>> +        }
>
> Pull the return 0; out one level. It would be really strange if we
> turned a successful prepare into reopen_abort just because the driver
> doesn't need a commit function.
>
> (The other consistent way would be to require that if a driver
> implements one reopen function, it has to implement all three of them.
> I'm fine either way.)
>

Will give flexibility to drivers, not mandating all the three functions.

>> +            ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> +            if (ret<  0) {
>> +                /*
>> +                 * Reopen with orig and modified flags failed
>> +                 */
>> +                abort();
>
> Make this bs->drv = NULL, so that trying to access to image will fail,
> but at least not the whole VM crashes.
>

ok

>>   static int raw_read(BlockDriverState *bs, int64_t sector_num,
>>                       uint8_t *buf, int nb_sectors)
>>   {
>> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
>>       .instance_size      = 1,
>>
>>       .bdrv_open          = raw_open,
>> +    .bdrv_reopen_prepare
>> +                        = raw_reopen,
>>       .bdrv_close         = raw_close,
>>       .bdrv_read          = raw_read,
>>       .bdrv_write         = raw_write,
>
> I think raw must pass through all three functions. Otherwise it could
> happen that we need to abort, but the image has already been reopened.
>

ok..got it..will have three separate functions to avoid unnecessary
dependencies.

Got a question..
    In raw-posix, the three functions are implemented for file_reopen 
for now. Should this be extended to hdev, cdrom and floppy?

> Kevin
>

Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Supriya Kannery]

On 10/14/2011 04:43 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote:
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret = 0, open_flags;
>> +    BDRVReopenState *rs;
>
> BDRVReopenState *rs = NULL;
>
> If the abort path is taken we need to make sure rs has a defined value.
> Note that the abort path currently doesn't handle rs == NULL and will
> segfault in raw_reopen_abort().
>

sure..will check on this.

thanks, Supriya

Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen

[Kevin Wolf]

Am 14.10.2011 15:42, schrieb Supriya Kannery:
>>> +    /* Use driver specific reopen() if available */
>>> +    if (drv->bdrv_reopen_prepare) {
>>> +        ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags);
>>>           if (ret<  0) {
>>> -            /* Reopen failed with orig and modified flags */
>>> -            abort();
>>> +            goto fail;
>>>           }
>>> -    }
>>> +        if (drv->bdrv_reopen_commit) {
>>> +            ret = drv->bdrv_reopen_commit(rs);
>>> +            if (ret<  0) {
>>> +                goto fail;
>>> +            }
>>> +            return 0;
>>> +        }
>>
>> Pull the return 0; out one level. It would be really strange if we
>> turned a successful prepare into reopen_abort just because the driver
>> doesn't need a commit function.
>>
>> (The other consistent way would be to require that if a driver
>> implements one reopen function, it has to implement all three of them.
>> I'm fine either way.)
>>
> 
> Will give flexibility to drivers, not mandating all the three functions.

Do we have a use case where it is actually possible to implement less
functions without introducing bugs?

If yes, let's keep it as it is.

> Got a question..
>     In raw-posix, the three functions are implemented for file_reopen 
> for now. Should this be extended to hdev, cdrom and floppy?

Yes, that would be good. And I think the same implementation can be used
for all of them.

Kevin