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:

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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