[PATCH 0/6] [XFS] introduce a AG inode tree walker

[PATCH 0/6] [XFS] introduce a AG inode tree walker

[Dave Chinner]

This series splits up the sync and reclaim code into three
separate actions. The first is the tree walker, the second is
the inode validation and the third is the operation to execute
on the inode.

This allows us to somewhat abstract the radix tree away from the
act of walking the cached inodes and puts in place mechanisms that
can be extended for bulk inode cache lookups.

This also splits the inode writeback into separate data and metadata
sync operations and optimises them a little......

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

[PATCH 1/6] [XFS] Split inode data writeback from inode sync.

[Dave Chinner]

In many cases we only want to sync inode data. Start
spliting the inode sync into data sync and inode sync
by factoring out the inode data flush.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |   54 +++++++++++++++++++++++++++----------------
 1 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index f7ba766..fd024e2 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -47,6 +47,37 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 
+static int
+xfs_sync_inode_data(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	struct inode	*inode = VFS_I(ip);
+	int		error = 0;
+
+	if (VN_DIRTY(inode)) {
+		int	locked = 0;
+		if (flags & SYNC_TRYLOCK) {
+			if (xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
+				locked = 1;
+		} else {
+			xfs_ilock(ip, XFS_IOLOCK_SHARED);
+			locked = 1;
+		}
+		if (locked) {
+			error = xfs_flush_pages(ip, 0, -1,
+					(flags & SYNC_WAIT) ? 0 : XFS_B_ASYNC,
+					FI_NONE);
+			xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+		}
+	}
+
+	if (flags & SYNC_IOWAIT)
+		xfs_ioend_wait(ip);
+
+	return error;
+}
+
 /*
  * Sync all the inodes in the given AG according to the
  * direction given by the flags.
@@ -122,27 +153,10 @@ xfs_sync_inodes_ag(
 		 * If we have to flush data or wait for I/O completion
 		 * we need to hold the iolock.
 		 */
-		if (flags & SYNC_DELWRI) {
-			if (VN_DIRTY(inode)) {
-				if (flags & SYNC_TRYLOCK) {
-					if (xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
-						lock_flags |= XFS_IOLOCK_SHARED;
-				} else {
-					xfs_ilock(ip, XFS_IOLOCK_SHARED);
-					lock_flags |= XFS_IOLOCK_SHARED;
-				}
-				if (lock_flags & XFS_IOLOCK_SHARED) {
-					error = xfs_flush_pages(ip, 0, -1,
-							(flags & SYNC_WAIT) ? 0
-								: XFS_B_ASYNC,
-							FI_NONE);
-				}
-			}
-			if (VN_CACHED(inode) && (flags & SYNC_IOWAIT))
-				xfs_ioend_wait(ip);
-		}
-		xfs_ilock(ip, XFS_ILOCK_SHARED);
+		if (flags & SYNC_DELWRI)
+			error = xfs_sync_inode_data(ip, flags);
 
+		xfs_ilock(ip, XFS_ILOCK_SHARED);
 		if ((flags & SYNC_ATTR) && !xfs_inode_clean(ip)) {
 			if (flags & SYNC_WAIT) {
 				xfs_iflock(ip);
-- 
1.6.2

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

[PATCH 2/6] [XFS] Use xfs_inode_flush() in xfs_sync_inodes_ag()

[Dave Chinner]

xfs_sync_inodes_ag() effectively open-codes xfs_inode_flush()
to do blocking vs non-blocking inode flushing. It doesn't have
all the optimisations that xfs_inode_flush() has but does
the same thing. Call xfs_inode_flush() instead.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |   63 +++++++++++++++++++++++++++++++-----------
 1 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index fd024e2..b3d4f7a 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -43,6 +43,7 @@
 #include "xfs_buf_item.h"
 #include "xfs_inode_item.h"
 #include "xfs_rw.h"
+#include "linux-2.6/xfs_vnode.h"
 
 #include <linux/kthread.h>
 #include <linux/freezer.h>
@@ -78,6 +79,47 @@ xfs_sync_inode_data(
 	return error;
 }
 
+static int
+xfs_sync_inode_flush(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	int		error = 0;
+
+	/*
+	 * Bypass inodes which have already been cleaned by
+	 * the inode flush clustering code inside xfs_iflush
+	 */
+	if (xfs_inode_clean(ip))
+		return 0;
+
+	/*
+	 * We make this non-blocking if the inode is contended,
+	 * return EAGAIN to indicate to the caller that they
+	 * did not succeed. This prevents the flush path from
+	 * blocking on inodes inside another operation right
+	 * now, they get caught later by xfs_sync.
+	 */
+	if (flags & SYNC_WAIT) {
+		xfs_ilock(ip, XFS_ILOCK_SHARED);
+		xfs_iflock(ip);
+
+		error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
+	} else {
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
+			goto out;
+		if (xfs_ipincount(ip) || !xfs_iflock_nowait(ip))
+			goto out_unlock;
+
+		error = xfs_iflush(ip, XFS_IFLUSH_ASYNC_NOBLOCK);
+	}
+
+out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+out:
+	return error;
+}
+
 /*
  * Sync all the inodes in the given AG according to the
  * direction given by the flags.
@@ -97,7 +139,6 @@ xfs_sync_inodes_ag(
 	do {
 		struct inode	*inode;
 		xfs_inode_t	*ip = NULL;
-		int		lock_flags = XFS_ILOCK_SHARED;
 
 		/*
 		 * use a gang lookup to find the next inode in the tree
@@ -156,22 +197,10 @@ xfs_sync_inodes_ag(
 		if (flags & SYNC_DELWRI)
 			error = xfs_sync_inode_data(ip, flags);
 
-		xfs_ilock(ip, XFS_ILOCK_SHARED);
-		if ((flags & SYNC_ATTR) && !xfs_inode_clean(ip)) {
-			if (flags & SYNC_WAIT) {
-				xfs_iflock(ip);
-				if (!xfs_inode_clean(ip))
-					error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
-				else
-					xfs_ifunlock(ip);
-			} else if (xfs_iflock_nowait(ip)) {
-				if (!xfs_inode_clean(ip))
-					error = xfs_iflush(ip, XFS_IFLUSH_DELWRI);
-				else
-					xfs_ifunlock(ip);
-			}
-		}
-		xfs_iput(ip, lock_flags);
+		if (flags & SYNC_ATTR)
+			error = xfs_sync_inode_flush(ip, flags);
+
+		IRELE(ip);
 
 		if (error)
 			last_error = error;
-- 
1.6.2

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

[PATCH 3/6] [XFS] Factor out inode validation for sync

[Dave Chinner]

Separate the validation of inodes found by the radix
tree walk from the radix tree lookup.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |   56 ++++++++++++++++++++++++++----------------
 1 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index b3d4f7a..a83438a 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -120,6 +120,37 @@ out:
 	return error;
 }
 
+int
+xfs_sync_inode_valid(
+	xfs_mount_t	*mp,
+	xfs_inode_t	*ip)
+{
+	struct inode	*inode;
+	int		error;
+
+	/* nothing to sync during shutdown */
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return EFSCORRUPTED;
+
+	/*
+	 * If we can't get a reference on the inode, it must be in reclaim.
+	 * Leave it for the reclaim code to flush. Also avoid inodes that
+	 * haven't been fully initialised.
+	 */
+	error = ENOENT;
+	inode = VFS_I(ip);
+	if (!igrab(inode))
+		goto error;
+	if (is_bad_inode(inode) ||
+	    xfs_iflags_test(ip, XFS_INEW))
+		goto rele;
+	return 0;
+rele:
+	IRELE(ip);
+error:
+	return error;
+}
+
 /*
  * Sync all the inodes in the given AG according to the
  * direction given by the flags.
@@ -137,7 +168,6 @@ xfs_sync_inodes_ag(
 	int		last_error = 0;
 
 	do {
-		struct inode	*inode;
 		xfs_inode_t	*ip = NULL;
 
 		/*
@@ -166,27 +196,11 @@ xfs_sync_inodes_ag(
 			break;
 		}
 
-		/* nothing to sync during shutdown */
-		if (XFS_FORCED_SHUTDOWN(mp)) {
+		error = xfs_sync_inode_valid(mp, ip);
+		if (error) {
 			read_unlock(&pag->pag_ici_lock);
-			return 0;
-		}
-
-		/*
-		 * If we can't get a reference on the inode, it must be
-		 * in reclaim. Leave it for the reclaim code to flush.
-		 */
-		inode = VFS_I(ip);
-		if (!igrab(inode)) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-		read_unlock(&pag->pag_ici_lock);
-
-		/* avoid new or bad inodes */
-		if (is_bad_inode(inode) ||
-		    xfs_iflags_test(ip, XFS_INEW)) {
-			IRELE(ip);
+			if (error == EFSCORRUPTED)
+				return 0;
 			continue;
 		}
 
-- 
1.6.2

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

[PATCH 4/6] [XFS] Factor out inode validation for reclaim

[Dave Chinner]

Separate the validation of inodes found by the radix
tree walk from the radix tree lookup.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index a83438a..1fa29de 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -711,6 +711,18 @@ xfs_inode_clear_reclaim_tag(
 	xfs_put_perag(mp, pag);
 }
 
+static int
+xfs_reclaim_inode_valid(
+	xfs_mount_t	*mp,
+	xfs_inode_t	*ip)
+{
+	ASSERT(xfs_iflags_test(ip, (XFS_IRECLAIMABLE|XFS_IRECLAIM)));
+
+	/* ignore if already under reclaim */
+	if (xfs_iflags_test(ip, XFS_IRECLAIM))
+		return -ENOENT;
+	return 0;
+}
 
 STATIC void
 xfs_reclaim_inodes_ag(
@@ -729,6 +741,8 @@ restart:
 	first_index = 0;
 	skipped = 0;
 	do {
+		int	error;
+
 		/*
 		 * use a gang lookup to find the next inode in the tree
 		 * as the tree is sparse and a gang lookup walks to find
@@ -756,8 +770,8 @@ restart:
 			break;
 		}
 
-		/* ignore if already under reclaim */
-		if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
+		error = xfs_reclaim_inode_valid(mp, ip);
+		if (error) {
 			read_unlock(&pag->pag_ici_lock);
 			continue;
 		}
-- 
1.6.2

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

[PATCH 5/6] [XFS] Remove unused parameter from xfs_reclaim_inodes

[Dave Chinner]

The noblock parameter of xfs_reclaim_inodes() is only
ever set to zero. Remove it and all the conditional
code that is never executed.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |   23 ++++-------------------
 fs/xfs/linux-2.6/xfs_sync.h |    2 +-
 fs/xfs/xfs_mount.c          |    2 +-
 3 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index 1fa29de..f9c2a56 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -398,7 +398,7 @@ xfs_quiesce_fs(
 	int	count = 0, pincount;
 
 	xfs_flush_buftarg(mp->m_ddev_targp, 0);
-	xfs_reclaim_inodes(mp, 0, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
+	xfs_reclaim_inodes(mp, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
 
 	/*
 	 * This loop must run at least twice.  The first instance of the loop
@@ -522,7 +522,7 @@ xfs_sync_worker(
 
 	if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
 		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
-		xfs_reclaim_inodes(mp, 0, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
+		xfs_reclaim_inodes(mp, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
 		/* dgc: errors ignored here */
 		error = XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
 		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
@@ -728,7 +728,6 @@ STATIC void
 xfs_reclaim_inodes_ag(
 	xfs_mount_t	*mp,
 	int		ag,
-	int		noblock,
 	int		mode)
 {
 	xfs_inode_t	*ip = NULL;
@@ -775,26 +774,13 @@ restart:
 			read_unlock(&pag->pag_ici_lock);
 			continue;
 		}
-
-		if (noblock) {
-			if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
-				read_unlock(&pag->pag_ici_lock);
-				continue;
-			}
-			if (xfs_ipincount(ip) ||
-			    !xfs_iflock_nowait(ip)) {
-				xfs_iunlock(ip, XFS_ILOCK_EXCL);
-				read_unlock(&pag->pag_ici_lock);
-				continue;
-			}
-		}
 		read_unlock(&pag->pag_ici_lock);
 
 		/*
 		 * hmmm - this is an inode already in reclaim. Do
 		 * we even bother catching it here?
 		 */
-		if (xfs_reclaim_inode(ip, noblock, mode))
+		if (xfs_reclaim_inode(ip, 0, mode))
 			skipped++;
 	} while (nr_found);
 
@@ -809,7 +795,6 @@ restart:
 int
 xfs_reclaim_inodes(
 	xfs_mount_t	*mp,
-	int		 noblock,
 	int		mode)
 {
 	int		i;
@@ -817,7 +802,7 @@ xfs_reclaim_inodes(
 	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
 		if (!mp->m_perag[i].pag_ici_init)
 			continue;
-		xfs_reclaim_inodes_ag(mp, i, noblock, mode);
+		xfs_reclaim_inodes_ag(mp, i, mode);
 	}
 	return 0;
 }
diff --git a/fs/xfs/linux-2.6/xfs_sync.h b/fs/xfs/linux-2.6/xfs_sync.h
index 308d5bf..d7b2b5f 100644
--- a/fs/xfs/linux-2.6/xfs_sync.h
+++ b/fs/xfs/linux-2.6/xfs_sync.h
@@ -48,7 +48,7 @@ void xfs_quiesce_attr(struct xfs_mount *mp);
 void xfs_flush_inodes(struct xfs_inode *ip);
 
 int xfs_reclaim_inode(struct xfs_inode *ip, int locked, int sync_mode);
-int xfs_reclaim_inodes(struct xfs_mount *mp, int noblock, int mode);
+int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
 
 void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
 void xfs_inode_clear_reclaim_tag(struct xfs_inode *ip);
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 664961e..30918da 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1235,7 +1235,7 @@ xfs_unmountfs(
 	 * need to force the log first.
 	 */
 	xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE | XFS_LOG_SYNC);
-	xfs_reclaim_inodes(mp, 0, XFS_IFLUSH_ASYNC);
+	xfs_reclaim_inodes(mp, XFS_IFLUSH_ASYNC);
 
 	XFS_QM_DQPURGEALL(mp, XFS_QMOPT_QUOTALL | XFS_QMOPT_UMOUNTING);
 
-- 
1.6.2

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

[PATCH 6/6] [XFS] introduce a per-ag inode iterator

[Dave Chinner]

Given that we walk across the per-ag inode lists so
often, it makes sense to introduce an iterator for this.
Break the operations on each inode into two operations -
a lookup validation step to confirm the inode is good to
be used and an execution step that performs the operation
on the inode.

Convert the sync and reclaim code to use this new iterator.
At some point the quota sync needs to be converted as well.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_sync.c |  320 ++++++++++++++++++++-----------------------
 1 files changed, 149 insertions(+), 171 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index f9c2a56..45dfff9 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -48,6 +48,132 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 
+
+STATIC xfs_inode_t *
+xfs_inode_ag_lookup(
+	xfs_mount_t	*mp,
+	xfs_perag_t	*pag,
+	int		(*lookup)(xfs_mount_t *mp, xfs_inode_t *ip),
+	uint32_t	*first_index,
+	int		tag)
+{
+	int		nr_found;
+	xfs_inode_t	*ip;
+	int		error = -ENOENT;
+
+	/*
+	 * use a gang lookup to find the next inode in the tree
+	 * as the tree is sparse and a gang lookup walks to find
+	 * the number of objects requested.
+	 */
+	read_lock(&pag->pag_ici_lock);
+	if (tag == -1) {
+		nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
+				(void**)&ip, *first_index, 1);
+	} else {
+		nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
+				(void**)&ip, *first_index, 1, tag);
+	}
+	if (!nr_found)
+		goto unlock;
+
+	/*
+	 * Update the index for the next lookup. Catch overflows
+	 * into the next AG range which can occur if we have inodes
+	 * in the last block of the AG and we are currently
+	 * pointing to the last inode.
+	 */
+	*first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
+	if (*first_index < XFS_INO_TO_AGINO(mp, ip->i_ino))
+		goto unlock;
+
+
+	error = lookup(mp, ip);
+	read_unlock(&pag->pag_ici_lock);
+	if (error)
+		return ERR_PTR(error);
+	return ip;
+
+unlock:
+	read_unlock(&pag->pag_ici_lock);
+	return NULL;
+}
+
+STATIC int
+xfs_inode_ag_walk(
+	xfs_mount_t	*mp,
+	xfs_agnumber_t	ag,
+	int		(*lookup)(xfs_mount_t *mp, xfs_inode_t *ip),
+	int		(*execute)(xfs_inode_t *ip, int flags),
+	int		flags,
+	int		tag)
+{
+	xfs_perag_t	*pag = &mp->m_perag[ag];
+	uint32_t	first_index;
+	int		last_error = 0;
+	int		skipped;
+
+restart:
+	skipped = 0;
+	first_index = 0;
+	do {
+		int		error = 0;
+		xfs_inode_t	*ip;
+
+		ip = xfs_inode_ag_lookup(mp, pag, lookup, &first_index, tag);
+		if (!ip)
+			break;
+		if (IS_ERR(ip))
+			continue;
+
+		error = execute(ip, flags);
+		if (error == EAGAIN) {
+			skipped++;
+			continue;
+		}
+		if (error)
+			last_error = error;
+		/*
+		 * bail out if the filesystem is corrupted.
+		 */
+		if (error == EFSCORRUPTED)
+			break;
+
+	} while (1);
+
+	if (skipped) {
+		delay(1);
+		goto restart;
+	}
+
+	xfs_put_perag(mp, pag);
+	return last_error;
+}
+
+STATIC int
+xfs_inode_ag_iterator(
+	xfs_mount_t	*mp,
+	int		(*lookup)(xfs_mount_t *mp, xfs_inode_t *ip),
+	int		(*execute)(xfs_inode_t *ip, int flags),
+	int		flags,
+	int		tag)
+{
+	int		error = 0;
+	int		last_error = 0;
+	xfs_agnumber_t	ag;
+
+	for (ag = 0; ag < mp->m_sb.sb_agcount; ag++) {
+		if (!mp->m_perag[ag].pag_ici_init)
+			continue;
+		error = xfs_inode_ag_walk(mp, ag, lookup, execute, flags, tag);
+		if (error)
+			last_error = error;
+		if (error == EFSCORRUPTED)
+			break;
+	}
+	return XFS_ERROR(last_error);
+}
+
 static int
 xfs_sync_inode_data(
 	struct xfs_inode	*ip,
@@ -76,6 +202,7 @@ xfs_sync_inode_data(
 	if (flags & SYNC_IOWAIT)
 		xfs_ioend_wait(ip);
 
+	IRELE(ip);
 	return error;
 }
 
@@ -91,7 +218,7 @@ xfs_sync_inode_flush(
 	 * the inode flush clustering code inside xfs_iflush
 	 */
 	if (xfs_inode_clean(ip))
-		return 0;
+		goto out_rele;
 
 	/*
 	 * We make this non-blocking if the inode is contended,
@@ -107,7 +234,7 @@ xfs_sync_inode_flush(
 		error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
 	} else {
 		if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
-			goto out;
+			goto out_rele;
 		if (xfs_ipincount(ip) || !xfs_iflock_nowait(ip))
 			goto out_unlock;
 
@@ -116,7 +243,8 @@ xfs_sync_inode_flush(
 
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-out:
+out_rele:
+	IRELE(ip);
 	return error;
 }
 
@@ -151,115 +279,32 @@ error:
 	return error;
 }
 
-/*
- * Sync all the inodes in the given AG according to the
- * direction given by the flags.
- */
-STATIC int
-xfs_sync_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	int		flags)
-{
-	xfs_perag_t	*pag = &mp->m_perag[ag];
-	int		nr_found;
-	uint32_t	first_index = 0;
-	int		error = 0;
-	int		last_error = 0;
-
-	do {
-		xfs_inode_t	*ip = NULL;
-
-		/*
-		 * use a gang lookup to find the next inode in the tree
-		 * as the tree is sparse and a gang lookup walks to find
-		 * the number of objects requested.
-		 */
-		read_lock(&pag->pag_ici_lock);
-		nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
-				(void**)&ip, first_index, 1);
-
-		if (!nr_found) {
-			read_unlock(&pag->pag_ici_lock);
-			break;
-		}
-
-		/*
-		 * Update the index for the next lookup. Catch overflows
-		 * into the next AG range which can occur if we have inodes
-		 * in the last block of the AG and we are currently
-		 * pointing to the last inode.
-		 */
-		first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
-		if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino)) {
-			read_unlock(&pag->pag_ici_lock);
-			break;
-		}
-
-		error = xfs_sync_inode_valid(mp, ip);
-		if (error) {
-			read_unlock(&pag->pag_ici_lock);
-			if (error == EFSCORRUPTED)
-				return 0;
-			continue;
-		}
-
-		/*
-		 * If we have to flush data or wait for I/O completion
-		 * we need to hold the iolock.
-		 */
-		if (flags & SYNC_DELWRI)
-			error = xfs_sync_inode_data(ip, flags);
-
-		if (flags & SYNC_ATTR)
-			error = xfs_sync_inode_flush(ip, flags);
-
-		IRELE(ip);
-
-		if (error)
-			last_error = error;
-		/*
-		 * bail out if the filesystem is corrupted.
-		 */
-		if (error == EFSCORRUPTED)
-			return XFS_ERROR(error);
-
-	} while (nr_found);
-
-	return last_error;
-}
-
 int
 xfs_sync_inodes(
 	xfs_mount_t	*mp,
 	int		flags)
 {
-	int		error;
-	int		last_error;
-	int		i;
+	int		error = 0;
 	int		lflags = XFS_LOG_FORCE;
 
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
 		return 0;
-	error = 0;
-	last_error = 0;
 
 	if (flags & SYNC_WAIT)
 		lflags |= XFS_LOG_SYNC;
 
-	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-		if (!mp->m_perag[i].pag_ici_init)
-			continue;
-		error = xfs_sync_inodes_ag(mp, i, flags);
-		if (error)
-			last_error = error;
-		if (error == EFSCORRUPTED)
-			break;
-	}
 	if (flags & SYNC_DELWRI)
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_valid,
+					xfs_sync_inode_data, flags, -1);
+
+	if (flags & SYNC_ATTR)
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_valid,
+					xfs_sync_inode_flush, flags, -1);
+
+	if (!error && (flags & SYNC_DELWRI))
 		xfs_log_force(mp, 0, lflags);
 
-	return XFS_ERROR(last_error);
+	return XFS_ERROR(error);
 }
 
 STATIC int
