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

Jeff Moyer
2010-01-04T14:58:24Z

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;