[Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Prerna Saxena]

The following patchset introduces monitor commands:

1. set_cache DEVICE CACHE-SETTING
Change cache settings for block device, DEVICE, through the monitor.
(Available options : 'none', 'writeback', 'writethrough')
Eg,
(qemu)set_cache ide0-hd0 none 
-> Changes cache setting for ide0-hd0 to 'none'

2. info block
Now extended to display cache settings for available block devices.

TODOS :
-------
1. Support 'unsafe' cache mode.
2. Display current cache setting for device, if the CACHE-SETTING option
is not supplied by the user. Eg, 
(qemu)set_cache ide0-hd0
presently errors out. Ideally, it should display current cache setting 
for the given device ide0-hd0

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] [RFC][PATCH 1/2] Add monitor command 'set-cache' to change cache settings for a block device.

[Prerna Saxena]

Usage :
(qemu) set_cache DEVICE CACHE-MODE
where CACHE-MODE can be one of writeback/ writethrough/ none.

At present, the image file is closed and re-opened with appropriate flags.
It might potentially cause problems if the underlying image is deleted 
while a running qemu instance is using it. A change in cache operations
will cause the image file to be closed, and a deleted file will be gone.
Suggestions to fix this ?

---
 blockdev.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   13 +++++++++
 3 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0690cc8..6735205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -636,6 +636,82 @@ out:
     return ret;
 }
 
