Kevin Wolf <kw...@redhat.com> writes: > Am 23.05.2011 18:01, schrieb Markus Armbruster: >> Kevin Wolf <kw...@redhat.com> writes: >> >>> Am 23.05.2011 12:00, schrieb Stefan Hajnoczi: >>>> On Mon, May 23, 2011 at 8:04 AM, Supriya Kannery <supri...@in.ibm.com> >>>> 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.