Re: [PATCH v2] cfq-iosched: fix the delay of cfq_group's vdisktime under iops mode

[Jan Kara <jack@suse.cz>]

On Wed 08-03-17 20:16:55, Hou Tao wrote:
> When adding a cfq_group into the cfq service tree, we use CFQ_IDLE_DELAY
> as the delay of cfq_group's vdisktime if there have been other cfq_groups
> already.
> 
> When cfq is under iops mode, commit 9a7f38c42c2b ("cfq-iosched: Convert
> from jiffies to nanoseconds") could result in a large iops delay and
> lead to an abnormal io schedule delay for the added cfq_group. To fix
> it, we just need to revert to the old CFQ_IDLE_DELAY value: HZ / 5
> when iops mode is enabled.
> 
> Despite having the same value, the delay of a cfq_queue in idle class
> and the delay of cfq_queue are different things, so I define two new
> macros for the delay of a cfq_group under time-slice mode and IOPs mode.
> 
> Fixes: 9a7f38c42c2b92391d9dabaf9f51df7cfe5608e4
> Cc: <stable@vger.kernel.org> # 4.8+
> Signed-off-by: Hou Tao <houtao1@huawei.com>

OK, the number 200 is somewhat arbitrary but so is HZ/5 so I guess we are
OK. You can add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/cfq-iosched.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> v2:
> - use constant instead of "nsecs_to_jiffies64(CFQ_IDLE_DELAY)"
>   as suggested by Jan Kara
> 
> v1:
> https://www.spinics.net/lists/stable/msg160580.html
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 440b95e..69754e8 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -38,9 +38,13 @@ static const u64 cfq_target_latency = (u64)NSEC_PER_SEC * 3/10; /* 300 ms */
>  static const int cfq_hist_divisor = 4;
>  
>  /*
> - * offset from end of service tree
> + * offset from end of queue service tree for idle class
>   */
>  #define CFQ_IDLE_DELAY		(NSEC_PER_SEC / 5)
> +/* offset from end of group service tree under time slice mode */
> +#define CFQ_SLICE_MODE_GROUP_DELAY (NSEC_PER_SEC / 5)
> +/* offset from end of group service under IOPS mode */
> +#define CFQ_IOPS_MODE_GROUP_DELAY (200)
>  
>  /*
>   * below this threshold, we consider thinktime immediate
> @@ -1362,6 +1366,14 @@ cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
>  	cfqg->vfraction = max_t(unsigned, vfr, 1);
>  }
>  
> +static inline u64 cfq_get_cfqg_vdisktime_delay(struct cfq_data *cfqd)
> +{
> +	if (!iops_mode(cfqd))
> +		return CFQ_SLICE_MODE_GROUP_DELAY;
> +	else
> +		return CFQ_IOPS_MODE_GROUP_DELAY;
> +}
> +
>  static void
>  cfq_group_notify_queue_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  {
> @@ -1381,7 +1393,8 @@ cfq_group_notify_queue_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  	n = rb_last(&st->rb);
>  	if (n) {
>  		__cfqg = rb_entry_cfqg(n);
> -		cfqg->vdisktime = __cfqg->vdisktime + CFQ_IDLE_DELAY;
> +		cfqg->vdisktime = __cfqg->vdisktime +
> +			cfq_get_cfqg_vdisktime_delay(cfqd);
>  	} else
>  		cfqg->vdisktime = st->min_vdisktime;
>  	cfq_group_service_tree_add(st, cfqg);
> -- 
> 2.5.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR