[Qemu-devel] [v2 Patch 0/9]block: Image file reopen and dynamic host pagecache change

[Qemu-devel] [v2 Patch 0/9]block: Image file reopen and 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.

V1 introduced:
 * 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 implemented for raw-posix, 
    raw-win32, vmdk, qcow, qcow2 and qed block drivers.
  
 * 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.

V2:
 * Changed ordering of patches such that code changes related to 
   generic framework for safely reopening images gets applied first.

 * For block drivers not having bdrv_reopen_xx functions 
   implemented, return "feature not supported" error.

Testing:
=======
[Thanks! to Yoganananth Subramanian for helping out with testing]

Steps:
1) boot up guest image of different formats qed, raw, qcow2, vmdk
2) run iozone in these guests
   command: iozone -a
3) view cache setting of image file through qemu monitor
   command: info block
            "hostcache =" 0/1 should be displayed
4) Toggle hostcache value using block_set_hostcache
   command: block_set_hostcache virtio0 on
5) Disable and enable hostcache at randon intervals while iozone is
   running inside guest.
   command: block_set_hostcache virtio0 on/off
6) Info block should reflect toggled hostcache value
   and iozone should complete without any issue

Results:
  Verified above steps for raw-posix, qcow2, qed and vmdk images.
  raw-posix, qed and vmdk images (split files) worked fine. With
  qcow2 image getting an error of double free after iozone running
  for a while.

To Do:
======
* Debug the issue with qcow2 image and resolve asap.
* Enhance code around dup3 in raw-posix to fall back to
  dup2/dup when dup3 is not supported by OS.
* Do some more extensive testing, especially with qcow2 and
  qed drivers.

 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           |   79 ++++++++++++++++++++++
 qemu/block.h           |    5 +
 qemu/block/qcow.c      |  108 ++++++++++++++++++++++++++++++
 qemu/block/qcow2.c     |  175 +++++++++++++++++++++++++++++++++++++++++++++++++
 qemu/block/qed.c       |  103 ++++++++++++++++++++++++++++
 qemu/block/raw-posix.c |  121 +++++++++++++++++++++++++++++++++
 qemu/block/raw-win32.c |   96 ++++++++++++++++++++++++++
 qemu/block/raw.c       |   20 +++++
 qemu/block/vmdk.c      |  103 ++++++++++++++++++++++++++++
 qemu/block_int.h       |   12 +++
 qemu/blockdev.c        |   19 +++++
 qemu/hmp-commands.hx   |   15 ++++
 qemu/hmp.c             |   11 ++
 qemu/hmp.h             |    1
 qemu/qapi-schema.json  |   21 ++++-
 qemu/qemu-common.h     |    1
 qemu/qmp-commands.hx   |   24 ++++++
 17 files changed, 912 insertions(+), 2 deletions(-)

[Qemu-devel] [v2 Patch 1/9]block: 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
@@ -859,6 +859,60 @@ 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);
+}
+
+void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+    int ret = 0;
+    BDRVReopenState *reopen_state = NULL;
+
+    /* Quiesce IO for the given block device */
+    bdrv_drain_all();
+    ret = bdrv_flush(bs);
+    if (ret != 0) {
+        error_set(errp, QERR_IO_ERROR);
+        return;
+    }
+
+    /* 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);
+            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
+            return;
+        }
+
+        bdrv_reopen_commit(bs, reopen_state);
+        bs->open_flags = bdrv_flags;
+    } else {
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  drv->format_name, bs->device_name,
+                  "reopening of file");
+        return;
+    }
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -136,6 +136,14 @@ struct BlockDriver {
     int instance_size;
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
+
+    /* 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_open)(BlockDriverState *bs, int flags);
     int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
@@ -335,6 +343,10 @@ struct BlockDriverState {
     BlockJob *job;
 };
 
+struct BDRVReopenState {
+    BlockDriverState *bs;
+};
+
 int get_tmp_filename(char *filename, int size);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -129,6 +129,10 @@ 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);
+void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+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);
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -223,6 +223,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;

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Stefan Hajnoczi]

On Mon, Jul 30, 2012 at 10:34 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = NULL;
> +
> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +
> +    /* Use driver specific reopen() if available */
> +    if (drv->bdrv_reopen_prepare) {
> +        ret = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> +         if (ret < 0) {

Indentation is off.

> +            bdrv_reopen_abort(bs, reopen_state);
> +            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
> +            return;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +    } else {
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, bs->device_name,
> +                  "reopening of file");
> +        return;
> +    }

This would be slightly simpler if written as:

if (drv->bdrv_reopen_prepare == NULL) {
    ...error return...
}

...success case...

Stefan

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Luiz Capitulino]

On Tue, 31 Jul 2012 03:04:22 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> 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
> @@ -859,6 +859,60 @@ 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);
> +}
> +
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = NULL;
> +
> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +
> +    /* 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);

Why do you have to call bdrv_reopen_abort()? I'd expect bdrv_reopen_prepare()
(to be able) to undo anything it has done.

> +            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
> +            return;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +    } else {
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, bs->device_name,
> +                  "reopening of file");
> +        return;
> +    }
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -136,6 +136,14 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* 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_open)(BlockDriverState *bs, int flags);
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> @@ -335,6 +343,10 @@ struct BlockDriverState {
>      BlockJob *job;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +};
> +
>  int get_tmp_filename(char *filename, int size);
>  
>  void bdrv_set_io_limits(BlockDriverState *bs,
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -129,6 +129,10 @@ 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);
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
> +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);
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -223,6 +223,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;
> 

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Supriya Kannery]

On 08/03/2012 01:49 AM, Luiz Capitulino wrote:
> On Tue, 31 Jul 2012 03:04:22 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com>  wrote:
> 

>> +
>> +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);
>> +}
>> +
>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int ret = 0;
>> +    BDRVReopenState *reopen_state = NULL;
>> +
>> +    /* Quiesce IO for the given block device */
>> +    bdrv_drain_all();
>> +    ret = bdrv_flush(bs);
>> +    if (ret != 0) {
>> +        error_set(errp, QERR_IO_ERROR);
>> +        return;
>> +    }
>> +
>> +    /* 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);
> 
> Why do you have to call bdrv_reopen_abort()? I'd expect bdrv_reopen_prepare()
> (to be able) to undo anything it has done.
> 
    Having separate abort function avoids cluttering of reopen-
prepare(). We wanted to logically separate out preparation, commit and 
abort. Same format is followed in implementations at block driver level
as well.
 -thanks, Supriya

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Jeff Cody]

On 07/30/2012 05:34 PM, 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
> @@ -859,6 +859,60 @@ 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);
> +}
> +
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = NULL;
> +
> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +
> +    /* 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);
> +            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
> +            return;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;

You also need to set bs->read_only here, like so:
     bs->read_only = !(bdrv_flags & BDRV_O_RDWR);


> +    } else {
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, bs->device_name,
> +                  "reopening of file");
> +        return;
> +    }
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -136,6 +136,14 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* 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_open)(BlockDriverState *bs, int flags);
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> @@ -335,6 +343,10 @@ struct BlockDriverState {
>      BlockJob *job;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +};
> +
>  int get_tmp_filename(char *filename, int size);
>  
>  void bdrv_set_io_limits(BlockDriverState *bs,
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -129,6 +129,10 @@ 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);
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
> +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);
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -223,6 +223,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;
> 
> 

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Supriya Kannery]

On 08/09/2012 02:43 AM, Jeff Cody wrote:
> On 07/30/2012 05:34 PM, 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.
>>
>> +
>> +        bdrv_reopen_commit(bs, reopen_state);
>> +        bs->open_flags = bdrv_flags;
>
> You also need to set bs->read_only here, like so:
>       bs->read_only = !(bdrv_flags&  BDRV_O_RDWR);
>
>

Sure..will include in updated patch.

- thanks, Supriya

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Jeff Cody]

On 07/30/2012 05:34 PM, 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
> @@ -859,6 +859,60 @@ 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);
> +}
> +
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +    BDRVReopenState *reopen_state = NULL;
> +

We should assert if drv is NULL:

    assert(drv != NULL);


> +    /* Quiesce IO for the given block device */
> +    bdrv_drain_all();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        error_set(errp, QERR_IO_ERROR);
> +        return;
> +    }
> +

We also need to reopen bs->file, so maybe something like this:

    /* open any file images */
    if (bs->file) {
        bdrv_reopen(bs->file, bdrv_flags, errp);
        if (errp && *errp) {
            goto exit;
        }
    }

