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, &local_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