| Message ID | 20110727113045.25109.54866.sendpatchset@skannery |
|---|---|
| State | New |
| Headers | show |
On 07/27/2011 06:30 AM, Supriya Kannery wrote: > New command "block_set" added for dynamically changing any of the block > device parameters. For now, dynamic setting of hostcache params using this > command is implemented. Other block device parameter changes, can be > integrated in similar lines. > > Signed-off-by: Supriya Kannery<supriyak@in.ibm.com> > > --- > block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ > block.h | 2 + > blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > blockdev.h | 1 > hmp-commands.hx | 14 ++++++++++++ > qemu-config.c | 13 +++++++++++ > qemu-option.c | 25 ++++++++++++++++++++++ > qemu-option.h | 2 + > qmp-commands.hx | 28 +++++++++++++++++++++++++ > 9 files changed, 200 insertions(+) > > Index: qemu/block.c > =================================================================== > --- qemu.orig/block.c > +++ qemu/block.c > @@ -651,6 +651,34 @@ 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(); > + if (bdrv_flush(bs)) { > + 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) { > @@ -691,6 +719,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 > @@ -71,6 +71,7 @@ void bdrv_delete(BlockDriverState *bs); > 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(BlockDriverState *bs, DeviceState *qdev); > void bdrv_detach(BlockDriverState *bs, DeviceState *qdev); > @@ -97,6 +98,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 > @@ -793,3 +793,64 @@ int do_block_resize(Monitor *mon, const > > return 0; > } > + > + > +/* > + * Handle changes to block device settings, like hostcache, > + * while guest is running. > +*/ > +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data) > +{ > + BlockDriverState *bs = NULL; > + QemuOpts *opts; > + int enable; > + const char *device, *driver; > + int ret; > + char usage[50]; > + > + /* Validate device */ > + device = qdict_get_str(qdict, "device"); > + bs = bdrv_find(device); > + if (!bs) { > + qerror_report(QERR_DEVICE_NOT_FOUND, device); > + return -1; > + } > + > + opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict); > + if (opts == NULL) { > + return -1; > + } > + > + /* If input not in "param=value" format, display error */ > + driver = qemu_opt_get(opts, "driver"); > + if (driver != NULL) { > + qerror_report(QERR_INVALID_PARAMETER, driver); > + return -1; > + } > + > + /* Before validating parameters, remove "device" option */ > + ret = qemu_opt_delete(opts, "device"); > + if (ret == 1) { > + strcpy(usage,"block_set device [prop=value][,...]"); > + qerror_report(QERR_INCORRECT_COMMAND_SYNTAX, usage); > + return 0; > + } > + > + /* Validate parameters with "-drive" parameter list */ > + ret = qemu_validate_opts(opts, "drive"); > + if (ret == -1) { > + return -1; > + } > + > + /* Check for 'hostcache' parameter */ > + enable = qemu_opt_get_bool(opts, "hostcache", -1); > + if (enable != -1) { > + return bdrv_change_hostcache(bs, enable); > + } else { > + qerror_report(QERR_INVALID_PARAMETER_VALUE, "hostcache","on/off"); > + } > + > + return 0; > + > +} > + > Index: qemu/blockdev.h > =================================================================== > --- qemu.orig/blockdev.h > +++ qemu/blockdev.h > @@ -65,5 +65,6 @@ 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(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", > + .args_type = "device:B,device:O", > + .params = "device [prop=value][,...]", > + .help = "Change block device parameters [hostcache=on/off]", > + .user_print = monitor_user_noop, > + .mhandler.cmd_new = do_block_set, > + }, > +STEXI > +@item block_set @var{config} > +@findex block_set > +Change block device parameters (eg: hostcache=on/off) while guest is running. > +ETEXI > + block_set_hostcache() please. Multiplexing commands is generally a bad idea. It weakens typing. In the absence of a generic way to set block device properties, implementing properties as generic in the QMP layer seems like a bad idea to me. Regards, Anthony Liguori
27.07.2011 15:30, Supriya Kannery wrote: > New command "block_set" added for dynamically changing any of the block > device parameters. For now, dynamic setting of hostcache params using this > command is implemented. Other block device parameter changes, can be > integrated in similar lines. > > Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> > > --- > block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ > block.h | 2 + > blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > blockdev.h | 1 > hmp-commands.hx | 14 ++++++++++++ > qemu-config.c | 13 +++++++++++ > qemu-option.c | 25 ++++++++++++++++++++++ > qemu-option.h | 2 + > qmp-commands.hx | 28 +++++++++++++++++++++++++ > 9 files changed, 200 insertions(+) > > Index: qemu/block.c > =================================================================== > --- qemu.orig/block.c > +++ qemu/block.c > @@ -651,6 +651,34 @@ 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(); > + if (bdrv_flush(bs)) { > + 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(); > + } Can we please avoid this stuff completely? Just keep the old device open still, until you're sure new one is ok. Or else it will be quite dangerous command in many cases. For example, after -runas/-chroot, or additional selinux settings or whatnot. And in this case, instead of merely returning an error, we'll see abort(). Boom. Thanks, /mjt
On 07/27/2011 08:43 AM, Michael Tokarev wrote: >> Index: qemu/block.c >> =================================================================== >> --- qemu.orig/block.c >> +++ qemu/block.c >> @@ -651,6 +651,34 @@ 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(); >> + if (bdrv_flush(bs)) { >> + 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(); >> + } > > Can we please avoid this stuff completely? Just keep the > old device open still, until you're sure new one is ok. I may be misremembering, but I thought Christoph had mentioned wanting to write a kernel patch to toggle O_DIRECT through fcntl(). This seems like the only way to make this not racy to me. Regards, Anthony Liguori
On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: >> 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", >> + .args_type = "device:B,device:O", >> + .params = "device [prop=value][,...]", >> + .help = "Change block device parameters >> [hostcache=on/off]", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_block_set, >> + }, >> +STEXI >> +@item block_set @var{config} >> +@findex block_set >> +Change block device parameters (eg: hostcache=on/off) while guest is >> running. >> +ETEXI >> + > > block_set_hostcache() please. > > Multiplexing commands is generally a bad idea. It weakens typing. In the > absence of a generic way to set block device properties, implementing > properties as generic in the QMP layer seems like a bad idea to me. The idea behind block_set was to have a unified interface for changing block device parameters at runtime. This prevents us from reinventing new commands from scratch. For example, block I/O throttling is already queued up to add run-time parameters. Without a unified command we have a bulkier QMP/HMP interface, duplicated code, and possibly inconsistencies in syntax between the commands. Isn't the best way to avoid these problems a unified interface? I understand the lack of type safety concern but in this case we already have to manually pull parsed arguments (i.e. cast to specific types and deal with invalid input). To me this is a reason *for* using a unified interface like block_set. Stefan
2011/7/27 Michael Tokarev <mjt@tls.msk.ru>: > 27.07.2011 15:30, Supriya Kannery wrote: >> New command "block_set" added for dynamically changing any of the block >> device parameters. For now, dynamic setting of hostcache params using this >> command is implemented. Other block device parameter changes, can be >> integrated in similar lines. >> >> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> >> >> --- >> block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ >> block.h | 2 + >> blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> blockdev.h | 1 >> hmp-commands.hx | 14 ++++++++++++ >> qemu-config.c | 13 +++++++++++ >> qemu-option.c | 25 ++++++++++++++++++++++ >> qemu-option.h | 2 + >> qmp-commands.hx | 28 +++++++++++++++++++++++++ >> 9 files changed, 200 insertions(+) >> >> Index: qemu/block.c >> =================================================================== >> --- qemu.orig/block.c >> +++ qemu/block.c >> @@ -651,6 +651,34 @@ 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(); >> + if (bdrv_flush(bs)) { >> + 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(); >> + } > > Can we please avoid this stuff completely? Just keep the > old device open still, until you're sure new one is ok. > > Or else it will be quite dangerous command in many cases. > For example, after -runas/-chroot, or additional selinux > settings or whatnot. And in this case, instead of merely > returning an error, we'll see abort(). Boom. Slight complication for image formats that use a dirty bit here. QED has a dirty bit. VMDK also specifies one but we don't implement it today. If the image file is dirty then all its metadata will be scanned for consistency when it is opened. This increases the bdrv_open() time and hence the downtime of the VM. So it is not ideal to open the image file twice, even though there is no consistency problem. I'll think about this some more, there are a couple of solutions like keeping only the file descriptor around, introducing a flush command that makes sure the file is in a clean state, or changing QED to not do this. Stefan
On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: > On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>> 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", >>> + .args_type = "device:B,device:O", >>> + .params = "device [prop=value][,...]", >>> + .help = "Change block device parameters >>> [hostcache=on/off]", >>> + .user_print = monitor_user_noop, >>> + .mhandler.cmd_new = do_block_set, >>> + }, >>> +STEXI >>> +@item block_set @var{config} >>> +@findex block_set >>> +Change block device parameters (eg: hostcache=on/off) while guest is >>> running. >>> +ETEXI >>> + >> >> block_set_hostcache() please. >> >> Multiplexing commands is generally a bad idea. It weakens typing. In the >> absence of a generic way to set block device properties, implementing >> properties as generic in the QMP layer seems like a bad idea to me. > > The idea behind block_set was to have a unified interface for changing > block device parameters at runtime. This prevents us from reinventing > new commands from scratch. For example, block I/O throttling is > already queued up to add run-time parameters. > > Without a unified command we have a bulkier QMP/HMP interface, > duplicated code, and possibly inconsistencies in syntax between the > commands. Isn't the best way to avoid these problems a unified > interface? > > I understand the lack of type safety concern but in this case we > already have to manually pull parsed arguments (i.e. cast to specific > types and deal with invalid input). To me this is a reason *for* > using a unified interface like block_set. Think about it from a client perspective. How do I determine which properties are supported by this version of QEMU? I have no way to identify programmatically what arguments are valid for block_set. OTOH, if you have strong types like block_set_hostcache, query-commands tells me exactly what's supported. Regards, Anthony Liguori > > Stefan >
On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >> >> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >> wrote: >>>> >>>> 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", >>>> + .args_type = "device:B,device:O", >>>> + .params = "device [prop=value][,...]", >>>> + .help = "Change block device parameters >>>> [hostcache=on/off]", >>>> + .user_print = monitor_user_noop, >>>> + .mhandler.cmd_new = do_block_set, >>>> + }, >>>> +STEXI >>>> +@item block_set @var{config} >>>> +@findex block_set >>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>> running. >>>> +ETEXI >>>> + >>> >>> block_set_hostcache() please. >>> >>> Multiplexing commands is generally a bad idea. It weakens typing. In >>> the >>> absence of a generic way to set block device properties, implementing >>> properties as generic in the QMP layer seems like a bad idea to me. >> >> The idea behind block_set was to have a unified interface for changing >> block device parameters at runtime. This prevents us from reinventing >> new commands from scratch. For example, block I/O throttling is >> already queued up to add run-time parameters. >> >> Without a unified command we have a bulkier QMP/HMP interface, >> duplicated code, and possibly inconsistencies in syntax between the >> commands. Isn't the best way to avoid these problems a unified >> interface? >> >> I understand the lack of type safety concern but in this case we >> already have to manually pull parsed arguments (i.e. cast to specific >> types and deal with invalid input). To me this is a reason *for* >> using a unified interface like block_set. > > Think about it from a client perspective. How do I determine which > properties are supported by this version of QEMU? I have no way to identify > programmatically what arguments are valid for block_set. > > OTOH, if you have strong types like block_set_hostcache, query-commands > tells me exactly what's supported. Use query-block and see if 'hostcache' is there. If yes, then the hostcache parameter is available. If we allow BlockDrivers to have their own runtime parameters then query-commands does not tell you anything because the specific BlockDriver may or may not support that runtime parameter - you need to use query-block. Stefan
On 07/27/2011 09:32 PM, Anthony Liguori wrote: > On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >> On Wed, Jul 27, 2011 at 1:58 PM, Anthony >> Liguori<anthony@codemonkey.ws> wrote: >>>> 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", >>>> + .args_type = "device:B,device:O", >>>> + .params = "device [prop=value][,...]", >>>> + .help = "Change block device parameters >>>> [hostcache=on/off]", >>>> + .user_print = monitor_user_noop, >>>> + .mhandler.cmd_new = do_block_set, >>>> + }, >>>> +STEXI >>>> +@item block_set @var{config} >>>> +@findex block_set >>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>> running. >>>> +ETEXI >>>> + >>> >>> block_set_hostcache() please. >>> >>> Multiplexing commands is generally a bad idea. It weakens typing. In the >>> absence of a generic way to set block device properties, implementing >>> properties as generic in the QMP layer seems like a bad idea to me. >> >> The idea behind block_set was to have a unified interface for changing >> block device parameters at runtime. This prevents us from reinventing >> new commands from scratch. For example, block I/O throttling is >> already queued up to add run-time parameters. >> >> Without a unified command we have a bulkier QMP/HMP interface, >> duplicated code, and possibly inconsistencies in syntax between the >> commands. Isn't the best way to avoid these problems a unified >> interface? >> >> I understand the lack of type safety concern but in this case we >> already have to manually pull parsed arguments (i.e. cast to specific >> types and deal with invalid input). To me this is a reason *for* >> using a unified interface like block_set. > > Think about it from a client perspective. How do I determine which > properties are supported by this version of QEMU? I have no way to > identify programmatically what arguments are valid for block_set. > User can programmatically find out valid parameters for block_set. Internally, validation of prop=value is done against -drive parameter list and then, only the valid/implemented commands (for now, hostcache) are accepted from that list. Please find execution output attached: ======================================================================== (qemu) info block ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.qcow2 ro=0 drv=qcow2 encrypted=0 floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw encrypted=0 ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] sd0: removable=1 locked=0 hostcache=1 [not inserted] (qemu) block block_resize block_set block_passwd (qemu) block_set block_set: block device name expected (qemu) block block_resize block_set block_passwd (qemu) help block_set block_set device [prop=value][,...] -- Change block device parameters [hostcache=on/off] (qemu) block_set ide Device 'ide' not found (qemu) block_set ide0-hd0 Usage: block_set device [prop=value][,...] (qemu) block_set ide0-hd0 cache Invalid parameter 'cache' (qemu) block_set ide0-hd0 cache=on Parameter 'hostcache' expects on/off (qemu) block_set ide0-hd0 hostcache=on (qemu) block_set ide0-hd0 hostcache=off (qemu) info block ide0-hd0: removable=0 hostcache=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 encrypted=0 floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw encrypted=0 ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] sd0: removable=1 locked=0 hostcache=1 [not inserted] ======================================================================== When we add further parameters, "usage" string can be enhanced to include those parameters for informing user. - Thanks, Supriya > OTOH, if you have strong types like block_set_hostcache, query-commands > tells me exactly what's supported. > > Regards, > > Anthony Liguori > >> >> Stefan >> > >
Am 27.07.2011 16:51, schrieb Stefan Hajnoczi: > 2011/7/27 Michael Tokarev <mjt@tls.msk.ru>: >> 27.07.2011 15:30, Supriya Kannery wrote: >>> New command "block_set" added for dynamically changing any of the block >>> device parameters. For now, dynamic setting of hostcache params using this >>> command is implemented. Other block device parameter changes, can be >>> integrated in similar lines. >>> >>> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> >>> >>> --- >>> block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> block.h | 2 + >>> blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> blockdev.h | 1 >>> hmp-commands.hx | 14 ++++++++++++ >>> qemu-config.c | 13 +++++++++++ >>> qemu-option.c | 25 ++++++++++++++++++++++ >>> qemu-option.h | 2 + >>> qmp-commands.hx | 28 +++++++++++++++++++++++++ >>> 9 files changed, 200 insertions(+) >>> >>> Index: qemu/block.c >>> =================================================================== >>> --- qemu.orig/block.c >>> +++ qemu/block.c >>> @@ -651,6 +651,34 @@ 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(); >>> + if (bdrv_flush(bs)) { >>> + 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(); >>> + } >> >> Can we please avoid this stuff completely? Just keep the >> old device open still, until you're sure new one is ok. >> >> Or else it will be quite dangerous command in many cases. >> For example, after -runas/-chroot, or additional selinux >> settings or whatnot. And in this case, instead of merely >> returning an error, we'll see abort(). Boom. > > Slight complication for image formats that use a dirty bit here. QED > has a dirty bit. VMDK also specifies one but we don't implement it > today. > > If the image file is dirty then all its metadata will be scanned for > consistency when it is opened. This increases the bdrv_open() time > and hence the downtime of the VM. So it is not ideal to open the > image file twice, even though there is no consistency problem. In general I really like understatements, but opening an image file twice isn't only "not ideal", but should be considered verboten. We're still doing it during migration and it means that in upstream qemu any non-raw images will be corrupted. > I'll think about this some more, there are a couple of solutions like > keeping only the file descriptor around, introducing a flush command > that makes sure the file is in a clean state, or changing QED to not > do this. Changing the format drivers doesn't really look like the right solution. Keeping the fd around looks okay, we can probably achieve this by introducing a bdrv_reopen function. It means that we may need to reopen the format layer, but it can't really fail. Kevin
On 07/28/2011 05:13 AM, Supriya Kannery wrote: > On 07/27/2011 09:32 PM, Anthony Liguori wrote: >> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony >>> Liguori<anthony@codemonkey.ws> wrote: >>>>> 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", >>>>> + .args_type = "device:B,device:O", >>>>> + .params = "device [prop=value][,...]", >>>>> + .help = "Change block device parameters >>>>> [hostcache=on/off]", >>>>> + .user_print = monitor_user_noop, >>>>> + .mhandler.cmd_new = do_block_set, >>>>> + }, >>>>> +STEXI >>>>> +@item block_set @var{config} >>>>> +@findex block_set >>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>> running. >>>>> +ETEXI >>>>> + >>>> >>>> block_set_hostcache() please. >>>> >>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>> the >>>> absence of a generic way to set block device properties, implementing >>>> properties as generic in the QMP layer seems like a bad idea to me. >>> >>> The idea behind block_set was to have a unified interface for changing >>> block device parameters at runtime. This prevents us from reinventing >>> new commands from scratch. For example, block I/O throttling is >>> already queued up to add run-time parameters. >>> >>> Without a unified command we have a bulkier QMP/HMP interface, >>> duplicated code, and possibly inconsistencies in syntax between the >>> commands. Isn't the best way to avoid these problems a unified >>> interface? >>> >>> I understand the lack of type safety concern but in this case we >>> already have to manually pull parsed arguments (i.e. cast to specific >>> types and deal with invalid input). To me this is a reason *for* >>> using a unified interface like block_set. >> >> Think about it from a client perspective. How do I determine which >> properties are supported by this version of QEMU? I have no way to >> identify programmatically what arguments are valid for block_set. >> > > User can programmatically find out valid parameters for > block_set. Internally, validation of prop=value is done against -drive > parameter list and then, only the valid/implemented commands (for now, > hostcache) are accepted from that list. Please find execution output > attached: > ======================================================================== > (qemu) info block > ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.qcow2 ro=0 drv=qcow2 > encrypted=0 > floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw > encrypted=0 > ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] > sd0: removable=1 locked=0 hostcache=1 [not inserted] > (qemu) block > block_resize block_set block_passwd > (qemu) block_set That's HMP btw, not QMP. > block_set: block device name expected > (qemu) block > block_resize block_set block_passwd > (qemu) help block_set > block_set device [prop=value][,...] -- Change block device parameters > [hostcache=on/off] Parsing help text is not introspection! Regards, Anthony Liguori > (qemu) block_set ide > Device 'ide' not found > (qemu) block_set ide0-hd0 > Usage: block_set device [prop=value][,...] > (qemu) block_set ide0-hd0 cache > Invalid parameter 'cache' > (qemu) block_set ide0-hd0 cache=on > Parameter 'hostcache' expects on/off > (qemu) block_set ide0-hd0 hostcache=on > (qemu) block_set ide0-hd0 hostcache=off > (qemu) info block > ide0-hd0: removable=0 hostcache=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 > encrypted=0 > floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw > encrypted=0 > ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted] > sd0: removable=1 locked=0 hostcache=1 [not inserted] > ======================================================================== > > When we add further parameters, "usage" string can be enhanced to > include those parameters for informing user. > > - Thanks, Supriya > >> OTOH, if you have strong types like block_set_hostcache, query-commands >> tells me exactly what's supported. >> >> Regards, >> >> Anthony Liguori >> >>> >>> Stefan >>> >> >> > >
On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.07.2011 16:51, schrieb Stefan Hajnoczi: >> 2011/7/27 Michael Tokarev <mjt@tls.msk.ru>: >>> 27.07.2011 15:30, Supriya Kannery wrote: >>>> New command "block_set" added for dynamically changing any of the block >>>> device parameters. For now, dynamic setting of hostcache params using this >>>> command is implemented. Other block device parameter changes, can be >>>> integrated in similar lines. >>>> >>>> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> >>>> >>>> --- >>>> block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>> block.h | 2 + >>>> blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> blockdev.h | 1 >>>> hmp-commands.hx | 14 ++++++++++++ >>>> qemu-config.c | 13 +++++++++++ >>>> qemu-option.c | 25 ++++++++++++++++++++++ >>>> qemu-option.h | 2 + >>>> qmp-commands.hx | 28 +++++++++++++++++++++++++ >>>> 9 files changed, 200 insertions(+) >>>> >>>> Index: qemu/block.c >>>> =================================================================== >>>> --- qemu.orig/block.c >>>> +++ qemu/block.c >>>> @@ -651,6 +651,34 @@ 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(); >>>> + if (bdrv_flush(bs)) { >>>> + 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(); >>>> + } >>> >>> Can we please avoid this stuff completely? Just keep the >>> old device open still, until you're sure new one is ok. >>> >>> Or else it will be quite dangerous command in many cases. >>> For example, after -runas/-chroot, or additional selinux >>> settings or whatnot. And in this case, instead of merely >>> returning an error, we'll see abort(). Boom. >> >> Slight complication for image formats that use a dirty bit here. QED >> has a dirty bit. VMDK also specifies one but we don't implement it >> today. >> >> If the image file is dirty then all its metadata will be scanned for >> consistency when it is opened. This increases the bdrv_open() time >> and hence the downtime of the VM. So it is not ideal to open the >> image file twice, even though there is no consistency problem. > > In general I really like understatements, but opening an image file > twice isn't only "not ideal", but should be considered verboten. Point taken. >> I'll think about this some more, there are a couple of solutions like >> keeping only the file descriptor around, introducing a flush command >> that makes sure the file is in a clean state, or changing QED to not >> do this. > > Changing the format drivers doesn't really look like the right solution. > > Keeping the fd around looks okay, we can probably achieve this by > introducing a bdrv_reopen function. It means that we may need to reopen > the format layer, but it can't really fail. My concern is that this assumes a single file descriptor. For vmdk there may be multiple split files. I'm almost starting to think bdrv_reopen() should be recursive down the BlockDriverState tree. Stefan
Am 28.07.2011 15:10, schrieb Stefan Hajnoczi: > On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 27.07.2011 16:51, schrieb Stefan Hajnoczi: >>> I'll think about this some more, there are a couple of solutions like >>> keeping only the file descriptor around, introducing a flush command >>> that makes sure the file is in a clean state, or changing QED to not >>> do this. >> >> Changing the format drivers doesn't really look like the right solution. >> >> Keeping the fd around looks okay, we can probably achieve this by >> introducing a bdrv_reopen function. It means that we may need to reopen >> the format layer, but it can't really fail. > > My concern is that this assumes a single file descriptor. For vmdk > there may be multiple split files. > > I'm almost starting to think bdrv_reopen() should be recursive down > the BlockDriverState tree. Yes, VMDK would have to call bdrv_reopen() for each file that it uses. Kevin
On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: >> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>> >>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >>> wrote: >>>>> >>>>> 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", >>>>> + .args_type = "device:B,device:O", >>>>> + .params = "device [prop=value][,...]", >>>>> + .help = "Change block device parameters >>>>> [hostcache=on/off]", >>>>> + .user_print = monitor_user_noop, >>>>> + .mhandler.cmd_new = do_block_set, >>>>> + }, >>>>> +STEXI >>>>> +@item block_set @var{config} >>>>> +@findex block_set >>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>> running. >>>>> +ETEXI >>>>> + >>>> >>>> block_set_hostcache() please. >>>> >>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>> the >>>> absence of a generic way to set block device properties, implementing >>>> properties as generic in the QMP layer seems like a bad idea to me. >>> >>> The idea behind block_set was to have a unified interface for changing >>> block device parameters at runtime. This prevents us from reinventing >>> new commands from scratch. For example, block I/O throttling is >>> already queued up to add run-time parameters. >>> >>> Without a unified command we have a bulkier QMP/HMP interface, >>> duplicated code, and possibly inconsistencies in syntax between the >>> commands. Isn't the best way to avoid these problems a unified >>> interface? >>> >>> I understand the lack of type safety concern but in this case we >>> already have to manually pull parsed arguments (i.e. cast to specific >>> types and deal with invalid input). To me this is a reason *for* >>> using a unified interface like block_set. >> >> Think about it from a client perspective. How do I determine which >> properties are supported by this version of QEMU? I have no way to identify >> programmatically what arguments are valid for block_set. >> >> OTOH, if you have strong types like block_set_hostcache, query-commands >> tells me exactly what's supported. > > Use query-block and see if 'hostcache' is there. If yes, then the > hostcache parameter is available. If we allow BlockDrivers to have > their own runtime parameters then query-commands does not tell you > anything because the specific BlockDriver may or may not support that > runtime parameter - you need to use query-block. Let's reach agreement here. The choices are: 1. Top-level block_set command. Supported parameters are discovered by looking query-block output. 2. Top-level command for each parameter (e.g. block_set_hostcache). Supported parameters are easily discoverable via query-commands. If individual block devices support different sets of parameters then they may have to return -ENOTSUPP. I like the block_set approach. Anthony, Kevin, Supriya: Any thoughts? Stefan
On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: > On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com> wrote: >> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>> >>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >>>> wrote: >>>>>> >>>>>> 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", >>>>>> + .args_type = "device:B,device:O", >>>>>> + .params = "device [prop=value][,...]", >>>>>> + .help = "Change block device parameters >>>>>> [hostcache=on/off]", >>>>>> + .user_print = monitor_user_noop, >>>>>> + .mhandler.cmd_new = do_block_set, >>>>>> + }, >>>>>> +STEXI >>>>>> +@item block_set @var{config} >>>>>> +@findex block_set >>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>>> running. >>>>>> +ETEXI >>>>>> + >>>>> >>>>> block_set_hostcache() please. >>>>> >>>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>>> the >>>>> absence of a generic way to set block device properties, implementing >>>>> properties as generic in the QMP layer seems like a bad idea to me. >>>> >>>> The idea behind block_set was to have a unified interface for changing >>>> block device parameters at runtime. This prevents us from reinventing >>>> new commands from scratch. For example, block I/O throttling is >>>> already queued up to add run-time parameters. >>>> >>>> Without a unified command we have a bulkier QMP/HMP interface, >>>> duplicated code, and possibly inconsistencies in syntax between the >>>> commands. Isn't the best way to avoid these problems a unified >>>> interface? >>>> >>>> I understand the lack of type safety concern but in this case we >>>> already have to manually pull parsed arguments (i.e. cast to specific >>>> types and deal with invalid input). To me this is a reason *for* >>>> using a unified interface like block_set. >>> >>> Think about it from a client perspective. How do I determine which >>> properties are supported by this version of QEMU? I have no way to identify >>> programmatically what arguments are valid for block_set. >>> >>> OTOH, if you have strong types like block_set_hostcache, query-commands >>> tells me exactly what's supported. >> >> Use query-block and see if 'hostcache' is there. If yes, then the >> hostcache parameter is available. If we allow BlockDrivers to have >> their own runtime parameters then query-commands does not tell you >> anything because the specific BlockDriver may or may not support that >> runtime parameter - you need to use query-block. > > Let's reach agreement here. The choices are: > > 1. Top-level block_set command. Supported parameters are discovered > by looking query-block output. I'm strongly opposed to this. There needs to be a single consistent way to determine supported operations with QMP. And that single mechanism already exists--query_commands. > 2. Top-level command for each parameter (e.g. block_set_hostcache). > Supported parameters are easily discoverable via query-commands. If > individual block devices support different sets of parameters then > they may have to return -ENOTSUPP. > > I like the block_set approach. > > Anthony, Kevin, Supriya: Any thoughts? For the sake of overall QMP sanity, I think block_set_hostcache is really our only option. Regards, Anthony Liguori > Stefan >
On Mon, Aug 1, 2011 at 4:28 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: >> >> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com> >> wrote: >>> >>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws> >>> wrote: >>>> >>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>>> >>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >>>>> wrote: >>>>>>> >>>>>>> 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", >>>>>>> + .args_type = "device:B,device:O", >>>>>>> + .params = "device [prop=value][,...]", >>>>>>> + .help = "Change block device parameters >>>>>>> [hostcache=on/off]", >>>>>>> + .user_print = monitor_user_noop, >>>>>>> + .mhandler.cmd_new = do_block_set, >>>>>>> + }, >>>>>>> +STEXI >>>>>>> +@item block_set @var{config} >>>>>>> +@findex block_set >>>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>>>> running. >>>>>>> +ETEXI >>>>>>> + >>>>>> >>>>>> block_set_hostcache() please. >>>>>> >>>>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>>>> the >>>>>> absence of a generic way to set block device properties, implementing >>>>>> properties as generic in the QMP layer seems like a bad idea to me. >>>>> >>>>> The idea behind block_set was to have a unified interface for changing >>>>> block device parameters at runtime. This prevents us from reinventing >>>>> new commands from scratch. For example, block I/O throttling is >>>>> already queued up to add run-time parameters. >>>>> >>>>> Without a unified command we have a bulkier QMP/HMP interface, >>>>> duplicated code, and possibly inconsistencies in syntax between the >>>>> commands. Isn't the best way to avoid these problems a unified >>>>> interface? >>>>> >>>>> I understand the lack of type safety concern but in this case we >>>>> already have to manually pull parsed arguments (i.e. cast to specific >>>>> types and deal with invalid input). To me this is a reason *for* >>>>> using a unified interface like block_set. >>>> >>>> Think about it from a client perspective. How do I determine which >>>> properties are supported by this version of QEMU? I have no way to >>>> identify >>>> programmatically what arguments are valid for block_set. >>>> >>>> OTOH, if you have strong types like block_set_hostcache, query-commands >>>> tells me exactly what's supported. >>> >>> Use query-block and see if 'hostcache' is there. If yes, then the >>> hostcache parameter is available. If we allow BlockDrivers to have >>> their own runtime parameters then query-commands does not tell you >>> anything because the specific BlockDriver may or may not support that >>> runtime parameter - you need to use query-block. >> >> Let's reach agreement here. The choices are: >> >> 1. Top-level block_set command. Supported parameters are discovered >> by looking query-block output. > > I'm strongly opposed to this. There needs to be a single consistent way to > determine supported operations with QMP. > > And that single mechanism already exists--query_commands. Okay, works for me. Supriya: this means that there should be a block_set_hostcache command and you don't need to worry about a generic block_set command. Stefan
Am 01.08.2011 17:28, schrieb Anthony Liguori: > On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: >> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com> wrote: >>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>>> >>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >>>>> wrote: >>>>>>> >>>>>>> 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", >>>>>>> + .args_type = "device:B,device:O", >>>>>>> + .params = "device [prop=value][,...]", >>>>>>> + .help = "Change block device parameters >>>>>>> [hostcache=on/off]", >>>>>>> + .user_print = monitor_user_noop, >>>>>>> + .mhandler.cmd_new = do_block_set, >>>>>>> + }, >>>>>>> +STEXI >>>>>>> +@item block_set @var{config} >>>>>>> +@findex block_set >>>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>>>> running. >>>>>>> +ETEXI >>>>>>> + >>>>>> >>>>>> block_set_hostcache() please. >>>>>> >>>>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>>>> the >>>>>> absence of a generic way to set block device properties, implementing >>>>>> properties as generic in the QMP layer seems like a bad idea to me. >>>>> >>>>> The idea behind block_set was to have a unified interface for changing >>>>> block device parameters at runtime. This prevents us from reinventing >>>>> new commands from scratch. For example, block I/O throttling is >>>>> already queued up to add run-time parameters. >>>>> >>>>> Without a unified command we have a bulkier QMP/HMP interface, >>>>> duplicated code, and possibly inconsistencies in syntax between the >>>>> commands. Isn't the best way to avoid these problems a unified >>>>> interface? >>>>> >>>>> I understand the lack of type safety concern but in this case we >>>>> already have to manually pull parsed arguments (i.e. cast to specific >>>>> types and deal with invalid input). To me this is a reason *for* >>>>> using a unified interface like block_set. >>>> >>>> Think about it from a client perspective. How do I determine which >>>> properties are supported by this version of QEMU? I have no way to identify >>>> programmatically what arguments are valid for block_set. >>>> >>>> OTOH, if you have strong types like block_set_hostcache, query-commands >>>> tells me exactly what's supported. >>> >>> Use query-block and see if 'hostcache' is there. If yes, then the >>> hostcache parameter is available. If we allow BlockDrivers to have >>> their own runtime parameters then query-commands does not tell you >>> anything because the specific BlockDriver may or may not support that >>> runtime parameter - you need to use query-block. >> >> Let's reach agreement here. The choices are: >> >> 1. Top-level block_set command. Supported parameters are discovered >> by looking query-block output. > > I'm strongly opposed to this. There needs to be a single consistent way > to determine supported operations with QMP. > > And that single mechanism already exists--query_commands. > >> 2. Top-level command for each parameter (e.g. block_set_hostcache). >> Supported parameters are easily discoverable via query-commands. If >> individual block devices support different sets of parameters then >> they may have to return -ENOTSUPP. >> >> I like the block_set approach. >> >> Anthony, Kevin, Supriya: Any thoughts? > > For the sake of overall QMP sanity, I think block_set_hostcache is > really our only option. Ideally we should have blockdev_add, and blockdev_set would just take the same arguments and update the given driver. But we don't have blockdev_add today, so whatever works for your as a temporary solution... Kevin
On 08/01/2011 10:44 AM, Kevin Wolf wrote: > Am 01.08.2011 17:28, schrieb Anthony Liguori: >>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>> Supported parameters are easily discoverable via query-commands. If >>> individual block devices support different sets of parameters then >>> they may have to return -ENOTSUPP. >>> >>> I like the block_set approach. >>> >>> Anthony, Kevin, Supriya: Any thoughts? >> >> For the sake of overall QMP sanity, I think block_set_hostcache is >> really our only option. > > Ideally we should have blockdev_add, and blockdev_set would just take > the same arguments and update the given driver. Ideally we'd have a backend_add, backend_set, etc. But in the absence of that, we should provide the best interface we can with the current tools we have. For now, using high level commands is the best we can do. Regards, Anthony Liguori > > But we don't have blockdev_add today, so whatever works for your as a > temporary solution... > > Kevin >
On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 01.08.2011 17:28, schrieb Anthony Liguori: >> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: >>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com> wrote: >>>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: >>>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote: >>>>>> >>>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws> >>>>>> wrote: >>>>>>>> >>>>>>>> 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", >>>>>>>> + .args_type = "device:B,device:O", >>>>>>>> + .params = "device [prop=value][,...]", >>>>>>>> + .help = "Change block device parameters >>>>>>>> [hostcache=on/off]", >>>>>>>> + .user_print = monitor_user_noop, >>>>>>>> + .mhandler.cmd_new = do_block_set, >>>>>>>> + }, >>>>>>>> +STEXI >>>>>>>> +@item block_set @var{config} >>>>>>>> +@findex block_set >>>>>>>> +Change block device parameters (eg: hostcache=on/off) while guest is >>>>>>>> running. >>>>>>>> +ETEXI >>>>>>>> + >>>>>>> >>>>>>> block_set_hostcache() please. >>>>>>> >>>>>>> Multiplexing commands is generally a bad idea. It weakens typing. In >>>>>>> the >>>>>>> absence of a generic way to set block device properties, implementing >>>>>>> properties as generic in the QMP layer seems like a bad idea to me. >>>>>> >>>>>> The idea behind block_set was to have a unified interface for changing >>>>>> block device parameters at runtime. This prevents us from reinventing >>>>>> new commands from scratch. For example, block I/O throttling is >>>>>> already queued up to add run-time parameters. >>>>>> >>>>>> Without a unified command we have a bulkier QMP/HMP interface, >>>>>> duplicated code, and possibly inconsistencies in syntax between the >>>>>> commands. Isn't the best way to avoid these problems a unified >>>>>> interface? >>>>>> >>>>>> I understand the lack of type safety concern but in this case we >>>>>> already have to manually pull parsed arguments (i.e. cast to specific >>>>>> types and deal with invalid input). To me this is a reason *for* >>>>>> using a unified interface like block_set. >>>>> >>>>> Think about it from a client perspective. How do I determine which >>>>> properties are supported by this version of QEMU? I have no way to identify >>>>> programmatically what arguments are valid for block_set. >>>>> >>>>> OTOH, if you have strong types like block_set_hostcache, query-commands >>>>> tells me exactly what's supported. >>>> >>>> Use query-block and see if 'hostcache' is there. If yes, then the >>>> hostcache parameter is available. If we allow BlockDrivers to have >>>> their own runtime parameters then query-commands does not tell you >>>> anything because the specific BlockDriver may or may not support that >>>> runtime parameter - you need to use query-block. >>> >>> Let's reach agreement here. The choices are: >>> >>> 1. Top-level block_set command. Supported parameters are discovered >>> by looking query-block output. >> >> I'm strongly opposed to this. There needs to be a single consistent way >> to determine supported operations with QMP. >> >> And that single mechanism already exists--query_commands. >> >>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>> Supported parameters are easily discoverable via query-commands. If >>> individual block devices support different sets of parameters then >>> they may have to return -ENOTSUPP. >>> >>> I like the block_set approach. >>> >>> Anthony, Kevin, Supriya: Any thoughts? >> >> For the sake of overall QMP sanity, I think block_set_hostcache is >> really our only option. > > Ideally we should have blockdev_add, and blockdev_set would just take > the same arguments and update the given driver. > > But we don't have blockdev_add today, so whatever works for your as a > temporary solution... Anthony's point is that blockdev_set does not fit with QMP command discoverability. Stefan
On 08/01/2011 10:44 AM, Stefan Hajnoczi wrote: > On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf<kwolf@redhat.com> wrote: >> Am 01.08.2011 17:28, schrieb Anthony Liguori: >>> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote: >>>> I like the block_set approach. >>>> >>>> Anthony, Kevin, Supriya: Any thoughts? >>> >>> For the sake of overall QMP sanity, I think block_set_hostcache is >>> really our only option. >> >> Ideally we should have blockdev_add, and blockdev_set would just take >> the same arguments and update the given driver. >> >> But we don't have blockdev_add today, so whatever works for your as a >> temporary solution... > > Anthony's point is that blockdev_set does not fit with QMP command > discoverability. It doesn't, but if we had a 'plug_set' that worked for devices and any type of backends, we could have a single introspection mechanism. But we should really try to avoid having every backend implement their own ways of doing things. Regards, Anthony Liguori > > Stefan >
On Thu, Aug 4, 2011 at 9:32 AM, Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote: > On 08/01/2011 09:14 PM, Anthony Liguori wrote: >> >> On 08/01/2011 10:44 AM, Kevin Wolf wrote: >>> >>> Am 01.08.2011 17:28, schrieb Anthony Liguori: >>>>> >>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>>>> Supported parameters are easily discoverable via query-commands. If >>>>> individual block devices support different sets of parameters then >>>>> they may have to return -ENOTSUPP. >>>>> >>>>> I like the block_set approach. >>>>> >>>>> Anthony, Kevin, Supriya: Any thoughts? >>>> >>>> For the sake of overall QMP sanity, I think block_set_hostcache is >>>> really our only option. >>> >>> Ideally we should have blockdev_add, and blockdev_set would just take >>> the same arguments and update the given driver. >> >> Ideally we'd have a backend_add, backend_set, etc. >> >> But in the absence of that, we should provide the best interface we can >> with the current tools we have. >> >> For now, using high level commands is the best we can do. > > Will be modifying code to have 'block_set_hostcache' command implemented. > Along with that, planning to implement 'query-block_set_hostcache', that > returns current hostcache setting > for all the applicable block devices. I don't think a query command is necessary since query-block already reports this information. Stefan
On 08/01/2011 09:14 PM, Anthony Liguori wrote: > On 08/01/2011 10:44 AM, Kevin Wolf wrote: >> Am 01.08.2011 17:28, schrieb Anthony Liguori: >>>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>>> Supported parameters are easily discoverable via query-commands. If >>>> individual block devices support different sets of parameters then >>>> they may have to return -ENOTSUPP. >>>> >>>> I like the block_set approach. >>>> >>>> Anthony, Kevin, Supriya: Any thoughts? >>> >>> For the sake of overall QMP sanity, I think block_set_hostcache is >>> really our only option. >> >> Ideally we should have blockdev_add, and blockdev_set would just take >> the same arguments and update the given driver. > > Ideally we'd have a backend_add, backend_set, etc. > > But in the absence of that, we should provide the best interface we can > with the current tools we have. > > For now, using high level commands is the best we can do. Will be modifying code to have 'block_set_hostcache' command implemented. Along with that, planning to implement 'query-block_set_hostcache', that returns current hostcache setting for all the applicable block devices. I am not able to find how "query-commands" is helping out to programmatically find out all the supported parameters of a specific command. When I tried out, "query-commands" is listing all the supported command names. "query-xx" is returning current settings related to command 'xx', but not any information related to supported parameters of 'xx'. Am I missing something? > > Regards, > > Anthony Liguori > >> >> But we don't have blockdev_add today, so whatever works for your as a >> temporary solution... >> >> Kevin >> > >
On 08/04/2011 02:02 PM, Supriya Kannery wrote: > On 08/01/2011 09:14 PM, Anthony Liguori wrote: >> On 08/01/2011 10:44 AM, Kevin Wolf wrote: >>> Am 01.08.2011 17:28, schrieb Anthony Liguori: >>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>>>> Supported parameters are easily discoverable via query-commands. If >>>>> individual block devices support different sets of parameters then >>>>> they may have to return -ENOTSUPP. >>>>> > > I am not able to find how "query-commands" is helping out > to programmatically find out all the supported parameters > of a specific command. When I tried out, "query-commands" > is listing all the supported command names. "query-xx" is > returning current settings related to command 'xx', > but not any information related to supported parameters > of 'xx'. > Am I missing something? > ok, I got it. "query commands" is not supposed to list out supported params for each command. Each block device parameter setting, when implemented as separate high level command, will get listed through query-commands. And this helps user to identify supported block parameters that can be changed. >> >> Regards, >> >> Anthony Liguori >> >>> >>> But we don't have blockdev_add today, so whatever works for your as a >>> temporary solution... >>> >>> Kevin >>> >> >> > >
On 08/04/2011 02:01 PM, Stefan Hajnoczi wrote: > On Thu, Aug 4, 2011 at 9:32 AM, Supriya Kannery > <supriyak@linux.vnet.ibm.com> wrote: >> On 08/01/2011 09:14 PM, Anthony Liguori wrote: >>> On 08/01/2011 10:44 AM, Kevin Wolf wrote: >>>> Am 01.08.2011 17:28, schrieb Anthony Liguori: >>>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache). >>>>>> Supported parameters are easily discoverable via query-commands. If >>>>>> individual block devices support different sets of parameters then >>>>>> they may have to return -ENOTSUPP. >>>>>> >>> >>> For now, using high level commands is the best we can do. >> Will be modifying code to have 'block_set_hostcache' command implemented. >> Along with that, planning to implement 'query-block_set_hostcache', that >> returns current hostcache setting >> for all the applicable block devices. > > I don't think a query command is necessary since query-block already > reports this information. > ok > Stefan
Index: qemu/block.c =================================================================== --- qemu.orig/block.c +++ qemu/block.c @@ -651,6 +651,34 @@ 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(); + if (bdrv_flush(bs)) { + 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) { @@ -691,6 +719,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 @@ -71,6 +71,7 @@ void bdrv_delete(BlockDriverState *bs); 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(BlockDriverState *bs, DeviceState *qdev); void bdrv_detach(BlockDriverState *bs, DeviceState *qdev); @@ -97,6 +98,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 @@ -793,3 +793,64 @@ int do_block_resize(Monitor *mon, const return 0; } + + +/* + * Handle changes to block device settings, like hostcache, + * while guest is running. +*/ +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data) +{ + BlockDriverState *bs = NULL; + QemuOpts *opts; + int enable; + const char *device, *driver; + int ret; + char usage[50]; + + /* Validate device */ + device = qdict_get_str(qdict, "device"); + bs = bdrv_find(device); + if (!bs) { + qerror_report(QERR_DEVICE_NOT_FOUND, device); + return -1; + } + + opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict); + if (opts == NULL) { + return -1; + } + + /* If input not in "param=value" format, display error */ + driver = qemu_opt_get(opts, "driver"); + if (driver != NULL) { + qerror_report(QERR_INVALID_PARAMETER, driver); + return -1; + } + + /* Before validating parameters, remove "device" option */ + ret = qemu_opt_delete(opts, "device"); + if (ret == 1) { + strcpy(usage,"block_set device [prop=value][,...]"); + qerror_report(QERR_INCORRECT_COMMAND_SYNTAX, usage); + return 0; + } + + /* Validate parameters with "-drive" parameter list */ + ret = qemu_validate_opts(opts, "drive"); + if (ret == -1) { + return -1; + } + + /* Check for 'hostcache' parameter */ + enable = qemu_opt_get_bool(opts, "hostcache", -1); + if (enable != -1) { + return bdrv_change_hostcache(bs, enable); + } else { + qerror_report(QERR_INVALID_PARAMETER_VALUE, "hostcache","on/off"); + } + + return 0; + +} + Index: qemu/blockdev.h =================================================================== --- qemu.orig/blockdev.h +++ qemu/blockdev.h @@ -65,5 +65,6 @@ 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(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", + .args_type = "device:B,device:O", + .params = "device [prop=value][,...]", + .help = "Change block device parameters [hostcache=on/off]", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_block_set, + }, +STEXI +@item block_set @var{config} +@findex block_set +Change block device parameters (eg: hostcache=on/off) while guest is running. +ETEXI + { .name = "eject", Index: qemu/qemu-config.c =================================================================== --- qemu.orig/qemu-config.c +++ qemu/qemu-config.c @@ -517,6 +517,19 @@ QemuOptsList *qemu_find_opts(const char return find_list(vm_config_groups, group); } +/* Validate given opts list with that of defined vm_group */ +int qemu_validate_opts(QemuOpts *opts, const char *group) +{ + QemuOptsList *vm_group; + + vm_group = qemu_find_opts(group); + if (vm_group == NULL) { + return -1; + } + + return qemu_opts_validate(opts, &vm_group->desc[0]); +} + void qemu_add_opts(QemuOptsList *list) { int entries, i; Index: qemu/qemu-option.c =================================================================== --- qemu.orig/qemu-option.c +++ qemu/qemu-option.c @@ -599,6 +599,31 @@ static void qemu_opt_del(QemuOpt *opt) qemu_free(opt); } +/* + * Delete specified parameter with name "name" from opts + * Return + * 0 - Deletion Successful + * -1 - Param doesn't exist in opts + * 1 - Deletion Successful and opts is empty. +*/ + +int qemu_opt_delete(QemuOpts *opts, const char *name) +{ + QemuOpt *opt = qemu_opt_find(opts, name); + if (opt == NULL) { + return -1; + } + + qemu_opt_del(opt); + + if ((&opts->head)->tqh_first == NULL) { + /* opt queue is empty */ + return 1; + } + + return 0; +} + int qemu_opt_set(QemuOpts *opts, const char *name, const char *value) { QemuOpt *opt; Index: qemu/qemu-option.h =================================================================== --- qemu.orig/qemu-option.h +++ qemu/qemu-option.h @@ -121,6 +121,7 @@ int qemu_opts_set(QemuOptsList *list, co const char *name, const char *value); const char *qemu_opts_id(QemuOpts *opts); void qemu_opts_del(QemuOpts *opts); +int qemu_opt_delete(QemuOpts *opts, const char *name); int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc); int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname); QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev); @@ -131,5 +132,6 @@ typedef int (*qemu_opts_loopfunc)(QemuOp int qemu_opts_print(QemuOpts *opts, void *dummy); int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, int abort_on_failure); +int qemu_validate_opts(QemuOpts *opts, const char *id); #endif Index: qemu/qmp-commands.hx =================================================================== --- qemu.orig/qmp-commands.hx +++ qemu/qmp-commands.hx @@ -727,7 +727,35 @@ Example: EQMP + { + .name = "block_set", + .args_type = "device:B,device:O", + .params = "device [prop=value][,...]", + .help = "Change block device parameters [hostcache=on/off]", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_block_set, + }, + +SQMP +block_set +--------- + +Change various block device parameters (eg: hostcache=on/off) + +Arguments: + +- "device": the device's ID, must be unique (json-string) +- device parameters to be changed (eg: "hostcache": "off") + +Example: + +-> { "execute": "block_set", "arguments": { "device": "ide0-hd0", "hostcache": "off"} } +<- { "return": {} } + +EQMP + + { .name = "balloon", .args_type = "value:M", .params = "target",
New command "block_set" added for dynamically changing any of the block device parameters. For now, dynamic setting of hostcache params using this command is implemented. Other block device parameter changes, can be integrated in similar lines. Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- block.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 2 + blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ blockdev.h | 1 hmp-commands.hx | 14 ++++++++++++ qemu-config.c | 13 +++++++++++ qemu-option.c | 25 ++++++++++++++++++++++ qemu-option.h | 2 + qmp-commands.hx | 28 +++++++++++++++++++++++++ 9 files changed, 200 insertions(+)