+int do_set_cache(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *cache = qdict_get_str(qdict, "cache");
+    BlockDriverState *bs;
+    BlockDriver *drv;
+    int ret = 0;
+    int bdrv_flags = 0;
+
+    if (!cache) {
+	/* TODO: in the absence of a change request,
+                 simply display current cache setting.
+                 Currently one needs 'info block' to query this */
+        qerror_report(QERR_MISSING_PARAMETER, "cache");
+        return -1;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    /* Clear old flags */
+    bdrv_flags = bs->open_flags;
+    if (bdrv_flags & BDRV_O_CACHE_MASK) {
+        bdrv_flags &= ~BDRV_O_CACHE_MASK;
+    }
+
+    /* Determine flags for requested cache setting */
+    if (!strcmp(cache, "none")) {
+        bdrv_flags |= BDRV_O_NOCACHE;
+    } else if (!strcmp(cache, "writeback")) {
+        bdrv_flags |= BDRV_O_CACHE_WB;
+    } else if (!strcmp(cache, "unsafe")) {
+	/* TODO : Support unsafe mode */
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, cache,
+                       "writeback, writethrough, none");
+        return -1;
+    } else if (!strcmp(cache, "writethrough")) {
+        /* Default setting */
+    } else {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, cache,
+                       "'cache' must be one of writeback, writethrough, none");
+        return -1;
+    }
+
+    /* Verify that the cache setting specified is different from current.
+     * Does NOT call for error return, since the 'request' is already
+     * honoured.
+     */
+    if (bdrv_flags == bs->open_flags) {
+        qerror_report(QERR_PROPERTY_VALUE_IN_USE, device, "cache", cache);
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    /* Change cache value and restart IO on the block device */
+    printf("Setting cache=%s for device %s [ filename %s ]", cache, device,
+                                                            bs->filename );
+    drv = bs->drv;
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+     */
+    if (ret != 0) {
+        abort();
+    }
+
+    return ret;
+}
+
 static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
 {
     if (!force) {
diff --git a/blockdev.h b/blockdev.h
index 2c9e780..9f35817 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -63,6 +63,7 @@ int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_set_cache(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 372bef4..18761cf 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1066,7 +1066,20 @@ STEXI
 @findex watchdog_action
 Change watchdog action.
 ETEXI
+    {
+        .name       = "set_cache",
+        .args_type  = "device:B,cache:s",
+        .params     = "device writeback|writethrough|none",
+        .help       = "change cache settings for device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_set_cache,
+    },
 
+STEXI
+@item set_cache
+@findex set_cache
+Set cache options for a block device.
+ETEXI
     {
         .name       = "acl_show",
         .args_type  = "aclname:s",
-- 
1.7.2.3



-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] [RFC][PATCH 2/2] Extend monitor command 'info block' to display cache settings for block devices.

[Prerna Saxena]

(qemu)info block
SAMPLE output :
ide0-hd0: type=hd removable=0 cache=none file=/tmp/abc.img ro=0
drv=qcow2 encrypted=0

---
 block.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index f7d91a2..c717888 100644
--- a/block.c
+++ b/block.c
@@ -1707,6 +1707,23 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
         monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
     }
 
+    if (qdict_haskey(bs_dict, "open_flags") &&
+        !strcmp(qdict_get_str(bs_dict, "type"), "hd")) {
+        int open_flags = qdict_get_int(bs_dict, "open_flags");
+        if (open_flags & BDRV_O_NOCACHE) {
+            monitor_printf(mon, " cache=none");
+        } else if (open_flags & BDRV_O_CACHE_WB) {
+            if (open_flags & BDRV_O_NO_FLUSH) {
+                monitor_printf(mon, " cache=unsafe");
+            }
+            else {
+                monitor_printf(mon, " cache=writeback");
+            }
+        } else {
+            monitor_printf(mon, " cache=writethrough");
+        }
+    }
+
     if (qdict_haskey(bs_dict, "inserted")) {
         QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
 
@@ -1756,9 +1773,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
         }
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
-                                    "'removable': %i, 'locked': %i }",
+                                    "'removable': %i, 'locked': %i, "
+                                    "'open_flags': %d }",
                                     bs->device_name, type, bs->removable,
-                                    bs->locked);
+                                    bs->locked, bs->open_flags);
 
         if (bs->drv) {
             QObject *obj;
-- 
1.7.2.3



-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 28.02.2011 12:49, schrieb Prerna Saxena:
> The following patchset introduces monitor commands:
> 
> 1. set_cache DEVICE CACHE-SETTING
> Change cache settings for block device, DEVICE, through the monitor.
> (Available options : 'none', 'writeback', 'writethrough')
> Eg,
> (qemu)set_cache ide0-hd0 none 
> -> Changes cache setting for ide0-hd0 to 'none'

Not sure if adding this interface is a good idea. I see that you only
add it for HMP, and we may consider that, but it's definitely not
suitable for QMP.

One reason is that none/writethrough/writeback/unsafe isn't really what
we want to use long term. We want to separate advertising a write cache
(which is guest visible) from things like whether to use O_DIRECT or not.

In the past, Christoph mentioned that he had patches to make these
separate and even let the guest change the "write cache enabled" flag,
which would probably solve most of the use cases of this patch.

Christoph, what's the status of these patches?

> 2. info block
> Now extended to display cache settings for available block devices.

Have you checked that libvirt is okay with this?

Kevin

Re: [Qemu-devel] [RFC][PATCH 2/2] Extend monitor command 'info block' to display cache settings for block devices.

[Kevin Wolf]

Am 28.02.2011 13:11, schrieb Prerna Saxena:
> (qemu)info block
> SAMPLE output :
> ide0-hd0: type=hd removable=0 cache=none file=/tmp/abc.img ro=0
> drv=qcow2 encrypted=0
> 
> ---
>  block.c |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f7d91a2..c717888 100644
> --- a/block.c
> +++ b/block.c
> @@ -1707,6 +1707,23 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
>          monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
>      }
>  
> +    if (qdict_haskey(bs_dict, "open_flags") &&
> +        !strcmp(qdict_get_str(bs_dict, "type"), "hd")) {
> +        int open_flags = qdict_get_int(bs_dict, "open_flags");
> +        if (open_flags & BDRV_O_NOCACHE) {
> +            monitor_printf(mon, " cache=none");
> +        } else if (open_flags & BDRV_O_CACHE_WB) {
> +            if (open_flags & BDRV_O_NO_FLUSH) {
> +                monitor_printf(mon, " cache=unsafe");
> +            }
> +            else {
> +                monitor_printf(mon, " cache=writeback");
> +            }
> +        } else {
> +            monitor_printf(mon, " cache=writethrough");
> +        }
> +    }
> +
>      if (qdict_haskey(bs_dict, "inserted")) {
>          QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
>  
> @@ -1756,9 +1773,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>          }
>  
>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
> -                                    "'removable': %i, 'locked': %i }",
> +                                    "'removable': %i, 'locked': %i, "
> +                                    "'open_flags': %d }",
>                                      bs->device_name, type, bs->removable,
> -                                    bs->locked);
> +                                    bs->locked, bs->open_flags);

