[PATCH] xfs: improve metadata I/O merging in the elevator

[PATCH] xfs: improve metadata I/O merging in the elevator

[Christoph Hellwig]

I had the patch below from Dave in my queue for a while, but previously
couldn't really reproduce his numbers.  After some discussions of the
bio types I've reteseted it again and can see constant improvements when
using cfq on my large array box with it (5-10% for the sequential create
workloads), but still nothing on deadline.  Given that people also want
it for better marking in blktrace it might be time to put it in.

Comments?

-- 

From: Dave Chinner <dgc@sgi.com>
Subject: xfs: improve metadata I/O merging in the elevator

Change all async metadata buffers to use [READ|WRITE]_META I/O types
so that the I/O doesn't get issued immediately. This allows merging
of adjacent metadata requests but still prioritises them over bulk
data. This shows a 10-15% improvement in sequential create speed of
small files.

Don't include the log buffers in this classification - leave them
as sync types so they are issued immediately.

Signed-off-by: Dave Chinner <dgc@sgi.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_buf.c	2009-11-12 17:10:19.852253847 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_buf.c	2009-11-12 17:13:55.334003777 +0100
@@ -1177,10 +1177,14 @@ _xfs_buf_ioapply(
 	if (bp->b_flags & XBF_ORDERED) {
 		ASSERT(!(bp->b_flags & XBF_READ));
 		rw = WRITE_BARRIER;
-	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
+	} else if (bp->b_flags & XBF_LOG_BUFFER) {
 		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
 		bp->b_flags &= ~_XBF_RUN_QUEUES;
 		rw = (bp->b_flags & XBF_WRITE) ? WRITE_SYNC : READ_SYNC;
+	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
+		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
+		bp->b_flags &= ~_XBF_RUN_QUEUES;
+		rw = (bp->b_flags & XBF_WRITE) ? WRITE : READ_META;
 	} else {
 		rw = (bp->b_flags & XBF_WRITE) ? WRITE :
 		     (bp->b_flags & XBF_READ_AHEAD) ? READA : READ;
Index: xfs/fs/xfs/linux-2.6/xfs_buf.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_buf.h	2009-11-12 17:10:19.857278370 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_buf.h	2009-11-12 17:13:55.334003777 +0100
@@ -55,6 +55,7 @@ typedef enum {
 	XBF_FS_MANAGED = (1 << 8),  /* filesystem controls freeing memory  */
  	XBF_ORDERED = (1 << 11),    /* use ordered writes		   */
 	XBF_READ_AHEAD = (1 << 12), /* asynchronous read-ahead		   */
+	XBF_LOG_BUFFER = (1 << 13), /* this is a buffer used for the log   */
 
 	/* flags used only as arguments to access routines */
 	XBF_LOCK = (1 << 14),       /* lock requested			   */
Index: xfs/fs/xfs/xfs_log.c
===================================================================
--- xfs.orig/fs/xfs/xfs_log.c	2009-11-12 17:10:20.267254560 +0100
+++ xfs/fs/xfs/xfs_log.c	2009-11-12 17:13:55.335004184 +0100
@@ -1524,6 +1524,7 @@ xlog_sync(xlog_t		*log,
 	XFS_BUF_ZEROFLAGS(bp);
 	XFS_BUF_BUSY(bp);
 	XFS_BUF_ASYNC(bp);
+	bp->b_flags |= XBF_LOG_BUFFER;
 	/*
 	 * Do an ordered write for the log block.
 	 * Its unnecessary to flush the first split block in the log wrap case.
@@ -1561,6 +1562,7 @@ xlog_sync(xlog_t		*log,
 		XFS_BUF_ZEROFLAGS(bp);
 		XFS_BUF_BUSY(bp);
 		XFS_BUF_ASYNC(bp);
+		bp->b_flags |= XBF_LOG_BUFFER;
 		if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
 			XFS_BUF_ORDERED(bp);
 		dptr = XFS_BUF_PTR(bp);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: improve metadata I/O merging in the elevator

[Dave Chinner]

On Thu, Nov 12, 2009 at 02:09:31PM -0500, Christoph Hellwig wrote:
> I had the patch below from Dave in my queue for a while, but previously
> couldn't really reproduce his numbers.  After some discussions of the
> bio types I've reteseted it again and can see constant improvements when
> using cfq on my large array box with it (5-10% for the sequential create
> workloads), but still nothing on deadline.  Given that people also want
> it for better marking in blktrace it might be time to put it in.
> 
> Comments?

Definitely should be done, but....

It looks like the patch you posted isn't quite doing what was
intended - async write buffers are being classified as WRITE, not
WRITE_META. That means we get more write combining in the elevator
(performance increase) like with WRITE_META, but don't get the
faster dispatch (latency reduction) by using the META queue to keep
the metadata writeback separate from the bulk data writeback.
That may be why deadline is not showing any improvement...

FWIW, the original patch here:

http://oss.sgi.com/archives/xfs/2008-01/msg00630.html

uses WRITE_META, but it looks like you've taken bits of this
patch:

http://oss.sgi.com/archives/xfs/2008-01/msg00653.html

and added the log buffer marking to this patch and accidentally
dropped the WRITE_META marking. i.e. this:

> +	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
> +		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
> +		bp->b_flags &= ~_XBF_RUN_QUEUES;
> +		rw = (bp->b_flags & XBF_WRITE) ? WRITE : READ_META;

I think should be:

+		rw = (bp->b_flags & XBF_WRITE) ? WRITE_META : READ_META;

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: improve metadata I/O merging in the elevator

[Christoph Hellwig]

On Mon, Nov 16, 2009 at 02:50:19PM +1100, Dave Chinner wrote:
> Definitely should be done, but....
> 
> It looks like the patch you posted isn't quite doing what was
> intended - async write buffers are being classified as WRITE, not
> WRITE_META. That means we get more write combining in the elevator
> (performance increase) like with WRITE_META, but don't get the
> faster dispatch (latency reduction) by using the META queue to keep
> the metadata writeback separate from the bulk data writeback.
> That may be why deadline is not showing any improvement...
> 
> FWIW, the original patch here:
> 
> http://oss.sgi.com/archives/xfs/2008-01/msg00630.html
> 
> uses WRITE_META, but it looks like you've taken bits of this
> patch:

Indeed.  I'l re-add the write-side markings and will re-bench and
re-submit.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: improve metadata I/O merging in the elevator

[Christoph Hellwig]

From: Dave Chinner <dgc@sgi.com>

Change all async metadata buffers to use [READ|WRITE]_META I/O types
so that the I/O doesn't get issued immediately. This allows merging
of adjacent metadata requests but still prioritises them over bulk
data. This shows a 10-15% improvement in sequential create speed of
small files.

Don't include the log buffers in this classification - leave them
as sync types so they are issued immediately.

Signed-off-by: Dave Chinner <dgc@sgi.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_buf.c	2009-11-16 12:03:21.940253900 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_buf.c	2009-11-16 12:05:54.224256517 +0100
@@ -1177,10 +1177,14 @@ _xfs_buf_ioapply(
 	if (bp->b_flags & XBF_ORDERED) {
 		ASSERT(!(bp->b_flags & XBF_READ));
 		rw = WRITE_BARRIER;
-	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
+	} else if (bp->b_flags & XBF_LOG_BUFFER) {
 		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
 		bp->b_flags &= ~_XBF_RUN_QUEUES;
 		rw = (bp->b_flags & XBF_WRITE) ? WRITE_SYNC : READ_SYNC;
+	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
+		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
+		bp->b_flags &= ~_XBF_RUN_QUEUES;
+		rw = (bp->b_flags & XBF_WRITE) ? WRITE_META : READ_META;
 	} else {
 		rw = (bp->b_flags & XBF_WRITE) ? WRITE :
 		     (bp->b_flags & XBF_READ_AHEAD) ? READA : READ;
Index: xfs/fs/xfs/linux-2.6/xfs_buf.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_buf.h	2009-11-16 12:03:21.945261731 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_buf.h	2009-11-16 12:03:23.796003934 +0100
@@ -55,6 +55,7 @@ typedef enum {
 	XBF_FS_MANAGED = (1 << 8),  /* filesystem controls freeing memory  */
  	XBF_ORDERED = (1 << 11),    /* use ordered writes		   */
 	XBF_READ_AHEAD = (1 << 12), /* asynchronous read-ahead		   */
+	XBF_LOG_BUFFER = (1 << 13), /* this is a buffer used for the log   */
 
 	/* flags used only as arguments to access routines */
 	XBF_LOCK = (1 << 14),       /* lock requested			   */
Index: xfs/fs/xfs/xfs_log.c
===================================================================
--- xfs.orig/fs/xfs/xfs_log.c	2009-11-16 12:03:21.952261854 +0100
+++ xfs/fs/xfs/xfs_log.c	2009-11-16 12:03:23.798003071 +0100
@@ -1524,6 +1524,7 @@ xlog_sync(xlog_t		*log,
 	XFS_BUF_ZEROFLAGS(bp);
 	XFS_BUF_BUSY(bp);
 	XFS_BUF_ASYNC(bp);
+	bp->b_flags |= XBF_LOG_BUFFER;
 	/*
 	 * Do an ordered write for the log block.
 	 * Its unnecessary to flush the first split block in the log wrap case.
@@ -1561,6 +1562,7 @@ xlog_sync(xlog_t		*log,
 		XFS_BUF_ZEROFLAGS(bp);
 		XFS_BUF_BUSY(bp);
 		XFS_BUF_ASYNC(bp);
+		bp->b_flags |= XBF_LOG_BUFFER;
 		if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
 			XFS_BUF_ORDERED(bp);
 		dptr = XFS_BUF_PTR(bp);
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2009-11-16 12:04:49.799002997 +0100
+++ xfs/include/linux/fs.h	2009-11-16 12:05:10.130255677 +0100
@@ -151,6 +151,7 @@ struct inodes_stat_t {
 #define READ_META	(READ | (1 << BIO_RW_META))
 #define WRITE_SYNC_PLUG	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_NOIDLE))
 #define WRITE_SYNC	(WRITE_SYNC_PLUG | (1 << BIO_RW_UNPLUG))
+#define WRITE_META	(WRITE | (1 << BIO_RW_META))
 #define WRITE_ODIRECT	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
 #define SWRITE_SYNC_PLUG	\
 			(SWRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_NOIDLE))

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

RE: [PATCH v2] xfs: improve metadata I/O merging in the elevator

[Alex Elder]

Christoph Hellwig wrote:
> From: Dave Chinner <dgc@sgi.com>
> 
> Change all async metadata buffers to use [READ|WRITE]_META I/O types
> so that the I/O doesn't get issued immediately. This allows merging
> of adjacent metadata requests but still prioritises them over bulk
> data. This shows a 10-15% improvement in sequential create speed of
> small files.
> 
> Don't include the log buffers in this classification - leave them
> as sync types so they are issued immediately.

Looks good.

> Signed-off-by: Dave Chinner <dgc@sgi.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Alex Elder <aelder@sgi.com>

> Index: xfs/fs/xfs/linux-2.6/xfs_buf.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_buf.c	2009-11-16 12:03:21.940253900 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_buf.c	2009-11-16 12:05:54.224256517 +0100
> @@ -1177,10 +1177,14 @@ _xfs_buf_ioapply(
>  	if (bp->b_flags & XBF_ORDERED) {
>  		ASSERT(!(bp->b_flags & XBF_READ));
>  		rw = WRITE_BARRIER;
> -	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
> +	} else if (bp->b_flags & XBF_LOG_BUFFER) {
>  		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
>  		bp->b_flags &= ~_XBF_RUN_QUEUES;
>  		rw = (bp->b_flags & XBF_WRITE) ? WRITE_SYNC : READ_SYNC;
> +	} else if (bp->b_flags & _XBF_RUN_QUEUES) {
> +		ASSERT(!(bp->b_flags & XBF_READ_AHEAD));
> +		bp->b_flags &= ~_XBF_RUN_QUEUES;
> +		rw = (bp->b_flags & XBF_WRITE) ? WRITE_META : READ_META;
>  	} else {
>  		rw = (bp->b_flags & XBF_WRITE) ? WRITE :
>  		     (bp->b_flags & XBF_READ_AHEAD) ? READA : READ;
> Index: xfs/fs/xfs/linux-2.6/xfs_buf.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_buf.h	2009-11-16 12:03:21.945261731 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_buf.h	2009-11-16 12:03:23.796003934 +0100
> @@ -55,6 +55,7 @@ typedef enum {
>  	XBF_FS_MANAGED = (1 << 8),  /* filesystem controls freeing memory  */
>   	XBF_ORDERED = (1 << 11),    /* use ordered writes		   */
>  	XBF_READ_AHEAD = (1 << 12), /* asynchronous read-ahead		   */
> +	XBF_LOG_BUFFER = (1 << 13), /* this is a buffer used for the log   */
> 
>  	/* flags used only as arguments to access routines */
>  	XBF_LOCK = (1 << 14),       /* lock requested			   */
> Index: xfs/fs/xfs/xfs_log.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log.c	2009-11-16 12:03:21.952261854 +0100
> +++ xfs/fs/xfs/xfs_log.c	2009-11-16 12:03:23.798003071 +0100
> @@ -1524,6 +1524,7 @@ xlog_sync(xlog_t		*log,
>  	XFS_BUF_ZEROFLAGS(bp);
>  	XFS_BUF_BUSY(bp);
>  	XFS_BUF_ASYNC(bp);
> +	bp->b_flags |= XBF_LOG_BUFFER;
>  	/*
>  	 * Do an ordered write for the log block.
>  	 * Its unnecessary to flush the first split block in the log wrap case.
> @@ -1561,6 +1562,7 @@ xlog_sync(xlog_t		*log,
>  		XFS_BUF_ZEROFLAGS(bp);
>  		XFS_BUF_BUSY(bp);
>  		XFS_BUF_ASYNC(bp);
> +		bp->b_flags |= XBF_LOG_BUFFER;
>  		if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
>  			XFS_BUF_ORDERED(bp);
>  		dptr = XFS_BUF_PTR(bp);
> Index: xfs/include/linux/fs.h
> ===================================================================
> --- xfs.orig/include/linux/fs.h	2009-11-16 12:04:49.799002997 +0100
> +++ xfs/include/linux/fs.h	2009-11-16 12:05:10.130255677 +0100
> @@ -151,6 +151,7 @@ struct inodes_stat_t {
>  #define READ_META	(READ | (1 << BIO_RW_META))
>  #define WRITE_SYNC_PLUG	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_NOIDLE))
>  #define WRITE_SYNC	(WRITE_SYNC_PLUG | (1 << BIO_RW_UNPLUG))
> +#define WRITE_META	(WRITE | (1 << BIO_RW_META))
>  #define WRITE_ODIRECT	(WRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG))
>  #define SWRITE_SYNC_PLUG	\
>  			(SWRITE | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_NOIDLE))
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs