[PATCH]cfq-iosched: don't stop async queue with async requests pending

[PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

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.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 918c7fd..8198079 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	/*
 	 * Drain async requests before we start sync IO
 	 */
-	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
+	if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
+		&& cfqd->rq_in_driver[BLK_RW_ASYNC])
 		return false;
 
 	/*

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Corrado Zoccolo]

On Wed, Jan 13, 2010 at 8:44 AM, Shaohua Li <shaohua.li@intel.com> 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.

Corrado
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 918c7fd..8198079 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>        /*
>         * Drain async requests before we start sync IO
>         */
> -       if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> +       if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
> +               && cfqd->rq_in_driver[BLK_RW_ASYNC])
>                return false;
>
>        /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:czoccolo@gmail.com
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

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@intel.com> 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.

Thanks,
Shaohua

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Vivek Goyal]

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@intel.com> 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.

Vivek

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

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@intel.com> 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.
you are the expert of iogroup. If I'm wrong, I'll be happy to change
cfq_should_idle(), which is the ideal place to be changed at my first glance.

Thanks,
Shaohua

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Gui Jianfeng]

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@intel.com> 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.

Thanks,
Gui

> you are the expert of iogroup. If I'm wrong, I'll be happy to change
> cfq_should_idle(), which is the ideal place to be changed at my first glance.
> 
> Thanks,
> Shaohua

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

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@intel.com> 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.

Thanks,
Shaohua

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Gui Jianfeng]

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@intel.com> 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.

I see your point, whether can we add the extra sync queue check into cfq_should_idle
for common use?

Thanks,
Gui

> 
> Thanks,
> Shaohua
> 
> 
> 

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

On Thu, Jan 14, 2010 at 04:21:52PM +0800, Gui Jianfeng wrote:
> 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@intel.com> 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.
> 
> I see your point, whether can we add the extra sync queue check into cfq_should_idle
> for common use?
yes.

Thanks,
Shaohua

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Vivek Goyal]

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@intel.com> 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.

Thanks
Vivek

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

[Li, Shaohua]

[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]



>-----Original Message-----
>From: Vivek Goyal [mailto:vgoyal@redhat.com]
>Sent: Thursday, January 14, 2010 7:09 PM
>To: Li, Shaohua
>Cc: Gui Jianfeng; Corrado Zoccolo; jens.axboe@oracle.com; linux-
>kernel@vger.kernel.org; jmoyer@redhat.com; yanmin_zhang@linux.intel.com
>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@intel.com>
>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 #2: cfq-async-idle.patch --]
[-- Type: application/octet-stream, Size: 1215 bytes --]

From: Shaohua Li <shaohua.li@intel.com>
Title: cfq-iosched: don't stop async queue with async requests pending

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.
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. As currently async queues are all in root group, no idle for async
queue doesn't lose group slice fairness. If the last queue is asyn queue,
we definitely shouldn't stop dispatch requests because of pending async
requests, which will make cfq_may_dispatch able to dispatch more requests.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 918c7fd..ce608a0 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1803,7 +1803,7 @@ static bool cfq_should_idle(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	 * Otherwise, we do only if they are the last ones
 	 * in their service tree.
 	 */
-	return service_tree->count == 1;
+	return service_tree->count == 1 && cfqq_type(cfqq) != ASYNC_WORKLOAD;
 }
 
 static void cfq_arm_slice_timer(struct cfq_data *cfqd)

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Vivek Goyal]

On Tue, Jan 19, 2010 at 08:52:01AM +0800, Li, Shaohua wrote:

[..]
> >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.

Hi Shaohua,

I did some testing on cfq group functionality and I did not see any
significant impact of this patch.

I am yet to write some test cases for mixed workload testing and see the
impact of this patch. Will get back to you soon.

Thanks
Vivek

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Vivek Goyal]

On Wed, Jan 13, 2010 at 03:44:42PM +0800, Shaohua Li 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.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 918c7fd..8198079 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  	/*
>  	 * Drain async requests before we start sync IO
>  	 */
> -	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> +	if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
> +		&& cfqd->rq_in_driver[BLK_RW_ASYNC])
>  		return false;

So are we driving queue depth as 1 when pure buffered writes are going on?
Because in that case service_tree->count=1 and cfq_should_idle() will
return 1 and looks like we will not dispatch next write till previous
write is over?