@@ -724,72 +769,12 @@ xfs_reclaim_inode_valid(
 	return 0;
 }
 
-STATIC void
-xfs_reclaim_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	int		mode)
+static int
+xfs_reclaim_inode_now(
+	xfs_inode_t	*ip,
+	int		flags)
 {
-	xfs_inode_t	*ip = NULL;
-	xfs_perag_t	*pag = &mp->m_perag[ag];
-	int		nr_found;
-	uint32_t	first_index;
-	int		skipped;
-
-restart:
-	first_index = 0;
-	skipped = 0;
-	do {
-		int	error;
-
-		/*
-		 * use a gang lookup to find the next inode in the tree
-		 * as the tree is sparse and a gang lookup walks to find
-		 * the number of objects requested.
-		 */
-		read_lock(&pag->pag_ici_lock);
-		nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
-					(void**)&ip, first_index, 1,
-					XFS_ICI_RECLAIM_TAG);
-
-		if (!nr_found) {
-			read_unlock(&pag->pag_ici_lock);
-			break;
-		}
-
-		/*
-		 * Update the index for the next lookup. Catch overflows
-		 * into the next AG range which can occur if we have inodes
-		 * in the last block of the AG and we are currently
-		 * pointing to the last inode.
-		 */
-		first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
-		if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino)) {
-			read_unlock(&pag->pag_ici_lock);
-			break;
-		}
-
-		error = xfs_reclaim_inode_valid(mp, ip);
-		if (error) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-		read_unlock(&pag->pag_ici_lock);
-
-		/*
-		 * hmmm - this is an inode already in reclaim. Do
-		 * we even bother catching it here?
-		 */
-		if (xfs_reclaim_inode(ip, 0, mode))
-			skipped++;
-	} while (nr_found);
-
-	if (skipped) {
-		delay(1);
-		goto restart;
-	}
-	return;
-
+	return xfs_reclaim_inode(ip, 0, flags);
 }
 
 int
