From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzKK9-0005fK-85 for qemu-devel@nongnu.org; Thu, 09 Aug 2012 00:26:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzKK6-0008HJ-Hb for qemu-devel@nongnu.org; Thu, 09 Aug 2012 00:26:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8793) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzKK6-0008GE-3F for qemu-devel@nongnu.org; Thu, 09 Aug 2012 00:26:14 -0400 Message-ID: <50233BDD.1000000@redhat.com> Date: Thu, 09 Aug 2012 00:26:05 -0400 From: Jeff Cody MIME-Version: 1.0 References: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com> In-Reply-To: <20120730213409.21536.7589.sendpatchset@skannery.in.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2 Patch 0/9]block: Image file reopen and dynamic host pagecache change Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Supriya Kannery Cc: Kevin Wolf , Shrinidhi Joshi , Stefan Hajnoczi , qemu-devel@nongnu.org, Luiz Capitulino , Christoph Hellwig 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