Re: [Qemu-devel] [V2 3/3] Command block_set for dynamic block params change
Am 22.06.2011 18:09, schrieb Supriya Kannery:
On 06/20/2011 08:04 PM, Kevin Wolf wrote:
Am 17.06.2011 18:38, schrieb Supriya Kannery:
+ if (bdrv_is_inserted(bs)) {
+ /* cache change applicable only if device inserted */
+ return bdrv_change_hostcache(bs, enable);
+ } else {
+ qerror_report(QERR_DEVICE_NOT_INSERTED, device);
+ return -1;
+ }
I'm not so sure about this one. Why shouldn't I change the cache mode
for a device which is currently? The next thing I want to do could be
inserting a medium and using it with the new cache mode.
Uninserted device has no file to be re-opened. So such a check was used
to avoid calling bdrv_reopen without a filename.
I agree with your point. Hostcache value change is applicable
for devices that are not inserted as well. If device is inserted,
request for hostcache setting change can lead to change in open_flags
and then reopen of file. Otherwise, the request can result in just
open_flags updation, which can be used for opening a file when
a device gets inserted.
Will make the above modification in V3
+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;
There could be other reasons for reopening besides changing flags, e.g.
invalidating cached metadata.
+
+ /* Quiesce IO for the given block device */
+ qemu_aio_flush();
+ bdrv_flush(bs);
Missing error handling.
will add in V3
+ 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();
+ }
Maybe we can retry with the old flags at least before aborting?
Also I would like to see a (Linux specific) version that uses the old fd
for the reopen, so that we can handle files that aren't accessible with
their old name any more. This would mean adding a .bdrv_reopen callback
in raw-posix.
Will work on to implement version of re-open using fd. Since for
block_set command processing, re-open using filename already works, is
it ok to have the 're-open using fd' done as a separate patchset ?
Sure, it's just an obvious improvement.
+ /* Reopen file with changed set of flags */
+ return bdrv_reopen(bs, bdrv_flags);
+}
Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the
monitor. We should probably expose the same functionality for the
command line, too.
hostcache is indirectly set/reset from qemu commandline, using cache=
(none/writeback/writethrough/unsafe) option. So should we be having
hostcache=xx option added to -drive in addition to cache= ?
I think that's the right thing to do in the long run. Christoph?
Kevin
Re: [Qemu-devel] [V2 3/3] Command block_set for dynamic block params change
On 06/20/2011 08:04 PM, Kevin Wolf wrote:
Am 17.06.2011 18:38, schrieb Supriya Kannery:
+ if (bdrv_is_inserted(bs)) {
+ /* cache change applicable only if device inserted */
+ return bdrv_change_hostcache(bs, enable);
+ } else {
+ qerror_report(QERR_DEVICE_NOT_INSERTED, device);
+ return -1;
+ }
I'm not so sure about this one. Why shouldn't I change the cache mode
for a device which is currently? The next thing I want to do could be
inserting a medium and using it with the new cache mode.
Uninserted device has no file to be re-opened. So such a check was used
to avoid calling bdrv_reopen without a filename.
I agree with your point. Hostcache value change is applicable
for devices that are not inserted as well. If device is inserted,
request for hostcache setting change can lead to change in open_flags
and then reopen of file. Otherwise, the request can result in just
open_flags updation, which can be used for opening a file when
a device gets inserted.
Will make the above modification in V3
+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;
There could be other reasons for reopening besides changing flags, e.g.
invalidating cached metadata.
+
+ /* Quiesce IO for the given block device */
+ qemu_aio_flush();
+ bdrv_flush(bs);
Missing error handling.
will add in V3
+ 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();
+ }
Maybe we can retry with the old flags at least before aborting?
Also I would like to see a (Linux specific) version that uses the old fd
for the reopen, so that we can handle files that aren't accessible with
their old name any more. This would mean adding a .bdrv_reopen callback
in raw-posix.
Will work on to implement version of re-open using fd. Since for
block_set command processing, re-open using filename already works, is
it ok to have the 're-open using fd' done as a separate patchset ?
+ /* Reopen file with changed set of flags */
+ return bdrv_reopen(bs, bdrv_flags);
+}
Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the
monitor. We should probably expose the same functionality for the
command line, too.
hostcache is indirectly set/reset from qemu commandline, using cache=
(none/writeback/writethrough/unsafe) option. So should we be having
hostcache=xx option added to -drive in addition to cache= ?
Kevin
Re: [Qemu-devel] [V2 3/3] Command block_set for dynamic block params change
Am 17.06.2011 18:38, schrieb Supriya Kannery:
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 parameters, can be integrated
in similar lines.
Signed-off-by: Supriya Kannery supri...@in.ibm.com
Coding style is off in this one as well.
Index: qemu/blockdev.c
===
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -797,3 +797,35 @@ 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)
+{
+ const char *device = qdict_get_str(qdict, device);
+ const char *name = qdict_get_str(qdict, name);
+ int enable = qdict_get_bool(qdict, enable);
+ BlockDriverState *bs;
+
+ bs = bdrv_find(device);
+ if (!bs) {
+ qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ return -1;
+ }
+
+ if (!(strcmp(name, hostcache))) {
The bracket after ! isn't necessary.
+ if (bdrv_is_inserted(bs)) {
+ /* cache change applicable only if device inserted */
+ return bdrv_change_hostcache(bs, enable);
+ } else {
+ qerror_report(QERR_DEVICE_NOT_INSERTED, device);
+ return -1;
+ }
I'm not so sure about this one. Why shouldn't I change the cache mode
for a device which is currently? The next thing I want to do could be
inserting a medium and using it with the new cache mode.
+ }
+
+ return 0;
+}
+
Index: qemu/block.c
===
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,33 @@ 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;
There could be other reasons for reopening besides changing flags, e.g.
invalidating cached metadata.
+
+ /* Quiesce IO for the given block device */
+ qemu_aio_flush();
+ bdrv_flush(bs);
Missing error handling.
+
+ bdrv_close(bs);
Here, too.
+ 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();
+ }
Maybe we can retry with the old flags at least before aborting?
Also I would like to see a (Linux specific) version that uses the old fd
for the reopen, so that we can handle files that aren't accessible with
their old name any more. This would mean adding a .bdrv_reopen callback
in raw-posix.
+
+ return ret;
+}
+
void bdrv_close(BlockDriverState *bs)
{
if (bs-drv) {
@@ -691,6 +718,20 @@ 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;
+
+ /* Reopen file with changed set of flags */
+ return bdrv_reopen(bs, bdrv_flags);
+}
Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the
monitor. We should probably expose the same functionality for the
command line, too.
Kevin
[Qemu-devel] [V2 3/3] Command block_set for dynamic block params change
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 parameters, can be integrated
in similar lines.
Signed-off-by: Supriya Kannery supri...@in.ibm.com
---
block.c | 41 +
block.h |2 ++
blockdev.c | 32
blockdev.h |1 +
hmp-commands.hx | 15 +++
qmp-commands.hx | 30 +-
6 files changed, 120 insertions(+), 1 deletion(-)
Index: qemu/hmp-commands.hx
===
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,21 @@ 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,paramname:s,enable:b,
+ .params = device paramname enable,
+ .help = On/Off block device parameters like hostcache,
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set,
+ },
+
+STEXI
+@item block_set
+@findex block_set
+Change block device params (eg:hostcache=on/off) while guest is running.
+ETEXI
+
{
.name = eject,
Index: qemu/qmp-commands.hx
===
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -693,7 +693,35 @@ Example:
EQMP
-{
+ {
+ .name = block_set,
+ .args_type = device:B,name:s,enable:b,
+ .params = device name enable,
+ .help = Enable/Disable block device params like
hostcache,
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set,
+ },
+
+SQMP
+block_set
+-
+
+Change various block device parameters like hostcache.
+
+Arguments:
+
+- device: the device's ID, must be unique (json-string)
+- name: name of the parameter to be changed (json-string)
+- enable: value to be set for the parameter, 'true' or 'false' (json-bool)
+
+Example:
+
+- { execute: block_set, arguments: { device: ide0-hd0, name:
hostcache, enable: true } }
+- { return: {} }
+
+EQMP
+
+ {
.name = balloon,
.args_type = value:M,
.params = target,
Index: qemu/blockdev.c
===
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -797,3 +797,35 @@ 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)
+{
+ const char *device = qdict_get_str(qdict, device);
+ const char *name = qdict_get_str(qdict, name);
+ int enable = qdict_get_bool(qdict, enable);
+ BlockDriverState *bs;
+
+ bs = bdrv_find(device);
+ if (!bs) {
+ qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ return -1;
+ }
+
+ if (!(strcmp(name, hostcache))) {
+ if (bdrv_is_inserted(bs)) {
+ /* cache change applicable only if device inserted */
+ return bdrv_change_hostcache(bs, enable);
+ } else {
+ qerror_report(QERR_DEVICE_NOT_INSERTED, device);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
Index: qemu/block.c
===
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,33 @@ 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();
+ }
+
+ return ret;
+}
+
void bdrv_close(BlockDriverState *bs)
{
if (bs-drv) {
@@ -691,6 +718,20 @@ 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;
+
+ /*
