Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1527891

Re: [PATCH V4 02/15] blk-throttle: add .high interface

Show all headers | View raw

Hello, Shaohua.

Sorry about the delay.

On Mon, Nov 14, 2016 at 02:22:09PM -0800, Shaohua Li wrote:
> @@ -1376,11 +1414,37 @@ static ssize_t tg_set_max(struct kernfs_open_file *of,
>  			goto out_finish;
>  	}
>  
> -	tg->bps[READ][LIMIT_MAX] = v[0];
> -	tg->bps[WRITE][LIMIT_MAX] = v[1];
> -	tg->iops[READ][LIMIT_MAX] = v[2];
> -	tg->iops[WRITE][LIMIT_MAX] = v[3];
> -
> +	if (index == LIMIT_MAX) {
> +		if ((v[0] < tg->bps[READ][LIMIT_HIGH] &&
> +		       tg->bps[READ][LIMIT_HIGH] != -1) ||
> +		    (v[1] < tg->bps[WRITE][LIMIT_HIGH] &&
> +		       tg->bps[WRITE][LIMIT_HIGH] != -1) ||
> +		    (v[2] < tg->iops[READ][LIMIT_HIGH] &&
> +		       tg->iops[READ][LIMIT_HIGH] != -1) ||
> +		    (v[3] < tg->iops[WRITE][LIMIT_HIGH] &&
> +		       tg->iops[WRITE][LIMIT_HIGH] != -1)) {
> +			ret = -EINVAL;
> +			goto out_finish;

Is this necessary?  memcg doesn't put restrictions on input but just
enforces whatever is configured.  I think it'd be better to follow the
same model here too.  Hmm... is this because throtl will be able to
choose either all high or max limits per cgroup?

And this isn't from your patches but can we please switch to
UINT64_MAX instead of -1?

> +		}
> +	} else if (index == LIMIT_HIGH) {
> +		if ((v[0] > tg->bps[READ][LIMIT_MAX] && v[0] != -1) ||
> +		    (v[1] > tg->bps[WRITE][LIMIT_MAX] && v[1] != -1) ||
> +		    (v[2] > tg->iops[READ][LIMIT_MAX] && v[2] != -1) ||
> +		    (v[3] > tg->iops[WRITE][LIMIT_MAX] && v[3] != -1)) {

Ditto here.

> @@ -1412,6 +1484,7 @@ static struct blkcg_policy blkcg_policy_throtl = {
>  	.pd_alloc_fn		= throtl_pd_alloc,
>  	.pd_init_fn		= throtl_pd_init,
>  	.pd_online_fn		= throtl_pd_online,
> +	.pd_offline_fn		= throtl_pd_offline,
>  	.pd_free_fn		= throtl_pd_free,
>  };

I haven't read the whole thing yet but this looks a bit suspicious.  A
css going offline indicates that the destruction of the css started.
I don't get why that'd reset high limits.  There can be a lot of async
IOs after offline.

Thanks.

-- 
tejun

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH V4 00/15] blk-throttle: add .high limit Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
  [PATCH V4 09/15] blk-throttle: make bandwidth change smooth Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
    Re: [PATCH V4 09/15] blk-throttle: make bandwidth change smooth Tejun Heo <tj@kernel.org> - 2016-11-23 22:30 +0100
  [PATCH V4 11/15] blk-throttle: add interface to configure think time threshold Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
    Re: [PATCH V4 11/15] blk-throttle: add interface to configure think  time threshold Tejun Heo <tj@kernel.org> - 2016-11-23 22:40 +0100
  [PATCH V4 03/15] blk-throttle: configure bps/iops limit for cgroup in high limit Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
    Re: [PATCH V4 03/15] blk-throttle: configure bps/iops limit for  cgroup in high limit Tejun Heo <tj@kernel.org> - 2016-11-22 21:20 +0100
  [PATCH V4 01/15] blk-throttle: prepare support multiple limits Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
  [PATCH V4 15/15] blk-throttle: add latency target support Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
  [PATCH V4 13/15] blk-throttle: add a mechanism to estimate IO latency Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
  [PATCH V4 02/15] blk-throttle: add .high interface Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100
    Re: [PATCH V4 02/15] blk-throttle: add .high interface Tejun Heo <tj@kernel.org> - 2016-11-22 21:10 +0100
      Re: [PATCH V4 02/15] blk-throttle: add .high interface Tejun Heo <tj@kernel.org> - 2016-11-23 22:20 +0100
  [PATCH V4 08/15] blk-throttle: detect completed idle cgroup Shaohua Li <shli@fb.com> - 2016-11-14 23:30 +0100

csiph-web