@@ -797,14 +782,7 @@ xfs_reclaim_inodes(
 	xfs_mount_t	*mp,
 	int		mode)
 {
-	int		i;
-
-	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-		if (!mp->m_perag[i].pag_ici_init)
-			continue;
-		xfs_reclaim_inodes_ag(mp, i, mode);
-	}
-	return 0;
+	return xfs_inode_ag_iterator(mp, xfs_reclaim_inode_valid,
+					xfs_reclaim_inode_now, mode,
+					XFS_ICI_RECLAIM_TAG);
 }
-
-
-- 
1.6.2

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

Re: [PATCH 0/6] [XFS] introduce a AG inode tree walker

[Christoph Hellwig]

On Sun, Mar 15, 2009 at 10:46:37PM +1100, Dave Chinner wrote:
> This series splits up the sync and reclaim code into three
> separate actions. The first is the tree walker, the second is
> the inode validation and the third is the operation to execute
> on the inode.
> 
> This allows us to somewhat abstract the radix tree away from the
> act of walking the cached inodes and puts in place mechanisms that
> can be extended for bulk inode cache lookups.
> 
> This also splits the inode writeback into separate data and metadata
> sync operations and optimises them a little......

Just did a quick XFSQA run with your whole patch series applied and
I get this oops in 001:

