| Message ID | 14cbc4c207ba6451894101aae39d146144a3c6dc.1423842044.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 02/13/2015 09:06 AM, Alberto Garcia wrote: > Replace also throttle_group_compare() with throttle_group_get_name() > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 2 +- > block/qapi.c | 5 +++++ > block/throttle-groups.c | 14 ++++---------- > hmp.c | 6 ++++-- > include/block/throttle-groups.h | 2 +- > qapi/block-core.json | 4 +++- > 6 files changed, 18 insertions(+), 15 deletions(-) > > +++ b/qapi/block-core.json > @@ -255,6 +255,8 @@ > # > # @iops_size: #optional an I/O size in bytes (Since 1.7) > # > +# @group: #optional throttle group name (Since 2.3) > +# > # @cache: the cache mode used for the block device (since: 2.3) Ugh - now I'm getting confused by context. Looks like 7/9 touched block_set_io_throttle, and 9/9 touches BlockDeviceInfo. > # > # @write_threshold: configured write threshold for the device. > @@ -274,7 +276,7 @@ > '*bps_max': 'int', '*bps_rd_max': 'int', > '*bps_wr_max': 'int', '*iops_max': 'int', > '*iops_rd_max': 'int', '*iops_wr_max': 'int', > - '*iops_size': 'int', 'cache': 'BlockdevCacheInfo', > + '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo', > 'write_threshold': 'int' } } > Questions - with this series in place, is it ever possible to have throttling parameters without a throttle group name? Do we auto-generate a group name (perhaps based on the node name) for any throttling parameters set without reference to a group name? When using block_set_io_throttle, is it legal to pass parameters (like bps_max) and a group name at the same time, and if so, what happens if there is already a throttle group by that name? Is there a command that can return the list of all throttle group names?
On Tue, Feb 24, 2015 at 09:54:13AM -0700, Eric Blake wrote: > Questions - with this series in place, is it ever possible to have > throttling parameters without a throttle group name? Yes, and it will work the same as before. If the throttling parameters are set but no group name is specified then we auto-generate one. > When using block_set_io_throttle, is it legal to pass parameters > (like bps_max) and a group name at the same time Yes, you're actually supposed to do it like that. If the group does not exist yet, it's created on the fly. If the group name is not set, then we auto-generate one. > and if so, what happens if there is already a throttle group by that > name? All members of the same group share the same ThrottleState configuration (it's stored in the group), so when you set the throttling parameters you set them for the whole group. For the same reason, removing a member from the group doesn't change the throttling parameters. But once the group is empty, it is destroyed. > Is there a command that can return the list of all throttle group > names? Not currently, but I think I can add one easily. Any suggestion for the name of the command, and for the data that you would like it to return? Berto
On 02/25/2015 03:56 AM, Alberto Garcia wrote: >> Is there a command that can return the list of all throttle group >> names? > > Not currently, but I think I can add one easily. Any suggestion for > the name of the command, and for the data that you would like it to > return? How about query-block-throttle, returning an array of dicts. Ideas for what it could contain would be the name of the block group, its current settings, and the node names associated with the group. Maybe something like: => { "execute":"query-block-throttle" } <= { "return": [ { "name": "throttle1", "bps_max": 100000, "nodes": [ "block0", "block1" ] }, { "name": "throttle2", "iops_max": 10000, "nodes": [ "block2" ] } ] }
On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote: > >> Is there a command that can return the list of all throttle group > >> names? > > > > Not currently, but I think I can add one easily. Any suggestion > > for the name of the command, and for the data that you would like > > it to return? > > How about query-block-throttle, returning an array of dicts. Ideas > for what it could contain would be the name of the block group, its > current settings, and the node names associated with the group. > Maybe something like: > > => { "execute":"query-block-throttle" } > <= { "return": [ > { "name": "throttle1", "bps_max": 100000, > "nodes": [ "block0", "block1" ] }, > { "name": "throttle2", "iops_max": 10000, > "nodes": [ "block2" ] } > ] } Sounds reasonable, I think it should be easily doable, I can give it a try. One thing to note, not that it's directly related to group throttling, but as far as I'm aware the current throttling code cannot be used in arbitrary nodes, only in the root (block_set_io_throttle receives a device name). Berto
On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote: > How about query-block-throttle, returning an array of dicts. > > => { "execute":"query-block-throttle" } > <= { "return": [ > { "name": "throttle1", "bps_max": 100000, > "nodes": [ "block0", "block1" ] }, > { "name": "throttle2", "iops_max": 10000, > "nodes": [ "block2" ] } > ] } Ok I wrote it, however I have some doubts about what to put exactly in the 'nodes' array. We can just put node names as you suggest, however at the moment not all nodes have a name so we could end up with a list of empty strings. I think the easiest solution in terms of lines of code and simplicity of the return type is this one: { 'type': 'ThrottleGroupInfo', 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } } { 'command': 'query-block-throttle', 'returns': ['ThrottleGroupInfo'] } All the information is there, the code to fill the BlockDeviceInfo structure is already written so I can just make use of it. The throttling settings themselves are not present in ThrottleGroupInfo, but since all nodes from a group have the same settings, you can get them from any of them. It also keeps the ThrottleGroupInfo structure simpler. What do you think? If you're ok with this solution I can submit the patch series again. Berto
On 02/26/2015 06:56 AM, Alberto Garcia wrote: > On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote: > >> How about query-block-throttle, returning an array of dicts. >> >> => { "execute":"query-block-throttle" } >> <= { "return": [ >> { "name": "throttle1", "bps_max": 100000, >> "nodes": [ "block0", "block1" ] }, >> { "name": "throttle2", "iops_max": 10000, >> "nodes": [ "block2" ] } >> ] } > > Ok I wrote it, however I have some doubts about what to put exactly in > the 'nodes' array. > > We can just put node names as you suggest, however at the moment not > all nodes have a name so we could end up with a list of empty strings. At one point, we were considering a patch from Jeff Cody that guarantees ALL nodes have a name. Maybe that's still worthwhile. > > I think the easiest solution in terms of lines of code and simplicity > of the return type is this one: > > { 'type': 'ThrottleGroupInfo', > 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } } That's a bit more verbose, but also a bit more usable (all the block data is available, rather than just a name that has to be looked up via another command), so that could work. > > { 'command': 'query-block-throttle', > 'returns': ['ThrottleGroupInfo'] } > > All the information is there, the code to fill the BlockDeviceInfo > structure is already written so I can just make use of it. > > The throttling settings themselves are not present in > ThrottleGroupInfo, but since all nodes from a group have the same > settings, you can get them from any of them. It also keeps the > ThrottleGroupInfo structure simpler. > > What do you think? If you're ok with this solution I can submit the > patch series again. Sure, sounds like it's worth that solution, since it reused code and made for less work on your part.
Eric Blake <eblake@redhat.com> writes: > On 02/26/2015 06:56 AM, Alberto Garcia wrote: >> On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote: >> >>> How about query-block-throttle, returning an array of dicts. >>> >>> => { "execute":"query-block-throttle" } >>> <= { "return": [ >>> { "name": "throttle1", "bps_max": 100000, >>> "nodes": [ "block0", "block1" ] }, >>> { "name": "throttle2", "iops_max": 10000, >>> "nodes": [ "block2" ] } >>> ] } >> >> Ok I wrote it, however I have some doubts about what to put exactly in >> the 'nodes' array. >> >> We can just put node names as you suggest, however at the moment not >> all nodes have a name so we could end up with a list of empty strings. > > At one point, we were considering a patch from Jeff Cody that guarantees > ALL nodes have a name. Maybe that's still worthwhile. > >> >> I think the easiest solution in terms of lines of code and simplicity >> of the return type is this one: >> >> { 'type': 'ThrottleGroupInfo', >> 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } } > > That's a bit more verbose, but also a bit more usable (all the block > data is available, rather than just a name that has to be looked up via > another command), so that could work. > >> >> { 'command': 'query-block-throttle', >> 'returns': ['ThrottleGroupInfo'] } >> >> All the information is there, the code to fill the BlockDeviceInfo >> structure is already written so I can just make use of it. >> >> The throttling settings themselves are not present in >> ThrottleGroupInfo, but since all nodes from a group have the same >> settings, you can get them from any of them. It also keeps the >> ThrottleGroupInfo structure simpler. >> >> What do you think? If you're ok with this solution I can submit the >> patch series again. > > Sure, sounds like it's worth that solution, since it reused code and > made for less work on your part. We need a coherent set of block queries. What we have is an ad hoc mess. Alberto proposes to add to the mess. Digs us deeper into the hole. But asking him to solve the complete block query problem so he can have his little feature is hardly fair.
On Wed, Mar 04, 2015 at 08:09:22AM +0100, Markus Armbruster wrote: > >> { 'type': 'ThrottleGroupInfo', > >> 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } } > >> > >> { 'command': 'query-block-throttle', > >> 'returns': ['ThrottleGroupInfo'] } > We need a coherent set of block queries. What we have is an ad hoc > mess. Alberto proposes to add to the mess. Digs us deeper into the > hole. If you can describe what you would like to have then I can try to help. Berto
diff --git a/block.c b/block.c index 625f1c8..a53cb76 100644 --- a/block.c +++ b/block.c @@ -271,7 +271,7 @@ void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group) } /* this bs is a part of the same group than the one we want */ - if (throttle_group_compare(bs->throttle_state, group)) { + if (!g_strcmp0(throttle_group_get_name(bs->throttle_state), group)) { return; } diff --git a/block/qapi.c b/block/qapi.c index 9ed3e68..3d61bab 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -64,9 +64,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) if (bs->io_limits_enabled) { ThrottleConfig cfg; + char *group_name; throttle_group_lock(bs->throttle_state); throttle_get_config(bs->throttle_state, &cfg); + group_name = g_strdup(throttle_group_get_name(bs->throttle_state)); throttle_group_unlock(bs->throttle_state); info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg; @@ -93,6 +95,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) info->has_iops_size = cfg.op_size; info->iops_size = cfg.op_size; + + info->has_group = true; + info->group = group_name; } info->write_threshold = bdrv_write_threshold_get(bs); diff --git a/block/throttle-groups.c b/block/throttle-groups.c index 399ae5e..98c0a5e 100644 --- a/block/throttle-groups.c +++ b/block/throttle-groups.c @@ -117,21 +117,15 @@ bool throttle_group_unref(ThrottleState *ts) return true; } -/* Compare a name with a given ThrottleState group name +/* Get the name from a ThrottleState's ThrottleGroup * * @ts: the throttle state whose group we are inspecting - * @name: the name to compare - * @ret: true if names are equal else false + * @ret: the name of the group */ -bool throttle_group_compare(ThrottleState *ts, const char *name) +const char *throttle_group_get_name(ThrottleState *ts) { ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts); - - if (!name) { - return false; - } - - return !strcmp(name, tg->name); + return tg->name; } /* Register a BlockDriverState in the doubly linked list diff --git a/hmp.c b/hmp.c index 47663ce..ae3ef15 100644 --- a/hmp.c +++ b/hmp.c @@ -369,7 +369,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info, " iops_max=%" PRId64 " iops_rd_max=%" PRId64 " iops_wr_max=%" PRId64 - " iops_size=%" PRId64 "\n", + " iops_size=%" PRId64 + " group=%s\n", inserted->bps, inserted->bps_rd, inserted->bps_wr, @@ -382,7 +383,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info, inserted->iops_max, inserted->iops_rd_max, inserted->iops_wr_max, - inserted->iops_size); + inserted->iops_size, + inserted->group); } if (verbose) { diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h index d000067..8f8d285 100644 --- a/include/block/throttle-groups.h +++ b/include/block/throttle-groups.h @@ -29,7 +29,7 @@ ThrottleState *throttle_group_incref(const char *name); bool throttle_group_unref(ThrottleState *ts); -bool throttle_group_compare(ThrottleState *ts, const char *name); +const char *throttle_group_get_name(ThrottleState *ts); void throttle_group_register_bs(ThrottleState *ts, BlockDriverState *bs); BlockDriverState *throttle_group_next_bs(BlockDriverState *bs); diff --git a/qapi/block-core.json b/qapi/block-core.json index 563b11f..5653924 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -255,6 +255,8 @@ # # @iops_size: #optional an I/O size in bytes (Since 1.7) # +# @group: #optional throttle group name (Since 2.3) +# # @cache: the cache mode used for the block device (since: 2.3) # # @write_threshold: configured write threshold for the device. @@ -274,7 +276,7 @@ '*bps_max': 'int', '*bps_rd_max': 'int', '*bps_wr_max': 'int', '*iops_max': 'int', '*iops_rd_max': 'int', '*iops_wr_max': 'int', - '*iops_size': 'int', 'cache': 'BlockdevCacheInfo', + '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo', 'write_threshold': 'int' } } ##
Replace also throttle_group_compare() with throttle_group_get_name() Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 2 +- block/qapi.c | 5 +++++ block/throttle-groups.c | 14 ++++---------- hmp.c | 6 ++++-- include/block/throttle-groups.h | 2 +- qapi/block-core.json | 4 +++- 6 files changed, 18 insertions(+), 15 deletions(-)