From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSlm9-0003Nw-7D for qemu-devel@nongnu.org; Tue, 22 Nov 2011 03:32:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSlm7-0005rt-LH for qemu-devel@nongnu.org; Tue, 22 Nov 2011 03:32:21 -0500 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:54326) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSlm7-0005rl-1U for qemu-devel@nongnu.org; Tue, 22 Nov 2011 03:32:19 -0500 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 22 Nov 2011 08:30:01 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pAM8VwNs3629210 for ; Tue, 22 Nov 2011 19:31:58 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pAM8VvnU030393 for ; Tue, 22 Nov 2011 19:31:58 +1100 Message-ID: <4ECB590D.2090602@in.ibm.com> Date: Tue, 22 Nov 2011 13:40:53 +0530 From: supriya kannery MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064802.15024.83662.sendpatchset@skannery.in.ibm.com> <4EC4991C.1070306@linux.vnet.ibm.com> <4ECA43F2.7080506@in.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , supriyak@linux.vnet.ibm.com, Luiz Capitulino , Christoph Hellwig , qemu-devel@nongnu.org Stefan Hajnoczi wrote: > On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery wrote: > >> Stefan Hajnoczi wrote: >> >>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery >>> wrote: >>> >>> >>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote: >>>> >>>> >>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery >>>>> wrote: >>>>> >>>>> >>>>>> + if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != >>>>>> -1) { >>>>>> >>>>>> >>>>> This does not work. qemu_opt_get_bool() takes a bool default argument >>>>> and returns a bool. (bool)-1 == true. But (int)true == 1 and you >>>>> cannot expect it to ever equal -1. >>>>> >>>>> Try this: >>>>> >>>>> if (qemu_opt_get(opts, "hostcache")&& >>>>> !qemu_opt_get_bool(opts, "hostcache", false)) { >>>>> bdrv_flags |= BDRV_O_NOCACHE; >>>>> } >>>>> >>>>> Stefan >>>>> >>>>> >>>>> >>>> Thanks! for pointing this. >>>> Does the following look ok? >>>> >>>> if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) { >>>> bdrv_flags |= BDRV_O_NOCACHE; >>>> } >>>> >>>> If either "hostcache" is not at all specified or it is specified >>>> as "on", qemu_opt_get_bool will return 1, which can be ignored >>>> as bdrv_flags is initialized to 0. >>>> >>>> >>> It depends on the overall way this should work. I think this captures >>> all the cases: >>> >>> 1. cache= and hostcache= may not be used together. >>> 2. cache= sets bdrv_flags. >>> 3. hostcache= may |= BDRV_O_NOCACHE. >>> 4. No option defaults to cache=writethrough (bdrv_flags &= >>> ~BDRV_O_CACHE_MASK). >>> >>> The code you posted will work although I find it a bit weird how it >>> also includes case #4. IMO it's cleanest to just do case #3 by >>> testing whether or not the hostcache= option is set. >>> >>> BTW, is there a check for case #1 in your patch series. I thought I >>> saw one earlier but now I can't find it. >>> >>> >> Following is what I tried to accomplish: >> >> 1. cache= and hostcache= may be used together. Cache= will override >> hostcache= if specified together >> This condition can be retained till cache-xx can be completely replaced by >> combinations of separate cmdline options defined for setting hostcache, >> flush, and WCE >> [I think I can change the current implementation to have Cache=xx ORed with >> hostcache=yy if they are specified together instead of just ignoring >> hostcache= ] >> > > Okay, I can see that line of reasoning but then hostcache= does not > provide much value on the command-line. Perhaps it's better to drop > this patch and not merge a new -drive option until the guest-visible > write cache enable support is also merged? > > Let us have the implementation for hostcache= as command line option, with the condition that if both cache= and hostcache= are specified together, then depending upon enable/disable value specified for hostcache, corresponding bit in cache flags can be set/reset. That way, there is a value add on specifying hostcache in cmdline as well as user will be able to control hostcache value from cmdline itself. > The interesting feature that this series adds is changing of hostcache > at run-time. > > > Yes.. will resume with dynamic hostcache change part to make it usable by qemu - thanks, Supriya > Stefan > >