This will necessitate making the handlers in the raw.c file just stubs
(I'll respond to that patch as well).


> +    /* 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);
> +            error_set(errp, QERR_OPEN_FILE_FAILED, bs->filename);
> +            return;
> +        }
> +
> +        bdrv_reopen_commit(bs, reopen_state);
> +        bs->open_flags = bdrv_flags;
> +    } else {
> +        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> +                  drv->format_name, bs->device_name,
> +                  "reopening of file");
> +        return;
> +    }
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -136,6 +136,14 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* 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_open)(BlockDriverState *bs, int flags);
>      int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
>      int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> @@ -335,6 +343,10 @@ struct BlockDriverState {
>      BlockJob *job;
>  };
>  
> +struct BDRVReopenState {
> +    BlockDriverState *bs;
> +};
> +
>  int get_tmp_filename(char *filename, int size);
>  
>  void bdrv_set_io_limits(BlockDriverState *bs,
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -129,6 +129,10 @@ 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);
> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
> +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);
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -223,6 +223,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;
> 
> 

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Kevin Wolf]

Am 09.08.2012 06:26, schrieb Jeff Cody:
> On 07/30/2012 05:34 PM, 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
>> @@ -859,6 +859,60 @@ 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);
>> +}
>> +
>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int ret = 0;
>> +    BDRVReopenState *reopen_state = NULL;
>> +
> 
> We should assert if drv is NULL:
> 
>     assert(drv != NULL);
> 
> 
>> +    /* Quiesce IO for the given block device */
>> +    bdrv_drain_all();
>> +    ret = bdrv_flush(bs);
>> +    if (ret != 0) {
>> +        error_set(errp, QERR_IO_ERROR);
>> +        return;
>> +    }
>> +
> 
> We also need to reopen bs->file, so maybe something like this:
> 
>     /* open any file images */
>     if (bs->file) {
>         bdrv_reopen(bs->file, bdrv_flags, errp);
>         if (errp && *errp) {
>             goto exit;
>         }
>     }
> 
> This will necessitate making the handlers in the raw.c file just stubs
> (I'll respond to that patch as well).

Doesn't this break the transactional semantics? I think you should only
prepare the bs->file reopen here and commit it when committing this one.

Kevin

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Jeff Cody]

On 08/09/2012 05:20 AM, Kevin Wolf wrote:
> Am 09.08.2012 06:26, schrieb Jeff Cody:
>> On 07/30/2012 05:34 PM, 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
>>> @@ -859,6 +859,60 @@ 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);
>>> +}
>>> +
>>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>>> +{
>>> +    BlockDriver *drv = bs->drv;
>>> +    int ret = 0;
>>> +    BDRVReopenState *reopen_state = NULL;
>>> +
>>
>> We should assert if drv is NULL:
>>
>>     assert(drv != NULL);
>>
>>
>>> +    /* Quiesce IO for the given block device */
>>> +    bdrv_drain_all();
>>> +    ret = bdrv_flush(bs);
>>> +    if (ret != 0) {
>>> +        error_set(errp, QERR_IO_ERROR);
>>> +        return;
>>> +    }
>>> +
>>
>> We also need to reopen bs->file, so maybe something like this:
>>
>>     /* open any file images */
>>     if (bs->file) {
>>         bdrv_reopen(bs->file, bdrv_flags, errp);
>>         if (errp && *errp) {
>>             goto exit;
>>         }
>>     }
>>
>> This will necessitate making the handlers in the raw.c file just stubs
>> (I'll respond to that patch as well).
> 
> Doesn't this break the transactional semantics? I think you should only
> prepare the bs->file reopen here and commit it when committing this one.
> 

Yes, it would, so if everything stayed as it is now, that would be the
right way to do it... but one thing that would be nice (I also mention
this in my comments on patch 0/9) is that it become transactional for
multiple BlockDriverStates to be reopened.  That would obviously change
the interface a bit, so that multiple BDS entries could be queued, but
then it shouldn't be any different to queue the bs->file as well as the
bs.

All prepare() stages would happen on queued items, and only
progress to commit() if all prepare() stages passed, as you mentioned.

One other thing, and perhaps this is nit-picking some - but the
prepare() functions all modify the 'live' structures, after backing them
up into stashes.  So, on abort, the structures are restored from the
stashes, and on commit the stashes are discarded.  Conceptually, it
seems cleaner to this the opposite way - prepare() does it work into
temporary stashes, and the commit() then copies the data over from the
stash to the live structure, and abort() would just discard the stashes.

Re: [Qemu-devel] [v2 Patch 1/9]block: Framework for reopening image files safely

[Supriya Kannery]

On 08/09/2012 06:32 PM, Jeff Cody wrote:
> On 08/09/2012 05:20 AM, Kevin Wolf wrote:
>> Am 09.08.2012 06:26, schrieb Jeff Cody:
>>> On 07/30/2012 05:34 PM, Supriya Kannery wrote:

>>>> +
>>>> +void bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>>>> +{
>>>> +    BlockDriver *drv = bs->drv;
>>>> +    int ret = 0;
>>>> +    BDRVReopenState *reopen_state = NULL;
>>>> +
>>>
>>> We should assert if drv is NULL:
>>>
>>>      assert(drv != NULL);
>>>
>>>
>>>> +    /* Quiesce IO for the given block device */
>>>> +    bdrv_drain_all();
>>>> +    ret = bdrv_flush(bs);
>>>> +    if (ret != 0) {
>>>> +        error_set(errp, QERR_IO_ERROR);
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> We also need to reopen bs->file, so maybe something like this:
>>>
>>>      /* open any file images */
>>>      if (bs->file) {
>>>          bdrv_reopen(bs->file, bdrv_flags, errp);
>>>          if (errp&&  *errp) {
>>>              goto exit;
>>>          }
>>>      }
>>>
>>> This will necessitate making the handlers in the raw.c file just stubs
>>> (I'll respond to that patch as well).
>>
>> Doesn't this break the transactional semantics? I think you should only
>> prepare the bs->file reopen here and commit it when committing this one.
>>
> 
> Yes, it would, so if everything stayed as it is now, that would be the
> right way to do it... but one thing that would be nice (I also mention
> this in my comments on patch 0/9) is that it become transactional for
> multiple BlockDriverStates to be reopened.  That would obviously change
> the interface a bit, so that multiple BDS entries could be queued, but
> then it shouldn't be any different to queue the bs->file as well as the
> bs.
> 
> All prepare() stages would happen on queued items, and only
> progress to commit() if all prepare() stages passed, as you mentioned.

  Yes, will work on to get the transactional semantics applied to bs->file 
reopen as well.

> 
> One other thing, and perhaps this is nit-picking some - but the
> prepare() functions all modify the 'live' structures, after backing them
> up into stashes.  So, on abort, the structures are restored from the
> stashes, and on commit the stashes are discarded.  Conceptually, it
> seems cleaner to this the opposite way - prepare() does it work into
> temporary stashes, and the commit() then copies the data over from the
> stash to the live structure, and abort() would just discard the stashes.
> 
> 
  prepare()/commit() and abort() are from the perspective of new changes 
to be tried on respective block driver state. Once block driver is ready for the
state change, then commit() means we are commiting these new changes to driver
state. Similarly abort() means we are aborting these new changes half way
and getting back to old stashed state. This is the intended logic.

- thanks, Supriya

[Qemu-devel] [v2 Patch 2/9]block: 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)
 {
@@ -113,6 +129,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
@@ -140,8 +140,15 @@ 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);
+static void raw_stash_state(BDRVRawState *stashed_state, BDRVRawState *s);
+static void raw_revert_state(BDRVRawState *s, BDRVRawState *stashed_state);
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 static int cdrom_reopen(BlockDriverState *bs);
@@ -283,6 +290,117 @@ 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));
+    raw_stash_state(raw_rs->stash_s, s);
+    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
+
+    *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;
+        }
+        ret = fcntl_setfl(s->fd, s->open_flags);
+    } else {
+
+        /* 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);
+    }
+    raw_revert_state(s, raw_rs->stash_s);
+    g_free(raw_rs->stash_s);
+    g_free(raw_rs);
+}
+
+static void raw_stash_state(BDRVRawState *stashed_s, BDRVRawState *s)
+{
+    stashed_s->fd = -1;
+    stashed_s->type = s->type;
+    stashed_s->open_flags = s->open_flags;
+#if defined(__linux__)
+    /* linux floppy specific */
+    stashed_s->fd_open_time = s->fd_open_time;
+    stashed_s->fd_error_time = s->fd_error_time;
+    stashed_s->fd_got_error = s->fd_got_error;
+    stashed_s->fd_media_changed = s->fd_media_changed;
+#endif
+#ifdef CONFIG_LINUX_AIO
+    stashed_s->use_aio = s->use_aio;
+    stashed_s->aio_ctx = s->aio_ctx;
+#endif
+    stashed_s->aligned_buf = s->aligned_buf;
+    stashed_s->aligned_buf_size = s->aligned_buf_size;
+#ifdef CONFIG_XFS
+    stashed_s->is_xfs = s->is_xfs;
+#endif
+
+}
+
+static void raw_revert_state(BDRVRawState *s, BDRVRawState *stashed_s)
+{
+
+    s->fd = stashed_s->fd;
+    s->type = stashed_s->type;
+    s->open_flags = stashed_s->open_flags;
+#if defined(__linux__)
+    /* linux floppy specific */
+    s->fd_open_time = stashed_s->fd_open_time;
+    s->fd_error_time = stashed_s->fd_error_time;
+    s->fd_got_error = stashed_s->fd_got_error;
+    s->fd_media_changed = stashed_s->fd_media_changed;
+#endif
+#ifdef CONFIG_LINUX_AIO
+    s->use_aio = stashed_s->use_aio;
+    s->aio_ctx = stashed_s->aio_ctx;
+#endif
+    s->aligned_buf = stashed_s->aligned_buf;
+    s->aligned_buf_size = stashed_s->aligned_buf_size;
+#ifdef CONFIG_XFS
+    s->is_xfs = stashed_s->is_xfs;
+#endif
+}
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
         {
@@ -735,6 +853,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] [v2 Patch 2/9]block: raw-posix image file reopen

[Eric Blake]

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

On 07/30/2012 03:34 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
> ===================================================================

> +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));
> +    raw_stash_state(raw_rs->stash_s, s);
> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);

You called it out in your cover letter, but just pointing out that this
is one of the locations that needs a conditional fallback to
dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.

More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
NOT dup3, so that you are duplicating to the first available fd rather
than accidentally overwriting whatever s->fd happened to be on entry.
Also, where is your error checking that you didn't just assign s->fd to
-1?  It looks like your goal here is to stash a copy of the fd, so that
on failure you can then pivot over to your copy.

> +
> +    *prs = &(raw_rs->reopen_state);
> +
> +    /* Flags that can be set using fcntl */
> +    int fcntl_flags = BDRV_O_NOCACHE;

This variable name is misleading; fcntl_flags in my mind implies O_* not
BDRV_O_*, as we are not guaranteed that they are the same values.  Maybe
bdrv_flags is a better name.  Or are you trying to list the subset of
BDRV_O flags that can be changed via mapping to the appropriate O_ flag
during fcntl?

> +
> +    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;
> +        }
> +        ret = fcntl_setfl(s->fd, s->open_flags);
> +    } else {
> +
> +        /* close and reopen using new flags */
> +        bs->drv->bdrv_close(bs);
> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
> +    }

At any rate, in spite of the poor choice of naming for fcntl_flags, the
logic here looked correct - if the only BDRV_O_ bits that changed
between existing flags and the requested flags can be handled by fcntl,
then use fcntl to change them, otherwise open from scratch.

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

At first, I worried that you needed to use dup3() here to restore s->fd
from the cached fd...

> +    raw_revert_state(s, raw_rs->stash_s);

then I read the code for raw_revert_state, where you are rewriting the
state to permanently use the dup'd copy rather than trying to revert
back to the old fd number.  As long as we are sure that all other points
in the code base always go through s->fd rather than caching the old
value, and therefore don't have a stale reference, then swapping the
struct instead of using dup3 to restore the exact fd value we previously
had should work.

-- 
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] [v2 Patch 2/9]block: raw-posix image file reopen

[Kevin Wolf]

