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

[Tejun Heo <htejun@gmail.com>]

Hello, Shaohua.

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

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?

> Index: linux/block/blk-core.c
> ===================================================================
> --- linux.orig/block/blk-core.c	2011-04-28 10:23:12.000000000 +0800
> +++ linux/block/blk-core.c	2011-04-28 10:53:18.000000000 +0800
> @@ -929,6 +929,8 @@ void blk_requeue_request(struct request_
>  
>  	BUG_ON(blk_queued_rq(rq));
>  
> +	if (rq == &q->flush_rq)
> +		q->flush_exclusive_running = 0;

I don't get this either.  What's requeueing got to do with it?  It
doesn't matter whether flush gets requeued or not.  Once flush is put
onto the dispatch list, isn't holding the queue until it's complete
enough?

Thanks.

-- 
tejun