IIUC, this is the structure that a QMP client will receive for a
query-block command. The open_flags bitmask is not considered an ABI and
completely meaningless outside qemu.

Kevin

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Stefan Hajnoczi]

On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 28.02.2011 12:49, schrieb Prerna Saxena:
>> The following patchset introduces monitor commands:
>>
>> 1. set_cache DEVICE CACHE-SETTING
>> Change cache settings for block device, DEVICE, through the monitor.
>> (Available options : 'none', 'writeback', 'writethrough')
>> Eg,
>> (qemu)set_cache ide0-hd0 none
>> -> Changes cache setting for ide0-hd0 to 'none'
>
> Not sure if adding this interface is a good idea. I see that you only
> add it for HMP, and we may consider that, but it's definitely not
> suitable for QMP.
>
> One reason is that none/writethrough/writeback/unsafe isn't really what
> we want to use long term. We want to separate advertising a write cache
> (which is guest visible) from things like whether to use O_DIRECT or not.
>
> In the past, Christoph mentioned that he had patches to make these
> separate and even let the guest change the "write cache enabled" flag,
> which would probably solve most of the use cases of this patch.

Toggling host page cache at runtime is useful too because it saves
having to restart VMs.  I agree that the guest should control the
emulated drive cache at runtime and we probably don't want to allow
toggling that from the host - it could be dangerous :).

Stefan

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
> On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 28.02.2011 12:49, schrieb Prerna Saxena:
>>> The following patchset introduces monitor commands:
>>>
>>> 1. set_cache DEVICE CACHE-SETTING
>>> Change cache settings for block device, DEVICE, through the monitor.
>>> (Available options : 'none', 'writeback', 'writethrough')
>>> Eg,
>>> (qemu)set_cache ide0-hd0 none
>>> -> Changes cache setting for ide0-hd0 to 'none'
>>
>> Not sure if adding this interface is a good idea. I see that you only
>> add it for HMP, and we may consider that, but it's definitely not
>> suitable for QMP.
>>
>> One reason is that none/writethrough/writeback/unsafe isn't really what
>> we want to use long term. We want to separate advertising a write cache
>> (which is guest visible) from things like whether to use O_DIRECT or not.
>>
>> In the past, Christoph mentioned that he had patches to make these
>> separate and even let the guest change the "write cache enabled" flag,
>> which would probably solve most of the use cases of this patch.
> 
> Toggling host page cache at runtime is useful too because it saves
> having to restart VMs.  

Not sure why I wanted to change that during runtime, but agreed,
allowing to change parameters using the monitor is generally a good thing.

However, I'm not sure if a command for changing the cache mode is the
right solution, or if it should be something like a command to change
block device options. (For example, what about toggling read-only or
snapshot mode?)

> I agree that the guest should control the
> emulated drive cache at runtime and we probably don't want to allow
> toggling that from the host - it could be dangerous :).

Good point. That's a NACK for this patch as long as we haven't separated
WCE from the host cache setting.

Kevin

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Stefan Hajnoczi]

On Mon, Feb 28, 2011 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
>> On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 28.02.2011 12:49, schrieb Prerna Saxena:
>>>> The following patchset introduces monitor commands:
>>>>
>>>> 1. set_cache DEVICE CACHE-SETTING
>>>> Change cache settings for block device, DEVICE, through the monitor.
>>>> (Available options : 'none', 'writeback', 'writethrough')
>>>> Eg,
>>>> (qemu)set_cache ide0-hd0 none
>>>> -> Changes cache setting for ide0-hd0 to 'none'
>>>
>>> Not sure if adding this interface is a good idea. I see that you only
>>> add it for HMP, and we may consider that, but it's definitely not
>>> suitable for QMP.
>>>
>>> One reason is that none/writethrough/writeback/unsafe isn't really what
>>> we want to use long term. We want to separate advertising a write cache
>>> (which is guest visible) from things like whether to use O_DIRECT or not.
>>>
>>> In the past, Christoph mentioned that he had patches to make these
>>> separate and even let the guest change the "write cache enabled" flag,
>>> which would probably solve most of the use cases of this patch.
>>
>> Toggling host page cache at runtime is useful too because it saves
>> having to restart VMs.
>
> Not sure why I wanted to change that during runtime, but agreed,
> allowing to change parameters using the monitor is generally a good thing.
>
> However, I'm not sure if a command for changing the cache mode is the
> right solution, or if it should be something like a command to change
> block device options. (For example, what about toggling read-only or
> snapshot mode?)