Am 31.07.2012 19:17, schrieb Eric Blake:
> On 07/30/2012 03:34 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
>> ===================================================================
> 
>> +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));
>> +    raw_stash_state(raw_rs->stash_s, s);
>> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> 
> You called it out in your cover letter, but just pointing out that this
> is one of the locations that needs a conditional fallback to
> dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.
> 
> More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
> NOT dup3, so that you are duplicating to the first available fd rather
> than accidentally overwriting whatever s->fd happened to be on entry.
> Also, where is your error checking that you didn't just assign s->fd to
> -1?  It looks like your goal here is to stash a copy of the fd, so that
> on failure you can then pivot over to your copy.

Doesn't Corey's fd passing series introduce a helper function for
F_DUP_CLOEXEC and emulation if it isn't support? Could be reused here then.

Kevin

Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Jeff Cody]

On 07/31/2012 01:17 PM, Eric Blake wrote:
> On 07/30/2012 03:34 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
>> ===================================================================
> 
>> +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));
>> +    raw_stash_state(raw_rs->stash_s, s);
>> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> 
> You called it out in your cover letter, but just pointing out that this
> is one of the locations that needs a conditional fallback to
> dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.
> 
> More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
> NOT dup3, so that you are duplicating to the first available fd rather
> than accidentally overwriting whatever s->fd happened to be on entry.
> Also, where is your error checking that you didn't just assign s->fd to
> -1?  It looks like your goal here is to stash a copy of the fd, so that
> on failure you can then pivot over to your copy.

In addition, as it is written above will always assign -1 to s->fd, and
stash_s->fd every time, with errno "Bad file descriptor".

The function raw_stash_state() does not save s->fd, but instead sets the
stashed copy (raw_rs->stash_s->fd) to be -1.

Then this:
    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);

assigns s->fd to -1.

I don't think dup3() will work at all, because if -1 is used for one of the
file descriptors, that is EBADF, and if stash_s->fd == s->fd, that is
EINVAL.

So I agree, I think we definitely want fcntl(F_DUP_CLOEXEC) here.

> 
>> +
>> +    *prs = &(raw_rs->reopen_state);
>> +
>> +    /* Flags that can be set using fcntl */
>> +    int fcntl_flags = BDRV_O_NOCACHE;
> 
> This variable name is misleading; fcntl_flags in my mind implies O_* not
> BDRV_O_*, as we are not guaranteed that they are the same values.  Maybe
> bdrv_flags is a better name.  Or are you trying to list the subset of
> BDRV_O flags that can be changed via mapping to the appropriate O_ flag
> during fcntl?
> 
>> +
>> +    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;
>> +        }
>> +        ret = fcntl_setfl(s->fd, s->open_flags);
>> +    } else {
>> +
>> +        /* close and reopen using new flags */
>> +        bs->drv->bdrv_close(bs);
>> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
>> +    }
> 
> At any rate, in spite of the poor choice of naming for fcntl_flags, the
> logic here looked correct - if the only BDRV_O_ bits that changed
> between existing flags and the requested flags can be handled by fcntl,
> then use fcntl to change them, otherwise open from scratch.
> 
>> +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);
>> +    }
> 
> At first, I worried that you needed to use dup3() here to restore s->fd
> from the cached fd...
> 
>> +    raw_revert_state(s, raw_rs->stash_s);
> 
> then I read the code for raw_revert_state, where you are rewriting the
> state to permanently use the dup'd copy rather than trying to revert
> back to the old fd number.  As long as we are sure that all other points
> in the code base always go through s->fd rather than caching the old
> value, and therefore don't have a stale reference, then swapping the
> struct instead of using dup3 to restore the exact fd value we previously
> had should work.
> 

Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Supriya Kannery]

On 07/31/2012 10:47 PM, Eric Blake wrote:
> On 07/30/2012 03:34 PM, Supriya Kannery wrote:
>> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> 
> You called it out in your cover letter, but just pointing out that this
> is one of the locations that needs a conditional fallback to
> dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.
> 
> More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
> NOT dup3, so that you are duplicating to the first available fd rather
> than accidentally overwriting whatever s->fd happened to be on entry.
> Also, where is your error checking that you didn't just assign s->fd to
> -1?  It looks like your goal here is to stash a copy of the fd, so that
> on failure you can then pivot over to your copy.
> 
 
Will use fcntl(F_DUP_CLOEXEC) in updated patch.
 
>> +
>> +    *prs =&(raw_rs->reopen_state);
>> +
>> +    /* Flags that can be set using fcntl */
>> +    int fcntl_flags = BDRV_O_NOCACHE;
> 
> This variable name is misleading; fcntl_flags in my mind implies O_* not
> BDRV_O_*, as we are not guaranteed that they are the same values.  Maybe
> bdrv_flags is a better name.  Or are you trying to list the subset of
> BDRV_O flags that can be changed via mapping to the appropriate O_ flag
> during fcntl?
> 

  From the list of flags in bdrv->openflags (BDRV_O*), only few of them can be changed
using fcntl. So to identify those allowed subset, I am using fcntl_flags.

Excerpt from man fcntl for F_SETFL:
            On Linux this command can only  change  the  O_APPEND,  O_ASYNC,
              O_DIRECT, O_NOATIME, and O_NONBLOCK flags.

May be naming it fcntl_supported_flags would be better?

- thanks, Supriya

Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Jeff Cody]

On 07/30/2012 05:34 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);
> +}
> +

I think the above should be stubs that do nothing (or, alternatively,
are not present at all), if we have the reopen() do the bs->file
reopen() (which is needed for qcow2 - see my comments for patch 1/9)

>  static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
>                                       int nb_sectors, QEMUIOVector *qiov)
>  {
> @@ -113,6 +129,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
> @@ -140,8 +140,15 @@ 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);
> +static void raw_stash_state(BDRVRawState *stashed_state, BDRVRawState *s);
> +static void raw_revert_state(BDRVRawState *s, BDRVRawState *stashed_state);
>  
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
>  static int cdrom_reopen(BlockDriverState *bs);
> @@ -283,6 +290,117 @@ 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));
> +    raw_stash_state(raw_rs->stash_s, s);
> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> +
> +    *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;
> +        }
> +        ret = fcntl_setfl(s->fd, s->open_flags);
> +    } else {
> +
> +        /* close and reopen using new flags */
> +        bs->drv->bdrv_close(bs);
> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);

This worries me, in that we close the BDS and then try
to reopen it.  At this point, if the bdrv_file_open() fails, we have no
recovery mechanism.

Maybe we could try to do something like a bdrv_file_open() into a new BDS
before we close, followed by something akin to a bdrv_swap() /
bdrv_append() if the new bdrv_file_open() was successful, and then do
the close on the original.  On failure, just abandon the new BDS.

> +    }
> +    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);
> +    }
> +    raw_revert_state(s, raw_rs->stash_s);
> +    g_free(raw_rs->stash_s);
> +    g_free(raw_rs);
> +}
> +
> +static void raw_stash_state(BDRVRawState *stashed_s, BDRVRawState *s)
> +{
> +    stashed_s->fd = -1;
> +    stashed_s->type = s->type;
> +    stashed_s->open_flags = s->open_flags;
> +#if defined(__linux__)
> +    /* linux floppy specific */
> +    stashed_s->fd_open_time = s->fd_open_time;
> +    stashed_s->fd_error_time = s->fd_error_time;
> +    stashed_s->fd_got_error = s->fd_got_error;
> +    stashed_s->fd_media_changed = s->fd_media_changed;
> +#endif
> +#ifdef CONFIG_LINUX_AIO
> +    stashed_s->use_aio = s->use_aio;
> +    stashed_s->aio_ctx = s->aio_ctx;
> +#endif
> +    stashed_s->aligned_buf = s->aligned_buf;
> +    stashed_s->aligned_buf_size = s->aligned_buf_size;
> +#ifdef CONFIG_XFS
> +    stashed_s->is_xfs = s->is_xfs;
> +#endif
> +
> +}
> +
> +static void raw_revert_state(BDRVRawState *s, BDRVRawState *stashed_s)
> +{
> +
> +    s->fd = stashed_s->fd;
> +    s->type = stashed_s->type;
> +    s->open_flags = stashed_s->open_flags;
> +#if defined(__linux__)
> +    /* linux floppy specific */
> +    s->fd_open_time = stashed_s->fd_open_time;
> +    s->fd_error_time = stashed_s->fd_error_time;
> +    s->fd_got_error = stashed_s->fd_got_error;
> +    s->fd_media_changed = stashed_s->fd_media_changed;
> +#endif
> +#ifdef CONFIG_LINUX_AIO
> +    s->use_aio = stashed_s->use_aio;
> +    s->aio_ctx = stashed_s->aio_ctx;
> +#endif
> +    s->aligned_buf = stashed_s->aligned_buf;
> +    s->aligned_buf_size = stashed_s->aligned_buf_size;
> +#ifdef CONFIG_XFS
> +    s->is_xfs = stashed_s->is_xfs;
> +#endif
> +}
> +
>  /* XXX: use host sector size if necessary with:
>  #ifdef DIOCGSECTORSIZE
>          {
> @@ -735,6 +853,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] [v2 Patch 2/9]block: raw-posix image file reopen

[Corey Bryant]

On 07/30/2012 05:34 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)
>   {
> @@ -113,6 +129,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
> @@ -140,8 +140,15 @@ 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);
> +static void raw_stash_state(BDRVRawState *stashed_state, BDRVRawState *s);
> +static void raw_revert_state(BDRVRawState *s, BDRVRawState *stashed_state);
>
>   #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
>   static int cdrom_reopen(BlockDriverState *bs);
> @@ -283,6 +290,117 @@ 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));
> +    raw_stash_state(raw_rs->stash_s, s);
> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> +
> +    *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;
> +        }
> +        ret = fcntl_setfl(s->fd, s->open_flags);
> +    } else {
> +
> +        /* close and reopen using new flags */
> +        bs->drv->bdrv_close(bs);
> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);

Will this allow the fdset refcount to get to zero?  I was hoping your 
patches would prevent that from happening.  Perhaps Kevin or Eric can 
weigh in.  qemu_open() increments the refcount for an fdset when an fd 
from it is used, and qemu_close() decrements it.  I think if you were 
able to perform the open before the close here that refcount wouldn't 
get to zero.

-- 
Regards,
Corey

Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Supriya Kannery]

On 08/10/2012 07:15 PM, Corey Bryant wrote:
> 
> 
> On 07/30/2012 05:34 PM, Supriya Kannery wrote:

>> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState 
>> **prs,
>> + int flags)
>> +{
>> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
>> + BDRVRawState *s = bs->opaque;
>> + int ret = 0;
>> +
>> + raw_rs->reopen_state.bs = bs;
>> +
>> + /* stash state before reopen */
>> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>> + raw_stash_state(raw_rs->stash_s, s);
>> + s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
>> +
>> + *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;
>> + }
>> + ret = fcntl_setfl(s->fd, s->open_flags);
>> + } else {
>> +
>> + /* close and reopen using new flags */
>> + bs->drv->bdrv_close(bs);
>> + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
> 
> Will this allow the fdset refcount to get to zero? I was hoping your 
> patches would prevent that from happening. Perhaps Kevin or Eric can 
> weigh in. qemu_open() increments the refcount for an fdset when an fd 
> from it is used, and qemu_close() decrements it. I think if you were 
> able to perform the open before the close here that refcount wouldn't 
> get to zero.
> 

 Since we are duping the file descriptor before reaching this bdrv_close(),
refcount for fd won't become zero.

- thanks, Supriya

Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Kevin Wolf]

Am 14.08.2012 13:13, schrieb Supriya Kannery:
> On 08/10/2012 07:15 PM, Corey Bryant wrote:
>>
>>
>> On 07/30/2012 05:34 PM, Supriya Kannery wrote:
> 
>>> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState 
>>> **prs,
>>> + int flags)
>>> +{
>>> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
>>> + BDRVRawState *s = bs->opaque;
>>> + int ret = 0;
>>> +
>>> + raw_rs->reopen_state.bs = bs;
>>> +
>>> + /* stash state before reopen */
>>> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>>> + raw_stash_state(raw_rs->stash_s, s);
>>> + s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
>>> +
>>> + *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;
>>> + }
>>> + ret = fcntl_setfl(s->fd, s->open_flags);
>>> + } else {
>>> +
>>> + /* close and reopen using new flags */
>>> + bs->drv->bdrv_close(bs);
>>> + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
>>
>> Will this allow the fdset refcount to get to zero? I was hoping your 
>> patches would prevent that from happening. Perhaps Kevin or Eric can 
>> weigh in. qemu_open() increments the refcount for an fdset when an fd 
>> from it is used, and qemu_close() decrements it. I think if you were 
>> able to perform the open before the close here that refcount wouldn't 
>> get to zero.
>>
> 
>  Since we are duping the file descriptor before reaching this bdrv_close(),
> refcount for fd won't become zero.

We need to use a qemu_dup() here, so that the fdset implementation can
keep track of the new fd.

Kevin

[Qemu-devel] [v2 Patch 3/9]block: raw-win32 image file reopen

[Supriya Kannery]

raw-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>
Signed-off-by: Shrinidhi Joshi <spjoshi31@gmail.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)
 {

[Qemu-devel] [v2 Patch 4/9]block: 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 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,14 @@ typedef struct VmdkGrainMarker {
     uint8_t  data[0];
 } VmdkGrainMarker;
 
+typedef struct BDRVVmdkReopenState {
+    BDRVReopenState reopen_state;
+    BDRVVmdkState *stash_s;
+} BDRVVmdkReopenState;
+
+static void vmdk_stash_state(BDRVVmdkState *stashed_state, BDRVVmdkState *s);
+static void vmdk_revert_state(BDRVVmdkState *s, BDRVVmdkState *stashed_state);
+
 static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     uint32_t magic;
@@ -588,7 +596,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 +682,94 @@ 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));
+    vmdk_stash_state(vmdk_rs->stash_s, s);
+    s->num_extents = 0;
+    s->extents = NULL;
+    s->migration_blocker = 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);
+    vmdk_revert_state(s, vmdk_rs->stash_s);
+    g_free(vmdk_rs->stash_s);
+    g_free(vmdk_rs);
+}
+
+static void vmdk_stash_state(BDRVVmdkState *stashed_state, BDRVVmdkState *s)
+{
+   stashed_state->lock = s->lock;
+   stashed_state->desc_offset = s->desc_offset;
+   stashed_state->cid_updated = s->cid_updated;
+   stashed_state->parent_cid = s->parent_cid;
+   stashed_state->num_extents = s->num_extents;
+   stashed_state->extents = s->extents;
+   stashed_state->migration_blocker = s->migration_blocker;
+}
+
+static void vmdk_revert_state(BDRVVmdkState *s, BDRVVmdkState *stashed_state)
+{
+   s->lock = stashed_state->lock;
+   s->desc_offset = stashed_state->desc_offset;
+   s->cid_updated = stashed_state->cid_updated;
+   s->parent_cid = stashed_state->parent_cid;
+   s->num_extents = stashed_state->num_extents;
+   s->extents = stashed_state->extents;
+   s->migration_blocker = stashed_state->migration_blocker;
+}
+
 static int get_whole_cluster(BlockDriverState *bs,
                 VmdkExtent *extent,
                 uint64_t cluster_offset,
@@ -1593,6 +1688,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] [v2 Patch 4/9]block: vmdk image file reopen

[Eric Blake]

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

On 07/30/2012 03:35 PM, Supriya Kannery wrote:
> vmdk 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>
> 
> ---

> @@ -588,7 +596,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);

Spurious whitespace change.

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

[Qemu-devel] [v2 Patch 5/9]block: qcow2 image file reopen

[Supriya Kannery]

qcow2 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/qcow2.c
===================================================================
--- qemu.orig/block/qcow2.c
+++ qemu/block/qcow2.c
@@ -52,10 +52,19 @@ typedef struct {
     uint32_t magic;
     uint32_t len;
 } QCowExtension;
+
+typedef struct BDRVQcowReopenState {
+    BDRVReopenState reopen_state;
+    BDRVQcowState *stash_s;
+} BDRVQcowReopenState;
+
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 
+static void qcow2_stash_state(BDRVQcowState *stashed_state, BDRVQcowState *s);
+static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_state);
+
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     const QCowHeader *cow_header = (const void *)buf;
@@ -434,6 +443,169 @@ static int qcow2_open(BlockDriverState *
     return ret;
 }
 
