Re: [PATCH 1/2]block: optimize non-queueable flush request drive

[Shaohua Li <shaohua.li@intel.com>]

Hi,
> On Thu, Apr 28, 2011 at 03:50:55PM +0800, Shaohua Li wrote:
> > Index: linux/block/blk-flush.c
> > ===================================================================
> > --- linux.orig/block/blk-flush.c	2011-04-28 10:23:12.000000000 +0800
> > +++ linux/block/blk-flush.c	2011-04-28 14:12:50.000000000 +0800
> > @@ -158,6 +158,17 @@ static bool blk_flush_complete_seq(struc
> >  	switch (seq) {
> >  	case REQ_FSEQ_PREFLUSH:
> >  	case REQ_FSEQ_POSTFLUSH:
> > +		/*
> > +		 * If queue doesn't support queueable flush request, we just
> > +		 * merge the flush with running flush. For such queue, there
> > +		 * are no normal requests running when flush request is
> > +		 * running, so this still guarantees the correctness.
> > +		 */
> > +		if (!blk_queue_flush_queueable(q)) {
> > +			list_move_tail(&rq->flush.list,
> > +				&q->flush_queue[q->flush_running_idx]);
> > +			break;
> > +		}
> 
> 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.

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

> The second patch can optimize back-to-back execution, which might or
> might not buy us tangible performance gain, so it would be nice to
> have some measurement for this change.  Also, this logic isn't
> necessarily related with queueability of flushes, right?  As such, I
> think it would be better for it to be implemented separately from the
> queueability thing, unless doing such increases complexity too much.
> 
> > Index: linux/include/linux/blkdev.h
> > ===================================================================
> > --- linux.orig/include/linux/blkdev.h	2011-04-28 10:23:12.000000000 +0800
> > +++ linux/include/linux/blkdev.h	2011-04-28 10:32:54.000000000 +0800
> > @@ -364,6 +364,13 @@ struct request_queue
> >  	 * for flush operations
> >  	 */
> >  	unsigned int		flush_flags;
> > +	unsigned int		flush_not_queueable:1;
> > +	/*
> > +	 * flush_exclusive_running and flush_queue_delayed are only meaningful
> > +	 * when flush request isn't queueable
> > +	 */
> > +	unsigned int		flush_exclusive_running:1;
> > +	unsigned int		flush_queue_delayed:1;
> 
> 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.

Thanks,
Shaohua