Re: [Qemu-block] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list

[Alberto Garcia]

On Fri 25 Aug 2017 03:23:30 PM CEST, Manos Pitsidianakis wrote:
> @@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const 
> char *group,  Error **errp)
>  BlockDriverState *bs = blk_bs(blk), *throttle_node;
>  QDict *options = qdict_new();
>  Error *local_err = NULL;
> -ThrottleState *ts;
> -
> -bdrv_drained_begin(bs);
>  
>  /* Force creation of group in case it doesn't exist */
> -ts = throttle_group_incref(group);
> +if (!throttle_group_exists(group)) {
> +throttle_group_new_legacy(group, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}

After this, the reference count of the new group is 2 (as is already
explained in throttle_group_new_legacy()).

> +bdrv_drained_begin(bs);
> +
>  qdict_set_default_str(options, "file", bs->node_name);
>  qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
>  throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"), NULL,

And the implicit throttle node will hold and extra reference, making it
3.

How do you delete the legacy throttle group then? block_set_io_throttle
with everything set to 0 disables I/O limits and destroys the node, but
the reference count is still 2 and the group is not destroyed.

> +static int query_throttle_groups_foreach(Object *obj, void *data)
> +{
> +ThrottleGroup *tg;
> +struct ThrottleGroupQuery *query = data;
> +
> +tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
> +if (!tg) {
> +return 0;
> +}
> +
> +if (!g_strcmp0(query->name, tg->name)) {
> +query->result = tg;
> +return 1;
> +}
> +
> +return 0;
> +}

If you want you can make this a bit shorter if you merge both ifs:

   if (tg && !g_strcmp0(query->name, tg->name)) {
   query->result = tg;
   return 1;
   }

   return 0;

> +void throttle_group_new_legacy(const char *name, Error **errp)
> +{
> +ThrottleGroup *tg = NULL;

No need to initialize tg here.

>  ThrottleState *throttle_group_incref(const char *name)
>  {

I think that you can make this one static and remove it from
throttle-groups.h (no one else is using it). Same with
throttle_group_unref.

Berto

[Qemu-block] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list

[Manos Pitsidianakis]

block/throttle-groups.c uses a list to keep track of all groups and make
sure all names are unique.

This patch moves all ThrottleGroup objects under the root container.
While block/throttle.c requires that all throttle groups are created
with object-add or -object, the legacy throttling interface still used
the throttle_groups list. By using the root container we get the name
collision check for free and have all groups, legacy or not, available
through the qom-get/qom-set commands. Legacy groups are marked and freed
when they have no users left instead of waiting for an explicit
object-del.

Signed-off-by: Manos Pitsidianakis 
---
 include/block/throttle-groups.h |   1 +
 block/block-backend.c   |  15 +++--
 block/throttle-groups.c | 145 +++-
 tests/test-throttle.c   |   3 +
 4 files changed, 96 insertions(+), 68 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 8493540766..13fbc63f1e 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -58,6 +58,7 @@ typedef struct ThrottleGroupMember {
 
 const char *throttle_group_get_name(ThrottleGroupMember *tgm);
 
+void throttle_group_new_legacy(const char *name, Error **errp);
 ThrottleState *throttle_group_incref(const char *name);
 void throttle_group_unref(ThrottleState *ts);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index 693ad27fc9..65f458ce8f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const char 
*group,  Error **errp)
 BlockDriverState *bs = blk_bs(blk), *throttle_node;
 QDict *options = qdict_new();
 Error *local_err = NULL;
-ThrottleState *ts;
-
-bdrv_drained_begin(bs);
 
 /* Force creation of group in case it doesn't exist */
-ts = throttle_group_incref(group);
+if (!throttle_group_exists(group)) {
+throttle_group_new_legacy(group, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+}
+
+bdrv_drained_begin(bs);
+
 qdict_set_default_str(options, "file", bs->node_name);
 qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
 throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"), NULL,
@@ -1987,7 +1993,6 @@ void blk_io_limits_enable(BlockBackend *blk, const char 
*group,  Error **errp)
 assert(throttle_node->refcnt == 1);
 
 end:
-throttle_group_unref(ts);
 bdrv_drained_end(bs);
 blk_get_public(blk)->throttle_node = throttle_node;
 }
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 454bfcb067..238b648489 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -33,8 +33,10 @@
 #include "qapi-visit.h"
 #include "qom/object.h"
 #include "qom/object_interfaces.h"
+#include "qapi/qobject-input-visitor.h"
 
 static void throttle_group_obj_init(Object *obj);
+static bool throttle_group_can_be_deleted(UserCreatable *uc, Error **errp);
 static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
 
 /* The ThrottleGroup structure (with its ThrottleState) is shared
@@ -66,6 +68,7 @@ typedef struct ThrottleGroup {
 
 /* refuse individual property change if initialization is complete */
 bool is_initialized;
+bool legacy;
 char *name; /* This is constant during the lifetime of the group */
 
 QemuMutex lock; /* This lock protects the following four fields */
@@ -74,34 +77,47 @@ typedef struct ThrottleGroup {
 ThrottleGroupMember *tokens[2];
 bool any_timer_armed[2];
 QEMUClockType clock_type;
-
-/* This field is protected by the global QEMU mutex */
-QTAILQ_ENTRY(ThrottleGroup) list;
 } ThrottleGroup;
 
-/* This is protected by the global QEMU mutex */
-static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
-QTAILQ_HEAD_INITIALIZER(throttle_groups);
 
+struct ThrottleGroupQuery {
+const char *name;
+ThrottleGroup *result;
+};
 
-/* This function reads throttle_groups and must be called under the global
- * mutex.
+static int query_throttle_groups_foreach(Object *obj, void *data)
+{
+ThrottleGroup *tg;
+struct ThrottleGroupQuery *query = data;
+
+tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
+if (!tg) {
+return 0;
+}
+
+if (!g_strcmp0(query->name, tg->name)) {
+query->result = tg;
+return 1;
+}
+
+return 0;
+}
+
+
+/* This function reads the QOM root container and must be called under the
+ * global mutex.
  */
 static ThrottleGroup *throttle_group_by_name(const char *name)
 {
-ThrottleGroup *iter;
+struct ThrottleGroupQuery query = { name = name };
 
 /* Look for an existing group with that name */
-QTAILQ_FOREACH(iter, _groups, list) {
-if (!g_strcmp0(name, iter->name)) {
-return iter;
-}
-}
-
-return NULL;
+