Re: [Qemu-block] [PATCH v2 4/6] block: remove legacy I/O throttling

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:54 PM CEST, Manos Pitsidianakis wrote:
> +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error 
> **errp)
>  {
> +    ThrottleGroupMember *tgm;
> +
>      /* this BB is not part of any group */
> -    if (!blk->public.throttle_group_member.throttle_state) {
> +    if (!blk->public.throttle_node) {
>          return;
>      }
>  
> +    tgm = throttle_get_tgm(blk->public.throttle_node);
>      /* this BB is a part of the same group than the one we want */
> -    if 
> (!g_strcmp0(throttle_group_get_name(&blk->public.throttle_group_member),
> -                group)) {
> +    if (!g_strcmp0(throttle_group_get_name(tgm),
> +                   group)) {

You can join these two lines, no need to have 'group' in its own line.

> @@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>      BlockDriverState *bs;
>      BlockBackend *blk;
>      AioContext *aio_context;
> +    BlockDriverState *throttle_node = NULL;
> +    ThrottleGroupMember *tgm;
> +    Error *local_err = NULL;
>  
>      blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
>                        arg->has_id ? arg->id : NULL,
> @@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>      if (throttle_enabled(&cfg)) {
>          /* Enable I/O limits if they're not enabled yet, otherwise
>           * just update the throttling group. */
> -        if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
> -            blk_io_limits_enable(blk,
> -                                 arg->has_group ? arg->group :
> -                                 arg->has_device ? arg->device :
> -                                 arg->id);
> -        } else if (arg->has_group) {
> -            blk_io_limits_update_group(blk, arg->group);
> +        if (!blk_get_public(blk)->throttle_node) {
> +            blk_io_limits_enable(blk, arg->has_group ? arg->group :
> +                                 arg->has_device ? arg->device : arg->id,
> +                                 &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                goto out;
> +            }
>          }
> -        /* Set the new throttling configuration */
> -        blk_set_io_limits(blk, &cfg);
> -    } else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
> -        /* If all throttling settings are set to 0, disable I/O limits */
> +
> +        if (arg->has_group) {
> +            /* move throttle node membership to arg->group */
> +            blk_io_limits_update_group(blk, arg->group, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                goto out;
> +            }
> +        }

This used to be

   if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
      /* Enable throttling */
   } else if (arg->has_group) {
      /* Update group */
   }

but now it is

   if (!blk_get_throttle_node(blk)) {
      /* Enable throttling */
   }
   if (arg->has_group) {
      /* Update group */
   }

Now if I/O limits are not set then the code sets the same group twice.

Everything else looks fine.

Berto