On Tue, 2011-05-03 at 16:23 +0800, Tejun Heo wrote: > Hello, > > On Tue, May 03, 2011 at 02:44:31PM +0800, Shaohua Li wrote: > > > As I've said several times already, I really don't like this magic > > > being done in the completion path. Can't you detect the condition on > > > issue of the second/following flush and append it to the running list? > > > > hmm, don't understand it. blk_flush_complete_seq is called when the > > second flush is issued. or do you mean do this when the second flush is > > issued to disk? but when the second flush is issued the first flush is > > already finished. > > Ah, okay, my bad. That's the next sequence logic, so the right place. > Still, please do the followings. > > * Put it in a separate patch. > > * Preferably, detect the actual condition (back to back flush) rather > than the queueability test unless it's too complicated. > > * Please make pending/running paths look more symmetrical. I retested, and appears just holding queue is already good enough. After holding queue, merging back to back flush hasn't too much benefit. So I'll not pursue do the back-to-back merge. I'll post my latest patches out soon. > > > If you already have tried that but this way still seems better, can > > > you please explain why? > > > > > > Also, this is a separate logic. Please put it in a separate patch. > > > The first patch should implement queue holding while flushing, which > > > should remove the regression, right? > > > > ok. holding queue has no performance gain in my test, but it reduced a > > lot of request requeue. > > No, holding the queue should remove the regression completely. Please > read on. > > > > Hmmm... why do you need separate ->flush_exclusive_running? Doesn't > > > pending_idx != running_idx already have the same information? > > > > when pending_idx != running_idx, flush request is added into queue tail, > > but this doesn't mean flush request is dispatched to disk. there might > > be other requests in the queue head, which we should dispatch. And flush > > request might be reqeueud. Just checking pending_idx != running_idx will > > cause queue hang because we thought flush is dispatched and then hold > > the queue, but actually flush isn't dispatched yet, the queue should > > dispatch other normal requests. > > Don't hold elv_next_request(). Hold ->elevator_dispatch_fn(). ok, this works. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
