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

["Zhang, Yanmin" <yanmin_zhang@linux.intel.com>]

On Mon, 2010-01-11 at 16:05 +0100, Corrado Zoccolo wrote: 
> On Mon, Jan 11, 2010 at 6:20 AM, Zhang, Yanmin
> <yanmin_zhang@linux.intel.com> wrote:
> > On Thu, 2010-01-07 at 09:30 -0500, Jeff Moyer wrote:
> >> Corrado Zoccolo <czoccolo@gmail.com> writes:
> >>
> >> > On Tue, Jan 5, 2010 at 10:16 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
> >> >>
> >> >> For now, I'm leaning towards asking Jens to revert this.  It may still
> >> >> be worth making sure that we don't merge a seeky queue with a non-seeky
> >> >> queue.  I have a patch for that if folks are interested.
> >> > Jeff, can you send this patch to Yanmin, that is investigating a
> >> > regression apparently caused by excessive queue merge?
> >> >
> >> > http://lkml.org/lkml/2010/1/4/194
> >>
> >>
> >> You first have to back out Shaohua's patch, then apply this one.
> > Thanks for forwarding me the patches.
> > Actually, we found tiobench randread has about 20% regression since kernel
> > 2.6.33-rc1, and fio randread has more than 40% regression.
> >
> >>
> >> Cheers,
> >> Jeff
> >>
> >> cfq-iosched: don't allow merging with seeky queues
> > With your new patch applied on 2.6.33-rc1, I don't see improvement on
> > both tiobench and fio randread regression. I know unexpected merge/unmerge
> > is just one root cause of the regressions. A couple of other patches
> > are also related to them.
> 
> Hi Yanmin,
> are you testing Jeff's patch with your full fio script, instead of the
> simplified one?
Thanks for your reminder. I tested the patch with simplified one.

> Since they are fixing the merging part, that happens only with the
> full fio script.
Ok. I tested the full fio script a moment ago and didn't find improvement.

> 
> >
> > I also tried to apply both your patch and Corrado's patch at
> > http://lkml.org/lkml/2010/1/9/57. The result seems like the one when I just apply
> > Corrado's patch, with which regression almost disappears when low_latency=0. But
> > when low_latency=1, there is still about 25% regression.
> >
> Yes, low_latency is supposed to bring some regression.
> http://lkml.org/lkml/2009/12/30/47
> 
> Thanks,
> Corrado
> >
> >>
> >> Shaohua Li noticed that cfq currently can merge with seeky queues, which
> >> causes unwanted merge/unmerge activity.  We already know that the
> >> cur_cfqq is not seeky, so this patch just makes sure that the non-seeky
> >> queue is not merged with a seeky one.
> >>
> >> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> >>
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index 8df4fe5..3db9050 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -1677,6 +1677,10 @@ static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> >>       return cfq_dist_from_last(cfqd, rq) <= sdist;
> >>  }
> >>
> >> +/*
> >> + * Search for a cfqq that is issuing non-seeky I/Os within the seek
> >> + * mean of the current cfqq.
> >> + */
> >>  static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
> >>                                   struct cfq_queue *cur_cfqq)
> >>  {
> >> @@ -1701,7 +1705,14 @@ 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 the cfqq does not have enough seek samples, assume it is
> >> +      * sequential until proven otherwise.  If it is assumed that the
> >> +      * queue is seeky first, then the close cooperator detection logic
> >> +      * may never trigger as one queue strays further from the other(s).
> >> +      */
> >> +     if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq) &&
> >> +         (!sample_valid(__cfqq->seek_samples) || !CFQQ_SEEKY(__cfqq)))
> >>               return __cfqq;
> >>
> >>       if (blk_rq_pos(__cfqq->next_rq) < sector)
> >> @@ -1712,7 +1723,8 @@ 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) &&
> >> +         (!sample_valid(__cfqq->seek_samples) || !CFQQ_SEEKY(__cfqq)))
> >>               return __cfqq;
> >>
> >>       return NULL;