+static int qcow2_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                               int flags)
+{
+    BDRVQcowReopenState *qcow2_rs = g_malloc0(sizeof(BDRVQcowReopenState));
+    int ret = 0;
+    BDRVQcowState *s = bs->opaque;
+
+    qcow2_rs->reopen_state.bs = bs;
+
+    /* save state before reopen */
+    qcow2_rs->stash_s = g_malloc0(sizeof(BDRVQcowState));
+    qcow2_stash_state(qcow2_rs->stash_s, s);
+    *prs = &(qcow2_rs->reopen_state);
+
+    /* Reopen file with new flags */
+     ret = qcow2_open(bs, flags);
+     return ret;
+}
+
+static void qcow2_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQcowReopenState *qcow2_rs;
+    BDRVQcowState *stashed_s;
+
+    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
+    stashed_s = qcow2_rs->stash_s;
+
+    qcow2_cache_flush(bs, stashed_s->l2_table_cache);
+    qcow2_cache_flush(bs, stashed_s->refcount_block_cache);
+
+    qcow2_cache_destroy(bs, stashed_s->l2_table_cache);
+    qcow2_cache_destroy(bs, stashed_s->refcount_block_cache);
+
+    g_free(stashed_s->unknown_header_fields);
+    cleanup_unknown_header_ext(bs);
+
+    g_free(stashed_s->cluster_cache);
+    qemu_vfree(stashed_s->cluster_data);
+    qcow2_refcount_close(bs);
+    qcow2_free_snapshots(bs);
+
+    g_free(stashed_s);
+    g_free(qcow2_rs);
+}
+
+static void qcow2_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQcowReopenState *qcow2_rs;
+    BDRVQcowState *s = bs->opaque;
+    BDRVQcowState *stashed_s;
+
+    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
+
+    /* Revert to stashed state */
+    qcow2_revert_state(s, qcow2_rs->stash_s);
+    stashed_s = qcow2_rs->stash_s;
+
+    g_free(stashed_s);
+    g_free(qcow2_rs);
+}
+
+static void qcow2_stash_state(BDRVQcowState *stashed_s, BDRVQcowState *s)
+{
+    stashed_s->cluster_bits = s->cluster_bits;
+    stashed_s->cluster_size = s->cluster_size;
+    stashed_s->cluster_sectors = s->cluster_sectors;
+    stashed_s->l2_bits = s->l2_bits;
+    stashed_s->l2_size = s->l2_size;
+    stashed_s->l1_size = s->l1_size;
+    stashed_s->l1_vm_state_index = s->l1_vm_state_index;
+    stashed_s->csize_shift = s->csize_shift;
+    stashed_s->csize_mask = s->csize_mask;
+    stashed_s->cluster_offset_mask = s->cluster_offset_mask;
+    stashed_s->l1_table_offset = s->l1_table_offset;
+    stashed_s->l1_table = s->l1_table;
+
+    stashed_s->l2_table_cache = s->l2_table_cache;
+    stashed_s->refcount_block_cache = s->refcount_block_cache;
+
+    stashed_s->cluster_cache = s->cluster_cache;
+    stashed_s->cluster_data = s->cluster_data;
+    stashed_s->cluster_cache_offset = s->cluster_cache_offset;
+    stashed_s->cluster_allocs = s->cluster_allocs;
+
+    stashed_s->refcount_table = s->refcount_table;
+    stashed_s->refcount_table_offset = s->refcount_table_offset;
+    stashed_s->refcount_table_size = s->refcount_table_size;
+    stashed_s->free_cluster_index = s->free_cluster_index;
+    stashed_s->free_byte_offset = s->free_byte_offset;
+
+    stashed_s->lock = s->lock;
+
+    stashed_s->crypt_method = s->crypt_method;
+    stashed_s->crypt_method_header = s->crypt_method_header;
+    stashed_s->aes_encrypt_key = s->aes_encrypt_key;
+    stashed_s->aes_decrypt_key = s->aes_decrypt_key;
+    stashed_s->snapshots_offset = s->snapshots_offset;
+    stashed_s->snapshots_size = s->snapshots_size;
+    stashed_s->nb_snapshots = s->nb_snapshots;
+    stashed_s->snapshots = s->snapshots;
+
+    stashed_s->flags = s->flags;
+    stashed_s->qcow_version = s->qcow_version;
+
+    stashed_s->incompatible_features = s->incompatible_features;
+    stashed_s->compatible_features = s->compatible_features;
+
+    stashed_s->unknown_header_fields_size = s->unknown_header_fields_size;
+    stashed_s->unknown_header_fields = s->unknown_header_fields;
+    stashed_s->unknown_header_ext = s->unknown_header_ext;
+
+}
+
+static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_s)
+{
+   s->cluster_bits = stashed_s->cluster_bits;
+   s->cluster_size = stashed_s->cluster_size;
+   s->cluster_sectors = stashed_s->cluster_sectors;
+   s->l2_bits = stashed_s->l2_bits;
+   s->l2_size = stashed_s->l2_size;
+   s->l1_size = stashed_s->l1_size;
+   s->l1_vm_state_index = stashed_s->l1_vm_state_index;
+   s->csize_shift = stashed_s->csize_shift;
+   s->csize_mask = stashed_s->csize_mask;
+   s->cluster_offset_mask = stashed_s->cluster_offset_mask;
+   s->l1_table_offset = stashed_s->l1_table_offset;
+   s->l1_table = stashed_s->l1_table;
+   s->l2_table_cache = stashed_s->l2_table_cache;
+   s->refcount_block_cache = stashed_s->refcount_block_cache;
+
+   s->cluster_cache = stashed_s->cluster_cache;
+   s->cluster_data = stashed_s->cluster_data;
+   s->cluster_cache_offset = stashed_s->cluster_cache_offset;
+   s->cluster_allocs = stashed_s->cluster_allocs;
+
+   s->refcount_table = stashed_s->refcount_table;
+   s->refcount_table_offset = stashed_s->refcount_table_offset;
+   s->refcount_table_size = stashed_s->refcount_table_size;
+   s->free_cluster_index = stashed_s->free_cluster_index;
+   s->free_byte_offset = stashed_s->free_byte_offset;
+
+   s->lock = stashed_s->lock;
+
+   s->crypt_method = stashed_s->crypt_method;
+   s->crypt_method_header = stashed_s->crypt_method_header;
+   s->aes_encrypt_key = stashed_s->aes_encrypt_key;
+   s->aes_decrypt_key = stashed_s->aes_decrypt_key;
+   s->snapshots_offset = stashed_s->snapshots_offset;
+   s->snapshots_size = stashed_s->snapshots_size;
+   s->nb_snapshots = stashed_s->nb_snapshots;
+   s->snapshots = stashed_s->snapshots;
+
+   s->flags = stashed_s->flags;
+   s->qcow_version = stashed_s->qcow_version;
+
+   s->incompatible_features = stashed_s->incompatible_features;
+   s->compatible_features = stashed_s->compatible_features;
+
+   s->unknown_header_fields_size = stashed_s->unknown_header_fields_size;
+   s->unknown_header_fields = stashed_s->unknown_header_fields;
+   s->unknown_header_ext = stashed_s->unknown_header_ext;
+}
+
 static int qcow2_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -1567,6 +1739,9 @@ static BlockDriver bdrv_qcow2 = {
     .instance_size      = sizeof(BDRVQcowState),
     .bdrv_probe         = qcow2_probe,
     .bdrv_open          = qcow2_open,
+    .bdrv_reopen_prepare = qcow2_reopen_prepare,
+    .bdrv_reopen_commit = qcow2_reopen_commit,
+    .bdrv_reopen_abort  = qcow2_reopen_abort,
     .bdrv_close         = qcow2_close,
     .bdrv_create        = qcow2_create,
     .bdrv_co_is_allocated = qcow2_co_is_allocated,

Re: [Qemu-devel] [v2 Patch 5/9]block: qcow2 image file reopen

[Jeff Cody]

On 07/30/2012 05:35 PM, Supriya Kannery wrote:
> qcow2 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/qcow2.c
> ===================================================================
> --- qemu.orig/block/qcow2.c
> +++ qemu/block/qcow2.c
> @@ -52,10 +52,19 @@ typedef struct {
>      uint32_t magic;
>      uint32_t len;
>  } QCowExtension;
> +
> +typedef struct BDRVQcowReopenState {
> +    BDRVReopenState reopen_state;
> +    BDRVQcowState *stash_s;
> +} BDRVQcowReopenState;
> +
>  #define  QCOW2_EXT_MAGIC_END 0
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>  
> +static void qcow2_stash_state(BDRVQcowState *stashed_state, BDRVQcowState *s);
> +static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_state);
> +
>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
>      const QCowHeader *cow_header = (const void *)buf;
> @@ -434,6 +443,169 @@ static int qcow2_open(BlockDriverState *
>      return ret;
>  }
>  
> +static int qcow2_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> +                               int flags)
> +{
> +    BDRVQcowReopenState *qcow2_rs = g_malloc0(sizeof(BDRVQcowReopenState));
> +    int ret = 0;
> +    BDRVQcowState *s = bs->opaque;
> +
> +    qcow2_rs->reopen_state.bs = bs;
> +
> +    /* save state before reopen */
> +    qcow2_rs->stash_s = g_malloc0(sizeof(BDRVQcowState));
> +    qcow2_stash_state(qcow2_rs->stash_s, s);
> +    *prs = &(qcow2_rs->reopen_state);
> +
> +    /* Reopen file with new flags */
> +     ret = qcow2_open(bs, flags);
> +     return ret;
> +}
> +
> +static void qcow2_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BDRVQcowReopenState *qcow2_rs;
> +    BDRVQcowState *stashed_s;
> +
> +    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
> +    stashed_s = qcow2_rs->stash_s;
> +
> +    qcow2_cache_flush(bs, stashed_s->l2_table_cache);
> +    qcow2_cache_flush(bs, stashed_s->refcount_block_cache);
> +
> +    qcow2_cache_destroy(bs, stashed_s->l2_table_cache);
> +    qcow2_cache_destroy(bs, stashed_s->refcount_block_cache);
> +
> +    g_free(stashed_s->unknown_header_fields);
> +    cleanup_unknown_header_ext(bs);
> +
> +    g_free(stashed_s->cluster_cache);
> +    qemu_vfree(stashed_s->cluster_data);



> +    qcow2_refcount_close(bs);
> +    qcow2_free_snapshots(bs);

I assume these are unintentional?  I believe this is what is causing your
double-free issue.

