Patchwork [v2,2/3] block: hold queue if flush is running for non-queueable flush drive

login
register
mail settings
Submitter Shaohua Li
Date May 4, 2011, 8:17 a.m.
Message ID <20110504082115.270298766@sli10-conroe.sh.intel.com>
Download mbox | patch
Permalink /patch/93989/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Shaohua Li - May 4, 2011, 8:17 a.m.
In some drives, flush requests are non-queueable. When flush request is running,
normal read/write requests can't run. If block layer dispatches such request,
driver can't handle it and requeue it.
Tejun suggested we can hold the queue when flush is running. This can avoid
unnecessary requeue.
Also this can improve performance. Say we have requests f1, w1, f2 (f is flush
request, and w is write request). When f1 is running, queue will be hold, so w1
will not be added to queue list. Just after f1 is finished, f2 will be
dispatched. Since f1 already flushs cache out, f2 can be finished very quickly.
In my test, the queue holding completely solves a regression introduced by
commit 53d63e6b0dfb9588, which is about 20% regression running a sysbench fileio
workload.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 block/blk-flush.c      |    3 +++
 block/blk.h            |   12 +++++++++++-
 include/linux/blkdev.h |    1 +
 3 files changed, 15 insertions(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - May 4, 2011, 9:09 a.m.
Hello,

On Wed, May 04, 2011 at 04:17:27PM +0800, shaohua.li@intel.com wrote:
> In some drives, flush requests are non-queueable. When flush request is running,
> normal read/write requests can't run. If block layer dispatches such request,
> driver can't handle it and requeue it.
> Tejun suggested we can hold the queue when flush is running. This can avoid
> unnecessary requeue.
> Also this can improve performance. Say we have requests f1, w1, f2 (f is flush
> request, and w is write request). When f1 is running, queue will be hold, so w1
> will not be added to queue list. Just after f1 is finished, f2 will be
> dispatched. Since f1 already flushs cache out, f2 can be finished very quickly.
> In my test, the queue holding completely solves a regression introduced by
> commit 53d63e6b0dfb9588, which is about 20% regression running a sysbench fileio
> workload.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

It looks good to me now, but some nitpicks.

> Index: linux/block/blk-flush.c
> ===================================================================
> --- linux.orig/block/blk-flush.c	2011-05-04 14:20:33.000000000 +0800
> +++ linux/block/blk-flush.c	2011-05-04 15:23:50.000000000 +0800
> @@ -199,6 +199,9 @@ static void flush_end_io(struct request
>  
>  	BUG_ON(q->flush_pending_idx == q->flush_running_idx);
>  
> +	queued |= q->flush_queue_delayed;
> +	q->flush_queue_delayed = 0;
> +
>  	/* account completion of the flush request */
>  	q->flush_running_idx ^= 1;
>  	elv_completed_request(q, flush_rq);

Can you please do if (queued || q->flush_queue_delayed) instead of
setting queued?  And please also update the comment above the if
statement so that it explains the flush_queue_delayed case too.

> Index: linux/block/blk.h
> ===================================================================
> --- linux.orig/block/blk.h	2011-05-04 14:20:33.000000000 +0800
> +++ linux/block/blk.h	2011-05-04 16:09:42.000000000 +0800
> @@ -61,7 +61,17 @@ static inline struct request *__elv_next
>  			rq = list_entry_rq(q->queue_head.next);
>  			return rq;
>  		}
> -
> +		/*
> +		 *  Flush request is running and flush request isn't queeueable
> +		 *  in the drive, we can hold the queue till flush request is
> +		 *  finished. Even we don't do this, driver can't dispatch next
> +		 *  requests and will requeue them.
> +		 */

Please explain the f1, w1, f2 case here as that's the biggest reason
this optimization is implemented and also explain the use of
flush_queue_delayed (just explain briefly and refer to
flush_end_io()).

Thank you.
Sergei Shtylyov - May 4, 2011, 10:42 a.m.
Hello.

On 04-05-2011 12:17, shaohua.li@intel.com wrote:

> In some drives, flush requests are non-queueable. When flush request is running,
> normal read/write requests can't run. If block layer dispatches such request,
> driver can't handle it and requeue it.
> Tejun suggested we can hold the queue when flush is running. This can avoid
> unnecessary requeue.
> Also this can improve performance. Say we have requests f1, w1, f2 (f is flush
> request, and w is write request). When f1 is running, queue will be hold, so w1
> will not be added to queue list. Just after f1 is finished, f2 will be
> dispatched. Since f1 already flushs cache out, f2 can be finished very quickly.
> In my test, the queue holding completely solves a regression introduced by
> commit 53d63e6b0dfb9588, which is about 20% regression running a sysbench fileio

    Please specify that commit's summary -- for human readers. The ID is only 
immediately usable to gitweb.

> workload.

> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

[...]

> Index: linux/block/blk.h
> ===================================================================
> --- linux.orig/block/blk.h	2011-05-04 14:20:33.000000000 +0800
> +++ linux/block/blk.h	2011-05-04 16:09:42.000000000 +0800
> @@ -61,7 +61,17 @@ static inline struct request *__elv_next
>   			rq = list_entry_rq(q->queue_head.next);
>   			return rq;
>   		}
> -
> +		/*
> +		 *  Flush request is running and flush request isn't queeueable

    Queueable.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: linux/block/blk-flush.c
===================================================================
--- linux.orig/block/blk-flush.c	2011-05-04 14:20:33.000000000 +0800
+++ linux/block/blk-flush.c	2011-05-04 15:23:50.000000000 +0800
@@ -199,6 +199,9 @@  static void flush_end_io(struct request
 
 	BUG_ON(q->flush_pending_idx == q->flush_running_idx);
 
+	queued |= q->flush_queue_delayed;
+	q->flush_queue_delayed = 0;
+
 	/* account completion of the flush request */
 	q->flush_running_idx ^= 1;
 	elv_completed_request(q, flush_rq);
Index: linux/include/linux/blkdev.h
===================================================================
--- linux.orig/include/linux/blkdev.h	2011-05-04 14:24:40.000000000 +0800
+++ linux/include/linux/blkdev.h	2011-05-04 14:29:29.000000000 +0800
@@ -365,6 +365,7 @@  struct request_queue
 	 */
 	unsigned int		flush_flags;
 	unsigned int		flush_not_queueable:1;
+	unsigned int		flush_queue_delayed:1;
 	unsigned int		flush_pending_idx:1;
 	unsigned int		flush_running_idx:1;
 	unsigned long		flush_pending_since;
Index: linux/block/blk.h
===================================================================
--- linux.orig/block/blk.h	2011-05-04 14:20:33.000000000 +0800
+++ linux/block/blk.h	2011-05-04 16:09:42.000000000 +0800
@@ -61,7 +61,17 @@  static inline struct request *__elv_next
 			rq = list_entry_rq(q->queue_head.next);
 			return rq;
 		}
-
+		/*
+		 *  Flush request is running and flush request isn't queeueable
+		 *  in the drive, we can hold the queue till flush request is
+		 *  finished. Even we don't do this, driver can't dispatch next
+		 *  requests and will requeue them.
+		 */
+		if (q->flush_pending_idx != q->flush_running_idx &&
+				!blk_queue_flush_queueable(q)) {
+			q->flush_queue_delayed = 1;
+			return NULL;
+		}
 		if (!q->elevator->ops->elevator_dispatch_fn(q, 0))
 			return NULL;
 	}