| Message ID | 20111111064802.15024.83662.sendpatchset@skannery.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 > >
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
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 > >
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
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
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
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 */ } },
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(-)