Messages in this thread |  | | | Date | Wed, 7 Jul 2010 11:46:31 -0400 | | From | Vivek Goyal <> | | Subject | Re: [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD |
| |
On Wed, Jul 07, 2010 at 05:23:47PM +0200, Corrado Zoccolo wrote: > RQ_NOIDLE flag is meaningful and should be honored for SYNC_WORKLOAD, > without further checks. > RQ_NOIDLE can be used to mark the last request of a sequence for which > - we want to idle between the requests of the sequence, to keep locality > - we don't want to idle after the sequence, because we know that no new > nearby requests will follow, so we should switch servicing other > queues.
Corrado, in higher layers any WRITE_SYNC request currently is marked as RQ_NOIDLE. At that point it is just not known whether there will be another request after this or not. So I would not think of RQ_NOIDLE as being conclusively telling us that this is last request in the sequence.
I think requst being WRITE_SYNC, we just don't know if the application is going to write more or not immediately. fsync, O_SYNC etc fall in this category.
But in general I like the idea of getting rid of idling on as many cases as possiblle. Jeff's recent posting to fix fsync issue depends on idling even on WRITE_SYNC queues so your patch and his patchsets are fundamentally incompatible.
Whether to idle on WRITE_SYNC or not, I will leave it to Jens (I just don't know the answer to that question. :-)). But in general I want to get rid of idling as much as possible otherwise it becomes a serious bottleneck in any kind of performance testing on higher end storage.
At the same time not idling runs the risk of process doing WRITE_SYNC not getting fair share in presence of sequential readers if writer does not keep the queue busy.
I will do some testing with this patchset little later.
Thanks Vivek
> This patch fixes this behaviour, making it similar to how it behaved > before 8e55063, but still fixing the corner cases that were the > motivation for it. > > Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com> > --- > block/cfq-iosched.c | 15 ++++++++++----- > 1 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c > index 5ef9a5d..cac3afb 100644 > --- a/block/cfq-iosched.c > +++ b/block/cfq-iosched.c > @@ -3356,12 +3356,17 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq) > cfqd->noidle_tree_requires_idle |= bitmask; > > /* > - * Idling is enabled for SYNC_WORKLOAD. > - * SYNC_NOIDLE_WORKLOAD idles at the end of the tree > - * only if we processed at least one !rq_noidle request > + * Idling is enabled for: > + * - the last sync queue of a group > + * - SYNC_WORKLOAD queues, for !rq_noidle requests > + * - SYNC_NOIDLE_WORKLOAD "at the end of the tree" > + * if at least one queue sent !rq_noidle requests > + * not followed by at least one rq_noidle request. > */ > - if (cfqd->serving_type == SYNC_WORKLOAD > - || cfqd->noidle_tree_requires_idle > + if ((cfqd->serving_type == SYNC_WORKLOAD > + && !rq_noidle(rq)) > + || (cfqd->serving_type == SYNC_NOIDLE_WORKLOAD > + && cfqd->noidle_tree_requires_idle) > || cfqq->cfqg->nr_cfqq == 1) > cfq_arm_slice_timer(cfqd); > } > -- > 1.6.4.4
|  |