RE: [PATCH]cfq-iosched: don't stop async queue with async requestspending

[Li, Shaohua]

>-----Original Message-----
>From: Vivek Goyal [mailto:vgoyal@xxxxxxxxxx]
>Sent: Thursday, January 14, 2010 7:09 PM
>To: Li, Shaohua
>Cc: Gui Jianfeng; Corrado Zoccolo; jens.axboe@xxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; jmoyer@xxxxxxxxxx; yanmin_zhang@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH]cfq-iosched: don't stop async queue with async
>requests pending
>
>On Thu, Jan 14, 2010 at 02:17:31PM +0800, Shaohua Li wrote:
>> On Thu, Jan 14, 2010 at 01:27:21PM +0800, Gui Jianfeng wrote:
>> > Shaohua Li wrote:
>> > > On Wed, Jan 13, 2010 at 07:13:41PM +0800, Vivek Goyal wrote:
>> > >> On Wed, Jan 13, 2010 at 04:23:22PM +0800, Shaohua Li wrote:
>> > >>> On Wed, Jan 13, 2010 at 04:18:47PM +0800, Corrado Zoccolo wrote:
>> > >>>> On Wed, Jan 13, 2010 at 8:44 AM, Shaohua Li <shaohua.li@xxxxxxxxx>
>wrote:
>> > >>>>> My SSD speed of direct write is about 80m/s, while I test page
>writeback,
>> > >>>>> the speed can only go to 68m/s. Below patch fixes this.
>> > >>>>> It appears we missused cfq_should_idle in cfq_may_dispatch.
>cfq_should_idle
>> > >>>>> means a queue should idle because it's seekless sync queue or
>it's the last queue,
>> > >>>>> which is to maintain service tree time slice. So it doesn't mean
>the
>> > >>>>> last queue is always a sync queue. If the last queue is asyn
>queue,
>> > >>>>> we definitely shouldn't stop dispatch requests because of
>pending async
>> > >>>>> requests.
>> > >>>> An other option is that cfq_should_idle returns false for async
>> > >>>> queues, since cfq will never idle on them.
>> > >>> I'm considering this option too, but it appears we need make async
>queue
>> > >>> idle to maintain domain time slice.
>> > >> IMHO, we don't have to wait on async write service tree. Generally
>aysnc
>> > >> write queus contain many requests and they are not like reads where
>next
>> > >> request is expected. So idling on aysnc write service tree is waste
>of
>> > >> time and will lead to reduced throughput.
>> > > I fully agree async queue doesn't need wait. I thought the purpose
>we add the last
>> > > queue check in cfq_should_idle is we want a service tree or a group
>has dedicated
>> > > slice, because before the service tree/group slice is expired, new
>queue can jump
>> > > in and if we don't idle, the new queue can only run at next slice.
>Not sure if I
>> > > understand the code correctly.
>> >
>> > Hi Shaohua,
>> >
>> > If a cfq queue is the last one in the io group, if we expire this cfqq
>immediately,
>> > io group will be removed from service tree. When io group gets
>backlogged again, it
>> > will be put at the end of service tree, so it loses its previous share.
>so we add
>> > the last check here from the fairness point of view.
>> ya, this is what I'm understanding. So we can't return false for async
>queue
>> in cfq_should_idle if the queue is the last one of service tree.
>>
>
>Yes cfq_should_idle() can check for async queue and return false.
>
>Regarding group loosing fair share, currently all async queues are in root
>group and not in individual groups, so this particular change should not
>affect a lot. We will continue to idle on sync-idle and sync-noidle
>service tree. Only async service tree is the exception.
>
>Once we introduce per group async queue in future, we shall have to come
>up with something else, if need be.
>
>So keep this as a separate patch. I think in the presence of mixed
>workload, (readers and buffered writers), it might give little performance
>boost. We need to test it though.
Ok, if you thought this method doesn't break group, here is the updated
patch. I'm sorry to send the attached patch, my mailbox has trouble.

Attachment: cfq-async-idle.patch
Description: cfq-async-idle.patch