| Message ID | 20111111064748.15024.14207.sendpatchset@skannery.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote: > + { > + .name = "block_set_hostcache", > + .args_type = "device:B,option:b", > + .params = "device on|off", > + .help = "Change setting of host pagecache", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_block_set_hostcache, > + }, > +STEXI > +@item block_set_hostcache @var{device} @var{setting} @var{option} > +@findex block_set_hostcache > +Change host pagecache setting of a block device while guest is running. > +ETEXI > + > > { > .name = "eject", > Index: qemu/qmp-commands.hx > =================================================================== > --- qemu.orig/qmp-commands.hx > +++ qemu/qmp-commands.hx > @@ -716,7 +716,34 @@ Example: > > EQMP > > + > { > + .name = "block_set_hostcache", > + .args_type = "device:B,option:b", > + .params = "device on|off", > + .help = "Change setting of host pagecache (true|false)", It would be more consistent to use "on|off" instead of "true|false". Or eliminate it entirely by saying "Enable or disable host pagecache usage". Stefan
On 11/17/2011 12:04 AM, Stefan Hajnoczi wrote: > On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery > <supriyak@linux.vnet.ibm.com> wrote: >> + { >> + .name = "block_set_hostcache", >> + .args_type = "device:B,option:b", >> + .params = "device on|off", >> + .help = "Change setting of host pagecache", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_block_set_hostcache, >> + }, >> +STEXI >> +@item block_set_hostcache @var{device} @var{setting} > > @var{option} Will send updated patch > >> +@findex block_set_hostcache >> +Change host pagecache setting of a block device while guest is running. >> +ETEXI >> + >> >> { >> .name = "eject", >> Index: qemu/qmp-commands.hx >> =================================================================== >> --- qemu.orig/qmp-commands.hx >> +++ qemu/qmp-commands.hx >> @@ -716,7 +716,34 @@ Example: >> >> EQMP >> >> + >> { >> + .name = "block_set_hostcache", >> + .args_type = "device:B,option:b", >> + .params = "device on|off", >> + .help = "Change setting of host pagecache (true|false)", > > It would be more consistent to use "on|off" instead of "true|false". > Or eliminate it entirely by saying "Enable or disable host pagecache > usage". > > Stefan > Followed similar way how set_link is done. Specified 'true/false' in brackets as 'on' or 'off' are not accepted as bool parameter in qmp prompt.
On Fri, 11 Nov 2011 12:17:48 +0530 Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote: > New command "block_set_hostcache" added for dynamically changing > host pagecache setting of a block device. > > Usage: > block_set_hostcache <device> <option> > <device> = block device > <option> = on/off > > Example: > (qemu) block_set_hostcache ide0-hd0 off > > Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com> > > --- > block.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block.h | 2 ++ > blockdev.c | 26 ++++++++++++++++++++++++++ > blockdev.h | 2 ++ > hmp-commands.hx | 14 ++++++++++++++ > qmp-commands.hx | 27 +++++++++++++++++++++++++++ > 6 files changed, 125 insertions(+) > > Index: qemu/block.c > =================================================================== > --- qemu.orig/block.c > +++ qemu/block.c > @@ -696,6 +696,35 @@ unlink_and_fail: > return ret; > } > > +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags) > +{ > + BlockDriver *drv = bs->drv; > + int ret = 0, open_flags; > + > + /* Quiesce IO for the given block device */ > + qemu_aio_flush(); > + ret = bdrv_flush(bs); > + if (ret != 0) { > + qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); > + return ret; > + } > + open_flags = bs->open_flags; > + bdrv_close(bs); > + > + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); > + if (ret < 0) { > + /* Reopen failed. Try to open with original flags */ > + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); > + ret = bdrv_open(bs, bs->filename, open_flags, drv); > + if (ret < 0) { > + /* Reopen failed with orig and modified flags */ > + abort(); > + } > + } > + > + return ret; > +} In this thread: http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html Juan uses a similar method (well, at least it looks similar to me) to fix a problem with migration. However, it was said that that method can cause problems with -snapshot and encrypted images. Won't we have the same sort of problems with this series? > + > void bdrv_close(BlockDriverState *bs) > { > if (bs->drv) { > @@ -733,6 +762,32 @@ void bdrv_close_all(void) > } > } > > +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache) > +{ > + int bdrv_flags = bs->open_flags; > + > + /* set hostcache flags (without changing WCE/flush bits) */ > + if (enable_host_cache) { > + bdrv_flags &= ~BDRV_O_NOCACHE; > + } else { > + bdrv_flags |= BDRV_O_NOCACHE; > + } > + > + /* If no change in flags, no need to reopen */ > + if (bdrv_flags == bs->open_flags) { > + return 0; > + } > + > + if (bdrv_is_inserted(bs)) { > + /* Reopen file with changed set of flags */ > + return bdrv_reopen(bs, bdrv_flags); > + } else { > + /* Save hostcache change for future use */ > + bs->open_flags = bdrv_flags; > + return 0; > + } I'm wondering if the simplest (and best) thing to do here is to fail if the drive is not inserted. Just wondering, not exactly asking you to change it. But it should at least be clearly documented if you keep it. > +} > + > /* make a BlockDriverState anonymous by removing from bdrv_state list. > Also, NULL terminate the device_name to prevent double remove */ > void bdrv_make_anon(BlockDriverState *bs) > Index: qemu/block.h > =================================================================== > --- qemu.orig/block.h > +++ qemu/block.h > @@ -104,6 +104,7 @@ int bdrv_parse_cache_flags(const char *m > int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags); > int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > BlockDriver *drv); > +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags); > void bdrv_close(BlockDriverState *bs); > int bdrv_attach_dev(BlockDriverState *bs, void *dev); > void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev); > @@ -138,6 +139,7 @@ void bdrv_commit_all(void); > int bdrv_change_backing_file(BlockDriverState *bs, > const char *backing_file, const char *backing_fmt); > void bdrv_register(BlockDriver *bdrv); > +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache); > > > typedef struct BdrvCheckResult { > Index: qemu/blockdev.c > =================================================================== > --- qemu.orig/blockdev.c > +++ qemu/blockdev.c > @@ -776,3 +776,29 @@ int do_block_resize(Monitor *mon, const > > return 0; > } > + > + > +/* > + * Change host page cache setting while guest is running. > +*/ > +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, > + QObject **ret_data) > +{ > + BlockDriverState *bs = NULL; > + int enable; > + const char *device; > + > + /* Validate device */ > + device = qdict_get_str(qdict, "device"); > + bs = bdrv_find(device); > + if (!bs) { > + qerror_report(QERR_DEVICE_NOT_FOUND, device); > + return -1; > + } > + > + /* Read hostcache setting */ > + enable = qdict_get_bool(qdict, "option"); > + return bdrv_change_hostcache(bs, enable); > + > +} This is not using the QAPI. > + > Index: qemu/blockdev.h > =================================================================== > --- qemu.orig/blockdev.h > +++ qemu/blockdev.h > @@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const > 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_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data); > +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, > + QObject **ret_data); > > #endif > Index: qemu/hmp-commands.hx > =================================================================== > --- qemu.orig/hmp-commands.hx > +++ qemu/hmp-commands.hx > @@ -70,6 +70,20 @@ but should be used with extreme caution. > resizes image files, it can not resize block devices like LVM volumes. > ETEXI > > + { > + .name = "block_set_hostcache", > + .args_type = "device:B,option:b", "option" is not good enough. I'd call it "enable" or "disable". > + .params = "device on|off", > + .help = "Change setting of host pagecache", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_block_set_hostcache, > + }, > +STEXI > +@item block_set_hostcache @var{device} @var{setting} > +@findex block_set_hostcache > +Change host pagecache setting of a block device while guest is running. > +ETEXI > + > > { > .name = "eject", > Index: qemu/qmp-commands.hx > =================================================================== > --- qemu.orig/qmp-commands.hx > +++ qemu/qmp-commands.hx > @@ -716,7 +716,34 @@ Example: > > EQMP > > + > { > + .name = "block_set_hostcache", > + .args_type = "device:B,option:b", > + .params = "device on|off", > + .help = "Change setting of host pagecache (true|false)", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_block_set_hostcache, > + }, > + > +SQMP > +block_set_hostcache > +------------------- > + > +Change host pagecache setting of a block device (on|off) > + > +Arguments: > + > +- "device": the device's ID, must be unique (json-string) > +- "option": hostcache setting (json-bool) > + > +Example: > +-> { "execute": "block_set_hostcache", "arguments": { "device": "ide0-hd0", "option": false } } > +<- { "return": {} } > + > +EQMP > + > + { > .name = "balloon", > .args_type = "value:M", > .params = "target", >
On Thu, 17 Nov 2011 11:15:06 +0530 Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote: > On 11/17/2011 12:04 AM, Stefan Hajnoczi wrote: > > On Fri, Nov 11, 2011 at 6:47 AM, Supriya Kannery > > <supriyak@linux.vnet.ibm.com> wrote: > >> + { > >> + .name = "block_set_hostcache", > >> + .args_type = "device:B,option:b", > >> + .params = "device on|off", > >> + .help = "Change setting of host pagecache", > >> + .user_print = monitor_user_noop, > >> + .mhandler.cmd_new = do_block_set_hostcache, > >> + }, > >> +STEXI > >> +@item block_set_hostcache @var{device} @var{setting} > > > > @var{option} > > Will send updated patch > > > > >> +@findex block_set_hostcache > >> +Change host pagecache setting of a block device while guest is running. > >> +ETEXI > >> + > >> > >> { > >> .name = "eject", > >> Index: qemu/qmp-commands.hx > >> =================================================================== > >> --- qemu.orig/qmp-commands.hx > >> +++ qemu/qmp-commands.hx > >> @@ -716,7 +716,34 @@ Example: > >> > >> EQMP > >> > >> + > >> { > >> + .name = "block_set_hostcache", > >> + .args_type = "device:B,option:b", > >> + .params = "device on|off", > >> + .help = "Change setting of host pagecache (true|false)", > > > > It would be more consistent to use "on|off" instead of "true|false". > > Or eliminate it entirely by saying "Enable or disable host pagecache > > usage". > > > > Stefan > > > > Followed similar way how set_link is done. > Specified 'true/false' in brackets as 'on' or 'off' are not accepted as > bool parameter in qmp prompt. on/off is used in HMP, while true/false is used in QMP.
Luiz Capitulino wrote: > On Fri, 11 Nov 2011 12:17:48 +0530 > Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote: > > >> + >> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); >> + if (ret < 0) { >> + /* Reopen failed. Try to open with original flags */ >> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >> + ret = bdrv_open(bs, bs->filename, open_flags, drv); >> + if (ret < 0) { >> + /* Reopen failed with orig and modified flags */ >> + abort(); >> + } >> + } >> + >> + return ret; >> +} >> > > In this thread: > > http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html > > Juan uses a similar method (well, at least it looks similar to me) > to fix a problem with migration. However, it was said that that method > can cause problems with -snapshot and encrypted images. > > Won't we have the same sort of problems with this series? > > > Thanks! for this link. Yes, this series also need to look at such issues. Will go through the discussions and see what needs to be looked at. >> + if (bdrv_is_inserted(bs)) { >> + /* Reopen file with changed set of flags */ >> + return bdrv_reopen(bs, bdrv_flags); >> + } else { >> + /* Save hostcache change for future use */ >> + bs->open_flags = bdrv_flags; >> + return 0; >> + } >> > > I'm wondering if the simplest (and best) thing to do here is to fail > if the drive is not inserted. Just wondering, not exactly asking you > to change it. But it should at least be clearly documented if you keep it. > > I initially started the patchset with returning error message on detecting uninserted drive. But later thought that, user should be given the flexibility of changing hostcache setting irrespective of whether drive is full or not. To what I understand, hostcache setting is independent of what user puts in to the drive. Need to make these saved status flags used when opening a newly inserted device. >> + /* Read hostcache setting */ >> + enable = qdict_get_bool(qdict, "option"); >> + return bdrv_change_hostcache(bs, enable); >> + >> +} >> > > This is not using the QAPI. > will change to use QAPI. > >> + >> Index: qemu/blockdev.h >> =================================================================== >> --- qemu.orig/blockdev.h >> +++ qemu/blockdev.h >> @@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const >> 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_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data); >> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, >> + QObject **ret_data); >> >> #endif >> Index: qemu/hmp-commands.hx >> =================================================================== >> --- qemu.orig/hmp-commands.hx >> +++ qemu/hmp-commands.hx >> @@ -70,6 +70,20 @@ but should be used with extreme caution. >> resizes image files, it can not resize block devices like LVM volumes. >> ETEXI >> >> + { >> + .name = "block_set_hostcache", >> + .args_type = "device:B,option:b", >> > > "option" is not good enough. I'd call it "enable" or "disable". > > ok
Index: qemu/block.c =================================================================== --- qemu.orig/block.c +++ qemu/block.c @@ -696,6 +696,35 @@ unlink_and_fail: return ret; } +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags) +{ + BlockDriver *drv = bs->drv; + int ret = 0, open_flags; + + /* Quiesce IO for the given block device */ + qemu_aio_flush(); + ret = bdrv_flush(bs); + if (ret != 0) { + qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); + return ret; + } + open_flags = bs->open_flags; + bdrv_close(bs); + + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); + if (ret < 0) { + /* Reopen failed. Try to open with original flags */ + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); + ret = bdrv_open(bs, bs->filename, open_flags, drv); + if (ret < 0) { + /* Reopen failed with orig and modified flags */ + abort(); + } + } + + return ret; +} + void bdrv_close(BlockDriverState *bs) { if (bs->drv) { @@ -733,6 +762,32 @@ void bdrv_close_all(void) } } +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache) +{ + int bdrv_flags = bs->open_flags; + + /* set hostcache flags (without changing WCE/flush bits) */ + if (enable_host_cache) { + bdrv_flags &= ~BDRV_O_NOCACHE; + } else { + bdrv_flags |= BDRV_O_NOCACHE; + } + + /* If no change in flags, no need to reopen */ + if (bdrv_flags == bs->open_flags) { + return 0; + } + + if (bdrv_is_inserted(bs)) { + /* Reopen file with changed set of flags */ + return bdrv_reopen(bs, bdrv_flags); + } else { + /* Save hostcache change for future use */ + bs->open_flags = bdrv_flags; + return 0; + } +} + /* make a BlockDriverState anonymous by removing from bdrv_state list. Also, NULL terminate the device_name to prevent double remove */ void bdrv_make_anon(BlockDriverState *bs) Index: qemu/block.h =================================================================== --- qemu.orig/block.h +++ qemu/block.h @@ -104,6 +104,7 @@ int bdrv_parse_cache_flags(const char *m int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags); int bdrv_open(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv); +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags); void bdrv_close(BlockDriverState *bs); int bdrv_attach_dev(BlockDriverState *bs, void *dev); void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev); @@ -138,6 +139,7 @@ void bdrv_commit_all(void); int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, const char *backing_fmt); void bdrv_register(BlockDriver *bdrv); +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache); typedef struct BdrvCheckResult { Index: qemu/blockdev.c =================================================================== --- qemu.orig/blockdev.c +++ qemu/blockdev.c @@ -776,3 +776,29 @@ int do_block_resize(Monitor *mon, const return 0; } + + +/* + * Change host page cache setting while guest is running. +*/ +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, + QObject **ret_data) +{ + BlockDriverState *bs = NULL; + int enable; + const char *device; + + /* Validate device */ + device = qdict_get_str(qdict, "device"); + bs = bdrv_find(device); + if (!bs) { + qerror_report(QERR_DEVICE_NOT_FOUND, device); + return -1; + } + + /* Read hostcache setting */ + enable = qdict_get_bool(qdict, "option"); + return bdrv_change_hostcache(bs, enable); + +} + Index: qemu/blockdev.h =================================================================== --- qemu.orig/blockdev.h +++ qemu/blockdev.h @@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const 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_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data); +int do_block_set_hostcache(Monitor *mon, const QDict *qdict, + QObject **ret_data); #endif Index: qemu/hmp-commands.hx =================================================================== --- qemu.orig/hmp-commands.hx +++ qemu/hmp-commands.hx @@ -70,6 +70,20 @@ but should be used with extreme caution. resizes image files, it can not resize block devices like LVM volumes. ETEXI + { + .name = "block_set_hostcache", + .args_type = "device:B,option:b", + .params = "device on|off", + .help = "Change setting of host pagecache", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_block_set_hostcache, + }, +STEXI +@item block_set_hostcache @var{device} @var{setting} +@findex block_set_hostcache +Change host pagecache setting of a block device while guest is running. +ETEXI + { .name = "eject", Index: qemu/qmp-commands.hx =================================================================== --- qemu.orig/qmp-commands.hx +++ qemu/qmp-commands.hx @@ -716,7 +716,34 @@ Example: EQMP + { + .name = "block_set_hostcache", + .args_type = "device:B,option:b", + .params = "device on|off", + .help = "Change setting of host pagecache (true|false)", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_block_set_hostcache, + }, + +SQMP +block_set_hostcache +------------------- + +Change host pagecache setting of a block device (on|off) + +Arguments: + +- "device": the device's ID, must be unique (json-string) +- "option": hostcache setting (json-bool) + +Example: +-> { "execute": "block_set_hostcache", "arguments": { "device": "ide0-hd0", "option": false } } +<- { "return": {} } + +EQMP + + { .name = "balloon", .args_type = "value:M", .params = "target",
New command "block_set_hostcache" added for dynamically changing host pagecache setting of a block device. Usage: block_set_hostcache <device> <option> <device> = block device <option> = on/off Example: (qemu) block_set_hostcache ide0-hd0 off Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com> --- block.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 2 ++ blockdev.c | 26 ++++++++++++++++++++++++++ blockdev.h | 2 ++ hmp-commands.hx | 14 ++++++++++++++ qmp-commands.hx | 27 +++++++++++++++++++++++++++ 6 files changed, 125 insertions(+)