[Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change

[Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change

[Supriya Kannery]

    For changing host pagecache setting of a running VM, it is
important to have a safe way of reopening its image file.

Following patchset introduces:
 * a generic way to reopen image files safely. 
        In this approach, before reopening an image, for each
    block driver, its state will be stashed. Incase preparation
    (bdrv_reopen_prepare) for reopening returns success, the stashed 
    state will be cleared (bdrv_reopen_commit) and reopened state will 
    be used further. Incase preparation of reopening returns failure, 
    the state of the driver will be rolled back (bdrv_reopen_abort) 
    to the stashed state.
        This approach is extended to raw-posix, raw-win32 and vmdk block 
    drivers in this patchset. Once this is reviewed and finalised, I will 
    extend the implementation to other drivers like qcow2, qed etc..
  
 * qmp and hmp command 'block_set_hostcache' using which host 
   pagecache setting for a block device can be changed 
   when the VM is running.

 * BDRVReopenState, a generic structure which can be 
   extended by each of the block drivers to reopen 
   respective image files.

ToDo:
* memcpy is used to save driver state. Replace this with copying
  individual fields of driver state (?)
* Extend this implementation to other block drivers.
* Build and verify raw-win32 driver changes in windows
 
Earlier discussions related to dynamic change of host pagecache can be found 
at: http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01482.html

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


 qemu/block.c           |  112 +++++++++++++++++++++++++++++++++++++++++++++----
 qemu/block.h           |    5 +
 qemu/block/raw-posix.c |   74 ++++++++++++++++++++++++++++++++
 qemu/block/raw-win32.c |   95 +++++++++++++++++++++++++++++++++++++++++
 qemu/block/raw.c       |   20 ++++++++
 qemu/block/vmdk.c      |   80 +++++++++++++++++++++++++++++++++--
 qemu/block_int.h       |   11 ++++
 qemu/blockdev.c        |   26 +++++++++++
 qemu/blockdev.h        |    2
 qemu/hmp-commands.hx   |   14 ++++++
 qemu/hmp.c             |    2
 qemu/qapi-schema.json  |    4 +
 qemu/qemu-common.h     |    1
 qemu/qerror.c          |    8 +++
 qemu/qerror.h          |    6 ++
 qemu/qmp-commands.hx   |   27 +++++++++++
 18 files changed, 474 insertions(+), 13 deletions(-)                                                                                                                                                                                                                                                                                                                      
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                          
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                           
                                                                                                                                                                       
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
~                                                                                                                                                                           
-- INSERT --

[Qemu-devel] [RFC Patch 1/7]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
@@ -423,6 +423,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)
 #
@@ -436,7 +438,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
@@ -2285,6 +2285,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
@@ -209,6 +209,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] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting

[Luiz Capitulino]

On Wed, 01 Feb 2012 08:36:14 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:

> Enhance "info block" to display hostcache setting for each
> block device.
> 
> Example:
> (qemu) info block
> ide0-hd0: removable=0 file=../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

The day we'll want to refactor 'info block' output is coming...

> 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
> @@ -423,6 +423,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)
>  #
> @@ -436,7 +438,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
> @@ -2285,6 +2285,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
> @@ -209,6 +209,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] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

On 02/08/2012 05:30 PM, Luiz Capitulino wrote:
> On Wed, 01 Feb 2012 08:36:14 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com>  wrote:
>
>> Enhance "info block" to display hostcache setting for each
>> block device.
>>
>> Example:
>> (qemu) info block
>> ide0-hd0: removable=0 file=../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
>
> The day we'll want to refactor 'info block' output is coming...
>

ok :-)

[Qemu-devel] [RFC Patch 2/7]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
@@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
         .desc      = "Device '%(device)' has multiple child busses",
     },
     {
+        .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
@@ -117,6 +117,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 } }"
 
@@ -180,6 +183,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_PERMISSION_DENIED \
     "{ 'class': 'PermissionDenied', 'data': {} }"
 
+#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] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure

[Stefan Hajnoczi]

On Wed, Feb 01, 2012 at 08:36:28AM +0530, Supriya Kannery wrote:
> Index: qemu/qerror.c
> ===================================================================
> --- qemu.orig/qerror.c
> +++ qemu/qerror.c
> @@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
>          .desc      = "Device '%(device)' has multiple child busses",
>      },
>      {
> +        .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)'",
> +    },

The comment in qerror.c says:

"Please keep the entries in alphabetical order.
Use scripts/check-qerror.sh to check."

> +    {
>          .error_fmt = QERR_DEVICE_NO_BUS,
>          .desc      = "Device '%(device)' has no child bus",
>      },
> Index: qemu/qerror.h
> ===================================================================
> --- qemu.orig/qerror.h
> +++ qemu/qerror.h
> @@ -117,6 +117,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 } }"
> +

Same here:

/*
 * QError class list
 * Please keep the definitions in alphabetical order.
 * Use scripts/check-qerror.sh to check.
 */

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