Yes, I think you're right.  We should aim for a general interface
rather than having to add many more specific interfaces in the future.

CQ: Do you see a relation to the "update" interface you added to
adjust drive options at runtime for FVD?

Stefan

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 01.03.2011 10:55, schrieb Stefan Hajnoczi:
> On Mon, Feb 28, 2011 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
>>> On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>> Am 28.02.2011 12:49, schrieb Prerna Saxena:
>>>>> The following patchset introduces monitor commands:
>>>>>
>>>>> 1. set_cache DEVICE CACHE-SETTING
>>>>> Change cache settings for block device, DEVICE, through the monitor.
>>>>> (Available options : 'none', 'writeback', 'writethrough')
>>>>> Eg,
>>>>> (qemu)set_cache ide0-hd0 none
>>>>> -> Changes cache setting for ide0-hd0 to 'none'
>>>>
>>>> Not sure if adding this interface is a good idea. I see that you only
>>>> add it for HMP, and we may consider that, but it's definitely not
>>>> suitable for QMP.
>>>>
>>>> One reason is that none/writethrough/writeback/unsafe isn't really what
>>>> we want to use long term. We want to separate advertising a write cache
>>>> (which is guest visible) from things like whether to use O_DIRECT or not.
>>>>
>>>> In the past, Christoph mentioned that he had patches to make these
>>>> separate and even let the guest change the "write cache enabled" flag,
>>>> which would probably solve most of the use cases of this patch.
>>>
>>> Toggling host page cache at runtime is useful too because it saves
>>> having to restart VMs.
>>
>> Not sure why I wanted to change that during runtime, but agreed,
>> allowing to change parameters using the monitor is generally a good thing.
>>
>> However, I'm not sure if a command for changing the cache mode is the
>> right solution, or if it should be something like a command to change
>> block device options. (For example, what about toggling read-only or
>> snapshot mode?)
> 
> Yes, I think you're right.  We should aim for a general interface
> rather than having to add many more specific interfaces in the future.
> 
> CQ: Do you see a relation to the "update" interface you added to
> adjust drive options at runtime for FVD?

On one hand it's a different set of options today. IIUC, qemu-img update
refers to persistent per-image options like backing file, encryption,
etc. This monitor command refers to temporary command line options like
cache, snapshot mode etc.

On the other hand, we've had discussions before about storing a
copy-on-read flag in images which makes sense as a command line option
as well. The same may apply to things like the read-only flags.

So maybe these two sets of flags aren't that distinct from each other.

Kevin

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Christoph Hellwig]

The only way to change the cache settings is from the guest.  Without
that we're guranteed to lose data when going from WCE=0 to WCE=1.

I have patches to do that, and to allow changing O_DIRECT via a monitor
command, but to toggle O_SYNC via fcntl I first need to get a kernel
patch in as that's currently not allowed to be changed at runtime.

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Stefan Hajnoczi]

On Tue, Mar 01, 2011 at 01:42:54PM +0100, Christoph Hellwig wrote:
> I have patches to do that, and to allow changing O_DIRECT via a monitor
> command, but to toggle O_SYNC via fcntl I first need to get a kernel
> patch in as that's currently not allowed to be changed at runtime.

Great it sounds like you have already implemented the two cases (guest wce and
host O_DIRECT) that we're talking about.

Stefan

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Christoph Hellwig]

On Tue, Mar 01, 2011 at 12:48:34PM +0000, Stefan Hajnoczi wrote:
> On Tue, Mar 01, 2011 at 01:42:54PM +0100, Christoph Hellwig wrote:
> > I have patches to do that, and to allow changing O_DIRECT via a monitor
> > command, but to toggle O_SYNC via fcntl I first need to get a kernel
> > patch in as that's currently not allowed to be changed at runtime.
> 
> Great it sounds like you have already implemented the two cases (guest wce and
> host O_DIRECT) that we're talking about.

