Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[supriya kannery <supriyak@in.ibm.com>]

Stefan Hajnoczi wrote:
> On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <supriyak@in.ibm.com> wrote:
>   
>> Stefan Hajnoczi wrote:
>>     
>>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
>>> <supriyak@linux.vnet.ibm.com> wrote:
>>>
>>>       
>>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>>>
>>>>         
>>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>>> <supriyak@linux.vnet.ibm.com>  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
>
>