Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
On 07/25/2011 07:04 PM, Kevin Wolf wrote:
Am 25.07.2011 14:50, schrieb Stefan Hajnoczi:
On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery wrote:
On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+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;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Intention here is to use the changed hostcache setting during device
insertion and there after. To apply this to 'change' command (during
insertion of a device), will need to make the following code changes
in do_change_block.
+
+bdrv_flags = bs->open_flags;
+bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
if (bdrv_open(bs, filename, bdrv_flags, drv)< 0) {
qerror_report(QERR_OPEN_FILE_FAILED, filename);
Need to track bs->open_flags a bit more to see what other changes
needed to ensure usage of changed hostcache setting until user
initiates next hostcache change explicitly for that drive.
Please suggest... should we be saving the hostcache change for
future use or just inhibit user from changing hostcache setting
for an empty drive?
The 'change' command does not support any cache= options today. It
always opens the new image with cache=writethrough semantics.
This seems like a bug in 'change' to me. We should preserve cache=
settings across change or at least provide a way to specify them as
arguments.
Perhaps your existing code is fine. When 'change' is fixed or
replaced then 'block_set' will work across 'change' too.
Kevin: Thoughts?
I'm not sure if I would say it's a bug. Probably preserving would be
more useful in most cases, but using the default cache mode doesn't
appear completely wrong either. If you like to change it, I'm not
opposed to it.
Kevin
ok, so for now, will retain the code that saves hostcache setting
for empty drives. When other commands (like 'change') are focused,
can look at using this saved setting.
Will post V5 with other error_report related code changes.
thanks, Supriya
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
Am 25.07.2011 14:50, schrieb Stefan Hajnoczi:
> On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery wrote:
>> On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
>>>
>>> On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+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;
>>
>>>
>>> Can you explain the scenario where this works?
>>>
>>> Looking at do_change_block() the flags will be clobbered so saving them
>>> away does not help.
>>
>> Intention here is to use the changed hostcache setting during device
>> insertion and there after. To apply this to 'change' command (during
>> insertion of a device), will need to make the following code changes
>> in do_change_block.
>>
>> +
>> +bdrv_flags = bs->open_flags;
>> +bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>> bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>> if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
>> qerror_report(QERR_OPEN_FILE_FAILED, filename);
>>
>> Need to track bs->open_flags a bit more to see what other changes
>> needed to ensure usage of changed hostcache setting until user
>> initiates next hostcache change explicitly for that drive.
>>
>> Please suggest... should we be saving the hostcache change for
>> future use or just inhibit user from changing hostcache setting
>> for an empty drive?
>
> The 'change' command does not support any cache= options today. It
> always opens the new image with cache=writethrough semantics.
>
> This seems like a bug in 'change' to me. We should preserve cache=
> settings across change or at least provide a way to specify them as
> arguments.
>
> Perhaps your existing code is fine. When 'change' is fixed or
> replaced then 'block_set' will work across 'change' too.
>
> Kevin: Thoughts?
I'm not sure if I would say it's a bug. Probably preserving would be
more useful in most cases, but using the default cache mode doesn't
appear completely wrong either. If you like to change it, I'm not
opposed to it.
Kevin
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery wrote:
> On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
>>
>> On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
>>> + 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;
>
>>
>> Can you explain the scenario where this works?
>>
>> Looking at do_change_block() the flags will be clobbered so saving them
>> away does not help.
>
> Intention here is to use the changed hostcache setting during device
> insertion and there after. To apply this to 'change' command (during
> insertion of a device), will need to make the following code changes
> in do_change_block.
>
> +
> + bdrv_flags = bs->open_flags;
> + bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> qerror_report(QERR_OPEN_FILE_FAILED, filename);
>
> Need to track bs->open_flags a bit more to see what other changes
> needed to ensure usage of changed hostcache setting until user
> initiates next hostcache change explicitly for that drive.
>
> Please suggest... should we be saving the hostcache change for
> future use or just inhibit user from changing hostcache setting
> for an empty drive?
The 'change' command does not support any cache= options today. It
always opens the new image with cache=writethrough semantics.
This seems like a bug in 'change' to me. We should preserve cache=
settings across change or at least provide a way to specify them as
arguments.
Perhaps your existing code is fine. When 'change' is fixed or
replaced then 'block_set' will work across 'change' too.
Kevin: Thoughts?
Stefan
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
+ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+if (ret< 0) {
+/* Reopen failed. Try to open with original flags */
+error_report("Opening file with changed flags...");
+qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
If the next open fails too then there will be two qerror_report() calls.
This causes a warning and the new qerror is dropped. Please consolidate
the error reporting so there is only one error before returning from
this function.
ok
+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;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
Intention here is to use the changed hostcache setting during device
insertion and there after. To apply this to 'change' command (during
insertion of a device), will need to make the following code changes
in do_change_block.
+
+bdrv_flags = bs->open_flags;
+bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
qerror_report(QERR_OPEN_FILE_FAILED, filename);
Need to track bs->open_flags a bit more to see what other changes
needed to ensure usage of changed hostcache setting until user
initiates next hostcache change explicitly for that drive.
Please suggest... should we be saving the hostcache change for
future use or just inhibit user from changing hostcache setting
for an empty drive?
+/* If input not in "param=value" format, display error */
+driver = qemu_opt_get(opts, "driver");
+if (driver != NULL) {
+error_report("Invalid parameter %s", driver);
error_report() only works for HMP. Please use qerror_report() so both
HMP and QMP see the error. Same issue further down.
ok..will change error_report to qerror_report.
Will make further code changes into patchset version 5
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
> Index: qemu/block.c
> ===
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -651,6 +651,40 @@ unlink_and_fail:
> return ret;
> }
>
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +BlockDriver *drv = bs->drv;
> +int ret = 0;
> +
> +/* 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;
> +}
> +bdrv_close(bs);
> +
> +
> +ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +if (ret < 0) {
> +/* Reopen failed. Try to open with original flags */
> +error_report("Opening file with changed flags...");
> +qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
If the next open fails too then there will be two qerror_report() calls.
This causes a warning and the new qerror is dropped. Please consolidate
the error reporting so there is only one error before returning from
this function.
> +
> +ret = bdrv_open(bs, bs->filename, bs->open_flags, drv);
bs->open_flags has been clobbered by the previous bdrv_open(). It would
be best to take a copy of bs->open_flags before bdrv_close(bs) above.
> +if (ret < 0) {
> +/*
> + * Reopen failed with orig and modified flags
> +*/
> +error_report("Opening file with original flags...");
> +qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +abort();
> +}
> +}
> +
> +return ret;
> +}
> +
> void bdrv_close(BlockDriverState *bs)
> {
> if (bs->drv) {
> @@ -691,6 +725,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;
Can you explain the scenario where this works?
Looking at do_change_block() the flags will be clobbered so saving them
away does not help.
> Index: qemu/blockdev.c
> ===
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -793,3 +793,63 @@ 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;
> +
> +/* 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) {
> +error_report("Invalid parameter %s", driver);
error_report() only works for HMP. Please use qerror_report() so both
HMP and QMP see the error. Same issue further down.
Stefan
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
On 07/13/2011 06:37 PM, Supriya Kannery wrote:
Updated "block_set" command to accept multiple -drive parameters.
Also, added code for re-opening of device file with original flags,
incase opening file using changed hostcache setting fails.
--
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
---
block.c | 60
block.h | 2 +
blockdev.c | 60
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, 205 insertions(+)
Index: qemu/block.c
===
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,40 @@ unlink_and_fail:
return ret;
}
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+ BlockDriver *drv = bs->drv;
+ int ret = 0;
+
+ /* 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;
+ }
+ bdrv_close(bs);
+
+
+ ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+ if (ret < 0) {
+ /* Reopen failed. Try to open with original flags */
+ error_report("Opening file with changed flags...");
+ qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+
+ ret = bdrv_open(bs, bs->filename, bs->open_flags, drv);
+ if (ret < 0) {
+ /*
+ * Reopen failed with orig and modified flags
+ */
+ error_report("Opening file with original flags...");
+ qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+ abort();
+ }
+ }
+
+ return ret;
+}
+
void bdrv_close(BlockDriverState *bs)
{
if (bs->drv) {
@@ -691,6 +725,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);
@@ -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.c
===
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -793,3 +793,63 @@ 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;
+
+ /* 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) {
+ error_report("Invalid parameter %s", driver);
+ return -1;
+ }
+
+ /* Before validating parameters, remove "device" option */
+ ret = qemu_opt_delete(opts, "device");
+ if (ret == 1) {
+ error_report("Specify parameter to be changed");
+ error_report("Usage: block_set device [prop=value][,...]");
+ return 0;
+ }
+
+ /* Validate parameters with "-drive" parameter list */
+ re
Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change
Updated "block_set" command to accept multiple -drive parameters.
Also, added code for re-opening of device file with original flags,
incase opening file using changed hostcache setting fails.
--
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
---
block.c | 60
block.h |2 +
blockdev.c | 60
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, 205 insertions(+)
Index: qemu/block.c
===
--- qemu.orig/block.c
+++ qemu/block.c
@@ -651,6 +651,40 @@ unlink_and_fail:
return ret;
}
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+BlockDriver *drv = bs->drv;
+int ret = 0;
+
+/* 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;
+}
+bdrv_close(bs);
+
+
+ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+if (ret < 0) {
+/* Reopen failed. Try to open with original flags */
+error_report("Opening file with changed flags...");
+qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+
+ret = bdrv_open(bs, bs->filename, bs->open_flags, drv);
+if (ret < 0) {
+/*
+ * Reopen failed with orig and modified flags
+*/
+error_report("Opening file with original flags...");
+qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+abort();
+}
+}
+
+return ret;
+}
+
void bdrv_close(BlockDriverState *bs)
{
if (bs->drv) {
@@ -691,6 +725,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);
@@ -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.c
===
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -793,3 +793,63 @@ 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;
+
+/* 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 (dri