At least in theory.  And for Linux I can add setting/clearing of O_SYNC
via fcntl easily, but what do we do for other hosts?  I'm not sure
closing/reopening the backing file is easily feasible.

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 01.03.2011 13:42, schrieb Christoph Hellwig:
> 
> The only way to change the cache settings is from the guest.  Without
> that we're guranteed to lose data when going from WCE=0 to WCE=1.
> 
> I have patches to do that, and to allow changing O_DIRECT via a monitor
> command, but to toggle O_SYNC via fcntl I first need to get a kernel
> patch in as that's currently not allowed to be changed at runtime.

We can re-open the file for now, and we need an implementation for older
kernels/other host OSes anyway, so I don't think we have to wait for the
kernel patch to be accepted.

Kevin

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

On Feb 28, 2011 10:48 AM, "Kevin Wolf" <kwolf@redhat.com> wrote:
>
> Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
> > On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> >> Am 28.02.2011 12:49, schrieb Prerna Saxena:
> >>> The following patchset introduces monitor commands:
> >>>
> >>> 1. set_cache DEVICE CACHE-SETTING
> >>> Change cache settings for block device, DEVICE, through the monitor.
> >>> (Available options : 'none', 'writeback', 'writethrough')
> >>> Eg,
> >>> (qemu)set_cache ide0-hd0 none
> >>> -> Changes cache setting for ide0-hd0 to 'none'
> >>
> >> Not sure if adding this interface is a good idea. I see that you only
> >> add it for HMP, and we may consider that, but it's definitely not
> >> suitable for QMP.
> >>
> >> One reason is that none/writethrough/writeback/unsafe isn't really what
> >> we want to use long term. We want to separate advertising a write cache
> >> (which is guest visible) from things like whether to use O_DIRECT or
not.
> >>
> >> In the past, Christoph mentioned that he had patches to make these
> >> separate and even let the guest change the "write cache enabled" flag,
> >> which would probably solve most of the use cases of this patch.
> >
> > Toggling host page cache at runtime is useful too because it saves
> > having to restart VMs.
>
> Not sure why I wanted to change that during runtime, but agreed,
> allowing to change parameters using the monitor is generally a good thing.
>
> However, I'm not sure if a command for changing the cache mode is the
> right solution, or if it should be something like a command to change
> block device options. (For example, what about toggling read-only or
> snapshot mode?)

Certainly good questions, but let me suggest not taking an HMP command and
not a QMP commans because of interface concerns.

My goal for 0.15 is to convert HMP to be implemented in terms of QMP.  To do
that, a bunch of new QMP commands are needed.  They all won't be perfect but
i'd rather support a bad QMP command forever than to continue to/ have
people rely on HMP.

Regards,

Anthony Liguori

> > I agree that the guest should control the
> > emulated drive cache at runtime and we probably don't want to allow
> > toggling that from the host - it could be dangerous :).
>
> Good point. That's a NACK for this patch as long as we haven't separated
> WCE from the host cache setting.
>
> Kevin
>

[-- Attachment #2: Type: text/html, Size: 3153 bytes --]

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 01.03.2011 14:03, schrieb Anthony Liguori:
> 
> On Feb 28, 2011 10:48 AM, "Kevin Wolf" <kwolf@redhat.com
> <mailto:kwolf@redhat.com>> wrote:
>>
>> Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
>> > On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com
> <mailto:kwolf@redhat.com>> wrote:
>> >> Am 28.02.2011 12:49, schrieb Prerna Saxena:
>> >>> The following patchset introduces monitor commands:
>> >>>
>> >>> 1. set_cache DEVICE CACHE-SETTING
>> >>> Change cache settings for block device, DEVICE, through the monitor.
>> >>> (Available options : 'none', 'writeback', 'writethrough')
>> >>> Eg,
>> >>> (qemu)set_cache ide0-hd0 none
>> >>> -> Changes cache setting for ide0-hd0 to 'none'
>> >>
>> >> Not sure if adding this interface is a good idea. I see that you only
>> >> add it for HMP, and we may consider that, but it's definitely not
>> >> suitable for QMP.
>> >>
>> >> One reason is that none/writethrough/writeback/unsafe isn't really what
>> >> we want to use long term. We want to separate advertising a write cache
>> >> (which is guest visible) from things like whether to use O_DIRECT
> or not.
>> >>
>> >> In the past, Christoph mentioned that he had patches to make these
>> >> separate and even let the guest change the "write cache enabled" flag,
>> >> which would probably solve most of the use cases of this patch.
>> >
>> > Toggling host page cache at runtime is useful too because it saves
>> > having to restart VMs.
>>
>> Not sure why I wanted to change that during runtime, but agreed,
>> allowing to change parameters using the monitor is generally a good thing.
>>
>> However, I'm not sure if a command for changing the cache mode is the
>> right solution, or if it should be something like a command to change
>> block device options. (For example, what about toggling read-only or
>> snapshot mode?)
> 
> Certainly good questions, but let me suggest not taking an HMP command
> and not a QMP commans because of interface concerns.
> 
> My goal for 0.15 is to convert HMP to be implemented in terms of QMP. 
> To do that, a bunch of new QMP commands are needed.  They all won't be
> perfect but i'd rather support a bad QMP command forever than to
> continue to/ have people rely on HMP.

Okay, makes sense. So we should reject patches that add new HMP commands
without adding a QMP counterpart.

>> > I agree that the guest should control the
>> > emulated drive cache at runtime and we probably don't want to allow
>> > toggling that from the host - it could be dangerous :).
>>
>> Good point. That's a NACK for this patch as long as we haven't separated
>> WCE from the host cache setting.

