RE: [PATCH]cfq-iosched: don't stop async queue with async requestspending
From: Li, Shaohua
Date: Mon Jan 18 2010 - 19:53:17 EST
>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
>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>
>> > >>>>> My SSD speed of direct write is about 80m/s, while I test page
>> > >>>>> the speed can only go to 68m/s. Below patch fixes this.
>> > >>>>> It appears we missused cfq_should_idle in cfq_may_dispatch.
>> > >>>>> 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
>> > >>>>> last queue is always a sync queue. If the last queue is asyn
>> > >>>>> we definitely shouldn't stop dispatch requests because of
>> > >>>>> 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
>> > >>> idle to maintain domain time slice.
>> > >> IMHO, we don't have to wait on async write service tree. Generally
>> > >> write queus contain many requests and they are not like reads where
>> > >> request is expected. So idling on aysnc write service tree is waste
>> > >> 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
>> > > 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
>> > 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
>> 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.