[   37.023076] Filesystem "vdb": Disabling barriers, not supported with external log device
[   37.055812] XFS mounting filesystem vdb
001 9s ...
[   46.688828] BUG: unable to handle kernel NULL pointer dereference at 00000332
[   46.711217] IP: [<c0477be5>] xfs_sync_inode_data+0x15/0xd0
[   46.733131] *pde = 00000000 
[   46.744335] Oops: 0000 [#1] SMP 
[   46.748325] last sysfs file: /sys/class/net/lo/operstate
[   46.748325] Modules linked in:
[   46.748325] 
[   46.748325] Pid: 5229, comm: umount Not tainted (2.6.29-rc7-xfs #207) 
[   46.748325] EIP: 0060:[<c0477be5>] EFLAGS: 00010246 CPU: 0
[   46.748325] EIP is at xfs_sync_inode_data+0x15/0xd0
[   46.748325] EAX: 00000002 EBX: 00000002 ECX: f2784530 EDX: 00000000
[   46.748325] ESI: c04777c0 EDI: 0000000a EBP: f2799df8 ESP: f2799dd8
[   46.748325]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[   46.748325] Process umount (pid: 5229, ti=f2798000 task=f2784530 task.ti=f2798000)
[   46.748325] Stack:
[   46.748325]  00000001 f2799e04 c04777c0 f57b168c f6fc99d0 f57b1638 c04777c0 f5654230
[   46.748325]  f2799e20 c0477de3 f2799e10 ffffffff 00000000 00000000 00000084 00000000
[   46.748325]  f5654230 00000000 f2799e48 c0477eae c0477bd0 0000000a ffffffff c0477bd0
[   46.748325] Call Trace:
[   46.748325]  [<c04777c0>] ? xfs_sync_inode_valid+0x0/0xa0
[   46.748325]  [<c04777c0>] ? xfs_sync_inode_valid+0x0/0xa0
[   46.748325]  [<c0477de3>] ? xfs_inode_ag_walk+0x63/0xd0
[   46.748325]  [<c0477eae>] ? xfs_inode_ag_iterator+0x5e/0xa0
[   46.748325]  [<c0477bd0>] ? xfs_sync_inode_data+0x0/0xd0
[   46.748325]  [<c0477bd0>] ? xfs_sync_inode_data+0x0/0xd0
[   46.748325]  [<c04777c0>] ? xfs_sync_inode_valid+0x0/0xa0
[   46.748325]  [<c047803c>] ? xfs_sync_inodes+0x7c/0xc0
[   46.748325]  [<c04781e1>] ? xfs_quiesce_data+0x11/0x70
[   46.748325]  [<c04754cf>] ? xfs_fs_sync_super+0x3f/0xe0
[   46.748325]  [<c04003e0>] ? xfs_fs_quota_sync+0x0/0x40
[   46.748325]  [<c0205bd5>] ? quota_sync_sb+0x35/0xf0
[   46.748325]  [<c04003e0>] ? xfs_fs_quota_sync+0x0/0x40
[   46.748325]  [<c0205cb3>] ? sync_dquots+0x23/0x140
[   46.748325]  [<c01c7b19>] ? __fsync_super+0x19/0x70
[   46.748325]  [<c01c7b7b>] ? fsync_super+0xb/0x20
[   46.748325]  [<c01c7df2>] ? generic_shutdown_super+0x22/0xf0
[   46.748325]  [<c07be830>] ? down_write+0x80/0x90
[   46.748325]  [<c01c7ee5>] ? kill_block_super+0x25/0x40
[   46.748325]  [<c01c81ba>] ? deactivate_super+0x7a/0x90
[   46.748325]  [<c01dbf42>] ? mntput_no_expire+0xe2/0x110
[   46.748325]  [<c01dc21f>] ? sys_umount+0x4f/0x310
[   46.748325]  [<c01dc4f9>] ? sys_oldumount+0x19/0x20
[   46.748325]  [<c0120c7e>] ? syscall_call+0x7/0xb
[   46.748325]  [<c0120000>] ? sys_sigreturn+0x20/0xe0
[   46.748325] Code: ff 84 c0 74 ed ba 06 00 00 00 89 d8 e8 d5 10 fd ff 89 c6 eb b3 90 55 89 e5 83 ec 20 89 5d f4 89 c3 89 7d fc 89 d7 31 d2 89 75 f8 <8b> 80 30 03 00 00 e8 40 75 d2 ff 85 c0 74 6e f7 c7 20 00 00 00 
[   46.748325] EIP: [<c0477be5>] xfs_sync_inode_data+0x15/0xd0 SS:ESP 0068:f2799dd8
[   47.513225] ---[ end trace 4957da56f1d7015c ]---


> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

Re: [PATCH 0/6] [XFS] introduce a AG inode tree walker

[Dave Chinner]

On Mon, Mar 16, 2009 at 03:53:38AM -0400, Christoph Hellwig wrote:
> On Sun, Mar 15, 2009 at 10:46:37PM +1100, Dave Chinner wrote:
> > This series splits up the sync and reclaim code into three
> > separate actions. The first is the tree walker, the second is
> > the inode validation and the third is the operation to execute
> > on the inode.
> > 
> > This allows us to somewhat abstract the radix tree away from the
> > act of walking the cached inodes and puts in place mechanisms that
> > can be extended for bulk inode cache lookups.
> > 
> > This also splits the inode writeback into separate data and metadata
> > sync operations and optimises them a little......
> 
> Just did a quick XFSQA run with your whole patch series applied and
> I get this oops in 001:

I haven't seen that - it runs through a UML test run just fine.
I'll see if I can reproduce it...

If you drop the last patch (the iterator patch) does it work
ok?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 0/6] [XFS] introduce a AG inode tree walker

[Christoph Hellwig]

On Mon, Mar 16, 2009 at 08:40:19PM +1100, Dave Chinner wrote:
> If you drop the last patch (the iterator patch) does it work
> ok?

No.  With patches 1-5 applied we deadlock early on because pag_ici_lock
never gets released in xfs_sync_inodes_ag for the normal non-error case.

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

Re: [PATCH 1/6] [XFS] Split inode data writeback from inode sync.

[Christoph Hellwig]

> +static int
> +xfs_sync_inode_data(
> +	struct xfs_inode	*ip,
> +	int			flags)
> +{
> +	struct inode	*inode = VFS_I(ip);
> +	int		error = 0;
> +
> +	if (VN_DIRTY(inode)) {
> +		int	locked = 0;
> +		if (flags & SYNC_TRYLOCK) {
> +			if (xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
> +				locked = 1;
> +		} else {
> +			xfs_ilock(ip, XFS_IOLOCK_SHARED);
> +			locked = 1;
> +		}
> +		if (locked) {
> +			error = xfs_flush_pages(ip, 0, -1,
> +					(flags & SYNC_WAIT) ? 0 : XFS_B_ASYNC,
> +					FI_NONE);
> +			xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> +		}
> +	}
> +
> +	if (flags & SYNC_IOWAIT)
> +		xfs_ioend_wait(ip);
> +
> +	return error;
> +}

In the end this should look more like:



static int
xfs_sync_inode_data(
	struct xfs_inode	*ip,
	int			flags)
{
	struct inode	*inode = VFS_I(ip);
	struct address_space *mapping = inode->i_mapping;
	int		error = 0;

	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
			if (flags & SYNC_TRYLOCK)
				goto out;
			xfs_ilock(ip, XFS_IOLOCK_SHARED);
		}

		if (flags & SYNC_WAIT) {
			error = filemap_write_and_wait(mapping);
		else
			error = filemap_fdatawrite(mapping);

		xfs_iflags_clear(ip, XFS_ITRUNCATED);
		xfs_iunlock(ip, XFS_IOLOCK_SHARED);
	}

 out:
	if (flags & SYNC_IOWAIT)
		xfs_ioend_wait(ip);
	return -error;
}


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

Re: [PATCH 0/6] [XFS] introduce a AG inode tree walker

[Dave Chinner]

On Mon, Mar 16, 2009 at 06:01:16AM -0400, Christoph Hellwig wrote:
> On Mon, Mar 16, 2009 at 08:40:19PM +1100, Dave Chinner wrote:
> > If you drop the last patch (the iterator patch) does it work
> > ok?
> 
> No.  With patches 1-5 applied we deadlock early on because pag_ici_lock
> never gets released in xfs_sync_inodes_ag for the normal non-error case.

Ah, then I screwed up the migration of the patches from one set of
trees to another. I'll go back and rework this one, including your
feedback and repost when done.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/6] [XFS] Split inode data writeback from inode sync.

