Re: [PATCH v3 RESEND 1/2] blk-throttle: track read and write request individually

[Joseph Qi <jiangqi903@gmail.com>]

Hi Jens,
Could you please give your advice for the two patches or pick them up if
you think they are good?

Thanks,
Joseph

On 17/11/21 09:38, Joseph Qi wrote:
> From: Joseph Qi <qijiang.qj@alibaba-inc.com>
> 
> In mixed read/write workload on SSD, write latency is much lower than
> read. But now we only track and record read latency and then use it as
> threshold base for both read and write io latency accounting. As a
> result, write io latency will always be considered as good and
> bad_bio_cnt is much smaller than 20% of bio_cnt. That is to mean, the
> tg to be checked will be treated as idle most of the time and still let
> others dispatch more ios, even it is truly running under low limit and
> wants its low limit to be guaranteed, which is not we expected in fact.
> So track read and write request individually, which can bring more
> precise latency control for low limit idle detection.
> 
> Signed-off-by: Joseph Qi <qijiang.qj@alibaba-inc.com>
> Reviewed-by: Shaohua Li <shli@fb.com>
> ---
>  block/blk-throttle.c | 134 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 79 insertions(+), 55 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 96ad326..bf52035 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -216,9 +216,9 @@ struct throtl_data
>  
>  	unsigned int scale;
>  
> -	struct latency_bucket tmp_buckets[LATENCY_BUCKET_SIZE];
> -	struct avg_latency_bucket avg_buckets[LATENCY_BUCKET_SIZE];
> -	struct latency_bucket __percpu *latency_buckets;
> +	struct latency_bucket tmp_buckets[2][LATENCY_BUCKET_SIZE];
> +	struct avg_latency_bucket avg_buckets[2][LATENCY_BUCKET_SIZE];
> +	struct latency_bucket __percpu *latency_buckets[2];
>  	unsigned long last_calculate_time;
>  	unsigned long filtered_latency;
>  
> @@ -2041,10 +2041,10 @@ static void blk_throtl_update_idletime(struct throtl_grp *tg)
>  #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>  static void throtl_update_latency_buckets(struct throtl_data *td)
>  {
> -	struct avg_latency_bucket avg_latency[LATENCY_BUCKET_SIZE];
> -	int i, cpu;
> -	unsigned long last_latency = 0;
> -	unsigned long latency;
> +	struct avg_latency_bucket avg_latency[2][LATENCY_BUCKET_SIZE];
> +	int i, cpu, rw;
> +	unsigned long last_latency[2] = { 0 };
> +	unsigned long latency[2];
>  
>  	if (!blk_queue_nonrot(td->queue))
>  		return;
> @@ -2053,56 +2053,67 @@ static void throtl_update_latency_buckets(struct throtl_data *td)
>  	td->last_calculate_time = jiffies;
>  
>  	memset(avg_latency, 0, sizeof(avg_latency));
> -	for (i = 0; i < LATENCY_BUCKET_SIZE; i++) {
> -		struct latency_bucket *tmp = &td->tmp_buckets[i];
> -
> -		for_each_possible_cpu(cpu) {
> -			struct latency_bucket *bucket;
> -
> -			/* this isn't race free, but ok in practice */
> -			bucket = per_cpu_ptr(td->latency_buckets, cpu);
> -			tmp->total_latency += bucket[i].total_latency;
> -			tmp->samples += bucket[i].samples;
> -			bucket[i].total_latency = 0;
> -			bucket[i].samples = 0;
> -		}
> +	for (rw = READ; rw <= WRITE; rw++) {
> +		for (i = 0; i < LATENCY_BUCKET_SIZE; i++) {
> +			struct latency_bucket *tmp = &td->tmp_buckets[rw][i];
> +
> +			for_each_possible_cpu(cpu) {
> +				struct latency_bucket *bucket;
> +
> +				/* this isn't race free, but ok in practice */
> +				bucket = per_cpu_ptr(td->latency_buckets[rw],
> +					cpu);
> +				tmp->total_latency += bucket[i].total_latency;
> +				tmp->samples += bucket[i].samples;
> +				bucket[i].total_latency = 0;
> +				bucket[i].samples = 0;
> +			}
>  
> -		if (tmp->samples >= 32) {
> -			int samples = tmp->samples;
> +			if (tmp->samples >= 32) {
> +				int samples = tmp->samples;
>  
> -			latency = tmp->total_latency;
> +				latency[rw] = tmp->total_latency;
>  
> -			tmp->total_latency = 0;
> -			tmp->samples = 0;
> -			latency /= samples;
> -			if (latency == 0)
> -				continue;
> -			avg_latency[i].latency = latency;
> +				tmp->total_latency = 0;
> +				tmp->samples = 0;
> +				latency[rw] /= samples;
> +				if (latency[rw] == 0)
> +					continue;
> +				avg_latency[rw][i].latency = latency[rw];
> +			}
>  		}
>  	}
>  
> -	for (i = 0; i < LATENCY_BUCKET_SIZE; i++) {
> -		if (!avg_latency[i].latency) {
> -			if (td->avg_buckets[i].latency < last_latency)
> -				td->avg_buckets[i].latency = last_latency;
> -			continue;
> -		}
> +	for (rw = READ; rw <= WRITE; rw++) {
> +		for (i = 0; i < LATENCY_BUCKET_SIZE; i++) {
> +			if (!avg_latency[rw][i].latency) {
> +				if (td->avg_buckets[rw][i].latency < last_latency[rw])
> +					td->avg_buckets[rw][i].latency =
> +						last_latency[rw];
> +				continue;
> +			}
>  
> -		if (!td->avg_buckets[i].valid)
> -			latency = avg_latency[i].latency;
> -		else
> -			latency = (td->avg_buckets[i].latency * 7 +
> -				avg_latency[i].latency) >> 3;
> +			if (!td->avg_buckets[rw][i].valid)
> +				latency[rw] = avg_latency[rw][i].latency;
> +			else
> +				latency[rw] = (td->avg_buckets[rw][i].latency * 7 +
> +					avg_latency[rw][i].latency) >> 3;
>  
> -		td->avg_buckets[i].latency = max(latency, last_latency);
> -		td->avg_buckets[i].valid = true;
> -		last_latency = td->avg_buckets[i].latency;
> +			td->avg_buckets[rw][i].latency = max(latency[rw],
> +				last_latency[rw]);
> +			td->avg_buckets[rw][i].valid = true;
> +			last_latency[rw] = td->avg_buckets[rw][i].latency;
> +		}
>  	}
>  
>  	for (i = 0; i < LATENCY_BUCKET_SIZE; i++)
>  		throtl_log(&td->service_queue,
> -			"Latency bucket %d: latency=%ld, valid=%d", i,
> -			td->avg_buckets[i].latency, td->avg_buckets[i].valid);
> +			"Latency bucket %d: read latency=%ld, read valid=%d, "
> +			"write latency=%ld, write valid=%d", i,
> +			td->avg_buckets[READ][i].latency,
> +			td->avg_buckets[READ][i].valid,
> +			td->avg_buckets[WRITE][i].latency,
> +			td->avg_buckets[WRITE][i].valid);
>  }
>  #else
>  static inline void throtl_update_latency_buckets(struct throtl_data *td)
> @@ -2249,16 +2260,17 @@ static void throtl_track_latency(struct throtl_data *td, sector_t size,
>  	struct latency_bucket *latency;
>  	int index;
>  
> -	if (!td || td->limit_index != LIMIT_LOW || op != REQ_OP_READ ||
> +	if (!td || td->limit_index != LIMIT_LOW ||
> +	    !(op == REQ_OP_READ || op == REQ_OP_WRITE) ||
>  	    !blk_queue_nonrot(td->queue))
>  		return;
>  
>  	index = request_bucket_index(size);
>  
> -	latency = get_cpu_ptr(td->latency_buckets);
> +	latency = get_cpu_ptr(td->latency_buckets[op]);
>  	latency[index].total_latency += time;
>  	latency[index].samples++;
> -	put_cpu_ptr(td->latency_buckets);
> +	put_cpu_ptr(td->latency_buckets[op]);
>  }
>  
>  void blk_throtl_stat_add(struct request *rq, u64 time_ns)
> @@ -2277,6 +2289,7 @@ void blk_throtl_bio_endio(struct bio *bio)
>  	unsigned long finish_time;
>  	unsigned long start_time;
>  	unsigned long lat;
> +	int rw = bio_data_dir(bio);
>  
>  	tg = bio->bi_cg_private;
>  	if (!tg)
> @@ -2305,7 +2318,7 @@ void blk_throtl_bio_endio(struct bio *bio)
>  
>  		bucket = request_bucket_index(
>  			blk_stat_size(&bio->bi_issue_stat));
> -		threshold = tg->td->avg_buckets[bucket].latency +
> +		threshold = tg->td->avg_buckets[rw][bucket].latency +
>  			tg->latency_target;
>  		if (lat > threshold)
>  			tg->bad_bio_cnt++;
> @@ -2398,9 +2411,16 @@ int blk_throtl_init(struct request_queue *q)
>  	td = kzalloc_node(sizeof(*td), GFP_KERNEL, q->node);
>  	if (!td)
>  		return -ENOMEM;
> -	td->latency_buckets = __alloc_percpu(sizeof(struct latency_bucket) *
> +	td->latency_buckets[READ] = __alloc_percpu(sizeof(struct latency_bucket) *
> +		LATENCY_BUCKET_SIZE, __alignof__(u64));
> +	if (!td->latency_buckets[READ]) {
> +		kfree(td);
> +		return -ENOMEM;
> +	}
> +	td->latency_buckets[WRITE] = __alloc_percpu(sizeof(struct latency_bucket) *
>  		LATENCY_BUCKET_SIZE, __alignof__(u64));
> -	if (!td->latency_buckets) {
> +	if (!td->latency_buckets[WRITE]) {
> +		free_percpu(td->latency_buckets[READ]);
>  		kfree(td);
>  		return -ENOMEM;
>  	}
> @@ -2419,7 +2439,8 @@ int blk_throtl_init(struct request_queue *q)
>  	/* activate policy */
>  	ret = blkcg_activate_policy(q, &blkcg_policy_throtl);
>  	if (ret) {
> -		free_percpu(td->latency_buckets);
> +		free_percpu(td->latency_buckets[READ]);
> +		free_percpu(td->latency_buckets[WRITE]);
>  		kfree(td);
>  	}
>  	return ret;
> @@ -2430,7 +2451,8 @@ void blk_throtl_exit(struct request_queue *q)
>  	BUG_ON(!q->td);
>  	throtl_shutdown_wq(q);
>  	blkcg_deactivate_policy(q, &blkcg_policy_throtl);
> -	free_percpu(q->td->latency_buckets);
> +	free_percpu(q->td->latency_buckets[READ]);
> +	free_percpu(q->td->latency_buckets[WRITE]);
>  	kfree(q->td);
>  }
>  
> @@ -2448,8 +2470,10 @@ void blk_throtl_register_queue(struct request_queue *q)
>  	} else {
>  		td->throtl_slice = DFL_THROTL_SLICE_HD;
>  		td->filtered_latency = LATENCY_FILTERED_HD;
> -		for (i = 0; i < LATENCY_BUCKET_SIZE; i++)
> -			td->avg_buckets[i].latency = DFL_HD_BASELINE_LATENCY;
> +		for (i = 0; i < LATENCY_BUCKET_SIZE; i++) {
> +			td->avg_buckets[READ][i].latency = DFL_HD_BASELINE_LATENCY;
> +			td->avg_buckets[WRITE][i].latency = DFL_HD_BASELINE_LATENCY;
> +		}
>  	}
>  #ifndef CONFIG_BLK_DEV_THROTTLING_LOW
>  	/* if no low limit, use previous default */
>