[PATCH 0/3] xfs: fixes for 3.12-rc2

[PATCH 0/3] xfs: fixes for 3.12-rc2

[Dave Chinner]

Hi folks,

Three patches for 3.12-rc2. The first has been posted previously,
and prevents an assert failure during a shutdown while zeroing space
with an uncached buffer.

The second patch is to fix a regression introduced in the 3.12 merge
by commit 46f9d2e ("xfs: aborted buf items can be in the AIL").

The third is fixing a thinko made a long time ago when adding RCU
freeing to the struct xfs_inode, but has only been exposed by the
recent merge of the per-node LRUs and shrinkers allowing concurrent
reclaim and lookup races to be hit easily. The RCU traversals of the
inode radix trees take the lock on the inode to determine if it is
being freed or not, and so that can race with freeing of the inode.
Hence we can't assert that the lock is not held when freeing the
inode.

These all fix regressions triggered by xfstests....

Cheers,

Dave.

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

[PATCH 1/3] xfs: don't try to mark uncached buffers stale on error.

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

fsstress failed during a shutdown with the following assert:

XFS: Assertion failed: xfs_buf_islocked(bp), file: fs/xfs/xfs_buf.c, line: 143
.....
 xfs_buf_stale+0x3f/0xf0
 xfs_bioerror_relse+0x2d/0x90
 xfsbdstrat+0x51/0xa0
 xfs_zero_remaining_bytes+0x1d1/0x2d0
 xfs_free_file_space+0x5d0/0x600
 xfs_change_file_space+0x251/0x3a0
 xfs_ioc_space+0xcc/0x130
.....

xfs_zero_remaining_bytes() works with uncached buffers, and hence if
we are preventing IO due to a shutdown, we should not be marking it
stale as that is only for cached buffers. Instead, just mark it with
an error and make sure it gets to the caller.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 2634700..956685f 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1093,25 +1093,20 @@ xfs_bioerror_relse(
 	struct xfs_buf	*bp)
 {
 	int64_t		fl = bp->b_flags;
+
 	/*
-	 * No need to wait until the buffer is unpinned.
-	 * We aren't flushing it.
-	 *
-	 * chunkhold expects B_DONE to be set, whether
-	 * we actually finish the I/O or not. We don't want to
-	 * change that interface.
+	 * No need to wait until the buffer is unpinned. We aren't flushing it.
 	 */
 	XFS_BUF_UNREAD(bp);
 	XFS_BUF_DONE(bp);
 	xfs_buf_stale(bp);
 	bp->b_iodone = NULL;
+
+	/*
+	 * There's no reason to mark error for ASYNC buffers as there is no-one
+	 * waiting to collect the error.
+	 */
 	if (!(fl & XBF_ASYNC)) {
-		/*
-		 * Mark b_error and B_ERROR _both_.
-		 * Lot's of chunkcache code assumes that.
-		 * There's no reason to mark error for
-		 * ASYNC buffers.
-		 */
 		xfs_buf_ioerror(bp, EIO);
 		complete(&bp->b_iowait);
 	} else {
@@ -1128,11 +1123,15 @@ xfs_bdstrat_cb(
 	if (XFS_FORCED_SHUTDOWN(bp->b_target->bt_mount)) {
 		trace_xfs_bdstrat_shut(bp, _RET_IP_);
 		/*
-		 * Metadata write that didn't get logged but
-		 * written delayed anyway. These aren't associated
-		 * with a transaction, and can be ignored.
+		 * If this is a cached write, then it is likely to be a delayed
+		 * write metadata buffer that can be ignored because the
+		 * contents are logged. If it's an uncached buffer or a read
+		 * operation, then the caller will get the error through the
+		 * normal IO completion path. We can tell if the buffer is
+		 * cached or not by looking to see if the b_pag field is NULL or
+		 * not.
 		 */
-		if (!bp->b_iodone && !XFS_BUF_ISREAD(bp))
+		if (!bp->b_iodone && !XFS_BUF_ISREAD(bp) && bp->b_pag)
 			return xfs_bioerror_relse(bp);
 		else
 			return xfs_bioerror(bp);
-- 
1.8.3.2

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

[PATCH 2/3] xfs: lock the AIL before removing the buffer item

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Regression introduced by commit 46f9d2e ("xfs: aborted buf items can
be in the AIL") which fails to lock the AIL before removing the
item. Spinlock debugging throws a warning about this.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 88c5ea7..f1d85cf 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -628,6 +628,7 @@ xfs_buf_item_unlock(
 		else if (aborted) {
 			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
 			if (lip->li_flags & XFS_LI_IN_AIL) {
+				spin_lock(&lip->li_ailp->xa_lock);
 				xfs_trans_ail_delete(lip->li_ailp, lip,
 						     SHUTDOWN_LOG_IO_ERROR);
 			}
-- 
1.8.3.2

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

[PATCH 3/3] xfs: asserting lock not held during freeing not valid

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When we free an inode, we do so via RCU. As an RCU lookup can occur
at any time before we free an inode, and that lookup takes the inode
flags lock, we cannot safely assert that the flags lock is not held
just before marking it dead and running call_rcu() to free the
inode.

We check on allocation of a new inode structre that the lock is not
held, so we still have protection against locks being leaked and
hence not correctly initialised when allocated out of the slab.
Hence just remove the assert...

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_icache.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 193206b..474807a 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -119,11 +119,6 @@ xfs_inode_free(
 		ip->i_itemp = NULL;
 	}
 
-	/* asserts to verify all state is correct here */
-	ASSERT(atomic_read(&ip->i_pincount) == 0);
-	ASSERT(!spin_is_locked(&ip->i_flags_lock));
-	ASSERT(!xfs_isiflocked(ip));
-
 	/*
 	 * Because we use RCU freeing we need to ensure the inode always
 	 * appears to be reclaimed with an invalid inode number when in the
@@ -135,6 +130,10 @@ xfs_inode_free(
 	ip->i_ino = 0;
 	spin_unlock(&ip->i_flags_lock);
 
+	/* asserts to verify all state is correct here */
+	ASSERT(atomic_read(&ip->i_pincount) == 0);
+	ASSERT(!xfs_isiflocked(ip));
+
 	call_rcu(&VFS_I(ip)->i_rcu, xfs_inode_free_callback);
 }
 
-- 
1.8.3.2

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