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(-)
> 
> 
> 
> 
>