Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On 08/04/2011 02:01 PM, Stefan Hajnoczi wrote: On Thu, Aug 4, 2011 at 9:32 AM, 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. 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Thu, Aug 4, 2011 at 9:32 AM, 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 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On 08/01/2011 10:44 AM, Stefan Hajnoczi wrote: On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf 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
>>> wrote:
On Wed, Jul 27, 2011 at 5:02 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
>> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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 wrote:
>>> On Wed, Jul 27, 2011 at 5:02 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
> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Mon, Aug 1, 2011 at 4:28 PM, Anthony Liguori wrote:
> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
>>
>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi
>> wrote:
>>>
>>> On Wed, Jul 27, 2011 at 5:02 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
> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi wrote:
On Wed, Jul 27, 2011 at 5:02 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
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi wrote:
> On Wed, Jul 27, 2011 at 5:02 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
>>> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
Am 28.07.2011 15:10, schrieb Stefan Hajnoczi: > On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf wrote:
> Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
>> 2011/7/27 Michael Tokarev :
>>> 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
---
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
> 2011/7/27 Michael Tokarev :
>> 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
>>>
>>> ---
>>> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Wed, Jul 27, 2011 at 5:02 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
>> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
2011/7/27 Michael Tokarev :
> 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
>>
>> ---
>> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
>
> ---
> 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
Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change
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
---
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);
+
