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

[Stefan Hajnoczi]

On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <address@hidden> wrote:
> Stefan Hajnoczi wrote:
>>
>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
>> <address@hidden> wrote:
>>
>>>
>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>>
>>>>
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <address@hidden>  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?

The interesting feature that this series adds is changing of hostcache
at run-time.

Stefan