Re: [Qemu-devel] [Proposal] I/O throttling through new -object interface

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

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

On Fri, Jun 09, 2017 at 03:42:41PM +0200, Alberto Garcia wrote:
>Hey Manos, thanks for your proposal and for the good work :)
>
>On Thu 08 Jun 2017 08:21:13 PM CEST, Manos Pitsidianakis wrote:
>
>> As part of my GSoC project, IO throttling is moved to a block filter
>> driver, which means multiple filter nodes sharing ThrottleGroup
>> instances, and also, chaining filter nodes. The old interface will be
>> retained as much as possible. By defining ThrottleGroups with QOM the
>> following interface can be used:
>>     -drive file=foo.qcow2,throttling.group=bar \
>>     -object throttle-group,iops-total=100,id=bar
>
>It should be --object

Both seem to have the result in my qemu build.

>, and I guess the name would be throttling-group?

Hm, I thought throttle-group would be nicer because it's shorter, but I 
can change it, of course. I am not certain if there's a semantic 
difference (because I am not a native speaker).

>
>> 'block_set_io_throttle' command can be extended with a "group"
>> argument, that when defined instead of "device", will find and edit
>> the object in question.
>
>Note that block_set_io_throttle() already has a 'group' argument. Its
>current semantics are:
>
>   - bps or iops != 0   -> set the I/O limits of a throttling group. The
>                           selected device is moved to that group if it
>                           wasn't there yet.
>
>   - bps and iops == 0  -> remove a device from a throttling group
>                           without touching that group's I/O limits.
>
>where 'device' is mandatory and 'group' defaults to the value of
>'device' if unset.
>
>If I get it right, what you propose is that 'device' is optional and in
>that case 'group' is mandatory and the command updates the throttling
>group parameters.
>
>Note that you can do that already if you select a device that is already
>in the group. These new semantics would allow modifying a group that is
>not being used by any block device.

That was my thinking as well.
>
>I'm not a big fan of overloading block_set_io_throttle() even more, but
>this proposal does actually sound fine.
>
>> Users can hotplug ThrottleGroups with object-add in QMP/HMP, then
>> attach drives to a throttle group (ie adding a filter node in the
>> drive path that points to that throttle group) with an
>> 'attach-throttle' and detach with 'detach-throttle', each taking the
>> drive and throttle group name as arguments. An alternative (but
>> uglier) approach would be to use 'block_set_io_throttle' for this as
>> well.
>
>Yeah I agree that the block_set_io_throttle() approach would be uglier.
>
>> Something that's also sound is a 'block_set_throttle_groups' that
>> takes a list of groups and a drive as arguments.
>
>Would that simply be a foreach(g, groups) { attach(device, g); } ?

Something like 

foreach(g, groups) { attach(device, g); }
foreach(g, device.groups - groups) { detach(device, g); } #where '-' is 
the set minus operation)

This would mean you would have to redefine previous groups as well 
everytime you use 'block_set_throttle_groups' otherwise they would get 
removed from the device. At least that's a way to avoid using more 
commands for attaching/detaching.
>
>> Regarding 'query-block'. This case is complicated by the fact that it
>> would be possible for a drive to have many nodes, thus the values
>> printed by 'query-block' might be inaccurate if they only report one
>> of the node configurations.  The current approach is to report only
>> the first filter node in the BDS tree. A user with advanced throttling
>> configurations might have no need for this command.
>
>I guess the user can use 'query-block' and walk the backing chain
>looking for throttling filter nodes?

I think it would be trivial to do this. Isn't the BDS tree always 
"short"? Otherwise the filter nodes should be cached in BlockBackend for 
performance, if it can be argued that it's needed.
>
>Berto
>

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