Doesn't make a difference for this one, though, because it's NACKed anyway.

Kevin

PS: Anthony, is there a specific reason why you started sending HTML emails?

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Chunqiang Tang]

> Am 01.03.2011 10:55, schrieb Stefan Hajnoczi:
> > On Mon, Feb 28, 2011 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> >> Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
> >>> On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <kwolf@redhat.com> 
wrote:
> >>>> Am 28.02.2011 12:49, schrieb Prerna Saxena:
> >>>>> The following patchset introduces monitor commands:
> >>>>>
> >>>>> 1. set_cache DEVICE CACHE-SETTING
> >>>>> Change cache settings for block device, DEVICE, through the 
monitor.
> >>>>> (Available options : 'none', 'writeback', 'writethrough')
> >>>>> Eg,
> >>>>> (qemu)set_cache ide0-hd0 none
> >>>>> -> Changes cache setting for ide0-hd0 to 'none'
> >>>>
> >>>> Not sure if adding this interface is a good idea. I see that you 
only
> >>>> add it for HMP, and we may consider that, but it's definitely not
> >>>> suitable for QMP.
> >>>>
> >>>> One reason is that none/writethrough/writeback/unsafe isn't really 
what
> >>>> we want to use long term. We want to separate advertising a write 
cache
> >>>> (which is guest visible) from things like whether to use O_DIRECT 
or not.
> >>>>
> >>>> In the past, Christoph mentioned that he had patches to make these
> >>>> separate and even let the guest change the "write cache enabled" 
flag,
> >>>> which would probably solve most of the use cases of this patch.
> >>>
> >>> Toggling host page cache at runtime is useful too because it saves
> >>> having to restart VMs.
> >>
> >> Not sure why I wanted to change that during runtime, but agreed,
> >> allowing to change parameters using the monitor is generally a good 
thing.
> >>
> >> However, I'm not sure if a command for changing the cache mode is the
> >> right solution, or if it should be something like a command to change
> >> block device options. (For example, what about toggling read-only or
> >> snapshot mode?)
> > 
> > Yes, I think you're right.  We should aim for a general interface
> > rather than having to add many more specific interfaces in the future.
> > 
> > CQ: Do you see a relation to the "update" interface you added to
> > adjust drive options at runtime for FVD?
> 
> On one hand it's a different set of options today. IIUC, qemu-img update
> refers to persistent per-image options like backing file, encryption,
> etc. This monitor command refers to temporary command line options like
> cache, snapshot mode etc.
> 
> On the other hand, we've had discussions before about storing a
> copy-on-read flag in images which makes sense as a command line option
> as well. The same may apply to things like the read-only flags.
> 
> So maybe these two sets of flags aren't that distinct from each other.
> 
> Kevin

Kevin is right. The 'qemu-img update' patch I posted recently is a general 
interface for manipulating persistent per-image options. Its 
implementation certainly differs from a one-time runtime monitor command. 

