Re: [Qemu-devel] [PATCH v3 5/7] block: add throttle block filter driver

[Alberto Garcia <berto@igalia.com>]

On Wed 09 Aug 2017 03:42:07 PM CEST, Manos Pitsidianakis wrote:
> On Wed, Aug 09, 2017 at 02:36:20PM +0200, Alberto Garcia wrote:
>>On Wed 09 Aug 2017 11:36:12 AM CEST, Manos Pitsidianakis wrote:
>>> On Tue, Aug 08, 2017 at 05:04:48PM +0200, Alberto Garcia wrote:
>>>>On Tue 08 Aug 2017 04:56:20 PM CEST, Manos Pitsidianakis wrote:
>>>>>>> So basically if we have anonymous groups, we accept limits in the
>>>>>>> driver options but only without a group-name.
>>>>>>
>>>>>>In the commit message you do however have limits and a group name, is
>>>>>>that a mistake?
>>>>>>
>>>>>>    -drive driver=throttle,file.filename=foo.qcow2, \
>>>>>>           limits.iops-total=...,throttle-group=bar
>>>>>
>>>>> Sorry this wasn't clear, I'm actually proposing to remove limits from
>>>>> the throttle driver options and only create/config throttle groups via
>>>>> -object/object-add.
>>>>
>>>>Sorry I think it was me who misunderstood :-) Anyway in the new
>>>>command-line API I would be more inclined to have limits defined using
>>>>"-object throttle-group" and -drive would only reference the group id.
>>>>
>>>>I understand that this implies that it wouldn't be possible to create
>>>>anonymous groups (at least not from the command line), is that a
>>>>problem?
>>>
>>> We can accept anonymous groups if a user specifies limits but not a
>>> group name in the throttle driver. (The only case where limits would
>>> be acccepted)
>>
>>Yeah but that's only if we have the limits.iops-total=... options in the
>>throttle driver. If we "remove limits from the throttle driver options
>>and only create/config throttle groups via -object/object-add" we 
>>cannot
>>do that.
>
> We can check that groups is not defined at the same time as limits,

I'm not sure if I'm following the conversation anymore :-) let's try to
recap:

  a) Groups are defined like this (with the current patches):

     -object throttle-group,id=foo,x-iops-total=100,x-..

  b) Throttle nodes are defined like this:

     -drive driver=throttle,file.filename=foo.qcow2, \
            limits.iops-total=...,throttle-group=bar

  c) Therefore limits can be defined either in (a) or (b)

  d) If we omit throttle-group=... in (b), we would create an anonymous
     group.

  e) The -drive syntax from (b) has the "problem" that it's possible to
     define several nodes with the same throttling group but different
     limits. The last one would win (as the legacy syntax does), but
     that's not something completely straightforward for the end user.

  f) The syntax from (b) also has the problem that there's one more
     place that needs code to parse throttling limits.

  g) We can solve (e) and (f) if we remove the limits.* options
     altogether from the throttling filter. In that case you would need
     to define a throttle-group object and use the throttle-group option
     of the filter node in all cases.

  h) If we remove the limits.* options we cannot have anonymous groups
     anymore (at least not using this API). My question is: is it a
     problem? What would we lose? What benefits do anonymous groups
     bring us?
     
  i) We can of course maintain the limits.* options, but disallow
     throttle-group when they are present. That way we would allow
     anonymous groups and we would solve the ambiguity problem described
     in (e). My question is: is it worth it?

>>> Not creating eponymous throttle groups via the throttle driver means
>>> we don't need throttle_groups anymore, since even anonymous ones
>>> don't need to be accounted for in a list.
>>
>>I don't follow you here, how else do you get a group by its name?
>
> If all eponymous groups are managed by the QOM tree, we should be able
> to iterate over the object root container for all ThrottleGroups just
> like qmp_query_iothreads() in iothread.c

Mmm... can't we actually use the root container now already? (even with
anonymous groups I mean). Why do we need throttle_groups?

Berto