[PATCH 0/7] inode sync refactoring

[PATCH 0/7] inode sync refactoring

[Christoph Hellwig]

This is a respin of Dave's xfs_sync_inodes refactoring plus a couple of new
patches.  The major difference to the original series is sorting out the
pag_ici_lock locking which was busted in many ways.  We now held the lock
exactly as long as the current code, to do that I had to give up on the
lookup vs execute split for the callbacks which actually improved the
code flow a bit, at the expense of exposing the lock to the callbacks.

The other change is that inode flushing still uses the same old algorithm
instead of bringing over the optimizations from xfs_fs_write_inode which
caused some spurious regressions in test 183 for me.  I still plan to get
these two paths unified, but let's leave that for a separate patch.

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

[PATCH 1/7] xfs: split inode data writeback from xfs_sync_inodes_ag

[Christoph Hellwig]

[-- Attachment #1: sync1.diff --]
[-- Type: text/plain, Size: 2389 bytes --]

From: Dave Chinner <david@fromorbit.com>

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.

[hch: minor cleanups]

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

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-12 23:28:45.023811837 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-13 10:00:27.169659271 +0200
@@ -48,6 +48,34 @@
 #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);
+	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_wait;
+			xfs_ilock(ip, XFS_IOLOCK_SHARED);
+		}
+
+		error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
+					0 : XFS_B_ASYNC, FI_NONE);
+		xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+	}
+
+ out_wait:
+	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.
@@ -123,27 +151,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);

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

[PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Christoph Hellwig]

