On Thu, Mar 04, 2010 at 09:34:51PM +0100, Corrado Zoccolo wrote:
you are right. The way cfq_should_idle() is written, it will return true
for bothe sync-idle and sync-noidle trees. With your patch two random
readers go onto different service trees and now we start driving queue
depth 1 because of follwing.
if (timer_pending(&cfqd->idle_slice_timer) ||
(cfqq->dispatched && cfq_should_idle(cfqd, cfqq))) {
cfqq = NULL;
goto keep_queue;
}
I tested following patch and now throughput is almost back at the same levels
as wihtout your patch.
---
block/cfq-iosched.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 40840da..296aa23 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1788,9 +1788,12 @@ static bool cfq_should_idle(struct cfq_data *cfqd, struct cfq_queue *cfqq)
if (prio == IDLE_WORKLOAD)
return false;
+ /* Don't idle on NCQ SSDs */
+ if (blk_queue_nonrot(cfqd->queue) && cfqd->hw_tag)
+ return false;
+
/* We do for queues that were marked with idle window flag. */
- if (cfq_cfqq_idle_window(cfqq) &&
- !(blk_queue_nonrot(cfqd->queue) && cfqd->hw_tag))
+ if (cfq_cfqq_idle_window(cfqq))
return true;
/*
--
1.6.2.5
Now I am wondering what are the side affects of above change.
One advantage of idling on service tree even on NCQ SSD is that READS get
more protection from WRITES. Especially if there are SSDs which implement
NCQ but really suck at WRITE speed or don't implement parallel IO
channels.
I see cfq_should_idle() being used at four places.
- cfq_select_queue()
if (cfqq->cfqg->nr_cfqq == 1 && RB_EMPTY_ROOT(&cfqq->sort_list)
&& cfqq->dispatched && cfq_should_idle(cfqd, cfqq)) {
cfqq = NULL;
goto keep_queue;
}
This is for fairness in group scheduling. I think that impact here should
not be much because this condition is primarily hit on media where we idle
on the queue.
On NCQ SSD, I don't think we get fairness in group scheduling much until
and unless a group is generating enough traffic to keep the request queue
full.
- cfq_select_queue()
(cfqq->dispatched && cfq_should_idle(cfqd, cfqq))) {
cfqq = NULL;
Should we really wait for a dispatched request to complete on NCQ SSD?
On SSD with parallel channel, I think this will reduce throughput?
- cfq_may_dispatch()
/*
* Drain async requests before we start sync IO
*/
if (cfq_should_idle(cfqd, cfqq) && cfqd->rq_in_driver[BLK_RW_ASYNC])
return false;
If NCQ SSD is implementing parallel IO channels, probably there is no need
to clear out WRITES before we start sync IO?
- cfq_arm_slice_timer()
if (!cfqd->cfq_slice_idle || !cfq_should_idle(cfqd, cfqq))
return;
cfq_arm_slice_timer() already check for NCQ SSD and does not arm timer
so this function should remain unaffected with this change.
So the question is, should we wait on service tree on an NCQ SSD or not?
Secondly, your patch of determining cfqq seeky nature based on request
size on SSD, helps only on non NCQ SSD.
Thanks
Vivek
--
