Re: [Qemu-devel] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

[-- Attachment #1: Type: text/plain, Size: 5321 bytes --]

On Thu, Aug 03, 2017 at 01:17:01PM +0200, Kevin Wolf wrote:
>Am 03.08.2017 um 12:53 hat Stefan Hajnoczi geschrieben:
>> On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
>> > Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
>> > > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
>> > > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
>> > > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
>> > > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
>> > > > > > > ThrottleGroup is converted to an object. This will allow the future
>> > > > > > > throttle block filter drive easy creation and configuration of throttle
>> > > > > > > groups in QMP and cli.
>> > > > > > >
>> > > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
>> > > > > > > struct for all throttle configuration needs in QMP.
>> > > > > > >
>> > > > > > > ThrottleGroups can be created via CLI as
>> > > > > > >     -object throttle-group,id=foo,x-iops-total=100,x-..
>> > > > > > > where x-* are individual limit properties. Since we can't add non-scalar
>> > > > > > > properties in -object this interface must be used instead. However,
>> > > > > > > setting these properties must be disabled after initialization because
>> > > > > > > certain combinations of limits are forbidden and thus configuration
>> > > > > > > changes should be done in one transaction. The individual properties
>> > > > > > > will go away when support for non-scalar values in CLI is implemented
>> > > > > > > and thus are marked as experimental.
>> > > > > > >
>> > > > > > > ThrottleGroup also has a `limits` property that uses the ThrottleLimits
>> > > > > > > struct.  It can be used to create ThrottleGroups or set the
>> > > > > > > configuration in existing groups as follows:
>> > > > > > >
>> > > > > > > { "execute": "object-add",
>> > > > > > >   "arguments": {
>> > > > > > >     "qom-type": "throttle-group",
>> > > > > > >     "id": "foo",
>> > > > > > >     "props" : {
>> > > > > > >       "limits": {
>> > > > > > >           "iops-total": 100
>> > > > > > >       }
>> > > > > > >     }
>> > > > > > >   }
>> > > > > > > }
>> > > > > > > { "execute" : "qom-set",
>> > > > > > >     "arguments" : {
>> > > > > > >         "path" : "foo",
>> > > > > > >         "property" : "limits",
>> > > > > > >         "value" : {
>> > > > > > >             "iops-total" : 99
>> > > > > > >         }
>> > > > > > >     }
>> > > > > > > }
>> > > > > > >
>> > > > > > > This also means a group's configuration can be fetched with qom-get.
>> > > > > > >
>> > > > > > > ThrottleGroups can be anonymous which means they can't get accessed by
>> > > > > > > other users ie they will always be units instead of group (Because they
>> > > > > > > have one ThrottleGroupMember).
>> > > > > >
>> > > > > > blockdev.c automatically assigns -drive id= to the group name if
>> > > > > > throttling.group= wasn't given.  So who will use anonymous single-drive
>> > > > > > mode?
>> > > > >
>> > > > > Manual filter nodes. It's possible to not pass a group name value and the
>> > > > > resulting group will be anonymous. Are you suggesting to move this change to
>> > > > > the throttle filter patch?
>> > > >
>> > > > What is the use case?  Human users will stick to the legacy syntax
>> > > > because it's convenient.  Management tools will use the filter
>> > > > explicitly in the future, and it's easy for them to choose a name.
>> > > >
>> > > > Unless there is a need for this case I'd prefer to make the group name
>> > > > mandatory.  That way there are less code paths to worry about.
>> > >
>> > > I think Kevin requested this though I don't really remember the use case.
>> >
>> > There is no legacy syntax for putting a throttle node anywhere but at
>> > the root of a BlockBackend. If you want to throttle e.g. only a specific
>> > backing file, you need to manually create a throttle node.
>> >
>> > (We tend to forget this occasionally, but the work you're doing is not
>> > only cleanup just for fun, but it's actually new features that enable
>> > new use cases by making everything more flexible.)
>>
>> It's not clear to me from your reply whether you support anonymous
>> throttle groups or not.  It is possible to throttle arbitrary nodes in
>> the graph either way.
>
>I think it would be nice for human users to have them, but on second
>thought you're right that it's just syntactic sugar for an explicit
>-object, so if you think there is any difficulty with supporting
>anonymous groups, feel free to drop them.
>
>Hm, actually... Does this mean that then the whole 'limits' option in
>the throttle driver could go away, with all of the parsing code, and the
>group name becomes mandatory? That certainly does look tempting.


Anonymous groups don't pose any difficulty. The only problem with groups 
not created with -object or object-add in general is that their limits 
cannot be set with qom-set afterwards; the throttle node will require a 
reopen or replacement with a new one. This is a good argument against 
doing throttle group manipulation in the driver. I don't know if that's 
very user friendly though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]