[Supriya Kannery]

On 02/07/2012 01:26 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:28AM +0530, Supriya Kannery wrote:
>> Index: qemu/qerror.c
>> ===================================================================
>> --- qemu.orig/qerror.c
>> +++ qemu/qerror.c
>> @@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
>>           .desc      = "Device '%(device)' has multiple child busses",
>>       },
>>       {
>> +        .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)'",
>> +    },
>
> The comment in qerror.c says:
>
> "Please keep the entries in alphabetical order.
> Use scripts/check-qerror.sh to check."
>

ok

>> +    {
>>           .error_fmt = QERR_DEVICE_NO_BUS,
>>           .desc      = "Device '%(device)' has no child bus",
>>       },
>> Index: qemu/qerror.h
>> ===================================================================
>> --- qemu.orig/qerror.h
>> +++ qemu/qerror.h
>> @@ -117,6 +117,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 } }"
>> +
>
> Same here:
>
> /*
>   * QError class list
>   * Please keep the definitions in alphabetical order.
>   * Use scripts/check-qerror.sh to check.
>   */
>

ok

[Qemu-devel] [RFC Patch 3/7]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
@@ -808,6 +808,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) {
@@ -870,6 +899,33 @@ void bdrv_drain_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 */
+        bdrv_flags &= ~BDRV_O_CACHE_WB;
+        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
@@ -119,6 +119,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);
@@ -160,6 +161,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
@@ -1080,3 +1080,29 @@ BlockJobInfoList *qmp_query_block_jobs(E
     bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
     return dummy.next;
 }
+
+
+/*
+ * 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
@@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(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
@@ -796,6 +796,20 @@ ETEXI
 	.mhandler.cmd_new = do_migrate,
     },
 
+    {
+        .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{option}
+@findex block_set_hostcache
+Change host pagecache setting of a block device while guest is running.
+ETEXI
+
 
 STEXI
 @item migrate [-d] [-b] [-i] @var{uri}
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -698,7 +698,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",
         .mhandler.cmd_new = qmp_marshal_input_balloon,

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

[Michael Roth]

On 01/31/2012 09:06 PM, Supriya Kannery 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
> @@ -808,6 +808,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) {
> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
> +        bdrv_flags&= ~BDRV_O_CACHE_WB;
> +        return bdrv_reopen(bs, bdrv_flags);

It seems like the real interface we're wanting here is bdrv_set_flags(), 
or something along that line, with the re-opening being more of an 
implementation detail.

For instance, with raw-posix.c:raw_reopen_prepare() we'll end up 
skipping the re-opening completely if fcntl() is sufficient.

> +    } 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
> @@ -119,6 +119,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);
> @@ -160,6 +161,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
> @@ -1080,3 +1080,29 @@ BlockJobInfoList *qmp_query_block_jobs(E
>       bdrv_iterate(do_qmp_query_block_jobs_one,&prev);
>       return dummy.next;
>   }
> +
> +
> +/*
> + * 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
> @@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
>                            bool has_format, const char *format, Error **errp);
>   void do_commit(Monitor *mon, const QDict *qdict);
>   int do_drive_del(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
> @@ -796,6 +796,20 @@ ETEXI
>   	.mhandler.cmd_new = do_migrate,
>       },
>
> +    {
> +        .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{option}
> +@findex block_set_hostcache
> +Change host pagecache setting of a block device while guest is running.
> +ETEXI
> +
>
>   STEXI
>   @item migrate [-d] [-b] [-i] @var{uri}
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -698,7 +698,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",
>           .mhandler.cmd_new = qmp_marshal_input_balloon,
>
>

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

[Kevin Wolf]

Am 02.02.2012 01:09, schrieb Michael Roth:
> On 01/31/2012 09:06 PM, Supriya Kannery 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
>> @@ -808,6 +808,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) {
>> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
>> +        bdrv_flags&= ~BDRV_O_CACHE_WB;
>> +        return bdrv_reopen(bs, bdrv_flags);
> 
> It seems like the real interface we're wanting here is bdrv_set_flags(), 
> or something along that line, with the re-opening being more of an 
> implementation detail.
> 
> For instance, with raw-posix.c:raw_reopen_prepare() we'll end up 
> skipping the re-opening completely if fcntl() is sufficient.

It's reopening a BlockDriverState, not necessarily reopening the image
file that backs it.

bdrv_set_flags() would be good name for what this series is doing, but
I've been thinking about adding a way to actually switch the image file
as well. We could need this for implementing external snapshots of
multiple images atomically.

Kevin

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

[Luiz Capitulino]

On Wed, 01 Feb 2012 08:36:41 +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
> @@ -808,6 +808,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);

OPEN_FILE_FAILED is fine.

> +        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) {
> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
> +        bdrv_flags &= ~BDRV_O_CACHE_WB;
> +        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
> @@ -119,6 +119,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);
> @@ -160,6 +161,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
> @@ -1080,3 +1080,29 @@ BlockJobInfoList *qmp_query_block_jobs(E
>      bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
>      return dummy.next;
>  }
> +
> +
> +/*
> + * Change host page cache setting while guest is running.
> +*/
> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
> +                           QObject **ret_data)

