From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QlaIh-0000Sj-Nb for qemu-devel@nongnu.org; Tue, 26 Jul 2011 01:35:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QlaIg-0007Al-Ic for qemu-devel@nongnu.org; Tue, 26 Jul 2011 01:35:27 -0400 Received: from e28smtp08.in.ibm.com ([122.248.162.8]:37195) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QlaIf-0007Aa-Rc for qemu-devel@nongnu.org; Tue, 26 Jul 2011 01:35:26 -0400 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp08.in.ibm.com (8.14.4/8.13.1) with ESMTP id p6Q5ECAe002536 for ; Tue, 26 Jul 2011 10:44:12 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p6Q5ZJBR913420 for ; Tue, 26 Jul 2011 11:05:19 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p6Q5ZIew000834 for ; Tue, 26 Jul 2011 11:05:19 +0530 Message-ID: <4E2E54DB.2010007@in.ibm.com> Date: Tue, 26 Jul 2011 11:17:07 +0530 From: Supriya Kannery MIME-Version: 1.0 References: <20110704104237.28170.97021.sendpatchset@skannery> <20110704104320.28170.29248.sendpatchset@skannery> <4E12ED9A.7070805@in.ibm.com> <4E1D9879.8060009@in.ibm.com> <20110725063015.GA12854@stefanha-thinkpad.localdomain> <4E2D6718.3060300@in.ibm.com> <4E2D70D8.8060505@redhat.com> In-Reply-To: <4E2D70D8.8060505@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Christoph Hellwig On 07/25/2011 07:04 PM, Kevin Wolf wrote: > Am 25.07.2011 14:50, schrieb Stefan Hajnoczi: >> On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery wrote: >>> On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote: >>>> On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote: >>>>> + if (bdrv_is_inserted(bs)) { >>>>> + /* Reopen file with changed set of flags */ >>>>> + return bdrv_reopen(bs, bdrv_flags); >>>>> + } else { >>>>> + /* Save hostcache change for future use */ >>>>> + bs->open_flags = bdrv_flags; >>>> Can you explain the scenario where this works? >>>> >>>> Looking at do_change_block() the flags will be clobbered so saving them >>>> away does not help. >>> Intention here is to use the changed hostcache setting during device >>> insertion and there after. To apply this to 'change' command (during >>> insertion of a device), will need to make the following code changes >>> in do_change_block. >>> >>> + >>> + bdrv_flags = bs->open_flags; >>> + bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; >>> bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; >>> if (bdrv_open(bs, filename, bdrv_flags, drv)< 0) { >>> qerror_report(QERR_OPEN_FILE_FAILED, filename); >>> >>> Need to track bs->open_flags a bit more to see what other changes >>> needed to ensure usage of changed hostcache setting until user >>> initiates next hostcache change explicitly for that drive. >>> >>> Please suggest... should we be saving the hostcache change for >>> future use or just inhibit user from changing hostcache setting >>> for an empty drive? >> The 'change' command does not support any cache= options today. It >> always opens the new image with cache=writethrough semantics. >> >> This seems like a bug in 'change' to me. We should preserve cache= >> settings across change or at least provide a way to specify them as >> arguments. >> >> Perhaps your existing code is fine. When 'change' is fixed or >> replaced then 'block_set' will work across 'change' too. >> >> Kevin: Thoughts? > > I'm not sure if I would say it's a bug. Probably preserving would be > more useful in most cases, but using the default cache mode doesn't > appear completely wrong either. If you like to change it, I'm not > opposed to it. > > Kevin > ok, so for now, will retain the code that saves hostcache setting for empty drives. When other commands (like 'change') are focused, can look at using this saved setting. Will post V5 with other error_report related code changes. thanks, Supriya