A general question. Why do we need to drain async requests before we start
sync IO? How does that help?

A related question, even if we have to do that, why do we check for
cfq_should_idle()? Why can't we just do following.

	if (cfq_cfqq_sync(cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])

Thanks
Vivek

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Corrado Zoccolo]

On Wed, Jan 13, 2010 at 12:10 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jan 13, 2010 at 03:44:42PM +0800, Shaohua Li 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.
>>
>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>>
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index 918c7fd..8198079 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>>       /*
>>        * Drain async requests before we start sync IO
>>        */
>> -     if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
>> +     if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
>> +             && cfqd->rq_in_driver[BLK_RW_ASYNC])
>>               return false;
>
> So are we driving queue depth as 1 when pure buffered writes are going on?
> Because in that case service_tree->count=1 and cfq_should_idle() will
> return 1 and looks like we will not dispatch next write till previous
> write is over?

Yes, it seems so. It has to be fixed.

>
> A general question. Why do we need to drain async requests before we start
> sync IO? How does that help?
>
> A related question, even if we have to do that, why do we check for
> cfq_should_idle()? Why can't we just do following.
>
>        if (cfq_cfqq_sync(cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
>
This would wait also for seeky queues. I think we drain to avoid
disrupting a sync
stream with far writes, but it is not needed when the queues are already seeky.

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

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Vivek Goyal]

On Wed, Jan 13, 2010 at 10:30:52PM +0100, Corrado Zoccolo wrote:
> On Wed, Jan 13, 2010 at 12:10 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Jan 13, 2010 at 03:44:42PM +0800, Shaohua Li 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.
> >>
> >> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> >>
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index 918c7fd..8198079 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> >>       /*
> >>        * Drain async requests before we start sync IO
> >>        */
> >> -     if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> >> +     if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
> >> +             && cfqd->rq_in_driver[BLK_RW_ASYNC])
> >>               return false;
> >
> > So are we driving queue depth as 1 when pure buffered writes are going on?
> > Because in that case service_tree->count=1 and cfq_should_idle() will
> > return 1 and looks like we will not dispatch next write till previous
> > write is over?
> 
> Yes, it seems so. It has to be fixed.
> 
> >
> > A general question. Why do we need to drain async requests before we start
> > sync IO? How does that help?
> >
> > A related question, even if we have to do that, why do we check for
> > cfq_should_idle()? Why can't we just do following.
> >
> >        if (cfq_cfqq_sync(cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> >
> This would wait also for seeky queues. I think we drain to avoid
> disrupting a sync
> stream with far writes, but it is not needed when the queues are already seeky.

Ok, that makes sense. But again the conflict is single disk vs RAID. RAID
array will most likely be doing some write caching or writes and reads
might be travelling to different disks hence sequential reads should not
be significantly impacted. Another case to optimize if we can reliably
identify RAIDs.

Thanks
Vivek

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Gui Jianfeng]

Shaohua Li 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.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 918c7fd..8198079 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  	/*
>  	 * Drain async requests before we start sync IO
>  	 */
> -	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> +	if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
> +		&& cfqd->rq_in_driver[BLK_RW_ASYNC])
>  		return false;

It seems only sync queue could open the idle window, so i don't think we really need this.

Thanks,
Gui

Re: [PATCH]cfq-iosched: don't stop async queue with async requests pending

[Shaohua Li]

On Thu, Jan 14, 2010 at 10:46:40AM +0800, Gui Jianfeng wrote:
> Shaohua Li 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.
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > 
> > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> > index 918c7fd..8198079 100644
> > --- a/block/cfq-iosched.c
> > +++ b/block/cfq-iosched.c
> > @@ -2222,7 +2222,8 @@ static bool cfq_may_dispatch(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> >  	/*
> >  	 * Drain async requests before we start sync IO
> >  	 */
> > -	if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
> > +	if (cfq_cfqq_sync(cfqq) && cfq_should_idle(cfqd, cfqq)
> > +		&& cfqd->rq_in_driver[BLK_RW_ASYNC])
> >  		return false;
> 
> It seems only sync queue could open the idle window, so i don't think we really need this.
the purpose here isn't to open idle window. with it, we could send more requests instead
of wait previous request finish. Vivek's mail explains the behavior correctly.

Thanks,
Shaohua