Patchwork [v9,4/6] Qemu: Add commandline -drive option 'hostcache'

login
register
mail settings
Submitter Supriya Kannery
Date Nov. 11, 2011, 6:48 a.m.
Message ID <20111111064802.15024.83662.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/125098/
State New
Headers show

Comments

Supriya Kannery - Nov. 11, 2011, 6:48 a.m.
qemu command option 'hostcache' added to -drive for block devices.
While starting a VM from qemu commandline, this option can be used 
for setting host cache usage for block data access.

Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>

---
 blockdev.c      |   13 +++++++++++++
 qemu-config.c   |    4 ++++
 qemu-options.hx |    2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)
Stefan Hajnoczi - Nov. 16, 2011, 8:06 p.m.
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
Supriya Kannery - Nov. 17, 2011, 5:18 a.m.
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.
Stefan Hajnoczi - Nov. 17, 2011, 2:11 p.m.
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.

Stefan
supriya kannery - Nov. 21, 2011, 12:28 p.m.
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= ]
2. If only cache= specified sets bdrv_flags.
3. If only hostcache= specified, bdrv_flags |= BDRV_O_NOCACHE
4. No option defaults to cache=writethrough (bdrv_flags &= ~BDRV_O_CACHE_MASK).

So the check I had earlier for case #1 in your list, I changed that to 
allow both the options to co-exist.

-thanks, Supriya
> Stefan
>
>
Stefan Hajnoczi - Nov. 21, 2011, 2:03 p.m.
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?

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

Stefan
supriya kannery - Nov. 22, 2011, 8:10 a.m.
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
>
>
Kevin Wolf - Nov. 22, 2011, 9:55 a.m.
Am 22.11.2011 09:10, schrieb supriya kannery:
> 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 additional thing this would introduce is cache=unsafe hostcache=off.
Probably not the most common thing to have, but why not allow it now.

Kevin
Stefan Hajnoczi - Nov. 22, 2011, 11:17 a.m.
On Tue, Nov 22, 2011 at 9:55 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 22.11.2011 09:10, schrieb supriya kannery:
>> 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 additional thing this would introduce is cache=unsafe hostcache=off.
> Probably not the most common thing to have, but why not allow it now.

QEMU's command-line is hairy enough already.  I don't think this
option is helpful until we can replace cache= completely.

This is a subjective issue though so if you want to take this option,
Kevin, then please do.

Stefan
Kevin Wolf - Nov. 22, 2011, 11:31 a.m.
Am 22.11.2011 12:17, schrieb Stefan Hajnoczi:
> On Tue, Nov 22, 2011 at 9:55 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 22.11.2011 09:10, schrieb supriya kannery:
>>> 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 additional thing this would introduce is cache=unsafe hostcache=off.
>> Probably not the most common thing to have, but why not allow it now.
> 
> QEMU's command-line is hairy enough already.  I don't think this
> option is helpful until we can replace cache= completely.
> 
> This is a subjective issue though so if you want to take this option,
> Kevin, then please do.

In fact, I think I suggested removing it in an earlier version of the
series because I felt the same. I didn't have a strong enough opinion on
it to really insist on it, though. Discuss it with Supriya and I'll be
happy with whatever you decide.

Kevin

Patch

Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -237,6 +237,7 @@  DriveInfo *drive_init(QemuOpts *opts, in
     DriveInfo *dinfo;
     int snapshot = 0;
     int ret;
+    int hostcache = 0;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
     media = MEDIA_DISK;
@@ -324,6 +325,12 @@  DriveInfo *drive_init(QemuOpts *opts, in
             error_report("invalid cache option");
             return NULL;
         }
+    } else {
+        if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
+            if (!hostcache) {
+                bdrv_flags |= BDRV_O_NOCACHE;
+            }
+        }
     }
 
 #ifdef CONFIG_LINUX_AIO
Index: qemu/qemu-options.hx
===================================================================
--- qemu.orig/qemu-options.hx
+++ qemu/qemu-options.hx
@@ -135,7 +135,7 @@  DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off]\n"
+    "       [,readonly=on|off][,hostcache=on|off]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
 STEXI
 @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
Index: qemu/qemu-config.c
===================================================================
--- qemu.orig/qemu-config.c
+++ qemu/qemu-config.c
@@ -85,6 +85,10 @@  static QemuOptsList qemu_drive_opts = {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
+        },{
+            .name = "hostcache",
+            .type = QEMU_OPT_BOOL,
+            .help = "set or reset hostcache (on/off)"
         },
         { /* end of list */ }
     },