This is not a QAPI command, please read docs/writing-qmp-commands.txt to know
how to write QMP commands using the QAPI.

> +{
> +    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
> @@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
>                           bool has_format, const char *format, Error **errp);
>  void do_commit(Monitor *mon, const QDict *qdict);
>  int do_drive_del(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
> @@ -796,6 +796,20 @@ ETEXI
>  	.mhandler.cmd_new = do_migrate,
>      },
>  
> +    {
> +        .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{option}
> +@findex block_set_hostcache
> +Change host pagecache setting of a block device while guest is running.
> +ETEXI
> +
>  
>  STEXI
>  @item migrate [-d] [-b] [-i] @var{uri}
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -698,7 +698,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",
>          .mhandler.cmd_new = qmp_marshal_input_balloon,
> 

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

[Supriya Kannery]

On 02/08/2012 05:37 PM, Luiz Capitulino wrote:
> On Wed, 01 Feb 2012 08:36:41 +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);
>
> OPEN_FILE_FAILED is fine.
>

ok

>> +/*
>> + * Change host page cache setting while guest is running.
>> +*/
>> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
>> +                           QObject **ret_data)
>
> This is not a QAPI command, please read docs/writing-qmp-commands.txt to know
> how to write QMP commands using the QAPI.
>

fine, will check the doc

-thanks, Supriya

[Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Supriya Kannery]

Struct BDRVReopenState along with three reopen related functions
introduced for handling reopening 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
@@ -808,10 +808,32 @@ unlink_and_fail:
     return ret;
 }
 
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+     BlockDriver *drv = bs->drv;
+
+     return drv->bdrv_reopen_prepare(bs, prs, flags);
+}
+
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BlockDriver *drv = bs->drv;
+
+    drv->bdrv_reopen_commit(bs, rs);
+}
+
+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();
@@ -820,17 +842,32 @@ 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);
+        bs->open_flags = 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
@@ -105,6 +105,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 **prs,
+                               int flags);
+    void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs);
+    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);
@@ -299,6 +306,10 @@ struct BlockDriverState {
     BlockJob *job;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+};
+
 struct BlockDriverAIOCB {
     AIOPool *pool;
     BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -210,6 +210,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
@@ -120,6 +120,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);
+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] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Stefan Hajnoczi]

On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening 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
> @@ -808,10 +808,32 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> +     BlockDriver *drv = bs->drv;
> +
> +     return drv->bdrv_reopen_prepare(bs, prs, flags);

Indentation should be 4 spaces.  I suggest configuring your editor so
this is always correct and done automatically.

> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +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();
> @@ -820,17 +842,32 @@ 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) {

Indentation

> +            bdrv_reopen_abort(bs, reopen_state);
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            return ret;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

Is bs->open_flags not assigned inside bdrv_reopen_*()?

> +
> +    } 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;

This should be a post-condition of bdrv_open().  If you have found a
case where bs->drv != NULL after bdrv_open() fails, then please fix that
code path instead of assigning NULL here.

Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Supriya Kannery]

On 02/07/2012 03:38 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote:
>> Struct BDRVReopenState along with three reopen related functions
>> introduced for handling reopening of images safely. This can be
>> extended by each of the block drivers to reopen respective
>> image files.
>>

>> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
>> +{
>> +     BlockDriver *drv = bs->drv;
>> +
>> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
>
> Indentation should be 4 spaces.  I suggest configuring your editor so
> this is always correct and done automatically.
>

ok

>> -    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) {
>
> Indentation
>
>> +            bdrv_reopen_abort(bs, reopen_state);
>> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> +            return ret;
>> +        }
>> +
>> +        bdrv_reopen_commit(bs, reopen_state);
>> +        bs->open_flags = bdrv_flags;
>
> Is bs->open_flags not assigned inside bdrv_reopen_*()?
>

No, Since it is generic for all the drivers, placed it here.

>> +
>> +    } 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;
>
> This should be a post-condition of bdrv_open().  If you have found a
> case where bs->drv != NULL after bdrv_open() fails, then please fix that
> code path instead of assigning NULL here.
>

ok, will check on this

-thanks for reviewing, Supriya

Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Kevin Wolf]

Am 01.02.2012 04:06, schrieb Supriya Kannery:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening 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
> @@ -808,10 +808,32 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> +     BlockDriver *drv = bs->drv;
> +
> +     return drv->bdrv_reopen_prepare(bs, prs, flags);
> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +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();
> @@ -820,17 +842,32 @@ 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);
> +        bs->open_flags = 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;
> +            }
>          }

Most image formats don't have a bdrv_reopen_* implementation after this
series, so usually you'll have something like qcow2 on top of file. This
code uses bdrv_close/open for the whole stack, even though the file
layer could actually make use of a bdrv_reopen_* implementation and the
qcow2 open isn't likely to fail if the image file could be opened.

