On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
> host cache change and display of host cache setting respectively.
A generic command for changing block device options would be nice,
althought I don't see other options where it makes sense to change them
at runtime.
The alternative would be:
block_set hostcache on
"block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
The hostcache_get information would be part of query-block output:
{
"device":"ide0-hd0",
"locked":false,
"removable":false,
"inserted":{
"ro":false,
"drv":"qcow2",
"encrypted":false,
"file":"disks/test.img"
"hostcache":true,
},
"type":"hd"
},
This approach is extensible if more options need to be exposed.
> Signed-off-by: Supriya Kannery <supri...@in.ibm.com>
>
> ---
> block.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> block.h | 2 ++
> blockdev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> blockdev.h | 2 ++
> hmp-commands.hx | 29 +++++++++++++++++++++++++++++
> qmp-commands.hx | 55
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 184 insertions(+)
>
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -70,6 +70,35 @@ but should be used with extreme caution.
> resizes image files, it can not resize block devices like LVM volumes.
> ETEXI
>
> + {
> + .name = "hostcache_get",
> + .args_type = "device:B",
> + .params = "device",
> + .help = "retrieve host cache settings for device",
Please make it clear these operations affect block devices:
"for block device"
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_hostcache_get,
> + },
> +
> +STEXI
> +@item hostcache_get
> +@findex hostcache_get
> +Display host cache settings for a block device while guest is running.
> +ETEXI
> +
> + {
> + .name = "hostcache_set",
> + .args_type = "device:B,hostcache:s",
> + .params = "device hostcache",
> + .help = "change host cache setting for device",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_hostcache_set,
> + },
> +
> +STEXI
> +@item hostcache_set
> +@findex hostcache_set
> +Change host cache options for a block device while guest is running.
> +ETEXI
>
> {
> .name = "eject",
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -657,6 +657,34 @@ unlink_and_fail:
> return ret;
> }
>
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> + BlockDriver *drv = bs->drv;
> + int ret = 0;
> +
> + /* No need to reopen as no change in flags */
> + if (bdrv_flags == bs->open_flags) {
> + return 0;
> + }
> +
> + /* Quiesce IO for the given block device */
> + qemu_aio_flush();
> + bdrv_flush(bs);
> +
> + 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) {
> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> + abort();
The error is never reported on a QMP monitor because qerror_report()
simply stashes away the qerror. The QMP client doesn't have a chance to
read the error before QEMU terminates.
> + }
> +
> + return ret;
> +}
> +
> void bdrv_close(BlockDriverState *bs)
> {
> if (bs->drv) {
> @@ -3049,3 +3077,23 @@ out:
>
> return ret;
> }
> +
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
Consistently using "hostcache" or "host_cache" would be nice.
> +{
> + int bdrv_flags = bs->open_flags;
> +
> + /* No change in existing hostcache setting */
> + if(!enable_host_cache == (bdrv_flags & BDRV_O_NOCACHE)) {
This expression doesn't work as expected. bool has a lower rank than
int. That means !enable_host_cache is converted to an int and compared
against bdrv_flags & BDRV_O_NOCACHE. This expression is always false
because a bool is 0 or 1 and BDRV_O_NOCACHE is 0x0020.
> + return -1;
This shouldn't be a failure and please don't use -1 when a negative
errno indicates failure. -1 == -EPERM. The return value should be 0
here.
> + }
Anyway, this whole check is unnecessary since bdrv_reopen() already
performs it.
> +
> + /* set hostcache flags (without changing WCE/flush bits) */
> + if(!enable_host_cache) {
> + bdrv_flags |= BDRV_O_NOCACHE;
> + } else {
> + bdrv_flags &= ~BDRV_O_NOCACHE;
> + }
> +
> + /* Reopen file with changed set of flags */
> + return(bdrv_reopen(bs, bdrv_flags));
Please run scripts/checkpatch.pl before submitting patches.
> +}
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -796,3 +796,51 @@ int do_block_resize(Monitor *mon, const
>
> return 0;
> }
> +
> +int do_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> + const char *device = qdict_get_str(qdict, "device");
> + BlockDriverState *bs;
> +
> + bs = bdrv_find(device);
> + if (!bs) {
> + qerror_report(QERR_DEVICE_NOT_FOUND, device);
> + return -1;
> + }
> +
> + monitor_printf(mon, "%s:", device);
> +
> + if (bs->open_flags & BDRV_O_NOCACHE) {
> + monitor_printf(mon, " hostcache=off\n");
> + } else {
> + monitor_printf(mon, " hostcache=on\n");
> + }
> +
> + return 0;
> +}
> +
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> + const char *device = qdict_get_str(qdict, "device");
> + const char *hostcache = qdict_get_str(qdict, "hostcache");
> + BlockDriverState *bs;
> + bool enable_host_cache = 0;
> +
> + bs = bdrv_find(device);
> + if (!bs) {
> + qerror_report(QERR_DEVICE_NOT_FOUND, device);
> + return -1;
> + }
> +
> + /* cache change applicable only if device inserted */
> + if (bdrv_is_inserted(bs)) {
> + if (!strcmp(hostcache,"on"))
> + enable_host_cache = 1;
> + else
> + enable_host_cache = 0;
Coding style, please use {} or remove the if:
enable_host_cache = !strcmp(hostcache, "on");
> + return(bdrv_change_hostcache(bs, enable_host_cache));
> + } else {
> + qerror_report(QERR_DEVICE_NOT_INSERTED, device);
> + return -1;
> + }
> +}
> 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);
> @@ -96,6 +97,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.h
> ===================================================================
> --- qemu.orig/blockdev.h
> +++ qemu/blockdev.h
> @@ -64,5 +64,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_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
>
> #endif
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -664,6 +664,61 @@ Example:
> -> { "execute": "block_resize", "arguments": { "device": "scratch", "size":
> 1073741824 } }
> <- { "return": {} }
>
> +
> +EQMP
> +
> + {
> + .name = "hostcache_get",
> + .args_type = "device:B",
> + .params = "device",
> + .help = "retrieve host cache settings for device",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_hostcache_get,
> + },
> +SQMP
> +cache_get
hostcache_get
> +---------
> +
> +Retrieve host page cache setting while a guest is running.
Please make it clear that this is an operation on block devices.
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +
> +Example:
> +
> +-> { "execute": "hostcache_set", "arguments": { "device": "ide0-hd0" } }
> +<- { "return": {} }
> +
> +
> +EQMP
> +
> + {
> + .name = "hostcache_set",
> + .args_type = "device:B,cache:s",
> + .params = "device cache",
> + .help = "change hostcache setting for device",
> + .user_print = monitor_user_noop,
> + .mhandler.cmd_new = do_hostcache_set,
> + },
> +
> +SQMP
> +hostcache_set
> +-------------
> +
> +Change host page cache setting while a guest is running.
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +- "cache": host cache value "off" or "on" (json-string)
There is a boolean value that can be used instead of string on|off. See
the set_link command.
Stefan
