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

[Shaohua Li <shaohua.li@intel.com>]

On Mon, Dec 28, 2009 at 05:11:25PM +0800, Corrado Zoccolo wrote:
> Hi Shaohua,
> On Mon, Dec 28, 2009 at 9:46 AM, Shaohua Li <shaohua.li@intel.com> wrote:
> > On Mon, Dec 28, 2009 at 04:36:39PM +0800, Corrado Zoccolo wrote:
> >> Hi Shaohua,
> >> On Mon, Dec 28, 2009 at 3:03 AM, Shaohua Li <shaohua.li@intel.com> wrote:
> >> > On Fri, Dec 25, 2009 at 06:16:27PM +0800, Corrado Zoccolo wrote:
> >> >> Hi Shaohua,
> >> >> On Thu, Dec 24, 2009 at 1:55 AM, Shaohua Li <shaohua.li@intel.com> wrote:
> >> >> > df5fe3e8e13883f58dc97489076bbcc150789a21
> >> >> > b3b6d0408c953524f979468562e7e210d8634150
> >> >> > The coop merge is too aggressive. For example, if two tasks are reading two
> >> >> > files where the two files have some adjecent blocks, cfq will immediately
> >> >> > merge them. cfq_rq_close() also has trouble, sometimes the seek_mean is very
> >> >> > big. I did a test to make cfq_rq_close() always checks the distence according
> >> >> > to CIC_SEEK_THR, but still saw a lot of wrong merge. (BTW, why we take a long
> >> >> > distence far away request as close. Taking them close doesn't improve any thoughtput
> >> >> > to me. Maybe we should always use CIC_SEEK_THR as close criteria).
> >> >> Yes, when deciding if two queues are going to be merged, we should use
> >> >> the constant CIC_SEEK_THR.
> >> >
> >> > seek_mean could be very big sometimes, using it as close criteria is meanless
> >> > as this doen't improve any performance. So if it's big, let's fallback to
> >> > default value.
> >>
> >> meanless -> meaningless (also in the comment within code)
> > oops
> >
> >> > Signed-off-by: Shaohua Li<shaohua.li@intel.com>
> >> >
> >> > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> > index e2f8046..8025605 100644
> >> > --- a/block/cfq-iosched.c
> >> > +++ b/block/cfq-iosched.c
> >> > @@ -1682,6 +1682,10 @@ static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> >> >        if (!sample_valid(cfqq->seek_samples))
> >> >                sdist = CFQQ_SEEK_THR;
> >> >
> >> > +       /* if seek_mean is big, using it as close criteria is meanless */
> >> > +       if (sdist > CFQQ_SEEK_THR)
> >> > +               sdist = CFQQ_SEEK_THR;
> >> > +
> >> >        return cfq_dist_from_last(cfqd, rq) <= sdist;
> >> >  }
> >> >
> >> >
> >> This changes also the cfq_should_preempt behaviour, where a large
> >> seek_mean could be meaningful, so I think it is better to add a
> >> boolean parameter to cfq_rq_close, to distinguish whether we are
> >> preempting or looking for queue merges, and make the new code
> >> conditional on merging.
> > can you explain why it's meaningful for cfq_should_preempt()? Unless sdist is
> > very big, for example > 10*seek_mean, the preempt seems not meaningless.
> 
> Disk access time is a complex function, but for rotational disks it is
> 'sort of' increasing with the amplitude of the seek. So, if you have a
> new request that is within the mean seek distance (even if it is
> larger than our constant), it is good to chose this request instead of
> waiting for an other one from the active queue (this behaviour is the
> same exhibited by AS, so we need a good reason to change).
I have no good reason, but just thought it's a little strange.
If other ioscheds take the same way, let's stick.

 
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.

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;