Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Kevin Wolf]

Am 23.05.2011 18:01, schrieb Markus Armbruster:
> Kevin Wolf <address@hidden> writes:
> 
>> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi:
>>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <address@hidden> wrote:
>>>> On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
>>>>>
>>>>> 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.
>>>>
>>>> Sure, I will resubmit this patchset, after making this feature more 
>>>> generic.
>>>> Can you pls help finding atleast one or two options (other than hostcache)
>>>> which can be changed dynamically. This will help me evaluate the generic
>>>> approach.
>>>
>>> Hang on, let's see if we can get agreement from Kevin and others
>>> before taking this approach.  Like I said, I don't see other options
>>> that should be changed at runtime.
>>
>> Things like enabling copy on read could fit here.
>>
>> Generally I'm in favour of having a generic command. We just need to pay
>> attention not to include things that we don't want to maintain long
>> term, i.e. just putting the current cache=... parameter into the
>> argument isn't going to work. Maybe two booleans 'o_direct' and
>> 'ignore_flushes' is what we want to have. The same structure should be
>> used for blkdev_add then, even though it will allow some more options.
>>
>> I'm also not completely sure how you would enable cache=writethrough
>> from the command line in a fully converted world. Would this be one of
>> the arguments that must be specified on BlockDriverState creation and
>> can't be changed later, and the device will pick it up from there? Or is
>> it a qdev property and somehow makes it way to the block layer?
> 
> qdev properties are the configurable bits of the device's guest part.
> For something to be a qdev property, it must belong to the guest part,
> and it must be configurable.
> 
> Example: a drive serial number belongs to the guest part.  It's
> guest-visible.  The host part doesn't care about it; in fact, if you
> unplug the device, you could reuse the same host part with another guest
> part with different serial number.
> 
> Example: the image format doesn't belong to the guest part.  It's not
> guest-visible.  Even the device model code is (should be!) ignorant of
> it.
> 
> Device guest part reconfiguration at run-time is done by the guest OS,
> just like for non-virtual devices.  This may make the device model call
> out to the block layer to adjust settings there.
> 
> Except for configuration knobs that match physical knobs (media eject
> and insert, for instance).  Those belong into the monitor.

Thanks Markus. I think I'm well aware what a guest and what a host is.
The trouble begins when there's a connection between both...

Let me try to list the facts with cache=writethrough:

* It is currently considered host state (part of bs->open_flags). It
  results in the image file being opened with O_SYNC.

* It is guest visible as something like a WCE bit (Write Cache Enable).
  The guest is allowed to toggle this bit on real hardware and we want
  to add this functionality, too.

* We need a way to configure it from the start. Currently done by
  cache=writethrough, but that is host state even though it's guest
  visible. That would be an argument for moving it into qdev.

* You're supposed to use blockdev_add first and then device_add. This
  means that we open the image file before the device and its qdev
  config exists. So the first thing the device must do is to reopen the
  image? What to do with the fd: protocol?

So, treating it as host state is wrong. Treating it as guest state
doesn't work really well either.

> So, if your configuration knob can be manipulated at run time from the
> monitor, and it doesn't correspond to a physical knob, then it
> *probably* belongs to the host part, and its setting isn't
> guest-visible.
> 
> Let's assume we're indeed talking about reconfiguring the host part.  A
> generic command for that makes sense to me.  Could look like
> "blockdev_set ID NAME=VAL,..." in the human monitor.  The NAME=VAL
> should be consistent with blockdev_add whenever possible.  Requires a
> crystal ball until we actually get blockdev_add.  If we're afraid of
> getting it wrong, we could do a drive_set now.

If we're always afraid of getting it wrong and try to evade, we'll never
get blockdev_add.

Kevin