I can imagine two sets of flags: 1) those should only be updated in the 
persistent image (e.g., backing file) and its effect must be reflected at 
the time of opening an image, and 2) those flags that can be updated both 
in the persistent image and at runtime. For 2), the flag value 
persistently stored in the image is taken as the default value when an 
image is opened, and the flag value possibly can be further modified by a 
runtime monitor command. It seems that copy_on_read and prefetching fall 
in to category 2), with 'qemu-img update' handling persistent changes in 
image and Stefan H.'s monitor commands in QED handling runtime changes. If 
we want a general monitor interface for runtime flag changes, the 
interface may be similar to 'qemu-img update' but the implementation would 
be very different.

Specifically regarding the set_cache monitor command, it  allows 
flexibility, but probably will be rarely used. It also poses additional 
requires on the image format drivers, e.g., to flush metadata cache on the 
change of cache mode and to re-initialize internal data structure 
properly. 

Regards,
ChunQiang (CQ) Tang
Homepage: http://www.research.ibm.com/people/c/ctang

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Anthony Liguori]

On 03/01/2011 08:22 AM, Kevin Wolf wrote:
>> Certainly good questions, but let me suggest not taking an HMP command
>> and not a QMP commans because of interface concerns.
>>
>> My goal for 0.15 is to convert HMP to be implemented in terms of QMP.
>> To do that, a bunch of new QMP commands are needed.  They all won't be
>> perfect but i'd rather support a bad QMP command forever than to
>> continue to/ have people rely on HMP.
>>      
> Okay, makes sense. So we should reject patches that add new HMP commands
> without adding a QMP counterpart.
>    

Definitely.  We essentially are supporting HMP today just as much as QMP 
but HMP is much harder to support (no standard way to interpret 
input/output/errors).

>>>> I agree that the guest should control the
>>>> emulated drive cache at runtime and we probably don't want to allow
>>>> toggling that from the host - it could be dangerous :).
>>>>          
>>> Good point. That's a NACK for this patch as long as we haven't separated
>>> WCE from the host cache setting.
>>>        
> Doesn't make a difference for this one, though, because it's NACKed anyway.
>
> Kevin
>
> PS: Anthony, is there a specific reason why you started sending HTML emails?
>    

Because I was stuck using my phone because of bad hotel wireless :-/

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Anthony Liguori]

On 03/01/2011 07:50 AM, Christoph Hellwig wrote:
> On Tue, Mar 01, 2011 at 12:48:34PM +0000, Stefan Hajnoczi wrote:
>    
>> On Tue, Mar 01, 2011 at 01:42:54PM +0100, Christoph Hellwig wrote:
>>      
>>> I have patches to do that, and to allow changing O_DIRECT via a monitor
>>> command, but to toggle O_SYNC via fcntl I first need to get a kernel
>>> patch in as that's currently not allowed to be changed at runtime.
>>>        
>> Great it sounds like you have already implemented the two cases (guest wce and
>> host O_DIRECT) that we're talking about.
>>      
> At least in theory.  And for Linux I can add setting/clearing of O_SYNC
> via fcntl easily, but what do we do for other hosts?

To start with, we can just fail the command at the QMP level.

>    I'm not sure
> closing/reopening the backing file is easily feasible.
>    

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC][PATCH 0/2] Allow cache settings for block devices to be changed at runtime.

[Kevin Wolf]

Am 01.03.2011 20:13, schrieb Anthony Liguori:
> On 03/01/2011 07:50 AM, Christoph Hellwig wrote:
>> On Tue, Mar 01, 2011 at 12:48:34PM +0000, Stefan Hajnoczi wrote:
>>    
>>> On Tue, Mar 01, 2011 at 01:42:54PM +0100, Christoph Hellwig wrote:
>>>      
>>>> I have patches to do that, and to allow changing O_DIRECT via a monitor
>>>> command, but to toggle O_SYNC via fcntl I first need to get a kernel
>>>> patch in as that's currently not allowed to be changed at runtime.
>>>>        
>>> Great it sounds like you have already implemented the two cases (guest wce and
>>> host O_DIRECT) that we're talking about.
>>>      
>> At least in theory.  And for Linux I can add setting/clearing of O_SYNC
>> via fcntl easily, but what do we do for other hosts?
> 
> To start with, we can just fail the command at the QMP level.

O_SYNC isn't toggled from QMP but from the guest.

Kevin