I think we can use drv->bdrv_close/open to reopen only one layer and try
using bdrv_reopen_* for the lower layer again.

This is an improvement that can be done in a separate patch, though.

Kevin

Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely

[Supriya Kannery]

On 02/08/2012 08:37 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:06, schrieb Supriya Kannery:
>> Struct BDRVReopenState along with three reopen related functions
>> introduced for handling reopening of images safely. This can be
>> extended by each of the block drivers to reopen respective
>> image files.

>> +    } 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;
>> +            }
>>           }
>
> Most image formats don't have a bdrv_reopen_* implementation after this
> series, so usually you'll have something like qcow2 on top of file. This
> code uses bdrv_close/open for the whole stack, even though the file
> layer could actually make use of a bdrv_reopen_* implementation and the
> qcow2 open isn't likely to fail if the image file could be opened.
>
> I think we can use drv->bdrv_close/open to reopen only one layer and try
> using bdrv_reopen_* for the lower layer again.
>
> This is an improvement that can be done in a separate patch, though.

What I understood is, in the enhancement patch, we will have something 
like (taking qcow2 as an example)

Implement bdrv_reopen_qcow2(image file) which reopens only the qcow2
image file

Then,  drv->bdrv_open(qcow2 driver) will reopen qcow2 driver
        => calls bdrv_reopen_qcow2(qcow2 image file) if image file has
           to be reopen

Can you please explain a bit more, it this is not what you meant.

-thanks, Supriya

[Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[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,22 @@ 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)
+{
+    bdrv_reopen_commit(bs->file, rs);
+}
+
+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)
 {
@@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
     .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,
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 {
+    BDRVReopenState reopen_state;
+    BDRVRawState *stash_s;
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 
@@ -279,6 +284,71 @@ 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.bs = bs;
+
+    /* stash state before reopen */
+    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
+    memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
+    s->fd = dup(raw_rs->stash_s->fd);
+
+    *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)) {
+        if ((flags & BDRV_O_NOCACHE)) {
+            s->open_flags |= O_DIRECT;
+        } else {
+            s->open_flags &= ~O_DIRECT;
+        }
+        printf("O_DIRECT flag\n");
+        ret = fcntl_setfl(s->fd, s->open_flags);
+    } else {
+
+        printf("close and open with new flags\n");
+        /* close and reopen using new flags */
+        bs->drv->bdrv_close(bs);
+        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
+    }
+    return ret;
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVRawReopenState *raw_rs;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    /* clean up stashed state */
+    close(raw_rs->stash_s->fd);
+    g_free(raw_rs->stash_s);
+    g_free(raw_rs);
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVRawReopenState *raw_rs;
+    BDRVRawState *s = bs->opaque;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    /* revert to stashed state */
+    if (s->fd != -1) {
+        close(s->fd);
+    }
+    memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState));
+    g_free(raw_rs->stash_s);
+    g_free(raw_rs);
+}
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -631,6 +701,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] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Michael Roth]

On 01/31/2012 09:07 PM, Supriya Kannery wrote:
> 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,22 @@ 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)
> +{
> +    bdrv_reopen_commit(bs->file, rs);
> +}
> +
> +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)
>   {
> @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
>       .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,
> 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 {
> +    BDRVReopenState reopen_state;
> +    BDRVRawState *stash_s;
> +} BDRVRawReopenState;
> +
>   static int fd_open(BlockDriverState *bs);
>   static int64_t raw_getlength(BlockDriverState *bs);
>
> @@ -279,6 +284,71 @@ 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.bs = bs;
> +
> +    /* stash state before reopen */
> +    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> +    memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
> +    s->fd = dup(raw_rs->stash_s->fd);
> +
> +    *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)) {
> +        if ((flags&  BDRV_O_NOCACHE)) {
> +            s->open_flags |= O_DIRECT;
> +        } else {
> +            s->open_flags&= ~O_DIRECT;
> +        }
> +        printf("O_DIRECT flag\n");
> +        ret = fcntl_setfl(s->fd, s->open_flags);

raw-posix.c:raw_aio_submit() does some extra alignment work if 
s->aligned_buf was set due to the image being opened O_DIRECT initially, 
not sure what the impact is but probably want to clean that up here.

> +    } else {
> +
> +        printf("close and open with new flags\n");
> +        /* close and reopen using new flags */
> +        bs->drv->bdrv_close(bs);
> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
> +    }
> +    return ret;
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BDRVRawReopenState *raw_rs;
> +
> +    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
> +
> +    /* clean up stashed state */
> +    close(raw_rs->stash_s->fd);
> +    g_free(raw_rs->stash_s);
> +    g_free(raw_rs);
> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BDRVRawReopenState *raw_rs;
> +    BDRVRawState *s = bs->opaque;
> +
> +    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
> +
> +    /* revert to stashed state */
> +    if (s->fd != -1) {
> +        close(s->fd);
> +    }
> +    memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState));
> +    g_free(raw_rs->stash_s);
> +    g_free(raw_rs);
> +}
> +
>   /* XXX: use host sector size if necessary with:
>   #ifdef DIOCGSECTORSIZE
>           {
> @@ -631,6 +701,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] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Supriya Kannery]

On 02/02/2012 05:45 AM, Michael Roth wrote:
> On 01/31/2012 09:07 PM, Supriya Kannery wrote:
>> 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.
>>

>> +
>> + /* Flags that can be set using fcntl */
>> + int fcntl_flags = BDRV_O_NOCACHE;
>> +
>> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) {
>> + if ((flags& BDRV_O_NOCACHE)) {
>> + s->open_flags |= O_DIRECT;
>> + } else {
>> + s->open_flags&= ~O_DIRECT;
>> + }
>> + printf("O_DIRECT flag\n");
>> + ret = fcntl_setfl(s->fd, s->open_flags);
>
> raw-posix.c:raw_aio_submit() does some extra alignment work if
> s->aligned_buf was set due to the image being opened O_DIRECT initially,
> not sure what the impact is but probably want to clean that up here.
>

ok, will check on this

thanks! for reviewing
Supriya

Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Stefan Hajnoczi]

