Allow cache settings for block devices to be changed at runtime.
cover Allow cache settings for block devices to be changed at runtime.
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
Signature · 5 lines
Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
RFC PATCH 1/2 Add monitor command 'set-cache' to change cache settings for a block device.
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(-)
blockdev.c +76 -0
@@ -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) {
blockdev.h +1 -0
@@ -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
hmp-commands.hx +13 -0
@@ -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",
Signature · 10 lines
1.7.2.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
RFC PATCH 2/2 Extend monitor command 'info block' to display cache settings for block devices.
In reply to 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(-)
block.c +20 -2
@@ -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;
Signature · 10 lines
1.7.2.3 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
Re: [Qemu-devel] [RFC][PATCH 2/2] Extend monitor command 'info block' to display cache settings for block devices.
Am 28.02.2011 13:11, schrieb Prerna Saxena:
Quoted 38 lines
(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(-)block.c +17
-0@@ -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" ));
block.c void bdrv_info(Monitor *mon, QObject **ret_data) · line 1779
Quoted 9 lines
block.c +3
-2@@ -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
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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?
Quoted 2 lines
2. info block
Now extended to display cache settings for available block devices.
Have you checked that libvirt is okay with this?
Kevin
Quoted 14 lines · Kevin Wolf
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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.
Quoted 7 lines
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_DIRECTor 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
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 19 lines · Kevin Wolf
Quoted 18 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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_DIRECTor not.
Quoted 6 lines
Quoted 3 lines
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?)
Quoted 3 lines
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
Quoted 27 lines · Kevin Wolf
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 19 lines · Kevin Wolf
Quoted 18 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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_DIRECTor not.Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
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
Am 01.03.2011 10:55, schrieb Stefan Hajnoczi:
Quoted 36 lines · Kevin Wolf
Quoted 27 lines
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 19 lines · Kevin Wolf
Quoted 18 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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_DIRECTor not.Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
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?)
Quoted 5 lines
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
Quoted 4 lines
Am 01.03.2011 10:55, schrieb Stefan Hajnoczi:
Quoted 3 lines · Kevin Wolf
Quoted 2 lines
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 1 line
On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <[email protected]>
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 4 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE CACHE-SETTING
Change cache settings for block device, DEVICE, through the
monitor.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
(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
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we want to use long term. We want to separate advertising a write
cache
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(which is guest visible) from things like whether to use
O_DIRECT
or not.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
In the past, Christoph mentioned that he had patches to make these separate and even let the guest change the "write cache enabled"
flag,
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 17 lines
Quoted 11 lines
Quoted 5 lines
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.
Quoted 7 lines
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
Quoted 20 lines · Kevin Wolf
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 18 lines · Kevin Wolf
Quoted 17 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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_DIRECTor
not.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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
Quoted 9 lines
Quoted 3 lines
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
Am 01.03.2011 14:03, schrieb Anthony Liguori:
Quoted 41 lines
Quoted 3 lines · On Feb 28, 2011 10:48 AM, "Kevin Wolf" <[email protected]…
Am 28.02.2011 16:35, schrieb Stefan Hajnoczi:
Quoted 1 line
On Mon, Feb 28, 2011 at 3:12 PM, Kevin Wolf <[email protected]
Quoted 10 lines · <mailto:[email protected]>> wrote:
Quoted 10 lines
Quoted 10 lines
Am 28.02.2011 12:49, schrieb Prerna Saxena:
Quoted 8 lines
The following patchset introduces monitor commands:
1.
set_cacheDEVICE 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'Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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_DIRECTor not.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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?)
Quoted 7 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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?
Quoted 11 lines · Kevin Wolf
Quoted 8 lines
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).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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 :).
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
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
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.
Quoted 3 lines · Christoph Hellwig
I have patches to do that, and to allow changing
O_DIRECTvia a monitor command, but to toggleO_SYNCvia 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
Quoted 7 lines · Stefan Hajnoczi
Quoted 3 lines · Christoph Hellwig
I have patches to do that, and to allow changing
O_DIRECTvia a monitor command, but to toggleO_SYNCvia 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.
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Stefan Hajnoczi
Quoted 4 lines · Christoph Hellwig
I have patches to do that, and to allow changing
O_DIRECTvia a monitor command, but to toggleO_SYNCvia fcntl I first need to get a kernel patch in as that's currently not allowed to be changed at runtime.
Quoted 5 lines
Quoted 3 lines
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_SYNCvia fcntl easily, but what do we do for other hosts?
To start with, we can just fail the command at the QMP level.
Quoted 3 lines
I'm not sure
closing/reopening the backing file is easily feasible.
Regards,
Anthony Liguori
Am 01.03.2011 20:13, schrieb Anthony Liguori:
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Stefan Hajnoczi
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines
I have patches to do that, and to allow changing
O_DIRECTvia a monitor command, but to toggleO_SYNCvia fcntl I first need to get a kernel patch in as that's currently not allowed to be changed at runtime.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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_SYNCvia 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
Am 01.03.2011 13:42, schrieb Christoph Hellwig:
Quoted 7 lines
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_DIRECTvia a monitor command, but to toggleO_SYNCvia 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