Re: [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM

[Stefan Hajnoczi]

On Tue, Aug 22, 2017 at 01:15:33PM +0300, Manos Pitsidianakis wrote:
>  /* Increments the reference count of a ThrottleGroup given its name.
>   *
>   * If no ThrottleGroup is found with the given name a new one is
>   * created.
>   *
> + * This function edits throttle_groups and must be called under the global
> + * mutex.
> + *
>   * @name: the name of the ThrottleGroup
>   * @ret:  the ThrottleState member of the ThrottleGroup
>   */
>  ThrottleState *throttle_group_incref(const char *name)
>  {
>  ThrottleGroup *tg = NULL;
> -ThrottleGroup *iter;
> -
> -qemu_mutex_lock(_groups_lock);
>  
>  /* Look for an existing group with that name */
> -QTAILQ_FOREACH(iter, _groups, list) {
> -if (!strcmp(name, iter->name)) {
> -tg = iter;
> -break;
> -}
> -}
> +tg = throttle_group_by_name(name);
>  
> -/* Create a new one if not found */
> -if (!tg) {
> -tg = g_new0(ThrottleGroup, 1);
> +if (tg) {
> +object_ref(OBJECT(tg));
> +} else {
> +/* Create a new one if not found */
> +/* new ThrottleGroup obj will have a refcnt = 1 */
> +tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
>  tg->name = g_strdup(name);
> -tg->clock_type = QEMU_CLOCK_REALTIME;
> -
> -if (qtest_enabled()) {
> -/* For testing block IO throttling only */
> -tg->clock_type = QEMU_CLOCK_VIRTUAL;
> -}
> -qemu_mutex_init(>lock);
> -throttle_init(>ts);
> -QLIST_INIT(>head);
> -
> -QTAILQ_INSERT_TAIL(_groups, tg, list);

This is where the ThrottleGroup needs to be added to the QOM tree:

object_property_add_child(object_get_objects_root(),
  id, obj, _err);

> +throttle_group_obj_complete((UserCreatable *)tg, _abort);

This cast is risky - if UserCreatable ever adds fields it can break.
Please use a QOM cast: USER_CREATABLE(tg).  It uses the type information
and finds the interface.

> +static ThrottleParamInfo properties[] = {

This array can be const.

> +{

Missing indentation.  Seems unusual but I guess you're trying to avoid
reaching 80 chars?  If checkpatch.pl doesn't complain then you can get
away with it but normal indentation would be best.

> +switch (info->category) {
> +case AVG:
> +cfg.buckets[info->type].avg = value;
> +break;
> +case MAX:
> +cfg.buckets[info->type].max = value;
> +break;
> +case BURST_LENGTH:
> +if (value > UINT_MAX) {
> +error_setg(_err, "%s value must be in the"
> +"range [0, %u]", info->name, UINT_MAX);
> +goto ret;
> +}
> +cfg.buckets[info->type].burst_length = value;
> +break;
> +case IOPS_SIZE:
> +cfg.op_size = value;
> +}

Please use break statements even for the last case.  If another case is
added later it might accidentally fall through without a break
statement.

Re: [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

On Tue, Aug 22, 2017 at 03:25:41PM +0200, Alberto Garcia wrote:

+/* This function edits throttle_groups and must be called under the global
+ * mutex */
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
+{
+ThrottleGroup *tg = THROTTLE_GROUP(obj);
+ThrottleConfig cfg;
+
+/* set group name to object id if it exists */
+if (!tg->name && tg->parent_obj.parent) {
+tg->name = object_get_canonical_path_component(OBJECT(obj));
+}
+/* We must have a group name at this point */
+assert(tg->name);
+
+/* error if name is duplicate */
+if (throttle_group_exists(tg->name)) {
+error_setg(errp, "A group with this name already exists");
+return;
+}
+
+/* check validity */
+throttle_get_config(>ts, );
+if (!throttle_is_valid(, errp)) {
+return;
+}


My fault here because I suggested its removal, but I actually think we
still need to call throttle_config() here so bkt->max is initialized in
throttle_fix_bucket(). I'll take care of updating this call once my
patches to remove throttle_fix_bucket() are applied, but for now we
still need it.


Oh yes, that is the reason I originally had done it but forgot why.


signature.asc
Description: PGP signature

[Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

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.

Signed-off-by: Manos Pitsidianakis 
---
 qapi/block-core.json|  48 +
 include/block/throttle-groups.h |   3 +
 include/qemu/throttle-options.h |  59 --
 include/qemu/throttle.h |   3 +
 block/throttle-groups.c | 419 
 tests/test-throttle.c   |   1 +
 util/throttle.c | 151 +++
 7 files changed, 624 insertions(+), 60 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602150..0bdc69aa5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1905,6 +1905,54 @@
 '*iops_size': 'int', '*group': 'str' } }
 
 ##
+# @ThrottleLimits:
+#
+# Limit parameters for throttling.
+# Since some limit combinations are illegal, limits should always be set in one
+# transaction. All fields are optional. When setting limits, if a field is
+# missing the current value is not changed.
+#
+# @iops-total: limit total I/O operations per second
+# @iops-total-max: I/O operations burst
+# @iops-total-max-length:  length of the iops-total-max burst period, in 
seconds
+#  It must only be set if @iops-total-max is set as 
well.
+# @iops-read:  limit read operations per second
+# @iops-read-max:  I/O operations read burst
+# @iops-read-max-length:   length of the iops-read-max burst period, in seconds
+#  It must only be set if @iops-read-max is set as 
well.
+# @iops-write: limit write operations per second
+# @iops-write-max: I/O operations write burst
+# @iops-write-max-length:  length of the iops-write-max burst period, in 
seconds
+#  It must only be set if @iops-write-max is set as 
well.
+# @bps-total:  limit total bytes per second
+# @bps-total-max:  total bytes burst
+# @bps-total-max-length:   length of the bps-total-max burst period, in 
seconds.
+#  It must only be set if @bps-total-max is set as 
well.
+# @bps-read:   limit read bytes per second
+# @bps-read-max:   total bytes read burst
+# @bps-read-max-length:length of the bps-read-max burst period, in seconds
+#  It must only be set if @bps-read-max is set as well.
+# @bps-write:  limit write bytes per second
+# @bps-write-max:  total bytes write burst
+# @bps-write-max-length:   length of the bps-write-max burst period, in seconds
+#  It must only be set if @bps-write-max is set as 
well.
+# @iops-size:  when limiting by iops max size of an I/O in bytes
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleLimits',
+  'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
+'*iops-total-max-length' : 'int', '*iops-read' : 'int',
+'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
+'*iops-write' : 'int', '*iops-write-max' : 'int',
+'*iops-write-max-length' : 'int', '*bps-total' : 'int',
+'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
+'*bps-read' : 'int', '*bps-read-max' : 'int',
+'*bps-read-max-length' : 'int', '*bps-write' : 'int',
+'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
+'*iops-size' : 'int' } }
+
+##
 # @block-stream:
 #
 # Copy data from a backing file into a block device.
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index