[-- Attachment #1: sync2.diff --]
[-- Type: text/plain, Size: 2243 bytes --]


In many cases we only want to sync inode metadata. Split out the inode
flushing into a separate helper to prepare factoring the inode sync code.

Based on a patch from Dave Chinner, but redone to keep the current behaviour
exactly and leave changes to the flushing logic to another patch.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:17:41.359813297 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:05:25.545684101 +0200
@@ -76,6 +76,34 @@ xfs_sync_inode_data(
 	return error;
 }
 
+STATIC int
+xfs_sync_inode_attr(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	int			error = 0;
+
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	if (xfs_inode_clean(ip))
+		goto out_unlock;
+	if (!xfs_iflock_nowait(ip)) {
+		if (!(flags & SYNC_WAIT))
+			goto out_unlock;
+		xfs_iflock(ip);
+	}
+
+	if (xfs_inode_clean(ip)) {
+		xfs_ifunlock(ip);
+		goto out_unlock;
+	}
+
+	error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
+
+ out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	return error;
+}
+
 /*
  * Sync all the inodes in the given AG according to the
  * direction given by the flags.
@@ -95,7 +123,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
@@ -154,22 +181,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_attr(ip, flags);
+
+		IRELE(ip);
 
 		if (error)
 			last_error = error;

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

[PATCH 3/7] xfs: factor out inode validation for sync

[Christoph Hellwig]

[-- Attachment #1: sync3.diff --]
[-- Type: text/plain, Size: 2310 bytes --]

From: Dave Chinner <david@fromorbit.com>

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

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

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:19:52.080661336 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:34.251688779 +0200
@@ -49,6 +49,39 @@
 #include <linux/freezer.h>
 
 
+/* must be called with pag_ici_lock held and releases it */
+STATIC int
+xfs_sync_inode_valid(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag)
+{
+	struct inode		*inode = VFS_I(ip);
+
+	/* nothing to sync during shutdown */
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
+		read_unlock(&pag->pag_ici_lock);
+		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.
+	 */
+	if (!igrab(inode)) {
+		read_unlock(&pag->pag_ici_lock);
+		return ENOENT;
+	}
+	read_unlock(&pag->pag_ici_lock);
+
+	if (is_bad_inode(inode) || xfs_iflags_test(ip, XFS_INEW)) {
+		IRELE(ip);
+		return ENOENT;
+	}
+
+	return 0;
+}
+
 STATIC int
 xfs_sync_inode_data(
 	struct xfs_inode	*ip,
@@ -121,7 +154,6 @@ xfs_sync_inodes_ag(
 	int		last_error = 0;
 
 	do {
-		struct inode	*inode;
 		xfs_inode_t	*ip = NULL;
 
 		/*
@@ -150,27 +182,10 @@ xfs_sync_inodes_ag(
 			break;
 		}
 
-		/* nothing to sync during shutdown */
-		if (XFS_FORCED_SHUTDOWN(mp)) {
-			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);
+		error = xfs_sync_inode_valid(ip, pag);
+		if (error) {
+			if (error == EFSCORRUPTED)
+				return 0;
 			continue;
 		}
 

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

[PATCH 4/7] xfs: remove unused parameter from xfs_reclaim_inodes

[Christoph Hellwig]

[-- Attachment #1: sync5.diff --]
[-- Type: text/plain, Size: 3602 bytes --]

From: Dave Chinner <david@fromorbit.com>

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>
Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:34.251688779 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
@@ -383,7 +383,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
@@ -507,7 +507,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_sync(mp, SYNC_BDFLUSH);
 		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
@@ -701,7 +701,6 @@ STATIC void
 xfs_reclaim_inodes_ag(
 	xfs_mount_t	*mp,
 	int		ag,
-	int		noblock,
 	int		mode)
 {
 	xfs_inode_t	*ip = NULL;
@@ -747,25 +746,13 @@ restart:
 			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);
 
@@ -780,7 +767,6 @@ restart:
 int
 xfs_reclaim_inodes(
 	xfs_mount_t	*mp,
-	int		 noblock,
 	int		mode)
 {
 	int		i;
@@ -788,7 +774,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;
 }
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 16:19:44.268809697 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 16:20:37.012658983 +0200
@@ -50,7 +50,7 @@ void xfs_quiesce_attr(struct xfs_mount *
 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);
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2009-05-14 16:19:44.318659340 +0200
+++ xfs/fs/xfs/xfs_mount.c	2009-05-14 16:20:37.013659110 +0200
@@ -1371,7 +1371,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_unmount(mp);
 

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

[PATCH 5/7] xfs: introduce a per-ag inode iterator

[Christoph Hellwig]

[-- Attachment #1: sync6.diff --]
[-- Type: text/plain, Size: 9224 bytes --]

From: Dave Chinner <david@fromorbit.com>

Given that we walk across the per-ag inode lists so often, it makes sense to
introduce an iterator for this.

Convert the sync and reclaim code to use this new iterator, quota code will
follow in the next patch.

[hch: merged the lookup and execute callbacks back into one to get the
 pag_ici_lock locking correct and simplify the code flow]

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

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:22:26.321659103 +0200
@@ -49,6 +49,122 @@
 #include <linux/freezer.h>
 
 
+STATIC xfs_inode_t *
+xfs_inode_ag_lookup(
+	struct xfs_mount	*mp,
+	struct xfs_perag	*pag,
+	uint32_t		*first_index,
+	int			tag)
+{
+	int			nr_found;
+	struct xfs_inode	*ip;
+
+	/*
+	 * 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;
+
+	return ip;
+
+unlock:
+	read_unlock(&pag->pag_ici_lock);
+	return NULL;
+}
+
+STATIC int
+xfs_inode_ag_walk(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		ag,
+	int			(*execute)(struct xfs_inode *ip,
+					   struct xfs_perag *pag, int flags),
+	int			flags,
+	int			tag)
+{
+	struct xfs_perag	*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, &first_index, tag);
+		if (!ip)
+			break;
+
+		error = execute(ip, pag, 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(
+	struct xfs_mount	*mp,
+	int			(*execute)(struct xfs_inode *ip,
+					   struct xfs_perag *pag, 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, execute, flags, tag);
+		if (error)
+			last_error = error;
+		if (error == EFSCORRUPTED)
+			break;
+	}
+	return XFS_ERROR(last_error);
+}
+
 /* must be called with pag_ici_lock held and releases it */
 STATIC int
 xfs_sync_inode_valid(
@@ -85,12 +201,17 @@ xfs_sync_inode_valid(
 STATIC int
 xfs_sync_inode_data(
 	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
 	int			flags)
 {
 	struct inode	*inode = VFS_I(ip);
 	struct address_space *mapping = inode->i_mapping;
 	int		error = 0;
 
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return 0;
+
 	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
 			if (flags & SYNC_TRYLOCK)
@@ -106,16 +227,22 @@ xfs_sync_inode_data(
  out_wait:
 	if (flags & SYNC_IOWAIT)
 		xfs_ioend_wait(ip);
+	IRELE(ip);
 	return error;
 }
 
 STATIC int
 xfs_sync_inode_attr(
 	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
 	int			flags)
 {
 	int			error = 0;
 
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return 0;
+
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	if (xfs_inode_clean(ip))
 		goto out_unlock;
@@ -134,117 +261,33 @@ xfs_sync_inode_attr(
 
  out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	IRELE(ip);
 	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(ip, pag);
-		if (error) {
-			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_attr(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)
-		xfs_log_force(mp, 0, lflags);
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
 
-	return XFS_ERROR(last_error);
+	if (flags & SYNC_ATTR)
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
+
+	if (!error && (flags & SYNC_DELWRI))
+		xfs_log_force(mp, 0, lflags);
+	return XFS_ERROR(error);
 }
 
 STATIC int
@@ -696,72 +739,20 @@ xfs_inode_clear_reclaim_tag(
 	xfs_put_perag(mp, pag);
 }
 
-
-STATIC void
-xfs_reclaim_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	int		mode)
+STATIC int
+xfs_reclaim_inode_now(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	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 {
-		/*
-		 * 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;
-		}
-
-		/* ignore if already under reclaim */
-		if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
+	/* ignore if already under reclaim */
+	if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
 		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 0;
 	}
-	return;
+	read_unlock(&pag->pag_ici_lock);
 
+	return xfs_reclaim_inode(ip, 0, flags);
 }
 
 int
@@ -769,14 +760,6 @@ 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_now, mode,
+					XFS_ICI_RECLAIM_TAG);
 }
-
-

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

[PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

[Christoph Hellwig]

[-- Attachment #1: xfs-quota-sync-iterators --]
[-- Type: text/plain, Size: 5356 bytes --]

Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
quota code.  Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
to allow using them from the quota code.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/quota/xfs_qm_syscalls.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_qm_syscalls.c	2009-05-13 14:52:54.087659167 +0200
+++ xfs/fs/xfs/quota/xfs_qm_syscalls.c	2009-05-13 14:57:36.531661369 +0200
@@ -846,105 +846,55 @@ xfs_qm_export_flags(
 }
 
 
-/*
- * Release all the dquots on the inodes in an AG.
- */
-STATIC void
-xfs_qm_dqrele_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	uint		flags)
+STATIC int
+xfs_dqrele_inode(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	int			flags)
 {
-	xfs_inode_t	*ip = NULL;
-	xfs_perag_t	*pag = &mp->m_perag[ag];
-	int		first_index = 0;
-	int		nr_found;
-
-	do {
-		/*
-		 * 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;
-		}
-
-		/* skip quota inodes */
-		if (ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
-			ASSERT(ip->i_udquot == NULL);
-			ASSERT(ip->i_gdquot == NULL);
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
+	int			error;
 
-		/*
-		 * If we can't get a reference on the inode, it must be
-		 * in reclaim. Leave it for the reclaim code to flush.
-		 */
-		if (!igrab(VFS_I(ip))) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
+	/* skip quota inodes */
+	if (ip == XFS_QI_UQIP(ip->i_mount) || ip == XFS_QI_GQIP(ip->i_mount)) {
+		ASSERT(ip->i_udquot == NULL);
+		ASSERT(ip->i_gdquot == NULL);
 		read_unlock(&pag->pag_ici_lock);
+		return 0;
+	}
 
-		/* avoid new inodes though we shouldn't find any here */
-		if (xfs_iflags_test(ip, XFS_INEW)) {
-			IRELE(ip);
-			continue;
-		}
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return 0;
 
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
-			xfs_qm_dqrele(ip->i_udquot);
-			ip->i_udquot = NULL;
-		}
-		if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) &&
-		    ip->i_gdquot) {
-			xfs_qm_dqrele(ip->i_gdquot);
-			ip->i_gdquot = NULL;
-		}
-		xfs_iput(ip, XFS_ILOCK_EXCL);
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+		xfs_qm_dqrele(ip->i_udquot);
+		ip->i_udquot = NULL;
+	}
+	if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) && ip->i_gdquot) {
+		xfs_qm_dqrele(ip->i_gdquot);
+		ip->i_gdquot = NULL;
+	}
+	xfs_iput(ip, XFS_ILOCK_EXCL);
+	IRELE(ip);
 
-	} while (nr_found);
+	return 0;
 }
 
+
 /*
  * Go thru all the inodes in the file system, releasing their dquots.
+ *
  * Note that the mount structure gets modified to indicate that quotas are off
- * AFTER this, in the case of quotaoff. This also gets called from
- * xfs_rootumount.
+ * AFTER this, in the case of quotaoff.
  */
 void
 xfs_qm_dqrele_all_inodes(
 	struct xfs_mount *mp,
 	uint		 flags)
 {
-	int		i;
-
 	ASSERT(mp->m_quotainfo);
-	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-		if (!mp->m_perag[i].pag_ici_init)
-			continue;
-		xfs_qm_dqrele_inodes_ag(mp, i, flags);
-	}
+	xfs_inode_ag_iterator(mp, xfs_dqrele_inode, flags, -1);
 }
 
 /*------------------------------------------------------------------------*/
Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-13 14:52:54.093659302 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-13 14:57:18.322814622 +0200
@@ -141,7 +141,7 @@ restart:
 	return last_error;
 }
 
-STATIC int
+int
 xfs_inode_ag_iterator(
 	struct xfs_mount	*mp,
 	int			(*execute)(struct xfs_inode *ip,
@@ -166,7 +166,7 @@ xfs_inode_ag_iterator(
 }
 
 /* must be called with pag_ici_lock held and releases it */
-STATIC int
+int
 xfs_sync_inode_valid(
 	struct xfs_inode	*ip,
 	struct xfs_perag	*pag)
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-13 14:52:54.100659565 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-13 14:57:18.331814510 +0200
@@ -56,4 +56,10 @@ void xfs_inode_set_reclaim_tag(struct xf
 void xfs_inode_clear_reclaim_tag(struct xfs_inode *ip);
 void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
 				struct xfs_inode *ip);
+
+int xfs_sync_inode_valid(struct xfs_inode *ip, struct xfs_perag *pag);
+int xfs_inode_ag_iterator(struct xfs_mount *mp,
+	int (*execute)(struct xfs_inode *ip, struct xfs_perag *pag, int flags),
+	int flags, int tag);
+
 #endif

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

[PATCH 7/7] xfs: split xfs_sync_inodes

[Christoph Hellwig]

[-- Attachment #1: xfs-split-xfs_sync_inodes --]
[-- Type: text/plain, Size: 7000 bytes --]

xfs_sync_inodes is used to write back either file data or inode metadata.
In generally we always do these separately, except for one fishy case in
xfs_fs_put_super that does both.  So separate xfs_sync_inodes into
separate xfs_sync_data and xfs_sync_attr functions.  In xfs_fs_put_super
we first call the data sync and then the attr sync as that was the previous
order.  The moved log force in that path doesn't make a different because
we will force the log again as part of the real unmount process.

The filesystem readonly checks are not performed by the new function but
instead moved into the callers, given that most callers alredy have it
further up in the stack.  Also add debug checks that we do not pass in
incorrect flags in the new xfs_sync_data and xfs_sync_attr function and
fix the one place that did pass in a wrong flag.

Also remove a comment mentioning xfs_sync_inodes that has been incorrect
for a while because we always take either the iolock or ilock in the
sync path these days.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:00.178792110 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:05.278808755 +0200
@@ -1070,7 +1070,18 @@ xfs_fs_put_super(
 	int			unmount_event_flags = 0;
 
 	xfs_syncd_stop(mp);
-	xfs_sync_inodes(mp, SYNC_ATTR|SYNC_DELWRI);
+
+	if (!(sb->s_flags & MS_RDONLY)) {
+		/*
+		 * XXX(hch): this should be SYNC_WAIT.
+		 *
+		 * Or more likely no needed at all because the VFS is already
+		 * calling ->sync_fs after shutting down all filestem
+		 * operations and just before calling ->put_super.
+		 */
+		xfs_sync_data(mp, 0);
+		xfs_sync_attr(mp, 0);
+	}
 
 #ifdef HAVE_DMAPI
 	if (mp->m_flags & XFS_MOUNT_DMAPI) {
Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:09:04.687659175 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:09:05.279808603 +0200
@@ -265,29 +265,40 @@ xfs_sync_inode_attr(
 	return error;
 }
 
+/*
+ * Write out pagecache data for the whole filesystem.
+ */
 int
-xfs_sync_inodes(
-	xfs_mount_t	*mp,
-	int		flags)
+xfs_sync_data(
+	struct xfs_mount	*mp,
+	int			flags)
 {
-	int		error = 0;
-	int		lflags = XFS_LOG_FORCE;
+	int			error;
 
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return 0;
+	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT|SYNC_IOWAIT)) == 0);
 
-	if (flags & SYNC_WAIT)
-		lflags |= XFS_LOG_SYNC;
+	error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
+	if (error)
+		return XFS_ERROR(error);
 
-	if (flags & SYNC_DELWRI)
-		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
+	xfs_log_force(mp, 0,
+		      (flags & SYNC_WAIT) ?
+		       XFS_LOG_FORCE | XFS_LOG_SYNC :
+		       XFS_LOG_FORCE);
+	return 0;
+}
 
-	if (flags & SYNC_ATTR)
-		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
+/*
+ * Write out inode metadata (attributes) for the whole filesystem.
+ */
+int
+xfs_sync_attr(
+	struct xfs_mount	*mp,
+	int			flags)
+{
+	ASSERT((flags & ~SYNC_WAIT) == 0);
 
-	if (!error && (flags & SYNC_DELWRI))
-		xfs_log_force(mp, 0, lflags);
-	return XFS_ERROR(error);
+	return xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
 }
 
 STATIC int
@@ -401,12 +412,12 @@ xfs_quiesce_data(
 	int error;
 
 	/* push non-blocking */
-	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_BDFLUSH);
+	xfs_sync_data(mp, 0);
 	xfs_qm_sync(mp, SYNC_BDFLUSH);
 	xfs_filestream_flush(mp);
 
 	/* push and block */
-	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_WAIT|SYNC_IOWAIT);
+	xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
 	xfs_qm_sync(mp, SYNC_WAIT);
 
 	/* write superblock and hoover up shutdown errors */
@@ -435,7 +446,7 @@ xfs_quiesce_fs(
 	 * logged before we can write the unmount record.
 	 */
 	do {
-		xfs_sync_inodes(mp, SYNC_ATTR|SYNC_WAIT);
+		xfs_sync_attr(mp, SYNC_WAIT);
 		pincount = xfs_flush_buftarg(mp->m_ddev_targp, 1);
 		if (!pincount) {
 			delay(50);
@@ -518,8 +529,8 @@ xfs_flush_inodes_work(
 	void		*arg)
 {
 	struct inode	*inode = arg;
-	xfs_sync_inodes(mp, SYNC_DELWRI | SYNC_TRYLOCK);
-	xfs_sync_inodes(mp, SYNC_DELWRI | SYNC_TRYLOCK | SYNC_IOWAIT);
+	xfs_sync_data(mp, SYNC_TRYLOCK);
+	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);
 	iput(inode);
 }
 
Index: xfs/fs/xfs/linux-2.6/xfs_quotaops.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_quotaops.c	2009-05-14 19:05:24.908659400 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_quotaops.c	2009-05-14 19:09:05.280834851 +0200
@@ -50,9 +50,11 @@ xfs_fs_quota_sync(
 {
 	struct xfs_mount	*mp = XFS_M(sb);
 
+	if (sb->s_flags & MS_RDONLY)
+		return -EROFS;
 	if (!XFS_IS_QUOTA_RUNNING(mp))
 		return -ENOSYS;
-	return -xfs_sync_inodes(mp, SYNC_DELWRI);
+	return -xfs_sync_data(mp, 0);
 }
 
 STATIC int
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 19:09:04.694659368 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 19:09:05.280834851 +0200
@@ -29,8 +29,6 @@ typedef struct xfs_sync_work {
 	struct completion	*w_completion;
 } xfs_sync_work_t;
 
-#define SYNC_ATTR		0x0001	/* sync attributes */
-#define SYNC_DELWRI		0x0002	/* look at delayed writes */
 #define SYNC_WAIT		0x0004	/* wait for i/o to complete */
 #define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
 #define SYNC_IOWAIT		0x0010  /* wait for all I/O to complete */
@@ -41,7 +39,8 @@ void xfs_syncd_stop(struct xfs_mount *mp
 
 int xfs_inode_flush(struct xfs_inode *ip, int sync);
 
-int xfs_sync_inodes(struct xfs_mount *mp, int flags);
+int xfs_sync_attr(struct xfs_mount *mp, int flags);
+int xfs_sync_data(struct xfs_mount *mp, int flags);
 int xfs_sync_fsdata(struct xfs_mount *mp, int flags);
 
 int xfs_quiesce_data(struct xfs_mount *mp);
Index: xfs/fs/xfs/xfs_filestream.c
===================================================================
--- xfs.orig/fs/xfs/xfs_filestream.c	2009-05-14 19:05:24.929659282 +0200
+++ xfs/fs/xfs/xfs_filestream.c	2009-05-14 19:09:05.283807995 +0200
@@ -542,10 +542,8 @@ xfs_filestream_associate(
 	 * waiting for the lock because someone else is waiting on the lock we
 	 * hold and we cannot drop that as we are in a transaction here.
 	 *
-	 * Lucky for us, this inversion is rarely a problem because it's a
-	 * directory inode that we are trying to lock here and that means the
-	 * only place that matters is xfs_sync_inodes() and SYNC_DELWRI is
-	 * used. i.e. freeze, remount-ro, quotasync or unmount.
+	 * Lucky for us, this inversion is not a problem because it's a
+	 * directory inode that we are trying to lock here.
 	 *
 	 * So, if we can't get the iolock without sleeping then just give up
 	 */

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

Re: [PATCH 1/7] xfs: split inode data writeback from xfs_sync_inodes_ag

[Sujit Karataparambil]

> +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_wait;
> +                       xfs_ilock(ip, XFS_IOLOCK_SHARED);
> +               }
> +
> +               error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
> +                                       0 : XFS_B_ASYNC, FI_NONE);
> +               xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> +       }
> +
> + out_wait:
> +       if (flags & SYNC_IOWAIT)
> +               xfs_ioend_wait(ip);
> +       return error;
> +}
> +
 should not there be an.

 error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
                       0 : XFS_B_ASYNC, FI_NONE);

for the out_wait. This will ensure flush while the xfs_ioend_wait is being
waited for. Would this be an better way to flush the data than waiting for
the inode to be flushed during power off or scheduler cycles.
Would this be an performance hit.

or better use

  error = xfs_sync_inode_data(ip, flags);


-- 
-- Sujit K M

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

Re: [PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Sujit Karataparambil]

> +STATIC int
> +xfs_sync_inode_attr(
> +       struct xfs_inode        *ip,
> +       int                     flags)
> +{
> +       int                     error = 0;
> +
> +       xfs_ilock(ip, XFS_ILOCK_SHARED);
> +       if (xfs_inode_clean(ip))
> +               goto out_unlock;
> +       if (!xfs_iflock_nowait(ip)) {
> +               if (!(flags & SYNC_WAIT))
> +                       goto out_unlock;
> +               xfs_iflock(ip);
> +       }
> +
> +       if (xfs_inode_clean(ip)) {
> +               xfs_ifunlock(ip);
> +               goto out_unlock;
> +       }
> +
> +       error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
> +
> + out_unlock:
> +       xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +       return error;

here also should there be an
xfs_sync_inode_data(ip, flags);
xfs_iflush(ip, XFS_IFLUSH_SYNC);

in out_unlock.



-- 
-- Sujit K M

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

Re: [PATCH 1/7] xfs: split inode data writeback from xfs_sync_inodes_ag

[Christoph Hellwig]

On Fri, May 15, 2009 at 10:19:11AM +0530, Sujit Karataparambil wrote:
>  should not there be an.
> 
>  error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
>                        0 : XFS_B_ASYNC, FI_NONE);
> 
> for the out_wait. This will ensure flush while the xfs_ioend_wait is being
> waited for. Would this be an better way to flush the data than waiting for
> the inode to be flushed during power off or scheduler cycles.
> Would this be an performance hit.

For now I don't want to change behaviour here.    It only matters for
the SYNC_TRYLOCK case which is used for delalloc flushing on ENOSPC, so
it's not too important.

That beeing said I don't really like the current implementation where we
have a SYNC_WAIT that waits for completion of data I/O and need a
separate SYNC_IOWAIT that waits for the after data I/O metadata
transaction completions.  I think we would be better unifying the two,
especially given the current callers:

fs/xfs/linux-2.6/xfs_quotaops.c:        return -xfs_sync_data(mp, 0);
fs/xfs/linux-2.6/xfs_super.c:           xfs_sync_data(mp, 0);
fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, 0);
fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_TRYLOCK);
fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);

So in most cases we do a purely asynchronous writeout, we have one case
that does a full synchronous writeout (SYNC_WAIT|SYNC_IOWAIT) and we
have the two ENOSPC flushing cases doing SYNC_TRYLOCK + async writeout
and SYNC_TRYLOCK + IOWAIT.  I don't really see any reason to only do the
IOWAIT here and will try to unify the two flags at some point.

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

Re: [PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Christoph Hellwig]

On Fri, May 15, 2009 at 10:22:31AM +0530, Sujit Karataparambil wrote:
> here also should there be an
> xfs_sync_inode_data(ip, flags);
> xfs_iflush(ip, XFS_IFLUSH_SYNC);
> 
> in out_unlock.

No.  We only want to perform the iflush if all the preconditions are met
in the !SYNC_WAIT case.  And we certainly do not want to do a data
writeout from the metadata flush case - for one thing it does require
the iolock, but most importantly the point of this series is to separate
the two actions.

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

Re: [PATCH 1/7] xfs: split inode data writeback from xfs_sync_inodes_ag

[Dave Chinner]

On Fri, May 15, 2009 at 01:21:22PM -0400, Christoph Hellwig wrote:
> That beeing said I don't really like the current implementation where we
> have a SYNC_WAIT that waits for completion of data I/O and need a
> separate SYNC_IOWAIT that waits for the after data I/O metadata
> transaction completions.

SYNC_IOWAIT is really only for waiting for direct IO to complete.
e.g. for synchronisation with truncate. I'm not sure that it even
matters for pure buffered data writeback.

> fs/xfs/linux-2.6/xfs_quotaops.c:        return -xfs_sync_data(mp, 0);
> fs/xfs/linux-2.6/xfs_super.c:           xfs_sync_data(mp, 0);
> fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, 0);
> fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
> fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_TRYLOCK);
> fs/xfs/linux-2.6/xfs_sync.c:    xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);

