On Tue, Jun 29, 2010 at 2:30 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Tue, Jun 29, 2010 at 11:06:19AM +0200, Corrado Zoccolo wrote: > > [..] >> > I'm now testing OCFS2, and I'm seeing performance that is not great >> > (even with the blk_yield patches applied). What happens is that we >> > successfully yield the queue to the journal thread, but then idle on the >> > journal thread (even though RQ_NOIDLE was set). >> > >> > So, can we just get rid of idling when RQ_NOIDLE is set? >> Hi Jeff, >> I think I spotted a problem with the initial implementation of the >> tree-wide idle when RQ_NOIDLE is set: I assumed that a queue would >> either send possibly-idling requests or no-idle requests, but it seems >> that RQ_NOIDLE is being used to mark the end of a stream of >> possibly-idling requests (in my initial implementation, this will then >> cause an unintended idle). The attached patch should fix it, and I >> think the logic is simpler than Vivek's. Can you give it a spin? >> Otherwise, I think that reverting the "noidle_tree_requires_idle" >> behaviour completely may be better than adding complexity, since it is >> really trying to solve corner cases (that maybe happen only on >> synthetic workloads), but affecting negatively more common cases. >> > > Hi Corrado, > > I think you forgot to attach the patch? Can't find it. No, it's there: http://linux.derkeiler.com/Mailing-Lists/Kernel/2010-06/msg10854.html > > >> About what it is trying to solve, since I think it was not clear: >> - we have a workload of 2 queues, both issuing requests that are being >> put in the no-idle tree (e.g. they are random) + 1 queue issuing >> idling requests (e.g. sequential). >> - if one of the 2 "random" queues marks its requests as RQ_NOIDLE, >> then the timeslice for the no-idle tree is not preserved, causing >> unfairness, as soon as an RQ_NOIDLE request is serviced and the tree >> is empty. > > I think Jeff's primary regressions were coming from the fact that we > will continue to idle on SYNC_WORKLOAD even if RQ_NOIDLE() was set. I see. There are probably two ways of handling this: - make sure the queues sending RQ_NOIDLE requests are marked as SYNC_NOIDLE_WORKLOAD. Something like this should work, even if it will increase switching between trees: @@ -3046,6 +3046,7 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq, cfq_mark_cfqq_deep(cfqq); if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle || + (cfqq->next_rq && rq_noidle(cfqq->next_rq)) || (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq))) enable_idle = 0; else if (sample_valid(cic->ttime_samples)) { - or we could explicitly check the rq_noidle() in cfq_completed_request for SYNC_WORKLOAD: @@ -3360,8 +3360,10 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq) * SYNC_NOIDLE_WORKLOAD idles at the end of the tree * only if we processed 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); } > Regarding giving up idling on sync-noidle workload, I think it still > makes some sense to keep track if some other random queue is doing IO on > that tree or not and if yes, then continue to idle. That's a different > thing that current logic if more coarse and could be fine grained a bit. The patch in previous message should fix this. > > Because I don't have a practical workload example at this point of time, I > also don't mind reverting your old patch and restoring the policy of not > idling if RQ_NOIDLE() was set. > > But it still does not answer the question that why O_DIRECT and O_SYNC > paths be different when it comes to RQ_NOIDLE. This is outside my knowledge. Thanks Corrado > > Thanks > Vivek > -- __________________________________________________________________________ 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-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
