Re: [PATCH]cfq-iosched: don't take requests with long distence as close

[Jeff Moyer <jmoyer@redhat.com>]

Shaohua Li <shaohua.li@intel.com> writes:
> seek_mean could be very big sometimes, using it as close criteria is meaningless
> as this doen't improve any performance. So if it's big, let's fallback to
> default value.

I think the real problem is that we lost a check for a seeky queue.  The
close cooperator code initially was written to help interleaved
sequential I/Os.  Given that, I'm not a big fan of the proposed patch.
I'd rather see an extra check for CFQQ_SEEKY added to the close
cooperator code paths and leave cfq_rq_close alone.

Cheers,
Jeff

>
> Signed-off-by: Shaohua Li<shaohua.li@intel.com>
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..e80bd47 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1675,13 +1675,17 @@ static inline sector_t cfq_dist_from_last(struct cfq_data *cfqd,
>  #define CFQQ_SEEKY(cfqq)	((cfqq)->seek_mean > CFQQ_SEEK_THR)
>  
>  static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> -			       struct request *rq)
> +			       struct request *rq, bool for_preempt)
>  {
>  	sector_t sdist = cfqq->seek_mean;
>  
>  	if (!sample_valid(cfqq->seek_samples))
>  		sdist = CFQQ_SEEK_THR;
>  
> +	/* if seek_mean is big, using it as close criteria is meaningless */
> +	if (sdist > CFQQ_SEEK_THR && !for_preempt)
> +		sdist = CFQQ_SEEK_THR;
> +
>  	return cfq_dist_from_last(cfqd, rq) <= sdist;
>  }
>  
> @@ -1709,7 +1713,7 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
>  	 * will contain the closest sector.
>  	 */
>  	__cfqq = rb_entry(parent, struct cfq_queue, p_node);
> -	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
> +	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq, false))
>  		return __cfqq;
>  
>  	if (blk_rq_pos(__cfqq->next_rq) < sector)
> @@ -1720,7 +1724,7 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
>  		return NULL;
>  
>  	__cfqq = rb_entry(node, struct cfq_queue, p_node);
> -	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
> +	if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq, false))
>  		return __cfqq;
>  
>  	return NULL;
> @@ -3143,7 +3147,7 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
>  	 * if this request is as-good as one we would expect from the
>  	 * current cfqq, let it preempt
>  	 */
> -	if (cfq_rq_close(cfqd, cfqq, rq))
> +	if (cfq_rq_close(cfqd, cfqq, rq, true))
>  		return true;
>  
>  	return false;