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

Li, Shaohua
2010-01-06T01:19:43Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5296 bytes --]

>-----Original Message-----
>From: Jeff Moyer [mailto:jmoyer@redhat.com]
>Sent: Wednesday, January 06, 2010 5:16 AM
>To: Li, Shaohua
>Cc: Corrado Zoccolo; linux-kernel@vger.kernel.org; jens.axboe@oracle.com;
>Zhang, Yanmin
>Subject: Re: [PATCH]cfq-iosched: don't take requests with long distence as
>close
>
>Shaohua Li <shaohua.li@intel.com> 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@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.
>
>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.
Ha, yes, you are right. I did see some improvement on tiobench test,
but maybe it's from another patch (the split patch) or I checked an old
code. So please revert the patch.

Thanks,
Shaohua
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éÝ¶\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dÊ‡Ú™ë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