And only the data queisce cares about direct IO (i.e. for freeze)
so I suspect that the SYNC_IOWAIT now could be removed and we
use SYNC_WAIT to trigger the ioend completion wait....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/7] xfs: split inode data writeback from xfs_sync_inodes_ag

[Eric Sandeen]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
> 
> 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.
> 
> [hch: minor cleanups]
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks nice.... xfs_sync_inode_data(), now with 100% more readability!
the old lock_flags uage was icky.

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-12 23:28:45.023811837 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-13 10:00:27.169659271 +0200
> @@ -48,6 +48,34 @@
>  #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);
> +	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_wait;
> +			xfs_ilock(ip, XFS_IOLOCK_SHARED);
> +		}
> +
> +		error = xfs_flush_pages(ip, 0, -1, (flags & SYNC_WAIT) ?
> +					0 : XFS_B_ASYNC, FI_NONE);
> +		xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> +	}
> +
> + out_wait:
> +	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.
> @@ -123,27 +151,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);
> 
> _______________________________________________
> 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

Re: [PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Eric Sandeen]

Christoph Hellwig wrote:
> In many cases we only want to sync inode metadata. Split out the inode
> flushing into a separate helper to prepare factoring the inode sync code.
> 
> Based on a patch from Dave Chinner, but redone to keep the current behaviour
> exactly and leave changes to the flushing logic to another patch.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:17:41.359813297 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:05:25.545684101 +0200
> @@ -76,6 +76,34 @@ xfs_sync_inode_data(
>  	return error;
>  }
>  
> +STATIC int
> +xfs_sync_inode_attr(
> +	struct xfs_inode	*ip,
> +	int			flags)
> +{
> +	int			error = 0;
> +
> +	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	if (xfs_inode_clean(ip))
> +		goto out_unlock;
> +	if (!xfs_iflock_nowait(ip)) {
> +		if (!(flags & SYNC_WAIT))
> +			goto out_unlock;
> +		xfs_iflock(ip);
> +	}
> +
> +	if (xfs_inode_clean(ip)) {
> +		xfs_ifunlock(ip);
> +		goto out_unlock;
> +	}
> +
> +	error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
> +
> + out_unlock:
> +	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	return error;
> +}
> +
>  /*
>   * Sync all the inodes in the given AG according to the
>   * direction given by the flags.

...

> @@ -154,22 +181,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);

What happened to the XFS_IFLUSH_DELWRI case?

You mentioned "keep the current behavior exactly" but this seems like a
change, no?

-Eric

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

Re: [PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Christoph Hellwig]

On Tue, May 26, 2009 at 03:45:47PM -0500, Eric Sandeen wrote:
> What happened to the XFS_IFLUSH_DELWRI case?
> 
> You mentioned "keep the current behavior exactly" but this seems like a
> change, no?

Yeah, this got lost when playing with variations of the patch.  Correct
version below:

Subject: xfs: split inode flushing from xfs_sync_inodes_ag
From: Christoph Hellwig <hch@lst.de>


In many cases we only want to sync inode metadata. Split out the inode
flushing into a separate helper to prepare factoring the inode sync code.

Based on a patch from Dave Chinner, but redone to keep the current behaviour
exactly and leave changes to the flushing logic to another patch.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 12:08:49.137850883 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 12:11:09.530815659 +0200
@@ -77,6 +77,35 @@ xfs_sync_inode_data(
 	return error;
 }
 
+STATIC int
+xfs_sync_inode_attr(
+	struct xfs_inode	*ip,
+	int			flags)
+{
+	int			error = 0;
+
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	if (xfs_inode_clean(ip))
+		goto out_unlock;
+	if (!xfs_iflock_nowait(ip)) {
+		if (!(flags & SYNC_WAIT))
+			goto out_unlock;
+		xfs_iflock(ip);
+	}
+
+	if (xfs_inode_clean(ip)) {
+		xfs_ifunlock(ip);
+		goto out_unlock;
+	}
+
+	error = xfs_iflush(ip, (flags & SYNC_WAIT) ?
+			   XFS_IFLUSH_SYNC : XFS_IFLUSH_DELWRI);
+
+ out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	return error;
+}
+
 /*
  * Sync all the inodes in the given AG according to the
  * direction given by the flags.
@@ -96,7 +125,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
@@ -155,22 +183,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_attr(ip, flags);
+
+		IRELE(ip);
 
 		if (error)
 			last_error = error;

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

Re: [PATCH 2/7] xfs: split inode flushing from xfs_sync_inodes_ag

[Eric Sandeen]

Christoph Hellwig wrote:
> On Tue, May 26, 2009 at 03:45:47PM -0500, Eric Sandeen wrote:
>> What happened to the XFS_IFLUSH_DELWRI case?
>>
>> You mentioned "keep the current behavior exactly" but this seems like a
>> change, no?
> 
> Yeah, this got lost when playing with variations of the patch.  Correct
> version below:
> 
> Subject: xfs: split inode flushing from xfs_sync_inodes_ag
> From: Christoph Hellwig <hch@lst.de>
> 
> 
> In many cases we only want to sync inode metadata. Split out the inode
> flushing into a separate helper to prepare factoring the inode sync code.
> 
> Based on a patch from Dave Chinner, but redone to keep the current behaviour
> exactly and leave changes to the flushing logic to another patch.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

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

Re: [PATCH 3/7] xfs: factor out inode validation for sync

[Eric Sandeen]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
> 
> Separate the validation of inodes found by the radix
> tree walk from the radix tree lookup.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:19:52.080661336 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:34.251688779 +0200
> @@ -49,6 +49,39 @@
>  #include <linux/freezer.h>
>  
>  
> +/* must be called with pag_ici_lock held and releases it */
> +STATIC int
> +xfs_sync_inode_valid(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag)
> +{
> +	struct inode		*inode = VFS_I(ip);
> +
> +	/* nothing to sync during shutdown */
> +	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
> +		read_unlock(&pag->pag_ici_lock);
> +		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.
> +	 */
> +	if (!igrab(inode)) {
> +		read_unlock(&pag->pag_ici_lock);
> +		return ENOENT;
> +	}
> +	read_unlock(&pag->pag_ici_lock);
> +
> +	if (is_bad_inode(inode) || xfs_iflags_test(ip, XFS_INEW)) {
> +		IRELE(ip);
> +		return ENOENT;
> +	}
> +
> +	return 0;
> +}
> +
>  STATIC int
>  xfs_sync_inode_data(
>  	struct xfs_inode	*ip,
> @@ -121,7 +154,6 @@ xfs_sync_inodes_ag(
>  	int		last_error = 0;
>  
>  	do {
> -		struct inode	*inode;
>  		xfs_inode_t	*ip = NULL;
>  
>  		/*
> @@ -150,27 +182,10 @@ xfs_sync_inodes_ag(
>  			break;
>  		}
>  
> -		/* nothing to sync during shutdown */
> -		if (XFS_FORCED_SHUTDOWN(mp)) {
> -			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);
> +		error = xfs_sync_inode_valid(ip, pag);
> +		if (error) {
> +			if (error == EFSCORRUPTED)
> +				return 0;
>  			continue;
>  		}


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

Re: [PATCH 4/7] xfs: remove unused parameter from xfs_reclaim_inodes

[Eric Sandeen]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
>
> 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>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:34.251688779 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
> @@ -383,7 +383,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
> @@ -507,7 +507,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_sync(mp, SYNC_BDFLUSH);
>  		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
> @@ -701,7 +701,6 @@ STATIC void
>  xfs_reclaim_inodes_ag(
>  	xfs_mount_t	*mp,
>  	int		ag,
> -	int		noblock,
>  	int		mode)
>  {
>  	xfs_inode_t	*ip = NULL;
> @@ -747,25 +746,13 @@ restart:
>  			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);
>  
> @@ -780,7 +767,6 @@ restart:
>  int
>  xfs_reclaim_inodes(
>  	xfs_mount_t	*mp,
> -	int		 noblock,
>  	int		mode)
>  {
>  	int		i;
> @@ -788,7 +774,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;
>  }
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 16:19:44.268809697 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 16:20:37.012658983 +0200
> @@ -50,7 +50,7 @@ void xfs_quiesce_attr(struct xfs_mount *
>  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);
> Index: xfs/fs/xfs/xfs_mount.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_mount.c	2009-05-14 16:19:44.318659340 +0200
> +++ xfs/fs/xfs/xfs_mount.c	2009-05-14 16:20:37.013659110 +0200
> @@ -1371,7 +1371,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_unmount(mp);

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

[PATCH 8/7] xfs: remove SYNC_IOWAIT

[Christoph Hellwig]

We want to wait for all I/O to finish when we do data integrity syncs.  So
there is no reason to keep SYNC_WAIT separate from SYNC_IOWAIT.  This
causes a little change in behaviour for the ENOSPC flushing code which no
does a second submission and wait of buffered I/O, but that should finish
ASAP as we already did an asynchronous writeout earlier.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 12:59:57.115813662 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:01:14.634816358 +0200
@@ -226,7 +226,7 @@ xfs_sync_inode_data(
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
  out_wait:
-	if (flags & SYNC_IOWAIT)
+	if (flags & SYNC_WAIT)
 		xfs_ioend_wait(ip);
 	IRELE(ip);
 	return error;
@@ -277,7 +277,7 @@ xfs_sync_data(
 {
 	int			error;
 
-	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT|SYNC_IOWAIT)) == 0);
+	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
 
 	error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
 	if (error)
@@ -419,7 +419,7 @@ xfs_quiesce_data(
 	xfs_filestream_flush(mp);
 
 	/* push and block */
-	xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
+	xfs_sync_data(mp, SYNC_WAIT);
 	xfs_qm_sync(mp, SYNC_WAIT);
 
 	/* write superblock and hoover up shutdown errors */
@@ -532,7 +532,7 @@ xfs_flush_inodes_work(
 {
 	struct inode	*inode = arg;
 	xfs_sync_data(mp, SYNC_TRYLOCK);
-	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);
+	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_WAIT);
 	iput(inode);
 }
 
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:00:29.045814647 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:01:39.162941539 +0200
@@ -31,7 +31,6 @@ typedef struct xfs_sync_work {
 
 #define SYNC_WAIT		0x0004	/* wait for i/o to complete */
 #define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
-#define SYNC_IOWAIT		0x0010  /* wait for all I/O to complete */
 #define SYNC_TRYLOCK		0x0020  /* only try to lock inodes */
 
 int xfs_syncd_init(struct xfs_mount *mp);

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

[PATCH 9/7] xfs: remove SYNC_BDFLUSH

[Christoph Hellwig]

SYNC_BDFLUSH is a leftover from IRIX and rather misnamed for todays
code.  Make xfs_sync_fsdata and xfs_dq_sync use the SYNC_TRYLOCK flag
for not blocking on logs just as the inode sync code already does.

For xfs_sync_fsdata it's a trivial 1:1 replacement, but for xfs_qm_sync
I use the opportunity to decouple the non-blocking lock case from the
different flushing modes, similar to the inode sync code.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:02:57.426938830 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:07:53.227939055 +0200
@@ -350,7 +350,7 @@ xfs_sync_fsdata(
 	 * If this is xfssyncd() then only sync the superblock if we can
 	 * lock it without sleeping and it is not pinned.
 	 */
-	if (flags & SYNC_BDFLUSH) {
+	if (flags & SYNC_TRYLOCK) {
 		ASSERT(!(flags & SYNC_WAIT));
 
 		bp = xfs_getsb(mp, XFS_BUF_TRYLOCK);
@@ -415,7 +415,7 @@ xfs_quiesce_data(
 
 	/* push non-blocking */
 	xfs_sync_data(mp, 0);
-	xfs_qm_sync(mp, SYNC_BDFLUSH);
+	xfs_qm_sync(mp, SYNC_TRYLOCK);
 	xfs_filestream_flush(mp);
 
 	/* push and block */
@@ -565,8 +565,8 @@ xfs_sync_worker(
 		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
 		xfs_reclaim_inodes(mp, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
 		/* dgc: errors ignored here */
-		error = xfs_qm_sync(mp, SYNC_BDFLUSH);
-		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
+		error = xfs_qm_sync(mp, SYNC_TRYLOCK);
+		error = xfs_sync_fsdata(mp, SYNC_TRYLOCK);
 		if (xfs_log_need_covered(mp))
 			error = xfs_commit_dummy_trans(mp, XFS_LOG_FORCE);
 	}
Index: xfs/fs/xfs/quota/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_qm.c	2009-05-27 13:04:00.607842293 +0200
+++ xfs/fs/xfs/quota/xfs_qm.c	2009-05-27 13:10:21.688940102 +0200
@@ -905,11 +905,6 @@ xfs_qm_dqdetach(
 	}
 }
 
-/*
- * This is called to sync quotas. We can be told to use non-blocking
- * semantics by either the SYNC_BDFLUSH flag or the absence of the
- * SYNC_WAIT flag.
- */
 int
 xfs_qm_sync(
 	xfs_mount_t	*mp,
@@ -918,17 +913,13 @@ xfs_qm_sync(
 	int		recl, restarts;
 	xfs_dquot_t	*dqp;
 	uint		flush_flags;
-	boolean_t	nowait;
 	int		error;
 
 	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
 		return 0;
 
+	flush_flags = (flags & SYNC_WAIT) ? XFS_QMOPT_SYNC : XFS_QMOPT_DELWRI;
 	restarts = 0;
-	/*
-	 * We won't block unless we are asked to.
-	 */
-	nowait = (boolean_t)(flags & SYNC_BDFLUSH || (flags & SYNC_WAIT) == 0);
 
   again:
 	xfs_qm_mplist_lock(mp);
@@ -948,18 +939,10 @@ xfs_qm_sync(
 		 * don't 'seem' to be dirty. ie. don't acquire dqlock.
 		 * This is very similar to what xfs_sync does with inodes.
 		 */
-		if (flags & SYNC_BDFLUSH) {
-			if (! XFS_DQ_IS_DIRTY(dqp))
+		if (flags & SYNC_TRYLOCK) {
+			if (!XFS_DQ_IS_DIRTY(dqp))
 				continue;
-		}
-
-		if (nowait) {
-			/*
-			 * Try to acquire the dquot lock. We are NOT out of
-			 * lock order, but we just don't want to wait for this
-			 * lock, unless somebody wanted us to.
-			 */
-			if (! xfs_qm_dqlock_nowait(dqp))
+			if (!xfs_qm_dqlock_nowait(dqp))
 				continue;
 		} else {
 			xfs_dqlock(dqp);
@@ -976,7 +959,7 @@ xfs_qm_sync(
 		/* XXX a sentinel would be better */
 		recl = XFS_QI_MPLRECLAIMS(mp);
 		if (!xfs_dqflock_nowait(dqp)) {
-			if (nowait) {
+			if (flags & SYNC_TRYLOCK) {
 				xfs_dqunlock(dqp);
 				continue;
 			}
@@ -994,7 +977,6 @@ xfs_qm_sync(
 		 * Let go of the mplist lock. We don't want to hold it
 		 * across a disk write
 		 */
-		flush_flags = (nowait) ? XFS_QMOPT_DELWRI : XFS_QMOPT_SYNC;
 		xfs_qm_mplist_unlock(mp);
 		xfs_dqtrace_entry(dqp, "XQM_SYNC: DQFLUSH");
 		error = xfs_qm_dqflush(dqp, flush_flags);
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:07:58.997814418 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:08:19.922972203 +0200
@@ -29,9 +29,8 @@ typedef struct xfs_sync_work {
 	struct completion	*w_completion;
 } xfs_sync_work_t;
 
-#define SYNC_WAIT		0x0004	/* wait for i/o to complete */
-#define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
-#define SYNC_TRYLOCK		0x0020  /* only try to lock inodes */
+#define SYNC_WAIT		0x0001	/* wait for i/o to complete */
+#define SYNC_TRYLOCK		0x0002  /* only try to lock inodes */
 
 int xfs_syncd_init(struct xfs_mount *mp);
 void xfs_syncd_stop(struct xfs_mount *mp);

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

Re: [PATCH 9/7] xfs: remove SYNC_BDFLUSH

[Sujit Karataparambil]

Hi,
Newbie question?
Kindly tell me where this code is called from do you have pagedeamon
on vfs or is it some thing else?

Sorry for bothering you.

On Thu, May 28, 2009 at 5:49 PM, Christoph Hellwig <hch@infradead.org> wrote:
> SYNC_BDFLUSH is a leftover from IRIX and rather misnamed for todays
> code.  Make xfs_sync_fsdata and xfs_dq_sync use the SYNC_TRYLOCK flag
> for not blocking on logs just as the inode sync code already does.
>
> For xfs_sync_fsdata it's a trivial 1:1 replacement, but for xfs_qm_sync
> I use the opportunity to decouple the non-blocking lock case from the
> different flushing modes, similar to the inode sync code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c        2009-05-27 13:02:57.426938830 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c     2009-05-27 13:07:53.227939055 +0200
> @@ -350,7 +350,7 @@ xfs_sync_fsdata(
>         * If this is xfssyncd() then only sync the superblock if we can
>         * lock it without sleeping and it is not pinned.
>         */
> -       if (flags & SYNC_BDFLUSH) {
> +       if (flags & SYNC_TRYLOCK) {
>                ASSERT(!(flags & SYNC_WAIT));
>
>                bp = xfs_getsb(mp, XFS_BUF_TRYLOCK);
> @@ -415,7 +415,7 @@ xfs_quiesce_data(
>
>        /* push non-blocking */
>        xfs_sync_data(mp, 0);
> -       xfs_qm_sync(mp, SYNC_BDFLUSH);
> +       xfs_qm_sync(mp, SYNC_TRYLOCK);
>        xfs_filestream_flush(mp);
>
>        /* push and block */
> @@ -565,8 +565,8 @@ xfs_sync_worker(
>                xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
>                xfs_reclaim_inodes(mp, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
>                /* dgc: errors ignored here */
> -               error = xfs_qm_sync(mp, SYNC_BDFLUSH);
> -               error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
> +               error = xfs_qm_sync(mp, SYNC_TRYLOCK);
> +               error = xfs_sync_fsdata(mp, SYNC_TRYLOCK);
>                if (xfs_log_need_covered(mp))
>                        error = xfs_commit_dummy_trans(mp, XFS_LOG_FORCE);
>        }
> Index: xfs/fs/xfs/quota/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_qm.c      2009-05-27 13:04:00.607842293 +0200
> +++ xfs/fs/xfs/quota/xfs_qm.c   2009-05-27 13:10:21.688940102 +0200
> @@ -905,11 +905,6 @@ xfs_qm_dqdetach(
>        }
>  }
>
> -/*
> - * This is called to sync quotas. We can be told to use non-blocking
> - * semantics by either the SYNC_BDFLUSH flag or the absence of the
> - * SYNC_WAIT flag.
> - */
>  int
>  xfs_qm_sync(
>        xfs_mount_t     *mp,
> @@ -918,17 +913,13 @@ xfs_qm_sync(
>        int             recl, restarts;
>        xfs_dquot_t     *dqp;
>        uint            flush_flags;
> -       boolean_t       nowait;
>        int             error;
>
>        if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>                return 0;
>
> +       flush_flags = (flags & SYNC_WAIT) ? XFS_QMOPT_SYNC : XFS_QMOPT_DELWRI;
>        restarts = 0;
> -       /*
> -        * We won't block unless we are asked to.
> -        */
> -       nowait = (boolean_t)(flags & SYNC_BDFLUSH || (flags & SYNC_WAIT) == 0);
>
>   again:
>        xfs_qm_mplist_lock(mp);
> @@ -948,18 +939,10 @@ xfs_qm_sync(
>                 * don't 'seem' to be dirty. ie. don't acquire dqlock.
>                 * This is very similar to what xfs_sync does with inodes.
>                 */
> -               if (flags & SYNC_BDFLUSH) {
> -                       if (! XFS_DQ_IS_DIRTY(dqp))
> +               if (flags & SYNC_TRYLOCK) {
> +                       if (!XFS_DQ_IS_DIRTY(dqp))
>                                continue;
> -               }
> -
> -               if (nowait) {
> -                       /*
> -                        * Try to acquire the dquot lock. We are NOT out of
> -                        * lock order, but we just don't want to wait for this
> -                        * lock, unless somebody wanted us to.
> -                        */
> -                       if (! xfs_qm_dqlock_nowait(dqp))
> +                       if (!xfs_qm_dqlock_nowait(dqp))
>                                continue;
>                } else {
>                        xfs_dqlock(dqp);
> @@ -976,7 +959,7 @@ xfs_qm_sync(
>                /* XXX a sentinel would be better */
>                recl = XFS_QI_MPLRECLAIMS(mp);
>                if (!xfs_dqflock_nowait(dqp)) {
> -                       if (nowait) {
> +                       if (flags & SYNC_TRYLOCK) {
>                                xfs_dqunlock(dqp);
>                                continue;
>                        }
> @@ -994,7 +977,6 @@ xfs_qm_sync(
>                 * Let go of the mplist lock. We don't want to hold it
>                 * across a disk write
>                 */
> -               flush_flags = (nowait) ? XFS_QMOPT_DELWRI : XFS_QMOPT_SYNC;
>                xfs_qm_mplist_unlock(mp);
>                xfs_dqtrace_entry(dqp, "XQM_SYNC: DQFLUSH");
>                error = xfs_qm_dqflush(dqp, flush_flags);
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h        2009-05-27 13:07:58.997814418 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h     2009-05-27 13:08:19.922972203 +0200
> @@ -29,9 +29,8 @@ typedef struct xfs_sync_work {
>        struct completion       *w_completion;
>  } xfs_sync_work_t;
>
> -#define SYNC_WAIT              0x0004  /* wait for i/o to complete */
> -#define SYNC_BDFLUSH           0x0008  /* BDFLUSH is calling -- don't block */
> -#define SYNC_TRYLOCK           0x0020  /* only try to lock inodes */
> +#define SYNC_WAIT              0x0001  /* wait for i/o to complete */
> +#define SYNC_TRYLOCK           0x0002  /* only try to lock inodes */
>
>  int xfs_syncd_init(struct xfs_mount *mp);
>  void xfs_syncd_stop(struct xfs_mount *mp);
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>



-- 
-- Sujit K M

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

Re: [PATCH 9/7] xfs: remove SYNC_BDFLUSH

[Christoph Hellwig]

On Fri, May 29, 2009 at 06:49:39PM +0530, Sujit Karataparambil wrote:
> Hi,
> Newbie question?
> Kindly tell me where this code is called from do you have pagedeamon
> on vfs or is it some thing else?

currently all calls with SYNC_BDFLUSH are from xfssyncd.

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

Re: [PATCH 9/7] xfs: remove SYNC_BDFLUSH

[Sujit Karataparambil]

Is this the journalling part of the code?
Does SYNC_BDFLUSH  and SYNC_IOWAIT Determine the journalling?
Is it that the Journals are a part of SYNC_BDFLUSH  and SYNC_WAIT
resource locking?

How does the SYNC_TRYLOCK change the scenario? Is it some sort
of semaphore implementation?

Thanks,




On Sat, May 30, 2009 at 1:40 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 29, 2009 at 06:49:39PM +0530, Sujit Karataparambil wrote:
>> Hi,
>> Newbie question?
>> Kindly tell me where this code is called from do you have pagedeamon
>> on vfs or is it some thing else?
>
> currently all calls with SYNC_BDFLUSH are from xfssyncd.
>
>



-- 
-- Sujit K M

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

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Eric Sandeen]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
> 
> Given that we walk across the per-ag inode lists so often, it makes sense to
> introduce an iterator for this.
> 
> Convert the sync and reclaim code to use this new iterator, quota code will
> follow in the next patch.
> 
> [hch: merged the lookup and execute callbacks back into one to get the
>  pag_ici_lock locking correct and simplify the code flow]
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Somehow I'm finding this hard to review, but...

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:22:26.321659103 +0200

...

> +STATIC int
> +xfs_inode_ag_walk(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		ag,
> +	int			(*execute)(struct xfs_inode *ip,
> +					   struct xfs_perag *pag, int flags),
> +	int			flags,
> +	int			tag)
> +{
> +	struct xfs_perag	*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, &first_index, tag);
> +		if (!ip)
> +			break;
> +
> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}
> +		if (error)
> +			last_error = error;
> +		/*
> +		 * bail out if the filesystem is corrupted.
> +		 */
> +		if (error == EFSCORRUPTED)
> +			break;

Ok so here we are looking for EFSCORRUPTED from the "execute" function.
 This might be xfs_sync_inode_data, xfs_sync_inode_attr, or
xfs_reclaim_inode_now.  But ...

> +
> +	} while (1);

...

> @@ -85,12 +201,17 @@ xfs_sync_inode_valid(
>  STATIC int
>  xfs_sync_inode_data(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	struct inode	*inode = VFS_I(ip);
>  	struct address_space *mapping = inode->i_mapping;
>  	int		error = 0;
>  
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return 0;xfs_sync_inode_attr(
> +

xfs_sync_inode_valid can return 0, ENOENT, or EFSCORRUPTED.

Aren't we losing the error here...

>  	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
>  		if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
>  			if (flags & SYNC_TRYLOCK)
> @@ -106,16 +227,22 @@ xfs_sync_inode_data(
>   out_wait:
>  	if (flags & SYNC_IOWAIT)
>  		xfs_ioend_wait(ip);
> +	IRELE(ip);
>  	return error;
>  }
>  
>  STATIC int
>  xfs_sync_inode_attr(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	int			error = 0;
>  
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return 0;

and here?

so xfs_sync_inode_data / xfs_sync_inode_attr are the "execute" in
xfs_inode_ag_walk():

> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}
> +		if (error)
> +			last_error = error;

above, and I think they're ignoring the return from
xfs_sync_inode_valid(), therefore xfs_inode_ag_walk won't see
EFSCORRUPTED from it either ... right?

-Eric

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

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Eric Sandeen]

Christoph Hellwig wrote:

> From: Dave Chinner <david@fromorbit.com>
> 
> Given that we walk across the per-ag inode lists so often, it makes sense to
> introduce an iterator for this.
> 
> Convert the sync and reclaim code to use this new iterator, quota code will
> follow in the next patch.
> 
> [hch: merged the lookup and execute callbacks back into one to get the
>  pag_ici_lock locking correct and simplify the code flow]
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

And a similar error handling question...

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:20:37.012658983 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 16:22:26.321659103 +0200

...

> +STATIC int
> +xfs_inode_ag_walk(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		ag,
> +	int			(*execute)(struct xfs_inode *ip,
> +					   struct xfs_perag *pag, int flags),
> +	int			flags,
> +	int			tag)
> +{
> +	struct xfs_perag	*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, &first_index, tag);
> +		if (!ip)
> +			break;
> +
> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}

Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
calling xfs_reclaim_inode_now, because...

...

> -STATIC void
> -xfs_reclaim_inodes_ag(
> -	xfs_mount_t	*mp,
> -	int		ag,
> -	int		mode)
> +STATIC int
> +xfs_reclaim_inode_now(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
> +	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 {

...

> -
> -		/*
> -		 * 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);

... because before, that's what we did above, after testing for a non-0
return from xfs_reclaim_inode.

But xfs_reclaim_inode_now() returns 0 or the result of
xfs_reclaim_inode, which is 0/1, so above:

> +		error = execute(ip, pag, flags);
> +		if (error == EAGAIN) {
> +			skipped++;
> +			continue;
> +		}

isn't going to see EAGAIN from xfs_reclaim_inode_now... am I following
this right?

-Eric

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

Re: [PATCH 7/7] xfs: split xfs_sync_inodes

[Josef 'Jeff' Sipek]

On Thu, May 14, 2009 at 01:12:40PM -0400, Christoph Hellwig wrote:
> xfs_sync_inodes is used to write back either file data or inode metadata.
> In generally we always do these separately, except for one fishy case in
> xfs_fs_put_super that does both.  So separate xfs_sync_inodes into
> separate xfs_sync_data and xfs_sync_attr functions.  In xfs_fs_put_super
> we first call the data sync and then the attr sync as that was the previous
> order.  The moved log force in that path doesn't make a different because
                                                          ^^^^^^^^^

Typo.

> Index: xfs/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:00.178792110 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:05.278808755 +0200
> @@ -1070,7 +1070,18 @@ xfs_fs_put_super(
>  	int			unmount_event_flags = 0;
>  
>  	xfs_syncd_stop(mp);
> -	xfs_sync_inodes(mp, SYNC_ATTR|SYNC_DELWRI);
> +
> +	if (!(sb->s_flags & MS_RDONLY)) {
> +		/*
> +		 * XXX(hch): this should be SYNC_WAIT.
> +		 *
> +		 * Or more likely no needed at all because the VFS is already
                                  ^^

Typo.

-- 
Real Programmers consider "what you see is what you get" to be just as bad a
concept in Text Editors as it is in women. No, the Real Programmer wants a
"you asked for it, you got it" text editor -- complicated, cryptic,
powerful, unforgiving, dangerous.

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

Re: [PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

[Josef 'Jeff' Sipek]

On Thu, May 14, 2009 at 01:12:39PM -0400, Christoph Hellwig wrote:
> Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
> quota code.  Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
> to allow using them from the quota code.

Nice cleanup.

I don't see any problems with it.

Jeff.

-- 
Keyboard not found!
Press F1 to enter Setup

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

Re: [PATCH 8/7] xfs: remove SYNC_IOWAIT

[Josef 'Jeff' Sipek]

On Thu, May 28, 2009 at 08:19:16AM -0400, Christoph Hellwig wrote:
> We want to wait for all I/O to finish when we do data integrity syncs.  So
> there is no reason to keep SYNC_WAIT separate from SYNC_IOWAIT.  This
> causes a little change in behaviour for the ENOSPC flushing code which no
                                                                         ^^

Typo, otherwise good.

Jeff.

-- 
My public GPG key can be found at
http://www.josefsipek.net/gpg/public-0xC7958FFE.txt

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

Re: [PATCH 7/7] xfs: split xfs_sync_inodes

[Christoph Hellwig]

On Wed, Jun 03, 2009 at 07:26:05PM -0400, Josef 'Jeff' Sipek wrote:
> > order.  The moved log force in that path doesn't make a different because
>                                                           ^^^^^^^^^
> 
> Typo.
> 

> > +		 * Or more likely no needed at all because the VFS is already
>                                   ^^
> 
> Typo.


Thanks, fixed both for the next iteration.

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

Re: [PATCH 8/7] xfs: remove SYNC_IOWAIT

[Christoph Hellwig]

On Wed, Jun 03, 2009 at 07:30:47PM -0400, Josef 'Jeff' Sipek wrote:
> > causes a little change in behaviour for the ENOSPC flushing code which no
>                                                                          ^^
> Typo, otherwise good.

Thanks, fixed for the next iteration.

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

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Christoph Hellwig]

On Wed, Jun 03, 2009 at 05:01:13PM -0500, Eric Sandeen wrote:
> Ok so here we are looking for EFSCORRUPTED from the "execute" function.
>  This might be xfs_sync_inode_data, xfs_sync_inode_attr, or
> xfs_reclaim_inode_now.  But ...

> 
> xfs_sync_inode_valid can return 0, ENOENT, or EFSCORRUPTED.
> 
> Aren't we losing the error here...

We can get the EFSCORRUPTED from xfs_iflush.

> >  
> > +	error = xfs_sync_inode_valid(ip, pag);
> > +	if (error)
> > +		return 0;
> 
> and here?
> 
> so xfs_sync_inode_data / xfs_sync_inode_attr are the "execute" in
> xfs_inode_ag_walk():

If you look at the old code we return early with 0 for the
XFS_FORCED_SHUTDOWN case, which is the only reason xfs_sync_inode_valid 
return xfs_sync_inode_valid in the new code.  We don't actually break
out of the loop in the new code, but don't do any action so the
behaviour is as similar as it gets.  An EFSCORRUPTED later in the
execute function (which AFAICS can only come from xfs_iflush) will
end up pssed down to xfs_inode_ag_iterator.

I can't say that I like this too much.  And in the end only
xfs_fs_quota_sync actually every propagates the return value from
xfs_sync_inodes, and then just directly to userspace.

So I think we are safe just propagating the EFSCORRUPTED down and
make all this a lot more logical.

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

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Christoph Hellwig]

On Wed, Jun 03, 2009 at 05:18:25PM -0500, Eric Sandeen wrote:
> Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
> calling xfs_reclaim_inode_now, because...
> 
> ...
> ... because before, that's what we did above, after testing for a non-0
> return from xfs_reclaim_inode.
> 
> But xfs_reclaim_inode_now() returns 0 or the result of
> xfs_reclaim_inode, which is 0/1, so above:

Yeah.  Updated patch below that besides addressing the other comments
makes xfs_reclaim_inode return -EAGAIN if it has to skip an inode.

Subject: xfs: introduce a per-ag inode iterator
From: Dave Chinner <david@fromorbit.com>

From: Dave Chinner <david@fromorbit.com>

Given that we walk across the per-ag inode lists so often, it makes sense to
introduce an iterator for this.

Convert the sync and reclaim code to use this new iterator, quota code will
follow in the next patch.

Also change xfs_reclaim_inode to return -EGAIN instead of 1 for an inode
already under reclaim.  This simplifies the AG iterator and doesn't
matter for the only other caller.

[hch: merged the lookup and execute callbacks back into one to get the
 pag_ici_lock locking correct and simplify the code flow]

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

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 12:50:25.380940755 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:09:06.199942249 +0200
@@ -49,6 +49,123 @@
 #include <linux/freezer.h>
 
 
+STATIC xfs_inode_t *
+xfs_inode_ag_lookup(
+	struct xfs_mount	*mp,
+	struct xfs_perag	*pag,
+	uint32_t		*first_index,
+	int			tag)
+{
+	int			nr_found;
+	struct xfs_inode	*ip;
+
+	/*
+	 * 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;
+
+	return ip;
+
+unlock:
+	read_unlock(&pag->pag_ici_lock);
+	return NULL;
+}
+
+STATIC int
+xfs_inode_ag_walk(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		ag,
+	int			(*execute)(struct xfs_inode *ip,
+					   struct xfs_perag *pag, int flags),
+	int			flags,
+	int			tag)
+{
+	struct xfs_perag	*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, &first_index, tag);
+		if (!ip)
+			break;
+
+		error = execute(ip, pag, 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(
+	struct xfs_mount	*mp,
+	int			(*execute)(struct xfs_inode *ip,
+					   struct xfs_perag *pag, 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, execute, flags, tag);
+		if (error) {
+			last_error = error;
+			if (error == EFSCORRUPTED)
+				break;
+		}
+	}
+	return XFS_ERROR(last_error);
+}
+
 /* must be called with pag_ici_lock held and releases it */
 STATIC int
 xfs_sync_inode_valid(
@@ -85,12 +202,17 @@ xfs_sync_inode_valid(
 STATIC int
 xfs_sync_inode_data(
 	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
 	int			flags)
 {
 	struct inode		*inode = VFS_I(ip);
 	struct address_space *mapping = inode->i_mapping;
 	int			error = 0;
 
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return error;
+
 	if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
 		goto out_wait;
 
@@ -107,16 +229,22 @@ xfs_sync_inode_data(
  out_wait:
 	if (flags & SYNC_IOWAIT)
 		xfs_ioend_wait(ip);
+	IRELE(ip);
 	return error;
 }
 
 STATIC int
 xfs_sync_inode_attr(
 	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
 	int			flags)
 {
 	int			error = 0;
 
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return error;
+
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	if (xfs_inode_clean(ip))
 		goto out_unlock;
@@ -136,117 +264,33 @@ xfs_sync_inode_attr(
 
  out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	IRELE(ip);
 	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(ip, pag);
-		if (error) {
-			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_attr(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)
-		xfs_log_force(mp, 0, lflags);
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
 
-	return XFS_ERROR(last_error);
+	if (flags & SYNC_ATTR)
+		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
+
+	if (!error && (flags & SYNC_DELWRI))
+		xfs_log_force(mp, 0, lflags);
+	return XFS_ERROR(error);
 }
 
 STATIC int
@@ -613,7 +657,7 @@ xfs_reclaim_inode(
 			xfs_ifunlock(ip);
 			xfs_iunlock(ip, XFS_ILOCK_EXCL);
 		}
-		return 1;
+		return -EAGAIN;
 	}
 	__xfs_iflags_set(ip, XFS_IRECLAIM);
 	spin_unlock(&ip->i_flags_lock);
@@ -698,72 +742,20 @@ xfs_inode_clear_reclaim_tag(
 	xfs_put_perag(mp, pag);
 }
 
-
-STATIC void
-xfs_reclaim_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	int		mode)
+STATIC int
+xfs_reclaim_inode_now(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	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 {
-		/*
-		 * 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;
-		}
-
-		/* ignore if already under reclaim */
-		if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
+	/* ignore if already under reclaim */
+	if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
 		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 0;
 	}
-	return;
+	read_unlock(&pag->pag_ici_lock);
 
+	return xfs_reclaim_inode(ip, 0, flags);
 }
 
 int
@@ -771,14 +763,6 @@ 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_now, mode,
+					XFS_ICI_RECLAIM_TAG);
 }
-
-

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

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

[Eric Sandeen]

Christoph Hellwig wrote:
> On Wed, Jun 03, 2009 at 05:18:25PM -0500, Eric Sandeen wrote:
>> Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
>> calling xfs_reclaim_inode_now, because...
>>
>> ...
>> ... because before, that's what we did above, after testing for a non-0
>> return from xfs_reclaim_inode.
>>
>> But xfs_reclaim_inode_now() returns 0 or the result of
>> xfs_reclaim_inode, which is 0/1, so above:
> 
> Yeah.  Updated patch below that besides addressing the other comments
> makes xfs_reclaim_inode return -EAGAIN if it has to skip an inode.
> 
> Subject: xfs: introduce a per-ag inode iterator
> From: Dave Chinner <david@fromorbit.com>
> 
> From: Dave Chinner <david@fromorbit.com>
> 
> Given that we walk across the per-ag inode lists so often, it makes sense to
> introduce an iterator for this.
> 
> Convert the sync and reclaim code to use this new iterator, quota code will
> follow in the next patch.
> 
> Also change xfs_reclaim_inode to return -EGAIN instead of 1 for an inode
> already under reclaim.  This simplifies the AG iterator and doesn't
> matter for the only other caller.
> 
> [hch: merged the lookup and execute callbacks back into one to get the
>  pag_ici_lock locking correct and simplify the code flow]
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ok, I like this version :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 12:50:25.380940755 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:09:06.199942249 +0200
> @@ -49,6 +49,123 @@
>  #include <linux/freezer.h>
>  
>  
> +STATIC xfs_inode_t *
> +xfs_inode_ag_lookup(
> +	struct xfs_mount	*mp,
> +	struct xfs_perag	*pag,
> +	uint32_t		*first_index,
> +	int			tag)
> +{
> +	int			nr_found;
> +	struct xfs_inode	*ip;
> +
> +	/*
> +	 * 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;
> +
> +	return ip;
> +
> +unlock:
> +	read_unlock(&pag->pag_ici_lock);
> +	return NULL;
> +}
> +
> +STATIC int
> +xfs_inode_ag_walk(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		ag,
> +	int			(*execute)(struct xfs_inode *ip,
> +					   struct xfs_perag *pag, int flags),
> +	int			flags,
> +	int			tag)
> +{
> +	struct xfs_perag	*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, &first_index, tag);
> +		if (!ip)
> +			break;
> +
> +		error = execute(ip, pag, 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(
> +	struct xfs_mount	*mp,
> +	int			(*execute)(struct xfs_inode *ip,
> +					   struct xfs_perag *pag, 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, execute, flags, tag);
> +		if (error) {
> +			last_error = error;
> +			if (error == EFSCORRUPTED)
> +				break;
> +		}
> +	}
> +	return XFS_ERROR(last_error);
> +}
> +
>  /* must be called with pag_ici_lock held and releases it */
>  STATIC int
>  xfs_sync_inode_valid(
> @@ -85,12 +202,17 @@ xfs_sync_inode_valid(
>  STATIC int
>  xfs_sync_inode_data(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	struct inode		*inode = VFS_I(ip);
>  	struct address_space *mapping = inode->i_mapping;
>  	int			error = 0;
>  
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return error;
> +
>  	if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
>  		goto out_wait;
>  
> @@ -107,16 +229,22 @@ xfs_sync_inode_data(
>   out_wait:
>  	if (flags & SYNC_IOWAIT)
>  		xfs_ioend_wait(ip);
> +	IRELE(ip);
>  	return error;
>  }
>  
>  STATIC int
>  xfs_sync_inode_attr(
>  	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
>  	int			flags)
>  {
>  	int			error = 0;
>  
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return error;
> +
>  	xfs_ilock(ip, XFS_ILOCK_SHARED);
>  	if (xfs_inode_clean(ip))
>  		goto out_unlock;
> @@ -136,117 +264,33 @@ xfs_sync_inode_attr(
>  
>   out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	IRELE(ip);
>  	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(ip, pag);
> -		if (error) {
> -			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_attr(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)
> -		xfs_log_force(mp, 0, lflags);
> +		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
>  
> -	return XFS_ERROR(last_error);
> +	if (flags & SYNC_ATTR)
> +		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
> +
> +	if (!error && (flags & SYNC_DELWRI))
> +		xfs_log_force(mp, 0, lflags);
> +	return XFS_ERROR(error);
>  }
>  
>  STATIC int
> @@ -613,7 +657,7 @@ xfs_reclaim_inode(
>  			xfs_ifunlock(ip);
>  			xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  		}
> -		return 1;
> +		return -EAGAIN;
>  	}
>  	__xfs_iflags_set(ip, XFS_IRECLAIM);
>  	spin_unlock(&ip->i_flags_lock);
> @@ -698,72 +742,20 @@ xfs_inode_clear_reclaim_tag(
>  	xfs_put_perag(mp, pag);
>  }
>  
> -
> -STATIC void
> -xfs_reclaim_inodes_ag(
> -	xfs_mount_t	*mp,
> -	int		ag,
> -	int		mode)
> +STATIC int
> +xfs_reclaim_inode_now(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
> +	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 {
> -		/*
> -		 * 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;
> -		}
> -
> -		/* ignore if already under reclaim */
> -		if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
> -			read_unlock(&pag->pag_ici_lock);
> -			continue;
> -		}
> -
> +	/* ignore if already under reclaim */
> +	if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
>  		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 0;
>  	}
> -	return;
> +	read_unlock(&pag->pag_ici_lock);
>  
> +	return xfs_reclaim_inode(ip, 0, flags);
>  }
>  
>  int
> @@ -771,14 +763,6 @@ 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_now, mode,
> +					XFS_ICI_RECLAIM_TAG);
>  }
> -
> -
> 
> _______________________________________________
> 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

Re: [PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

[Eric Sandeen]

Christoph Hellwig wrote:

> Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
> quota code.  Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
> to allow using them from the quota code.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/quota/xfs_qm_syscalls.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_qm_syscalls.c	2009-05-13 14:52:54.087659167 +0200
> +++ xfs/fs/xfs/quota/xfs_qm_syscalls.c	2009-05-13 14:57:36.531661369 +0200

...

> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return 0;

Does this need the same error propagation treatment as 5/7 ?

-Eric

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

Re: [PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

[Christoph Hellwig]

On Fri, Jun 05, 2009 at 02:15:38PM -0500, Eric Sandeen wrote:
> Does this need the same error propagation treatment as 5/7 ?

Yes, I've already fixed this up in my local copy:

Subject: xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes
From: Christoph Hellwig <hch@lst.de>

Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
quota code.  Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
to allow using them from the quota code.


Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>

Index: xfs/fs/xfs/quota/xfs_qm_syscalls.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_qm_syscalls.c	2009-06-04 12:47:30.742821242 +0200
+++ xfs/fs/xfs/quota/xfs_qm_syscalls.c	2009-06-04 13:03:51.039832673 +0200
@@ -847,105 +847,55 @@ xfs_qm_export_flags(
 }
 
 
-/*
- * Release all the dquots on the inodes in an AG.
- */
-STATIC void
-xfs_qm_dqrele_inodes_ag(
-	xfs_mount_t	*mp,
-	int		ag,
-	uint		flags)
+STATIC int
+xfs_dqrele_inode(
+	struct xfs_inode	*ip,
+	struct xfs_perag	*pag,
+	int			flags)
 {
-	xfs_inode_t	*ip = NULL;
-	xfs_perag_t	*pag = &mp->m_perag[ag];
-	int		first_index = 0;
-	int		nr_found;
-
-	do {
-		/*
-		 * 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;
-		}
-
-		/* skip quota inodes */
-		if (ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
-			ASSERT(ip->i_udquot == NULL);
-			ASSERT(ip->i_gdquot == NULL);
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
+	int			error;
 
-		/*
-		 * If we can't get a reference on the inode, it must be
-		 * in reclaim. Leave it for the reclaim code to flush.
-		 */
-		if (!igrab(VFS_I(ip))) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
+	/* skip quota inodes */
+	if (ip == XFS_QI_UQIP(ip->i_mount) || ip == XFS_QI_GQIP(ip->i_mount)) {
+		ASSERT(ip->i_udquot == NULL);
+		ASSERT(ip->i_gdquot == NULL);
 		read_unlock(&pag->pag_ici_lock);
+		return 0;
+	}
 
-		/* avoid new inodes though we shouldn't find any here */
-		if (xfs_iflags_test(ip, XFS_INEW)) {
-			IRELE(ip);
-			continue;
-		}
+	error = xfs_sync_inode_valid(ip, pag);
+	if (error)
+		return error;
 
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
-			xfs_qm_dqrele(ip->i_udquot);
-			ip->i_udquot = NULL;
-		}
-		if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) &&
-		    ip->i_gdquot) {
-			xfs_qm_dqrele(ip->i_gdquot);
-			ip->i_gdquot = NULL;
-		}
-		xfs_iput(ip, XFS_ILOCK_EXCL);
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
+		xfs_qm_dqrele(ip->i_udquot);
+		ip->i_udquot = NULL;
+	}
+	if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) && ip->i_gdquot) {
+		xfs_qm_dqrele(ip->i_gdquot);
+		ip->i_gdquot = NULL;
+	}
+	xfs_iput(ip, XFS_ILOCK_EXCL);
+	IRELE(ip);
 
-	} while (nr_found);
+	return 0;
 }
 
+
 /*
  * Go thru all the inodes in the file system, releasing their dquots.
+ *
  * Note that the mount structure gets modified to indicate that quotas are off
- * AFTER this, in the case of quotaoff. This also gets called from
- * xfs_rootumount.
+ * AFTER this, in the case of quotaoff.
  */
 void
 xfs_qm_dqrele_all_inodes(
 	struct xfs_mount *mp,
 	uint		 flags)
 {
-	int		i;
-
 	ASSERT(mp->m_quotainfo);
-	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-		if (!mp->m_perag[i].pag_ici_init)
-			continue;
-		xfs_qm_dqrele_inodes_ag(mp, i, flags);
-	}
+	xfs_inode_ag_iterator(mp, xfs_dqrele_inode, flags, -1);
 }
 
 /*------------------------------------------------------------------------*/
Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:01:29.388941822 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:02:26.249965001 +0200
@@ -141,7 +141,7 @@ restart:
 	return last_error;
 }
 
-STATIC int
+int
 xfs_inode_ag_iterator(
 	struct xfs_mount	*mp,
 	int			(*execute)(struct xfs_inode *ip,
@@ -167,7 +167,7 @@ xfs_inode_ag_iterator(
 }
 
 /* must be called with pag_ici_lock held and releases it */
-STATIC int
+int
 xfs_sync_inode_valid(
 	struct xfs_inode	*ip,
 	struct xfs_perag	*pag)
Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-06-04 12:47:30.804939977 +0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-06-04 13:02:26.249965001 +0200
@@ -54,4 +54,10 @@ void xfs_inode_set_reclaim_tag(struct xf
 void xfs_inode_clear_reclaim_tag(struct xfs_inode *ip);
 void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
 				struct xfs_inode *ip);
+
+int xfs_sync_inode_valid(struct xfs_inode *ip, struct xfs_perag *pag);
+int xfs_inode_ag_iterator(struct xfs_mount *mp,
+	int (*execute)(struct xfs_inode *ip, struct xfs_perag *pag, int flags),
+	int flags, int tag);
+
 #endif

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

Re: [PATCH 6/7] xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes

[Eric Sandeen]

Christoph Hellwig wrote:
> On Fri, Jun 05, 2009 at 02:15:38PM -0500, Eric Sandeen wrote:
>> Does this need the same error propagation treatment as 5/7 ?
> 
> Yes, I've already fixed this up in my local copy:
> 
> Subject: xfs: use generic inode iterator in xfs_qm_dqrele_all_inodes
> From: Christoph Hellwig <hch@lst.de>
> 
> Use xfs_inode_ag_iterator instead of opencoding the inode walk in the
> quota code.  Mark xfs_inode_ag_iterator and xfs_sync_inode_valid non-static
> to allow using them from the quota code.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>

Ok, this looks fine to me.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> 
> Index: xfs/fs/xfs/quota/xfs_qm_syscalls.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_qm_syscalls.c	2009-06-04 12:47:30.742821242 +0200
> +++ xfs/fs/xfs/quota/xfs_qm_syscalls.c	2009-06-04 13:03:51.039832673 +0200
> @@ -847,105 +847,55 @@ xfs_qm_export_flags(
>  }
>  
>  
> -/*
> - * Release all the dquots on the inodes in an AG.
> - */
> -STATIC void
> -xfs_qm_dqrele_inodes_ag(
> -	xfs_mount_t	*mp,
> -	int		ag,
> -	uint		flags)
> +STATIC int
> +xfs_dqrele_inode(
> +	struct xfs_inode	*ip,
> +	struct xfs_perag	*pag,
> +	int			flags)
>  {
> -	xfs_inode_t	*ip = NULL;
> -	xfs_perag_t	*pag = &mp->m_perag[ag];
> -	int		first_index = 0;
> -	int		nr_found;
> -
> -	do {
> -		/*
> -		 * 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;
> -		}
> -
> -		/* skip quota inodes */
> -		if (ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
> -			ASSERT(ip->i_udquot == NULL);
> -			ASSERT(ip->i_gdquot == NULL);
> -			read_unlock(&pag->pag_ici_lock);
> -			continue;
> -		}
> +	int			error;
>  
> -		/*
> -		 * If we can't get a reference on the inode, it must be
> -		 * in reclaim. Leave it for the reclaim code to flush.
> -		 */
> -		if (!igrab(VFS_I(ip))) {
> -			read_unlock(&pag->pag_ici_lock);
> -			continue;
> -		}
> +	/* skip quota inodes */
> +	if (ip == XFS_QI_UQIP(ip->i_mount) || ip == XFS_QI_GQIP(ip->i_mount)) {
> +		ASSERT(ip->i_udquot == NULL);
> +		ASSERT(ip->i_gdquot == NULL);
>  		read_unlock(&pag->pag_ici_lock);
> +		return 0;
> +	}
>  
> -		/* avoid new inodes though we shouldn't find any here */
> -		if (xfs_iflags_test(ip, XFS_INEW)) {
> -			IRELE(ip);
> -			continue;
> -		}
> +	error = xfs_sync_inode_valid(ip, pag);
> +	if (error)
> +		return error;
>  
> -		xfs_ilock(ip, XFS_ILOCK_EXCL);
> -		if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> -			xfs_qm_dqrele(ip->i_udquot);
> -			ip->i_udquot = NULL;
> -		}
> -		if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) &&
> -		    ip->i_gdquot) {
> -			xfs_qm_dqrele(ip->i_gdquot);
> -			ip->i_gdquot = NULL;
> -		}
> -		xfs_iput(ip, XFS_ILOCK_EXCL);
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
> +		xfs_qm_dqrele(ip->i_udquot);
> +		ip->i_udquot = NULL;
> +	}
> +	if (flags & (XFS_PQUOTA_ACCT|XFS_GQUOTA_ACCT) && ip->i_gdquot) {
> +		xfs_qm_dqrele(ip->i_gdquot);
> +		ip->i_gdquot = NULL;
> +	}
> +	xfs_iput(ip, XFS_ILOCK_EXCL);
> +	IRELE(ip);
>  
> -	} while (nr_found);
> +	return 0;
>  }
>  
> +
>  /*
>   * Go thru all the inodes in the file system, releasing their dquots.
> + *
>   * Note that the mount structure gets modified to indicate that quotas are off
> - * AFTER this, in the case of quotaoff. This also gets called from
> - * xfs_rootumount.
> + * AFTER this, in the case of quotaoff.
>   */
>  void
>  xfs_qm_dqrele_all_inodes(
>  	struct xfs_mount *mp,
>  	uint		 flags)
>  {
> -	int		i;
> -
>  	ASSERT(mp->m_quotainfo);
> -	for (i = 0; i < mp->m_sb.sb_agcount; i++) {
> -		if (!mp->m_perag[i].pag_ici_init)
> -			continue;
> -		xfs_qm_dqrele_inodes_ag(mp, i, flags);
> -	}
> +	xfs_inode_ag_iterator(mp, xfs_dqrele_inode, flags, -1);
>  }
>  
>  /*------------------------------------------------------------------------*/
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:01:29.388941822 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-06-04 13:02:26.249965001 +0200
> @@ -141,7 +141,7 @@ restart:
>  	return last_error;
>  }
>  
> -STATIC int
> +int
>  xfs_inode_ag_iterator(
>  	struct xfs_mount	*mp,
>  	int			(*execute)(struct xfs_inode *ip,
> @@ -167,7 +167,7 @@ xfs_inode_ag_iterator(
>  }
>  
>  /* must be called with pag_ici_lock held and releases it */
> -STATIC int
> +int
>  xfs_sync_inode_valid(
>  	struct xfs_inode	*ip,
>  	struct xfs_perag	*pag)
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-06-04 12:47:30.804939977 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-06-04 13:02:26.249965001 +0200
> @@ -54,4 +54,10 @@ void xfs_inode_set_reclaim_tag(struct xf
>  void xfs_inode_clear_reclaim_tag(struct xfs_inode *ip);
>  void __xfs_inode_clear_reclaim_tag(struct xfs_mount *mp, struct xfs_perag *pag,
>  				struct xfs_inode *ip);
> +
> +int xfs_sync_inode_valid(struct xfs_inode *ip, struct xfs_perag *pag);
> +int xfs_inode_ag_iterator(struct xfs_mount *mp,
> +	int (*execute)(struct xfs_inode *ip, struct xfs_perag *pag, int flags),
> +	int flags, int tag);
> +
>  #endif
> 
> _______________________________________________
> 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

Re: [PATCH 7/7] xfs: split xfs_sync_inodes

[Eric Sandeen]

Christoph Hellwig wrote:

> xfs_sync_inodes is used to write back either file data or inode metadata.
> In generally we always do these separately, except for one fishy case in
  ^^^ "In general"

> xfs_fs_put_super that does both.  So separate xfs_sync_inodes into
> separate xfs_sync_data and xfs_sync_attr functions.  In xfs_fs_put_super
> we first call the data sync and then the attr sync as that was the previous
> order.  The moved log force in that path doesn't make a different because
> we will force the log again as part of the real unmount process.
> 
> The filesystem readonly checks are not performed by the new function but
> instead moved into the callers, given that most callers alredy have it
> further up in the stack.  Also add debug checks that we do not pass in
> incorrect flags in the new xfs_sync_data and xfs_sync_attr function and
> fix the one place that did pass in a wrong flag.
> 
> Also remove a comment mentioning xfs_sync_inodes that has been incorrect
> for a while because we always take either the iolock or ilock in the
> sync path these days.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:00.178792110 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_super.c	2009-05-14 19:09:05.278808755 +0200
> @@ -1070,7 +1070,18 @@ xfs_fs_put_super(
>  	int			unmount_event_flags = 0;
>  
>  	xfs_syncd_stop(mp);
> -	xfs_sync_inodes(mp, SYNC_ATTR|SYNC_DELWRI);
> +
> +	if (!(sb->s_flags & MS_RDONLY)) {
> +		/*
> +		 * XXX(hch): this should be SYNC_WAIT.
> +		 *
> +		 * Or more likely no needed at all because the VFS is already
> +		 * calling ->sync_fs after shutting down all filestem
> +		 * operations and just before calling ->put_super.
> +		 */
> +		xfs_sync_data(mp, 0);
> +		xfs_sync_attr(mp, 0);
> +	}
>  
>  #ifdef HAVE_DMAPI
>  	if (mp->m_flags & XFS_MOUNT_DMAPI) {
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:09:04.687659175 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-14 19:09:05.279808603 +0200
> @@ -265,29 +265,40 @@ xfs_sync_inode_attr(
>  	return error;
>  }
>  
> +/*
> + * Write out pagecache data for the whole filesystem.
> + */
>  int
> -xfs_sync_inodes(
> -	xfs_mount_t	*mp,
> -	int		flags)
> +xfs_sync_data(
> +	struct xfs_mount	*mp,
> +	int			flags)
>  {
> -	int		error = 0;
> -	int		lflags = XFS_LOG_FORCE;
> +	int			error;
>  
> -	if (mp->m_flags & XFS_MOUNT_RDONLY)
> -		return 0;
> +	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT|SYNC_IOWAIT)) == 0);
>  
> -	if (flags & SYNC_WAIT)
> -		lflags |= XFS_LOG_SYNC;
> +	error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
> +	if (error)
> +		return XFS_ERROR(error);
>  
> -	if (flags & SYNC_DELWRI)
> -		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
> +	xfs_log_force(mp, 0,
> +		      (flags & SYNC_WAIT) ?
> +		       XFS_LOG_FORCE | XFS_LOG_SYNC :
> +		       XFS_LOG_FORCE);
> +	return 0;
> +}
>  
> -	if (flags & SYNC_ATTR)
> -		error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
> +/*
> + * Write out inode metadata (attributes) for the whole filesystem.
> + */
> +int
> +xfs_sync_attr(
> +	struct xfs_mount	*mp,
> +	int			flags)
> +{
> +	ASSERT((flags & ~SYNC_WAIT) == 0);
>  
> -	if (!error && (flags & SYNC_DELWRI))
> -		xfs_log_force(mp, 0, lflags);
> -	return XFS_ERROR(error);
> +	return xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, -1);
>  }
>  
>  STATIC int
> @@ -401,12 +412,12 @@ xfs_quiesce_data(
>  	int error;
>  
>  	/* push non-blocking */
> -	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_BDFLUSH);
> +	xfs_sync_data(mp, 0);
>  	xfs_qm_sync(mp, SYNC_BDFLUSH);
>  	xfs_filestream_flush(mp);
>  
>  	/* push and block */
> -	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_WAIT|SYNC_IOWAIT);
> +	xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
>  	xfs_qm_sync(mp, SYNC_WAIT);
>  
>  	/* write superblock and hoover up shutdown errors */
> @@ -435,7 +446,7 @@ xfs_quiesce_fs(
>  	 * logged before we can write the unmount record.
>  	 */
>  	do {
> -		xfs_sync_inodes(mp, SYNC_ATTR|SYNC_WAIT);
> +		xfs_sync_attr(mp, SYNC_WAIT);
>  		pincount = xfs_flush_buftarg(mp->m_ddev_targp, 1);
>  		if (!pincount) {
>  			delay(50);
> @@ -518,8 +529,8 @@ xfs_flush_inodes_work(
>  	void		*arg)
>  {
>  	struct inode	*inode = arg;
> -	xfs_sync_inodes(mp, SYNC_DELWRI | SYNC_TRYLOCK);
> -	xfs_sync_inodes(mp, SYNC_DELWRI | SYNC_TRYLOCK | SYNC_IOWAIT);
> +	xfs_sync_data(mp, SYNC_TRYLOCK);
> +	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);
>  	iput(inode);
>  }
>  
> Index: xfs/fs/xfs/linux-2.6/xfs_quotaops.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_quotaops.c	2009-05-14 19:05:24.908659400 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_quotaops.c	2009-05-14 19:09:05.280834851 +0200
> @@ -50,9 +50,11 @@ xfs_fs_quota_sync(
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
>  
> +	if (sb->s_flags & MS_RDONLY)
> +		return -EROFS;
>  	if (!XFS_IS_QUOTA_RUNNING(mp))
>  		return -ENOSYS;
> -	return -xfs_sync_inodes(mp, SYNC_DELWRI);
> +	return -xfs_sync_data(mp, 0);
>  }
>  
>  STATIC int
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 19:09:04.694659368 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-14 19:09:05.280834851 +0200
> @@ -29,8 +29,6 @@ typedef struct xfs_sync_work {
>  	struct completion	*w_completion;
>  } xfs_sync_work_t;
>  
> -#define SYNC_ATTR		0x0001	/* sync attributes */
> -#define SYNC_DELWRI		0x0002	/* look at delayed writes */
>  #define SYNC_WAIT		0x0004	/* wait for i/o to complete */
>  #define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
>  #define SYNC_IOWAIT		0x0010  /* wait for all I/O to complete */
> @@ -41,7 +39,8 @@ void xfs_syncd_stop(struct xfs_mount *mp
>  
>  int xfs_inode_flush(struct xfs_inode *ip, int sync);
>  
> -int xfs_sync_inodes(struct xfs_mount *mp, int flags);
> +int xfs_sync_attr(struct xfs_mount *mp, int flags);
> +int xfs_sync_data(struct xfs_mount *mp, int flags);
>  int xfs_sync_fsdata(struct xfs_mount *mp, int flags);
>  
>  int xfs_quiesce_data(struct xfs_mount *mp);
> Index: xfs/fs/xfs/xfs_filestream.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_filestream.c	2009-05-14 19:05:24.929659282 +0200
> +++ xfs/fs/xfs/xfs_filestream.c	2009-05-14 19:09:05.283807995 +0200
> @@ -542,10 +542,8 @@ xfs_filestream_associate(
>  	 * waiting for the lock because someone else is waiting on the lock we
>  	 * hold and we cannot drop that as we are in a transaction here.
>  	 *
> -	 * Lucky for us, this inversion is rarely a problem because it's a
> -	 * directory inode that we are trying to lock here and that means the
> -	 * only place that matters is xfs_sync_inodes() and SYNC_DELWRI is
> -	 * used. i.e. freeze, remount-ro, quotasync or unmount.
> +	 * Lucky for us, this inversion is not a problem because it's a
> +	 * directory inode that we are trying to lock here.
>  	 *
>  	 * So, if we can't get the iolock without sleeping then just give up
>  	 */
> 
> _______________________________________________
> 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

Re: [PATCH 8/7] xfs: remove SYNC_IOWAIT

[Eric Sandeen]

Christoph Hellwig wrote:
> We want to wait for all I/O to finish when we do data integrity syncs.  So
> there is no reason to keep SYNC_WAIT separate from SYNC_IOWAIT.  This
> causes a little change in behaviour for the ENOSPC flushing code which no
> does a second submission and wait of buffered I/O, but that should finish
> ASAP as we already did an asynchronous writeout earlier.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems ok to me.

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 12:59:57.115813662 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:01:14.634816358 +0200
> @@ -226,7 +226,7 @@ xfs_sync_inode_data(
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
>   out_wait:
> -	if (flags & SYNC_IOWAIT)
> +	if (flags & SYNC_WAIT)
>  		xfs_ioend_wait(ip);
>  	IRELE(ip);
>  	return error;
> @@ -277,7 +277,7 @@ xfs_sync_data(
>  {
>  	int			error;
>  
> -	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT|SYNC_IOWAIT)) == 0);
> +	ASSERT((flags & ~(SYNC_TRYLOCK|SYNC_WAIT)) == 0);
>  
>  	error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, -1);
>  	if (error)
> @@ -419,7 +419,7 @@ xfs_quiesce_data(
>  	xfs_filestream_flush(mp);
>  
>  	/* push and block */
> -	xfs_sync_data(mp, SYNC_WAIT|SYNC_IOWAIT);
> +	xfs_sync_data(mp, SYNC_WAIT);
>  	xfs_qm_sync(mp, SYNC_WAIT);
>  
>  	/* write superblock and hoover up shutdown errors */
> @@ -532,7 +532,7 @@ xfs_flush_inodes_work(
>  {
>  	struct inode	*inode = arg;
>  	xfs_sync_data(mp, SYNC_TRYLOCK);
> -	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_IOWAIT);
> +	xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_WAIT);
>  	iput(inode);
>  }
>  
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:00:29.045814647 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:01:39.162941539 +0200
> @@ -31,7 +31,6 @@ typedef struct xfs_sync_work {
>  
>  #define SYNC_WAIT		0x0004	/* wait for i/o to complete */
>  #define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
> -#define SYNC_IOWAIT		0x0010  /* wait for all I/O to complete */
>  #define SYNC_TRYLOCK		0x0020  /* only try to lock inodes */
>  
>  int xfs_syncd_init(struct xfs_mount *mp);
> 
> _______________________________________________
> 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

Re: [PATCH 9/7] xfs: remove SYNC_BDFLUSH

[Eric Sandeen]

Christoph Hellwig wrote:
> SYNC_BDFLUSH is a leftover from IRIX and rather misnamed for todays
> code.  Make xfs_sync_fsdata and xfs_dq_sync use the SYNC_TRYLOCK flag
> for not blocking on logs just as the inode sync code already does.
> 
> For xfs_sync_fsdata it's a trivial 1:1 replacement, but for xfs_qm_sync
> I use the opportunity to decouple the non-blocking lock case from the
> different flushing modes, similar to the inode sync code.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:02:57.426938830 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c	2009-05-27 13:07:53.227939055 +0200
> @@ -350,7 +350,7 @@ xfs_sync_fsdata(
>  	 * If this is xfssyncd() then only sync the superblock if we can
>  	 * lock it without sleeping and it is not pinned.
>  	 */
> -	if (flags & SYNC_BDFLUSH) {
> +	if (flags & SYNC_TRYLOCK) {
>  		ASSERT(!(flags & SYNC_WAIT));
>  
>  		bp = xfs_getsb(mp, XFS_BUF_TRYLOCK);
> @@ -415,7 +415,7 @@ xfs_quiesce_data(
>  
>  	/* push non-blocking */
>  	xfs_sync_data(mp, 0);
> -	xfs_qm_sync(mp, SYNC_BDFLUSH);
> +	xfs_qm_sync(mp, SYNC_TRYLOCK);
>  	xfs_filestream_flush(mp);
>  
>  	/* push and block */
> @@ -565,8 +565,8 @@ xfs_sync_worker(
>  		xfs_log_force(mp, (xfs_lsn_t)0, XFS_LOG_FORCE);
>  		xfs_reclaim_inodes(mp, XFS_IFLUSH_DELWRI_ELSE_ASYNC);
>  		/* dgc: errors ignored here */
> -		error = xfs_qm_sync(mp, SYNC_BDFLUSH);
> -		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
> +		error = xfs_qm_sync(mp, SYNC_TRYLOCK);
> +		error = xfs_sync_fsdata(mp, SYNC_TRYLOCK);
>  		if (xfs_log_need_covered(mp))
>  			error = xfs_commit_dummy_trans(mp, XFS_LOG_FORCE);
>  	}
> Index: xfs/fs/xfs/quota/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_qm.c	2009-05-27 13:04:00.607842293 +0200
> +++ xfs/fs/xfs/quota/xfs_qm.c	2009-05-27 13:10:21.688940102 +0200
> @@ -905,11 +905,6 @@ xfs_qm_dqdetach(
>  	}
>  }
>  
> -/*
> - * This is called to sync quotas. We can be told to use non-blocking
> - * semantics by either the SYNC_BDFLUSH flag or the absence of the
> - * SYNC_WAIT flag.
> - */
>  int
>  xfs_qm_sync(
>  	xfs_mount_t	*mp,
> @@ -918,17 +913,13 @@ xfs_qm_sync(
>  	int		recl, restarts;
>  	xfs_dquot_t	*dqp;
>  	uint		flush_flags;
> -	boolean_t	nowait;
>  	int		error;
>  
>  	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>  		return 0;
>  
> +	flush_flags = (flags & SYNC_WAIT) ? XFS_QMOPT_SYNC : XFS_QMOPT_DELWRI;
>  	restarts = 0;
> -	/*
> -	 * We won't block unless we are asked to.
> -	 */
> -	nowait = (boolean_t)(flags & SYNC_BDFLUSH || (flags & SYNC_WAIT) == 0);
>  
>    again:
>  	xfs_qm_mplist_lock(mp);
> @@ -948,18 +939,10 @@ xfs_qm_sync(
>  		 * don't 'seem' to be dirty. ie. don't acquire dqlock.
>  		 * This is very similar to what xfs_sync does with inodes.
>  		 */
> -		if (flags & SYNC_BDFLUSH) {
> -			if (! XFS_DQ_IS_DIRTY(dqp))
> +		if (flags & SYNC_TRYLOCK) {
> +			if (!XFS_DQ_IS_DIRTY(dqp))
>  				continue;
> -		}
> -
> -		if (nowait) {
> -			/*
> -			 * Try to acquire the dquot lock. We are NOT out of
> -			 * lock order, but we just don't want to wait for this
> -			 * lock, unless somebody wanted us to.
> -			 */
> -			if (! xfs_qm_dqlock_nowait(dqp))
> +			if (!xfs_qm_dqlock_nowait(dqp))
>  				continue;
>  		} else {
>  			xfs_dqlock(dqp);
> @@ -976,7 +959,7 @@ xfs_qm_sync(
>  		/* XXX a sentinel would be better */
>  		recl = XFS_QI_MPLRECLAIMS(mp);
>  		if (!xfs_dqflock_nowait(dqp)) {
> -			if (nowait) {
> +			if (flags & SYNC_TRYLOCK) {
>  				xfs_dqunlock(dqp);
>  				continue;
>  			}
> @@ -994,7 +977,6 @@ xfs_qm_sync(
>  		 * Let go of the mplist lock. We don't want to hold it
>  		 * across a disk write
>  		 */
> -		flush_flags = (nowait) ? XFS_QMOPT_DELWRI : XFS_QMOPT_SYNC;
>  		xfs_qm_mplist_unlock(mp);
>  		xfs_dqtrace_entry(dqp, "XQM_SYNC: DQFLUSH");
>  		error = xfs_qm_dqflush(dqp, flush_flags);
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:07:58.997814418 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h	2009-05-27 13:08:19.922972203 +0200
> @@ -29,9 +29,8 @@ typedef struct xfs_sync_work {
>  	struct completion	*w_completion;
>  } xfs_sync_work_t;
>  
> -#define SYNC_WAIT		0x0004	/* wait for i/o to complete */
> -#define SYNC_BDFLUSH		0x0008	/* BDFLUSH is calling -- don't block */
> -#define SYNC_TRYLOCK		0x0020  /* only try to lock inodes */
> +#define SYNC_WAIT		0x0001	/* wait for i/o to complete */
> +#define SYNC_TRYLOCK		0x0002  /* only try to lock inodes */
>  
>  int xfs_syncd_init(struct xfs_mount *mp);
>  void xfs_syncd_stop(struct xfs_mount *mp);
> 
> _______________________________________________
> 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