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

[Jeff Moyer]

Shaohua Li <shaohua.li@xxxxxxxxx> writes:

> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> >> >
>> >> > 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.

Sorry, I don't follow how you came to these conclusions.

cfq_close_cooperator checks whether cur_cfqq is seeky:

        if (CFQQ_SEEKY(cur_cfqq))
                return NULL;

So, by the time we get to the call to cfqq_close, we know that the
passed-in cfqq has a seek_mean within the CFQQ_SEEK_THR.

cfqq_close then calls cfq_rq_close with the non-seeky cfqq and the next
request of the queue it is checking:

        if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))

So, it seems to me that the checks you added are superfluous.  How did
you test this to ensure that your patch improved performance?  Your
statement about testing above seems to indicate that you did not see any
performance improvement.

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.

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/