> +
> +    g_free(stashed_s);
> +    g_free(qcow2_rs);
> +}
> +
> +static void qcow2_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> +    BDRVQcowReopenState *qcow2_rs;
> +    BDRVQcowState *s = bs->opaque;
> +    BDRVQcowState *stashed_s;
> +
> +    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
> +
> +    /* Revert to stashed state */
> +    qcow2_revert_state(s, qcow2_rs->stash_s);
> +    stashed_s = qcow2_rs->stash_s;
> +
> +    g_free(stashed_s);
> +    g_free(qcow2_rs);
> +}
> +
> +static void qcow2_stash_state(BDRVQcowState *stashed_s, BDRVQcowState *s)
> +{
> +    stashed_s->cluster_bits = s->cluster_bits;
> +    stashed_s->cluster_size = s->cluster_size;
> +    stashed_s->cluster_sectors = s->cluster_sectors;
> +    stashed_s->l2_bits = s->l2_bits;
> +    stashed_s->l2_size = s->l2_size;
> +    stashed_s->l1_size = s->l1_size;
> +    stashed_s->l1_vm_state_index = s->l1_vm_state_index;
> +    stashed_s->csize_shift = s->csize_shift;
> +    stashed_s->csize_mask = s->csize_mask;
> +    stashed_s->cluster_offset_mask = s->cluster_offset_mask;
> +    stashed_s->l1_table_offset = s->l1_table_offset;
> +    stashed_s->l1_table = s->l1_table;
> +
> +    stashed_s->l2_table_cache = s->l2_table_cache;
> +    stashed_s->refcount_block_cache = s->refcount_block_cache;
> +
> +    stashed_s->cluster_cache = s->cluster_cache;
> +    stashed_s->cluster_data = s->cluster_data;
> +    stashed_s->cluster_cache_offset = s->cluster_cache_offset;
> +    stashed_s->cluster_allocs = s->cluster_allocs;
> +
> +    stashed_s->refcount_table = s->refcount_table;
> +    stashed_s->refcount_table_offset = s->refcount_table_offset;
> +    stashed_s->refcount_table_size = s->refcount_table_size;
> +    stashed_s->free_cluster_index = s->free_cluster_index;
> +    stashed_s->free_byte_offset = s->free_byte_offset;
> +
> +    stashed_s->lock = s->lock;
> +
> +    stashed_s->crypt_method = s->crypt_method;
> +    stashed_s->crypt_method_header = s->crypt_method_header;
> +    stashed_s->aes_encrypt_key = s->aes_encrypt_key;
> +    stashed_s->aes_decrypt_key = s->aes_decrypt_key;
> +    stashed_s->snapshots_offset = s->snapshots_offset;
> +    stashed_s->snapshots_size = s->snapshots_size;
> +    stashed_s->nb_snapshots = s->nb_snapshots;
> +    stashed_s->snapshots = s->snapshots;
> +
> +    stashed_s->flags = s->flags;
> +    stashed_s->qcow_version = s->qcow_version;
> +
> +    stashed_s->incompatible_features = s->incompatible_features;
> +    stashed_s->compatible_features = s->compatible_features;
> +
> +    stashed_s->unknown_header_fields_size = s->unknown_header_fields_size;
> +    stashed_s->unknown_header_fields = s->unknown_header_fields;
> +    stashed_s->unknown_header_ext = s->unknown_header_ext;
> +
> +}
> +
> +static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_s)
> +{
> +   s->cluster_bits = stashed_s->cluster_bits;
> +   s->cluster_size = stashed_s->cluster_size;
> +   s->cluster_sectors = stashed_s->cluster_sectors;
> +   s->l2_bits = stashed_s->l2_bits;
> +   s->l2_size = stashed_s->l2_size;
> +   s->l1_size = stashed_s->l1_size;
> +   s->l1_vm_state_index = stashed_s->l1_vm_state_index;
> +   s->csize_shift = stashed_s->csize_shift;
> +   s->csize_mask = stashed_s->csize_mask;
> +   s->cluster_offset_mask = stashed_s->cluster_offset_mask;
> +   s->l1_table_offset = stashed_s->l1_table_offset;
> +   s->l1_table = stashed_s->l1_table;
> +   s->l2_table_cache = stashed_s->l2_table_cache;
> +   s->refcount_block_cache = stashed_s->refcount_block_cache;
> +
> +   s->cluster_cache = stashed_s->cluster_cache;
> +   s->cluster_data = stashed_s->cluster_data;
> +   s->cluster_cache_offset = stashed_s->cluster_cache_offset;
> +   s->cluster_allocs = stashed_s->cluster_allocs;
> +
> +   s->refcount_table = stashed_s->refcount_table;
> +   s->refcount_table_offset = stashed_s->refcount_table_offset;
> +   s->refcount_table_size = stashed_s->refcount_table_size;
> +   s->free_cluster_index = stashed_s->free_cluster_index;
> +   s->free_byte_offset = stashed_s->free_byte_offset;
> +
> +   s->lock = stashed_s->lock;
> +
> +   s->crypt_method = stashed_s->crypt_method;
> +   s->crypt_method_header = stashed_s->crypt_method_header;
> +   s->aes_encrypt_key = stashed_s->aes_encrypt_key;
> +   s->aes_decrypt_key = stashed_s->aes_decrypt_key;
> +   s->snapshots_offset = stashed_s->snapshots_offset;
> +   s->snapshots_size = stashed_s->snapshots_size;
> +   s->nb_snapshots = stashed_s->nb_snapshots;
> +   s->snapshots = stashed_s->snapshots;
> +
> +   s->flags = stashed_s->flags;
> +   s->qcow_version = stashed_s->qcow_version;
> +
> +   s->incompatible_features = stashed_s->incompatible_features;
> +   s->compatible_features = stashed_s->compatible_features;
> +
> +   s->unknown_header_fields_size = stashed_s->unknown_header_fields_size;
> +   s->unknown_header_fields = stashed_s->unknown_header_fields;
> +   s->unknown_header_ext = stashed_s->unknown_header_ext;
> +}
> +
>  static int qcow2_set_key(BlockDriverState *bs, const char *key)
>  {
>      BDRVQcowState *s = bs->opaque;
> @@ -1567,6 +1739,9 @@ static BlockDriver bdrv_qcow2 = {
>      .instance_size      = sizeof(BDRVQcowState),
>      .bdrv_probe         = qcow2_probe,
>      .bdrv_open          = qcow2_open,
> +    .bdrv_reopen_prepare = qcow2_reopen_prepare,
> +    .bdrv_reopen_commit = qcow2_reopen_commit,
> +    .bdrv_reopen_abort  = qcow2_reopen_abort,
>      .bdrv_close         = qcow2_close,
>      .bdrv_create        = qcow2_create,
>      .bdrv_co_is_allocated = qcow2_co_is_allocated,
> 
> 

Re: [Qemu-devel] [v2 Patch 5/9]block: qcow2 image file reopen

[Kevin Wolf]

Am 09.08.2012 06:26, schrieb Jeff Cody:
> On 07/30/2012 05:35 PM, Supriya Kannery wrote:
>> qcow2 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/qcow2.c
>> ===================================================================
>> --- qemu.orig/block/qcow2.c
>> +++ qemu/block/qcow2.c
>> @@ -52,10 +52,19 @@ typedef struct {
>>      uint32_t magic;
>>      uint32_t len;
>>  } QCowExtension;
>> +
>> +typedef struct BDRVQcowReopenState {
>> +    BDRVReopenState reopen_state;
>> +    BDRVQcowState *stash_s;
>> +} BDRVQcowReopenState;
>> +
>>  #define  QCOW2_EXT_MAGIC_END 0
>>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>  
>> +static void qcow2_stash_state(BDRVQcowState *stashed_state, BDRVQcowState *s);
>> +static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_state);
>> +
>>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>  {
>>      const QCowHeader *cow_header = (const void *)buf;
>> @@ -434,6 +443,169 @@ static int qcow2_open(BlockDriverState *
>>      return ret;
>>  }
>>  
>> +static int qcow2_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
>> +                               int flags)
>> +{
>> +    BDRVQcowReopenState *qcow2_rs = g_malloc0(sizeof(BDRVQcowReopenState));
>> +    int ret = 0;
>> +    BDRVQcowState *s = bs->opaque;
>> +
>> +    qcow2_rs->reopen_state.bs = bs;
>> +
>> +    /* save state before reopen */
>> +    qcow2_rs->stash_s = g_malloc0(sizeof(BDRVQcowState));
>> +    qcow2_stash_state(qcow2_rs->stash_s, s);
>> +    *prs = &(qcow2_rs->reopen_state);
>> +
>> +    /* Reopen file with new flags */
>> +     ret = qcow2_open(bs, flags);
>> +     return ret;
>> +}
>> +
>> +static void qcow2_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
>> +{
>> +    BDRVQcowReopenState *qcow2_rs;
>> +    BDRVQcowState *stashed_s;
>> +
>> +    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
>> +    stashed_s = qcow2_rs->stash_s;
>> +
>> +    qcow2_cache_flush(bs, stashed_s->l2_table_cache);
>> +    qcow2_cache_flush(bs, stashed_s->refcount_block_cache);
>> +
>> +    qcow2_cache_destroy(bs, stashed_s->l2_table_cache);
>> +    qcow2_cache_destroy(bs, stashed_s->refcount_block_cache);
>> +
>> +    g_free(stashed_s->unknown_header_fields);
>> +    cleanup_unknown_header_ext(bs);
>> +
>> +    g_free(stashed_s->cluster_cache);
>> +    qemu_vfree(stashed_s->cluster_data);
> 
> 
> 
>> +    qcow2_refcount_close(bs);
>> +    qcow2_free_snapshots(bs);
> 
> I assume these are unintentional?  I believe this is what is causing your
> double-free issue.

This whole patch looks a bit complicated and brittle. What qcow2 state
do you actually expect to change with a new qcow2_open()? Is it even
required to do more than just modifying bs->file? qcow2 doesn't really
do anything with the flags.

Kevin

[Qemu-devel] [v2 Patch 7/9]block: qed image file reopen

[Supriya Kannery]

qed 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/qed.c
===================================================================
--- qemu.orig/block/qed.c
+++ qemu/block/qed.c
@@ -18,6 +18,14 @@
 #include "qerror.h"
 #include "migration.h"
 
+typedef struct BDRVQEDReopenState {
+    BDRVReopenState reopen_state;
+    BDRVQEDState *stash_s;
+} BDRVQEDReopenState;
+
+static void qed_stash_state(BDRVQEDState *stashed_state, BDRVQEDState *s);
+static void qed_revert_state(BDRVQEDState *s, BDRVQEDState *stashed_state);
+
 static void qed_aio_cancel(BlockDriverAIOCB *blockacb)
 {
     QEDAIOCB *acb = (QEDAIOCB *)blockacb;
@@ -512,6 +520,98 @@ out:
     return ret;
 }
 
+static int bdrv_qed_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                               int flags)
+{
+    BDRVQEDReopenState *qed_rs = g_malloc0(sizeof(BDRVQEDReopenState));
+    int ret = 0;
+    BDRVQEDState *s = bs->opaque;
+
+    qed_rs->reopen_state.bs = bs;
+
+    /* save state before reopen */
+    qed_rs->stash_s = g_malloc0(sizeof(BDRVQEDState));
+    qed_stash_state(qed_rs->stash_s, s);
+    *prs = &(qed_rs->reopen_state);
+
+    /* Reopen file with new flags */
+     ret = bdrv_qed_open(bs, flags);
+     return ret;
+}
+
+static void bdrv_qed_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQEDReopenState *qed_rs;
+    BDRVQEDState *stashed_s;
+
+    qed_rs = container_of(rs, BDRVQEDReopenState, reopen_state);
+    stashed_s = qed_rs->stash_s;
+
+    qed_cancel_need_check_timer(stashed_s);
+    qemu_free_timer(stashed_s->need_check_timer);
+
+    qed_free_l2_cache(&stashed_s->l2_cache);
+    qemu_vfree(stashed_s->l1_table);
+
+    g_free(stashed_s);
+    g_free(qed_rs);
+}
+
+static void bdrv_qed_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQEDReopenState *qed_rs;
+    BDRVQEDState *s = bs->opaque;
+    BDRVQEDState *stashed_s;
+
+    qed_rs = container_of(rs, BDRVQEDReopenState, reopen_state);
+
+    /* Revert to stashed state */
+    qed_revert_state(s, qed_rs->stash_s);
+    stashed_s = qed_rs->stash_s;
+
+    g_free(stashed_s);
+    g_free(qed_rs);
+}
+
+static void qed_stash_state(BDRVQEDState *stashed_state, BDRVQEDState *s)
+{
+    stashed_state->bs = s->bs;
+    stashed_state->file_size = s->file_size;
+
+    stashed_state->header = s->header;
+    stashed_state->l1_table = s->l1_table;
+    stashed_state->l2_cache = s->l2_cache;
+    stashed_state->table_nelems = s->table_nelems;
+    stashed_state->l1_shift = s->l1_shift;
+    stashed_state->l2_shift = s->l2_shift;
+    stashed_state->l2_mask = s->l2_mask;
+
+    stashed_state->allocating_write_reqs = s->allocating_write_reqs;
+    stashed_state->allocating_write_reqs_plugged =
+                                         s->allocating_write_reqs_plugged;
+
+    stashed_state->need_check_timer = s->need_check_timer;
+}
+
+static void qed_revert_state(BDRVQEDState *s, BDRVQEDState *stashed_state)
+{
+    s->bs = stashed_state->bs;
+    s->file_size = stashed_state->file_size;
+
+    s->header = stashed_state->header;
+    s->l1_table = stashed_state->l1_table;
+    s->l2_cache = stashed_state->l2_cache;
+    s->table_nelems = stashed_state->table_nelems;
+    s->l1_shift = stashed_state->l1_shift;
+    s->l2_shift = stashed_state->l2_shift;
+    s->l2_mask = stashed_state->l2_mask;
+
+    s->allocating_write_reqs = s->allocating_write_reqs;
+    s->allocating_write_reqs_plugged = s->allocating_write_reqs_plugged;
+
+    s->need_check_timer = s->need_check_timer;
+}
+
 static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
@@ -1560,6 +1660,9 @@ static BlockDriver bdrv_qed = {
     .bdrv_rebind              = bdrv_qed_rebind,
     .bdrv_open                = bdrv_qed_open,
     .bdrv_close               = bdrv_qed_close,
+    .bdrv_reopen_prepare      = bdrv_qed_reopen_prepare,
+    .bdrv_reopen_commit       = bdrv_qed_reopen_commit,
+    .bdrv_reopen_abort        = bdrv_qed_reopen_abort,
     .bdrv_create              = bdrv_qed_create,
     .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
     .bdrv_make_empty          = bdrv_qed_make_empty,

[Qemu-devel] [v2 Patch 9/9]block: 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>

---
Index: qemu/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -453,6 +453,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)
 #
@@ -466,7 +468,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
@@ -2504,6 +2504,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
@@ -215,6 +215,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] [v2 Patch 9/9]block: Enhance "info block" to display host cache setting

[Eric Blake]

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

On 07/30/2012 03:36 PM, Supriya Kannery 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
> 
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
> 
> ---

