Re: [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD

[Corrado Zoccolo]

On Wed, Jul 7, 2010 at 5:46 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> 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.

Probably WRITE_SYNC are marked as RQ_NOIDLE because the application
can always send several write requests together (while for reads, you
usually need the result of one read to send the other). This means
that cfq will actually care only of the RQ_NOIDLE on the last request
in the queue (since, until the queue is empty, we don't even consider
idling).

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

I think this can be easily fixed by removing RQ_NOIDLE from those
requests on which Jeff wants to idle. Once no more requests can ever
be marked RQ_NOIDLE, then we can remove this code completely.

>
> 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, I've resent the patches for 2.6.36 (this version were based on 2.6.34).
Corrado
>
> 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@xxxxxxxxx>
>> ---
>> Â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
>



-- 
__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:czoccolo@xxxxxxxxx
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/