[Dave Chinner]

On Mon, Mar 16, 2009 at 06:10:08AM -0400, Christoph Hellwig wrote:
> > +static int
> > +xfs_sync_inode_data(
> > +	struct xfs_inode	*ip,
> > +	int			flags)
> > +{
> > +	struct inode	*inode = VFS_I(ip);
> > +	int		error = 0;
> > +
> > +	if (VN_DIRTY(inode)) {
> > +		int	locked = 0;
> > +		if (flags & SYNC_TRYLOCK) {
> > +			if (xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED))
> > +				locked = 1;
> > +		} else {
> > +			xfs_ilock(ip, XFS_IOLOCK_SHARED);
> > +			locked = 1;
> > +		}
> > +		if (locked) {
> > +			error = xfs_flush_pages(ip, 0, -1,
> > +					(flags & SYNC_WAIT) ? 0 : XFS_B_ASYNC,
> > +					FI_NONE);
> > +			xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> > +		}
> > +	}
> > +
> > +	if (flags & SYNC_IOWAIT)
> > +		xfs_ioend_wait(ip);
> > +
> > +	return error;
> > +}
> 
> In the end this should look more like:
> 
> 
> 
> static int
> xfs_sync_inode_data(
> 	struct xfs_inode	*ip,
> 	int			flags)
> {
> 	struct inode	*inode = VFS_I(ip);
> 	struct address_space *mapping = inode->i_mapping;
> 	int		error = 0;
> 
> 	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
> 		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
> 			if (flags & SYNC_TRYLOCK)
> 				goto out;
> 			xfs_ilock(ip, XFS_IOLOCK_SHARED);
> 		}
> 
> 		if (flags & SYNC_WAIT) {
> 			error = filemap_write_and_wait(mapping);
> 		else
> 			error = filemap_fdatawrite(mapping);
> 
> 		xfs_iflags_clear(ip, XFS_ITRUNCATED);
> 		xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> 	}
> 
>  out:
> 	if (flags & SYNC_IOWAIT)
> 		xfs_ioend_wait(ip);
> 	return -error;
> }

Seems sane. I'll rework it around this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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