On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote:
> +    /* stash state before reopen */
> +    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> +    memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));

Copying a struct is fragile, Mike Roth pointed out the potential issue
with aligned_buf.

If raw-posix could open from a given file descriptor as an alternative
to opening a filename, then it would be clean and natural to simply
re-initialize from the dup'd file descriptor in the abort case.  That's
the approach I would try instead of stashing the whole struct.

Stefan

Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Supriya Kannery]

On 02/07/2012 03:47 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote:
>> +    /* stash state before reopen */
>> +    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>> +    memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
>
> Copying a struct is fragile, Mike Roth pointed out the potential issue
> with aligned_buf.
>
> If raw-posix could open from a given file descriptor as an alternative
> to opening a filename, then it would be clean and natural to simply
> re-initialize from the dup'd file descriptor in the abort case.  That's
> the approach I would try instead of stashing the whole struct.
>
> Stefan
>

fine, will get V1 with stashing of only required fields wherever
possible instead of stashing the full struct.

Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Kevin Wolf]

Am 01.02.2012 04:07, schrieb Supriya Kannery:
> raw-posix driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while 
> changing hostcache dynamically is handled here.
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,22 @@ 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)
> +{
> +    bdrv_reopen_commit(bs->file, rs);
> +}
> +
> +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)
>  {
> @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
>      .instance_size      = 1,
>  
>      .bdrv_open          = raw_open,
> +    .bdrv_reopen_prepare
> +                        = raw_reopen_prepare,

You can just indent to the next level instead of line wrapping.

> +    .bdrv_reopen_commit = raw_reopen_commit,
> +    .bdrv_reopen_abort  = raw_reopen_abort,
>      .bdrv_close         = raw_close,
>  
>      .bdrv_co_readv          = raw_co_readv,
> 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 {
> +    BDRVReopenState reopen_state;
> +    BDRVRawState *stash_s;
> +} BDRVRawReopenState;

See Stefan's comment. If it's possible to save only the fd and maybe one
or two other fields, then we should do that.

>  static int fd_open(BlockDriverState *bs);
>  static int64_t raw_getlength(BlockDriverState *bs);
>  
> @@ -279,6 +284,71 @@ 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.bs = bs;
> +
> +    /* stash state before reopen */
> +    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> +    memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
> +    s->fd = dup(raw_rs->stash_s->fd);
> +
> +    *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)) {
> +        if ((flags & BDRV_O_NOCACHE)) {
> +            s->open_flags |= O_DIRECT;
> +        } else {
> +            s->open_flags &= ~O_DIRECT;
> +        }
> +        printf("O_DIRECT flag\n");

Debugging leftover?

> +        ret = fcntl_setfl(s->fd, s->open_flags);
> +    } else {
> +
> +        printf("close and open with new flags\n");

Same here.

Kevin

Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen

[Supriya Kannery]

On 02/08/2012 08:24 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:07, schrieb Supriya Kannery:
>> raw-posix driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>

>>
>> +typedef struct BDRVRawReopenState {
>> +    BDRVReopenState reopen_state;
>> +    BDRVRawState *stash_s;
>> +} BDRVRawReopenState;
>
> See Stefan's comment. If it's possible to save only the fd and maybe one
> or two other fields, then we should do that.
>

Yes, for V1 of this patchset, will look for stashing only those relevant
fields of a driver state wherever possible

>> +
>> +    if ((bs->open_flags&  ~fcntl_flags) == (flags&  ~fcntl_flags)) {
>> +        if ((flags&  BDRV_O_NOCACHE)) {
>> +            s->open_flags |= O_DIRECT;
>> +        } else {
>> +            s->open_flags&= ~O_DIRECT;
>> +        }
>> +        printf("O_DIRECT flag\n");
>
> Debugging leftover?
>

yes :-), didn't do a proper cleanup as this is RFC
for the stashing approach.

>> +        ret = fcntl_setfl(s->fd, s->open_flags);
>> +    } else {
>> +
>> +        printf("close and open with new flags\n");
>
> Same here.
>

V1 will be a clean one !

> Kevin
>

[Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen

[Supriya Kannery]

win32  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-win32.c
===================================================================
--- qemu.orig/block/raw-win32.c
+++ qemu/block/raw-win32.c
@@ -26,18 +26,27 @@
 #include "block_int.h"
 #include "module.h"
 #include <windows.h>
+#include <winbase.h>
 #include <winioctl.h>
 
 #define FTYPE_FILE 0
 #define FTYPE_CD     1
 #define FTYPE_HARDDISK 2
+#define WINDOWS_VISTA 6
 
 typedef struct BDRVRawState {
     HANDLE hfile;
     int type;
     char drive_path[16]; /* format: "d:\" */
+    DWORD overlapped;
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    BDRVReopenState reopen_state;
+    HANDLE stash_hfile;
+    DWORD  stash_overlapped;
+} BDRVRawReopenState;
+
 int qemu_ftruncate64(int fd, int64_t length)
 {
     LARGE_INTEGER li;
@@ -106,9 +115,96 @@ static int raw_open(BlockDriverState *bs
             return -EACCES;
         return -1;
     }
+    s->overlapped = overlapped;
     return 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;
+    OSVERSIONINFO osvi;
+    BOOL bIsWindowsVistaorLater;
+
+    raw_rs->bs = bs;
+    raw_rs->stash_hfile = s->hfile;
+    raw_rs->stash_overlapped = s->overlapped;
+    *prs = raw_rs;
+
+    if (flags & BDRV_O_NOCACHE) {
+        s->overlapped |= FILE_FLAG_NO_BUFFERING;
+    } else {
+        s->overlapped &= ~FILE_FLAG_NO_BUFFERING;
+    }
+
+    if (!(flags & BDRV_O_CACHE_WB)) {
+        s->overlapped |= FILE_FLAG_WRITE_THROUGH;
+    } else {
+        s->overlapped &= ~FILE_FLAG_WRITE_THROUGH;
+    }
+
+    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
+    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+
+    GetVersionEx(&osvi);
+
+    if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
+        s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
+                              overlapped);
+        if (s->hfile == INVALID_HANDLE_VALUE) {
+            int err = GetLastError();
+            if (err == ERROR_ACCESS_DENIED) {
+                ret = -EACCES;
+            } else {
+                ret = -1;
+            }
+        }
+    } else {
+
+        DuplicateHandle(GetCurrentProcess(),
+                    raw_rs->stash_hfile,
+                    GetCurrentProcess(),
+                    &s->hfile,
+                    0,
+                    FALSE,
+                    DUPLICATE_SAME_ACCESS);
+        bs->drv->bdrv_close(bs);
+        ret = bs->drv->bdrv_open(bs, bs->filename, flags);
+    }
+    return ret;
+}
+
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVRawReopenState *raw_rs;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    /* clean up stashed handle */
+    CloseHandle(raw_rs->stash_hfile);
+    g_free(raw_rs);
+
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+
+    BDRVRawReopenState *raw_rs;
+    BDRVRawState *s = bs->opaque;
+
+    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+    if (s->hfile && (s->hfile != INVALID_HANDLE_VALUE)) {
+        CloseHandle(s->hfile);
+    }
+    s->hfile = raw_rs->stash_hfile;
+    s->overlapped = raw_rs->stash_overlapped;
+    g_free(raw_rs);
+}
+
 static int raw_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {

Re: [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen

[Kevin Wolf]

Am 01.02.2012 04:07, schrieb Supriya Kannery:
> win32  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-win32.c
> ===================================================================
> --- qemu.orig/block/raw-win32.c
> +++ qemu/block/raw-win32.c
> @@ -26,18 +26,27 @@
>  #include "block_int.h"
>  #include "module.h"
>  #include <windows.h>
> +#include <winbase.h>
>  #include <winioctl.h>
>  
>  #define FTYPE_FILE 0
>  #define FTYPE_CD     1
>  #define FTYPE_HARDDISK 2
> +#define WINDOWS_VISTA 6
>  
>  typedef struct BDRVRawState {
>      HANDLE hfile;
>      int type;
>      char drive_path[16]; /* format: "d:\" */
> +    DWORD overlapped;
>  } BDRVRawState;
>  
> +typedef struct BDRVRawReopenState {
> +    BDRVReopenState reopen_state;
> +    HANDLE stash_hfile;
> +    DWORD  stash_overlapped;
> +} BDRVRawReopenState;
> +
>  int qemu_ftruncate64(int fd, int64_t length)
>  {
>      LARGE_INTEGER li;
> @@ -106,9 +115,96 @@ static int raw_open(BlockDriverState *bs
>              return -EACCES;
>          return -1;
>      }
> +    s->overlapped = overlapped;
>      return 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;
> +    OSVERSIONINFO osvi;
> +    BOOL bIsWindowsVistaorLater;
> +
> +    raw_rs->bs = bs;
> +    raw_rs->stash_hfile = s->hfile;
> +    raw_rs->stash_overlapped = s->overlapped;
> +    *prs = raw_rs;
> +
> +    if (flags & BDRV_O_NOCACHE) {
> +        s->overlapped |= FILE_FLAG_NO_BUFFERING;
> +    } else {
> +        s->overlapped &= ~FILE_FLAG_NO_BUFFERING;
> +    }
> +
> +    if (!(flags & BDRV_O_CACHE_WB)) {
> +        s->overlapped |= FILE_FLAG_WRITE_THROUGH;
> +    } else {
> +        s->overlapped &= ~FILE_FLAG_WRITE_THROUGH;
> +    }
> +
> +    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
> +    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +
> +    GetVersionEx(&osvi);
> +
> +    if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
> +        s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
> +                              overlapped);
> +        if (s->hfile == INVALID_HANDLE_VALUE) {
> +            int err = GetLastError();
> +            if (err == ERROR_ACCESS_DENIED) {
> +                ret = -EACCES;
> +            } else {
> +                ret = -1;

Returning -1 where -errno is expected is bad (turns out as -EPERM on
Linux, which is misleading). Maybe -EIO here.

Kevin

Re: [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen

[Supriya Kannery]

On 02/08/2012 08:32 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:07, schrieb Supriya Kannery:
>> win32  driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>> +
>> +    if (osvi.dwMajorVersion>= WINDOWS_VISTA) {
>> +        s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
>> +                              overlapped);
>> +        if (s->hfile == INVALID_HANDLE_VALUE) {
>> +            int err = GetLastError();
>> +            if (err == ERROR_ACCESS_DENIED) {
>> +                ret = -EACCES;
>> +            } else {
>> +                ret = -1;
>
> Returning -1 where -errno is expected is bad (turns out as -EPERM on
> Linux, which is misleading). Maybe -EIO here.

ok

-thanks, Supriya

[Qemu-devel] [RFC Patch 7/7]Qemu: vmdk image file reopen

[Supriya Kannery]

vmdk driver changes for bdrv_reopen_xx functions to
safely reopen image files. Reopening of image files while
changing hostcache flag dynamically is handled here.

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

Index: qemu/block/vmdk.c
===================================================================
--- qemu.orig/block/vmdk.c
+++ qemu/block/vmdk.c
@@ -115,6 +115,11 @@ typedef struct VmdkGrainMarker {
     uint8_t  data[0];
 } VmdkGrainMarker;
 
+typedef struct BDRVVmdkReopenState {
+    BDRVReopenState reopen_state;
+    BDRVVmdkState *stash_s;
+} BDRVVmdkReopenState;
+
 static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     uint32_t magic;
@@ -588,7 +593,6 @@ static int vmdk_parse_extents(const char
         if (!strcmp(type, "FLAT")) {
             /* FLAT extent */
             VmdkExtent *extent;
-
             extent = vmdk_add_extent(bs, extent_file, true, sectors,
                             0, 0, 0, 0, sectors);
             extent->flat_start_offset = flat_offset << 9;
@@ -675,6 +679,71 @@ fail:
     return ret;
 }
 
+static int vmdk_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                               int flags)
+{
+    BDRVVmdkReopenState *vmdk_rs = g_malloc0(sizeof(BDRVVmdkReopenState));
+    int ret = 0;
+    BDRVVmdkState *s = bs->opaque;
+
+    vmdk_rs->reopen_state.bs = bs;
+
+    /* save state before reopen */
+    vmdk_rs->stash_s = g_malloc0(sizeof(BDRVVmdkState));
+    memcpy(vmdk_rs->stash_s, s, sizeof(BDRVVmdkState));
+    s->num_extents = 0;
+    s->extents = NULL ;
+    *prs = &(vmdk_rs->reopen_state);
+
+    /* create extents afresh with new flags */
+     ret = vmdk_open(bs, flags);
+     return ret;
+}
+
+static void vmdk_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVVmdkReopenState *vmdk_rs;
+    BDRVVmdkState *stashed_s;
+    VmdkExtent *e;
+    int i;
+
+    vmdk_rs = container_of(rs, BDRVVmdkReopenState, reopen_state);
+    stashed_s = vmdk_rs->stash_s;
+
+    /* clean up stashed state */
+    for (i = 0; i < stashed_s->num_extents; i++) {
+        e = &stashed_s->extents[i];
+        g_free(e->l1_table);
+        g_free(e->l2_cache);
+        g_free(e->l1_backup_table);
+    }
+    g_free(stashed_s->extents);
+    g_free(vmdk_rs->stash_s);
+    g_free(vmdk_rs);
+}
+
+static void vmdk_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVVmdkReopenState *vmdk_rs;
+    BDRVVmdkState *s = bs->opaque;
+    VmdkExtent *e;
+    int i;
+
+    vmdk_rs = container_of(rs, BDRVVmdkReopenState, reopen_state);
+
+    /* revert to stashed state */
+    for (i = 0; i < s->num_extents; i++) {
+        e = &s->extents[i];
+        g_free(e->l1_table);
+        g_free(e->l2_cache);
+        g_free(e->l1_backup_table);
+    }
+    g_free(s->extents);
+    memcpy(s, vmdk_rs->stash_s, sizeof(BDRVVmdkState));
+    g_free(vmdk_rs->stash_s);
+    g_free(vmdk_rs);
+}
+
 static int get_whole_cluster(BlockDriverState *bs,
                 VmdkExtent *extent,
                 uint64_t cluster_offset,
@@ -1382,7 +1451,6 @@ static int vmdk_create(const char *filen
     if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
         return -EINVAL;
     }
-    printf("vmdk_create\n");
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
@@ -1517,8 +1585,8 @@ exit:
 static void vmdk_close(BlockDriverState *bs)
 {
     BDRVVmdkState *s = bs->opaque;
-
 printf("vmdk_close\n");
+
     vmdk_free_extents(bs);
 
     migrate_del_blocker(s->migration_blocker);
@@ -1595,6 +1663,12 @@ static BlockDriver bdrv_vmdk = {
     .instance_size  = sizeof(BDRVVmdkState),
     .bdrv_probe     = vmdk_probe,
     .bdrv_open      = vmdk_open,
+    .bdrv_reopen_prepare
+                    = vmdk_reopen_prepare,
+    .bdrv_reopen_commit
+                    = vmdk_reopen_commit,
+    .bdrv_reopen_abort
+                    = vmdk_reopen_abort,
     .bdrv_read      = vmdk_co_read,
     .bdrv_write     = vmdk_co_write,
     .bdrv_close     = vmdk_close,

Re: [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

On 01/31/2012 08:05 PM, Supriya Kannery wrote:
>     For changing host pagecache setting of a running VM, it is
> important to have a safe way of reopening its image file.
> 
> Following patchset introduces:
>  * a generic way to reopen image files safely. 
>         In this approach, before reopening an image, for each
>     block driver, its state will be stashed. Incase preparation
>     (bdrv_reopen_prepare) for reopening returns success, the stashed 
>     state will be cleared (bdrv_reopen_commit) and reopened state will 
>     be used further. Incase preparation of reopening returns failure, 
>     the state of the driver will be rolled back (bdrv_reopen_abort) 
>     to the stashed state.
>         This approach is extended to raw-posix, raw-win32 and vmdk block 
>     drivers in this patchset. Once this is reviewed and finalised, I will 
>     extend the implementation to other drivers like qcow2, qed etc..

How will this interplay with the goal of passing images in by fd rather
than by name?  I'd really like to start thinking about how we plan on
coordinating situations where an fd has to be reopened in order to
switch flags (such as O_RDONLY becoming O_RDWR, or adding or subtracting
O_DIRECT), but where SELinux or other isolation means that the
management app (such as libvirt) has to do the open and pass the fd via
'getfd' monitor command.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change

[Kevin Wolf]

Am 01.02.2012 23:41, schrieb Eric Blake:
> On 01/31/2012 08:05 PM, Supriya Kannery wrote:
>>     For changing host pagecache setting of a running VM, it is
>> important to have a safe way of reopening its image file.
>>
>> Following patchset introduces:
>>  * a generic way to reopen image files safely. 
>>         In this approach, before reopening an image, for each
>>     block driver, its state will be stashed. Incase preparation
>>     (bdrv_reopen_prepare) for reopening returns success, the stashed 
>>     state will be cleared (bdrv_reopen_commit) and reopened state will 
>>     be used further. Incase preparation of reopening returns failure, 
>>     the state of the driver will be rolled back (bdrv_reopen_abort) 
>>     to the stashed state.
>>         This approach is extended to raw-posix, raw-win32 and vmdk block 
>>     drivers in this patchset. Once this is reviewed and finalised, I will 
>>     extend the implementation to other drivers like qcow2, qed etc..
> 
> How will this interplay with the goal of passing images in by fd rather
> than by name?  I'd really like to start thinking about how we plan on
> coordinating situations where an fd has to be reopened in order to
> switch flags (such as O_RDONLY becoming O_RDWR, or adding or subtracting
> O_DIRECT), but where SELinux or other isolation means that the
> management app (such as libvirt) has to do the open and pass the fd via
> 'getfd' monitor command.

That's easy: Either your host OS allows to change the respective flag
using fcntl(), which I believe is true for O_DIRECT/O_DSYNC and recent
Linux kernels, or you're out of luck.

Is there any reason for switching between rw/ro other than modifying the
backing file chain (i.e. creating or deleting a snapshot)?

Kevin