No diffstat?  It's not a hard requirement, but it can make patch review
easier if 'git send-email' is set up to include diffstats.

> Index: qemu/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -453,6 +453,8 @@
>  # @locked: True if the guest has locked this device from having its media
>  #          removed
>  #
> +# @hostcache: True if host pagecache is enabled.
> +#

Missing a counterpart change to qmp-commands.hx to document it in use.

-- 
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] [v2 Patch 0/9]block: Image file reopen and dynamic host pagecache change

[Jeff Cody]

On 07/30/2012 05:34 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.
> 

Hi Supriya,

I never received patches 6 or 8, either through the list or at my direct
email address - looking at the qemu-devel archives, they are also
missing from there:

http://lists.gnu.org/archive/html/qemu-devel/2012-07/threads.html

Did those get lost somehow, or perhaps they didn't get sent?

Thanks,
Jeff

Re: [Qemu-devel] [v2 Patch 0/9]block: Image file reopen and dynamic host pagecache change

[Supriya Kannery]

On 08/01/2012 02:03 AM, Jeff Cody wrote:
> On 07/30/2012 05:34 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.
>>
>
> Hi Supriya,
>
> I never received patches 6 or 8, either through the list or at my direct
> email address - looking at the qemu-devel archives, they are also
> missing from there:
>
> http://lists.gnu.org/archive/html/qemu-devel/2012-07/threads.html
>
> Did those get lost somehow, or perhaps they didn't get sent?
>
> Thanks,
> Jeff
>
Thanks! Jeff, for pointing this out. Looks like they got lost somehow,
because the I had tried to send the patchset to myself as a trial before
posting to qemu-devel and there I can see 6 and 8 as well.
Will resend the lost patches now.
  -Rgds, Supriya

Re: [Qemu-devel] [v2 Patch 6/9]block: qcow image file reopen

[Supriya Kannery]

qcow 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: Shrinidhi Joshi <spjoshi31@gmail.com>

Index: qemu/block/qcow.c
===================================================================
--- qemu.orig/block/qcow.c
+++ qemu/block/qcow.c
@@ -78,7 +78,14 @@ typedef struct BDRVQcowState {
     Error *migration_blocker;
 } BDRVQcowState;
 
+typedef struct BDRVQcowReopenState {
+    BDRVReopenState reopen_state;
+    BDRVQcowState *stash_s;
+} BDRVQcowReopenState;
+
 static int decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
+static void qcow_stash_state(BDRVQcowState *stash_s, BDRVQcowState *s);
+static void qcow_revert_state(BDRVQcowState *s, BDRVQcowState *stash_s);
 
 static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -197,6 +204,103 @@ static int qcow_open(BlockDriverState *b
     return ret;
 }
 
+static int qcow_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+                                         int flags)
+{
+    BDRVQcowReopenState *qcow_rs = g_malloc0(sizeof(BDRVQcowReopenState));
+    int ret = 0;
+    BDRVQcowState *s = bs->opaque;
+
+    qcow_rs->reopen_state.bs = bs;
+    qcow_rs->stash_s = g_malloc0(sizeof(BDRVQcowState));
+    qcow_stash_state(qcow_rs->stash_s, s);
+    *prs = &(qcow_rs->reopen_state);
+
+    ret = qcow_open(bs, flags);
+    return ret;
+}
+
+static void qcow_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQcowReopenState *qcow_rs;
+
+    qcow_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
+    g_free(qcow_rs->stash_s);
+    g_free(qcow_rs);
+}
+
+static void qcow_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+    BDRVQcowReopenState *qcow_rs;
+    BDRVQcowState *s = bs->opaque;
+
+    qcow_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
+
+    /* revert to stashed state */
+    qcow_revert_state(s, qcow_rs->stash_s);
+
+    g_free(qcow_rs->stash_s);
+    g_free(qcow_rs);
+}
+
+static void qcow_stash_state(BDRVQcowState *stash_s, BDRVQcowState *s)
+{
+    int i;
+
+    stash_s->cluster_bits = s->cluster_bits;
+    stash_s->cluster_size = s->cluster_size;
+    stash_s->cluster_sectors = s->cluster_sectors;
+    stash_s->l2_bits = s->l2_bits;
+    stash_s->l2_size = s->l2_size;
+    stash_s->l1_size = s->l1_size;
+    stash_s->cluster_offset_mask = s->cluster_offset_mask;
+    stash_s->l1_table_offset = s->l1_table_offset;
+    stash_s->l1_table = s->l1_table;
+    stash_s->l2_cache = s->l2_cache;
+    for (i = 0; i < L2_CACHE_SIZE; i++) {
+        stash_s->l2_cache_offsets[i] = s->l2_cache_offsets[i];
+        stash_s->l2_cache_counts[i] = s->l2_cache_counts[i];
+    }
+    stash_s->cluster_cache = s->cluster_cache;
+    stash_s->cluster_data = s->cluster_data;
+    stash_s->cluster_cache_offset = s->cluster_cache_offset;
+    stash_s->crypt_method = s->crypt_method;
+    stash_s->crypt_method_header = s->crypt_method_header;
+    stash_s->aes_encrypt_key = s->aes_encrypt_key;
+    stash_s->aes_decrypt_key = s->aes_decrypt_key;
+    stash_s->lock = s->lock;
+    stash_s->migration_blocker = s->migration_blocker;
+}
+
+static void qcow_revert_state(BDRVQcowState *s, BDRVQcowState *stash_s)
+{
+    int i;
+
+    s->cluster_bits = stash_s->cluster_bits;
+    s->cluster_size = stash_s->cluster_size;
+    s->cluster_sectors = stash_s->cluster_sectors;
+    s->l2_bits = stash_s->l2_bits;
+    s->l2_size = stash_s->l2_size;
+    s->l1_size = stash_s->l1_size;
+    s->cluster_offset_mask = stash_s->cluster_offset_mask;
+    s->l1_table_offset = stash_s->l1_table_offset;
+    s->l1_table = stash_s->l1_table;
+    s->l2_cache = stash_s->l2_cache;
+    for (i = 0; i < L2_CACHE_SIZE; i++) {
+        s->l2_cache_offsets[i] = s->l2_cache_offsets[i];
+        s->l2_cache_counts[i] = stash_s->l2_cache_counts[i];
+    }
+    s->cluster_cache = stash_s->cluster_cache;
+    s->cluster_data = stash_s->cluster_data;
+    s->cluster_cache_offset = stash_s->cluster_cache_offset;
+    s->crypt_method = stash_s->crypt_method;
+    s->crypt_method_header = stash_s->crypt_method_header;
+    s->aes_encrypt_key = stash_s->aes_encrypt_key;
+    s->aes_decrypt_key = stash_s->aes_decrypt_key;
+    s->lock = stash_s->lock;
+    s->migration_blocker = stash_s->migration_blocker;
+}
+
 static int qcow_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -870,6 +974,10 @@ static BlockDriver bdrv_qcow = {
     .bdrv_close		= qcow_close,
     .bdrv_create	= qcow_create,
 
+    .bdrv_reopen_prepare    = qcow_reopen_prepare,
+    .bdrv_reopen_commit     = qcow_reopen_commit,
+    .bdrv_reopen_abort      = qcow_reopen_abort,
+
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
     .bdrv_co_is_allocated   = qcow_co_is_allocated,

[Qemu-devel] [v2 Patch 8/9]block: 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>

---
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -970,6 +970,30 @@ void bdrv_close_all(void)
     }
 }
 
+void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp)
+{
+    int bdrv_flags = bs->open_flags;
+
+    /* set hostcache flags (without changing WCE/flush bits) */
+    if (enable) {
+        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) {
+        if (bdrv_is_inserted(bs)) {
+            /* Reopen file with changed set of flags */
+            bdrv_flags &= ~BDRV_O_CACHE_WB;
+            bdrv_reopen(bs, bdrv_flags, errp);
+        } else {
+            /* Save hostcache change for future use */
+            bs->open_flags = bdrv_flags;
+        }
+    }
+}
+
 /*
  * Wait for pending requests to complete across all BlockDriverStates
  *
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -1191,3 +1191,22 @@ 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.
+*/
+void qmp_block_set_hostcache(const char *device, bool enable,
+                             Error **errp)
+{
+    BlockDriverState *bs = NULL;
+
+    /* Validate device */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bdrv_change_hostcache(bs, enable, errp);
+    return;
+}
Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -1344,6 +1344,21 @@ passed since 1970, i.e. unix epoch.
 ETEXI
 
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache",
+        .mhandler.cmd = hmp_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
+
+
+    {
         .name       = "info",
         .args_type  = "item:s?",
         .params     = "[subcommand]",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -987,6 +987,30 @@ Example:
 EQMP
 
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .mhandler.cmd_new = qmp_marshal_input_block_set_hostcache,
+    },
+
+SQMP
+block_set_hostcache
+-------------------
+
+Change host pagecache setting of a block device
+
+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       = "set_password",
         .args_type  = "protocol:s,password:s,connected:s?",
         .mhandler.cmd_new = qmp_marshal_input_set_password,
Index: qemu/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -1604,6 +1604,23 @@
             'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
 
 ##
+# @block_set_hostcache:
+#
+# Change host pagecache setting of a block device
+#
+# @device: name of the block device
+#
+# @option: hostcache setting (true/false)
+#
+# Returns: Nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
+# Since: 1.2
+##
+{ 'command': 'block_set_hostcache',
+  'data': { 'device': 'str', 'option': 'bool' } }
+
+##
 # @block-stream:
 #
 # Copy data from a backing file into a block device.
Index: qemu/hmp.c
===================================================================
--- qemu.orig/hmp.c
+++ qemu/hmp.c
@@ -837,6 +837,15 @@ void hmp_block_set_io_throttle(Monitor *
     hmp_handle_error(mon, &err);
 }
 
+void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
+                              qdict_get_bool(qdict, "option"), &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_block_stream(Monitor *mon, const QDict *qdict)
 {
     Error *error = NULL;
Index: qemu/hmp.h
===================================================================
--- qemu.orig/hmp.h
+++ qemu/hmp.h
@@ -66,5 +66,6 @@ void hmp_netdev_add(Monitor *mon, const 
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
 void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
+void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
 
 #endif
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -187,6 +187,7 @@ int bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
+void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp);
 
 
 typedef struct BdrvCheckResult {

Re: [Qemu-devel] [v2 Patch 8/9]block: Cmd "block_set_hostcache" for dynamic cache change

[Eric Blake]

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

On 08/01/2012 12:44 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>
> +++ qemu/qmp-commands.hx
> @@ -987,6 +987,30 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block_set_hostcache",

QMP version should be block-set-hostcache.

> +++ qemu/qapi-schema.json
> @@ -1604,6 +1604,23 @@
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
>  
>  ##
> +# @block_set_hostcache:
> +#
> +# Change host pagecache setting of a block device
> +#
> +# @device: name of the block device
> +#
> +# @option: hostcache setting (true/false)
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'block_set_hostcache',
> +  'data': { 'device': 'str', 'option': 'bool' } }

Here as well.

-- 
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] [v2 Patch 8/9]block: Cmd "block_set_hostcache" for dynamic cache change

