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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

Similar to read-only, except for the "guest can change it" bit.

For read-only, we concluded that it belongs to both guest and host part.
Connecting a read/write guest part to a read-only host part is not
allowed.  For convenience, the guest part's read-only bit could default
to match the host part's.

Can we solve the WCE bit problem similarly?

>> 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.

Yes.  But it's advisable to start with blockdev_add rather than with
blockdev_set.