[Luiz Capitulino]

On Thu, 02 Aug 2012 00:14:47 +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>
> 
> ---
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -970,6 +970,30 @@ void bdrv_close_all(void)
>      }
>  }
>  
> +void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable) {
> +        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) {
> +        if (bdrv_is_inserted(bs)) {
> +            /* Reopen file with changed set of flags */
> +            bdrv_flags &= ~BDRV_O_CACHE_WB;
> +            bdrv_reopen(bs, bdrv_flags, errp);
> +        } else {
> +            /* Save hostcache change for future use */
> +            bs->open_flags = bdrv_flags;
> +        }
> +    }
> +}
> +
>  /*
>   * Wait for pending requests to complete across all BlockDriverStates
>   *
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -1191,3 +1191,22 @@ 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.
> +*/
> +void qmp_block_set_hostcache(const char *device, bool enable,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    /* Validate device */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bdrv_change_hostcache(bs, enable, errp);
> +    return;
> +}
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -1344,6 +1344,21 @@ passed since 1970, i.e. unix epoch.
>  ETEXI
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache",
> +        .mhandler.cmd = hmp_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
> +
> +
> +    {
>          .name       = "info",
>          .args_type  = "item:s?",
>          .params     = "[subcommand]",
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -987,6 +987,30 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .mhandler.cmd_new = qmp_marshal_input_block_set_hostcache,
> +    },
> +
> +SQMP
> +block_set_hostcache
> +-------------------
> +
> +Change host pagecache setting of a block device
> +
> +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       = "set_password",
>          .args_type  = "protocol:s,password:s,connected:s?",
>          .mhandler.cmd_new = qmp_marshal_input_set_password,
> Index: qemu/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -1604,6 +1604,23 @@
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
>  
>  ##
> +# @block_set_hostcache:
> +#
> +# Change host pagecache setting of a block device
> +#
> +# @device: name of the block device
> +#
> +# @option: hostcache setting (true/false)

I'd call this enable, as you do in the code.

Otherwise (and apart from other review comments) this looks good to me.

> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'block_set_hostcache',
> +  'data': { 'device': 'str', 'option': 'bool' } }
> +
> +##
>  # @block-stream:
>  #
>  # Copy data from a backing file into a block device.
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -837,6 +837,15 @@ void hmp_block_set_io_throttle(Monitor *
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
> +                              qdict_get_bool(qdict, "option"), &err);
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  {
>      Error *error = NULL;
> Index: qemu/hmp.h
> ===================================================================
> --- qemu.orig/hmp.h
> +++ qemu/hmp.h
> @@ -66,5 +66,6 @@ void hmp_netdev_add(Monitor *mon, const 
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>  void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
>  
>  #endif
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -187,6 +187,7 @@ int bdrv_commit_all(void);
>  int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
> +void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp);
>  
>  
>  typedef struct BdrvCheckResult {
> 

Re: [Qemu-devel] [v2 Patch 8/9]block: Cmd "block_set_hostcache" for dynamic cache change

[Jeff Cody]

On 08/01/2012 02:44 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>
> 
> ---
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -970,6 +970,30 @@ void bdrv_close_all(void)
>      }
>  }
>  
> +void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable) {
> +        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) {
> +        if (bdrv_is_inserted(bs)) {
> +            /* Reopen file with changed set of flags */
> +            bdrv_flags &= ~BDRV_O_CACHE_WB;
> +            bdrv_reopen(bs, bdrv_flags, errp);
> +        } else {
> +            /* Save hostcache change for future use */
> +            bs->open_flags = bdrv_flags;
> +        }
> +    }
> +}
> +

BDRV_O_CACHE_WB is cleared here, and it never gets set again. Is that
intentional?


>  /*
>   * Wait for pending requests to complete across all BlockDriverStates
>   *
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -1191,3 +1191,22 @@ 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.
> +*/
> +void qmp_block_set_hostcache(const char *device, bool enable,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    /* Validate device */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    bdrv_change_hostcache(bs, enable, errp);
> +    return;
> +}
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -1344,6 +1344,21 @@ passed since 1970, i.e. unix epoch.
>  ETEXI
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache",
> +        .mhandler.cmd = hmp_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
> +
> +
> +    {
>          .name       = "info",
>          .args_type  = "item:s?",
>          .params     = "[subcommand]",
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -987,6 +987,30 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .mhandler.cmd_new = qmp_marshal_input_block_set_hostcache,
> +    },
> +
> +SQMP
> +block_set_hostcache
> +-------------------
> +
> +Change host pagecache setting of a block device
> +
> +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       = "set_password",
>          .args_type  = "protocol:s,password:s,connected:s?",
>          .mhandler.cmd_new = qmp_marshal_input_set_password,
> Index: qemu/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -1604,6 +1604,23 @@
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
>  
>  ##
> +# @block_set_hostcache:
> +#
> +# Change host pagecache setting of a block device
> +#
> +# @device: name of the block device
> +#
> +# @option: hostcache setting (true/false)
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'block_set_hostcache',
> +  'data': { 'device': 'str', 'option': 'bool' } }
> +
> +##
>  # @block-stream:
>  #
>  # Copy data from a backing file into a block device.
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -837,6 +837,15 @@ void hmp_block_set_io_throttle(Monitor *
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
> +                              qdict_get_bool(qdict, "option"), &err);
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  {
>      Error *error = NULL;
> Index: qemu/hmp.h
> ===================================================================
> --- qemu.orig/hmp.h
> +++ qemu/hmp.h
> @@ -66,5 +66,6 @@ void hmp_netdev_add(Monitor *mon, const 
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>  void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
>  
>  #endif
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -187,6 +187,7 @@ int bdrv_commit_all(void);
>  int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
> +void bdrv_change_hostcache(BlockDriverState *bs, bool enable, Error **errp);
>  
>  
>  typedef struct BdrvCheckResult {
> 
> 

Re: [Qemu-devel] [v2 Patch 0/9]block: Image file reopen and dynamic host pagecache change

[Jeff Cody]

On 07/30/2012 05:34 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.
> 
> V1 introduced:
>  * 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 implemented for raw-posix, 
>     raw-win32, vmdk, qcow, qcow2 and qed block drivers.
>   
>  * 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.
> 
> V2:
>  * Changed ordering of patches such that code changes related to 
>    generic framework for safely reopening images gets applied first.
> 
>  * For block drivers not having bdrv_reopen_xx functions 
>    implemented, return "feature not supported" error.
> 
> Testing:
> =======
> [Thanks! to Yoganananth Subramanian for helping out with testing]
> 
> Steps:
> 1) boot up guest image of different formats qed, raw, qcow2, vmdk
> 2) run iozone in these guests
>    command: iozone -a
> 3) view cache setting of image file through qemu monitor
>    command: info block
>             "hostcache =" 0/1 should be displayed
> 4) Toggle hostcache value using block_set_hostcache
>    command: block_set_hostcache virtio0 on
> 5) Disable and enable hostcache at randon intervals while iozone is
>    running inside guest.
>    command: block_set_hostcache virtio0 on/off
> 6) Info block should reflect toggled hostcache value
>    and iozone should complete without any issue
> 
> Results:
>   Verified above steps for raw-posix, qcow2, qed and vmdk images.
>   raw-posix, qed and vmdk images (split files) worked fine. With
>   qcow2 image getting an error of double free after iozone running
>   for a while.

I believe the double free you are seeing above is due to closing the
refcount at the end of qcow2_reopen_commit() (see comments on patch
5/9).  Whenever that image is closed with bdrv_close(),
qcow2_refcount_close() is called again.

I had to make just a few small changes to the patches to get them
working for reopen() calls that had file access permission changes
(r/o->r/w), so I will try and document each of these changes in replies
to your other patches.

I applied the changes in my other replies your patches to my github branch
so they would work for going from r/o->r/w->r/o, for testing block commit:
https://github.com/codyprime/qemu-kvm-jtc/commits/jtc-live-commit-v3

I only made patches for qcow2 and raw-posix, which is what I have been
testing with block commit (although I imagine the changes for the other
formats are similar).

Another thought, that is more design oriented: as it currently is,
reopen() is not transactional for multiple BlockDriverStates.  It would
be nice if we could queue up multiple BDSs to reopen into a transaction,
and only perform the commit() if the prepare() was successful for all of
the BDS entries. Perhaps using something like QSIMPLEQ could be useful
to accomplish this.

I think this is especially important for formats like qcow2, where we
need to reopen both the bs and bs->file.



> 
> To Do:
> ======
> * Debug the issue with qcow2 image and resolve asap.
> * Enhance code around dup3 in raw-posix to fall back to
>   dup2/dup when dup3 is not supported by OS.
> * Do some more extensive testing, especially with qcow2 and
>   qed drivers.
> 
>  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           |   79 ++++++++++++++++++++++
>  qemu/block.h           |    5 +
>  qemu/block/qcow.c      |  108 ++++++++++++++++++++++++++++++
>  qemu/block/qcow2.c     |  175 +++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu/block/qed.c       |  103 ++++++++++++++++++++++++++++
>  qemu/block/raw-posix.c |  121 +++++++++++++++++++++++++++++++++
>  qemu/block/raw-win32.c |   96 ++++++++++++++++++++++++++
>  qemu/block/raw.c       |   20 +++++
>  qemu/block/vmdk.c      |  103 ++++++++++++++++++++++++++++
>  qemu/block_int.h       |   12 +++
>  qemu/blockdev.c        |   19 +++++
>  qemu/hmp-commands.hx   |   15 ++++
>  qemu/hmp.c             |   11 ++
>  qemu/hmp.h             |    1
>  qemu/qapi-schema.json  |   21 ++++-
>  qemu/qemu-common.h     |    1
>  qemu/qmp-commands.hx   |   24 ++++++
>  17 files changed, 912 insertions(+), 2 deletions(-)
> 
> 
> 
> 
>