[PATCH 0/5] xfs: fixes for 3.12-rc3

[PATCH 0/5] xfs: fixes for 3.12-rc3

[Dave Chinner]

Hi folks,

These are fixes needed for regressions introduced in the 3.12 merge.
I posted the first 3 patches a week ago, and sinve then I've found a
couple more issues in working through all the xfstests failures I've
been seeing since 3.12-rc1 was released.

The last patch in the series fixes a filesystem corruption bug, so
getting these into -rc3 should be considered a priority....

Cheers,

Dave.

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

[PATCH 1/5] 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/5] 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/5] 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

[PATCH 4/5] xfs: fix XFS_IOC_FREE_EOFBLOCKS definition

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

It uses a kernel internal structure in it's definition rather than
the user visible structure that is passed to the ioctl.

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

diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index 1edb5cc..18272c7 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -515,7 +515,7 @@ typedef struct xfs_swapext
 /*	XFS_IOC_GETBIOSIZE ---- deprecated 47	   */
 #define XFS_IOC_GETBMAPX	_IOWR('X', 56, struct getbmap)
 #define XFS_IOC_ZERO_RANGE	_IOW ('X', 57, struct xfs_flock64)
-#define XFS_IOC_FREE_EOFBLOCKS	_IOR ('X', 58, struct xfs_eofblocks)
+#define XFS_IOC_FREE_EOFBLOCKS	_IOR ('X', 58, struct xfs_fs_eofblocks)
 
 /*
  * ioctl commands that replace IRIX syssgi()'s
-- 
1.8.3.2

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

[PATCH 5/5] xfs: log recovery lsn ordering needs uuid check

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

After a fair number of xfstests runs, xfs/182 started to fail
regularly with a corrupted directory - a directory read verifier was
failing after recovery because it found a block with a XARM magic
number (remote attribute block) rather than a directory data block.

The first time I saw this repeated failure I did /something/ and the
problem went away, so I was never able to find the underlying
problem. Test xfs/182 failed again today, and I found the root
cause before I did /something else/ that made it go away.

Tracing indicated that the block in question was being correctly
logged, the log was being flushed by sync, but the buffer was not
being written back before the shutdown occurred. Tracing also
indicated that log recovery was also reading the block, but then
never writing it before log recovery invalidated the cache,
indicating that it was not modified by log recovery.

More detailed analysis of the corpse indicated that the filesystem
had a uuid of "a4131074-1872-4cac-9323-2229adbcb886" but the XARM
block had a uuid of "8f32f043-c3c9-e7f8-f947-4e7f989c05d3", which
indicated it was a block from an older filesystem. The reason that
log recovery didn't replay it was that the LSN in the XARM block was
larger than the LSN of the transaction being replayed, and so the
block was not overwritten by log recovery.

Hence, log recovery cant blindly trust the magic number and LSN in
the block - it must verify that it belongs to the filesystem being
recovered before using the LSN. i.e. if the UUIDs don't match, we
need to unconditionally recovery the change held in the log.

This patch was first tested on a block device that was repeatedly
causing xfs/182 to fail with the same failure on the same block with
the same directory read corruption signature (i.e. XARM block). It
did not fail, and hasn't failed since.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 73 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index dabda95..cc17987 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1970,6 +1970,13 @@ xlog_recover_do_inode_buffer(
  * magic number.  If we don't recognise the magic number in the buffer, then
  * return a LSN of -1 so that the caller knows it was an unrecognised block and
  * so can recover the buffer.
+ *
+ * Note: we cannot rely solely on magic number matches to determine that the
+ * buffer has a valid LSN - we also need to verify that it belongs to this
+ * filesystem, so we need to extract the object's LSN and compare it to that
+ * which we read from the superblock. If the UUIDs don't match, then we've got a
+ * stale metadata block from an old filesystem instance that we need to recover
+ * over the top of.
  */
 static xfs_lsn_t
 xlog_recover_get_buf_lsn(
@@ -1980,6 +1987,8 @@ xlog_recover_get_buf_lsn(
 	__uint16_t		magic16;
 	__uint16_t		magicda;
 	void			*blk = bp->b_addr;
+	uuid_t			*uuid;
+	xfs_lsn_t		lsn = -1;
 
 	/* v4 filesystems always recover immediately */
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
@@ -1992,43 +2001,79 @@ xlog_recover_get_buf_lsn(
 	case XFS_ABTB_MAGIC:
 	case XFS_ABTC_MAGIC:
 	case XFS_IBT_CRC_MAGIC:
-	case XFS_IBT_MAGIC:
-		return be64_to_cpu(
-				((struct xfs_btree_block *)blk)->bb_u.s.bb_lsn);
+	case XFS_IBT_MAGIC: {
+		struct xfs_btree_block *btb = blk;
+
+		lsn = be64_to_cpu(btb->bb_u.s.bb_lsn);
+		uuid = &btb->bb_u.s.bb_uuid;
+		break;
+	}
 	case XFS_BMAP_CRC_MAGIC:
-	case XFS_BMAP_MAGIC:
-		return be64_to_cpu(
-				((struct xfs_btree_block *)blk)->bb_u.l.bb_lsn);
+	case XFS_BMAP_MAGIC: {
+		struct xfs_btree_block *btb = blk;
+
+		lsn = be64_to_cpu(btb->bb_u.l.bb_lsn);
+		uuid = &btb->bb_u.l.bb_uuid;
+		break;
+	}
 	case XFS_AGF_MAGIC:
-		return be64_to_cpu(((struct xfs_agf *)blk)->agf_lsn);
+		lsn = be64_to_cpu(((struct xfs_agf *)blk)->agf_lsn);
+		uuid = &((struct xfs_agf *)blk)->agf_uuid;
+		break;
 	case XFS_AGFL_MAGIC:
-		return be64_to_cpu(((struct xfs_agfl *)blk)->agfl_lsn);
+		lsn = be64_to_cpu(((struct xfs_agfl *)blk)->agfl_lsn);
+		uuid = &((struct xfs_agfl *)blk)->agfl_uuid;
+		break;
 	case XFS_AGI_MAGIC:
-		return be64_to_cpu(((struct xfs_agi *)blk)->agi_lsn);
+		lsn = be64_to_cpu(((struct xfs_agi *)blk)->agi_lsn);
+		uuid = &((struct xfs_agi *)blk)->agi_uuid;
+		break;
 	case XFS_SYMLINK_MAGIC:
-		return be64_to_cpu(((struct xfs_dsymlink_hdr *)blk)->sl_lsn);
+		lsn = be64_to_cpu(((struct xfs_dsymlink_hdr *)blk)->sl_lsn);
+		uuid = &((struct xfs_dsymlink_hdr *)blk)->sl_uuid;
+		break;
 	case XFS_DIR3_BLOCK_MAGIC:
 	case XFS_DIR3_DATA_MAGIC:
 	case XFS_DIR3_FREE_MAGIC:
-		return be64_to_cpu(((struct xfs_dir3_blk_hdr *)blk)->lsn);
+		lsn = be64_to_cpu(((struct xfs_dir3_blk_hdr *)blk)->lsn);
+		uuid = &((struct xfs_dir3_blk_hdr *)blk)->uuid;
+		break;
 	case XFS_ATTR3_RMT_MAGIC:
-		return be64_to_cpu(((struct xfs_attr3_rmt_hdr *)blk)->rm_lsn);
+		lsn = be64_to_cpu(((struct xfs_attr3_rmt_hdr *)blk)->rm_lsn);
+		uuid = &((struct xfs_attr3_rmt_hdr *)blk)->rm_uuid;
+		break;
 	case XFS_SB_MAGIC:
-		return be64_to_cpu(((struct xfs_dsb *)blk)->sb_lsn);
+		lsn = be64_to_cpu(((struct xfs_dsb *)blk)->sb_lsn);
+		uuid = &((struct xfs_dsb *)blk)->sb_uuid;
+		break;
 	default:
 		break;
 	}
 
+	if (lsn != (xfs_lsn_t)-1) {
+		if (!uuid_equal(&mp->m_sb.sb_uuid, uuid))
+			goto recover_immediately;
+		return lsn;
+	}
+
 	magicda = be16_to_cpu(((struct xfs_da_blkinfo *)blk)->magic);
 	switch (magicda) {
 	case XFS_DIR3_LEAF1_MAGIC:
 	case XFS_DIR3_LEAFN_MAGIC:
 	case XFS_DA3_NODE_MAGIC:
-		return be64_to_cpu(((struct xfs_da3_blkinfo *)blk)->lsn);
+		lsn = be64_to_cpu(((struct xfs_da3_blkinfo *)blk)->lsn);
+		uuid = &((struct xfs_da3_blkinfo *)blk)->uuid;
+		break;
 	default:
 		break;
 	}
 
+	if (lsn != (xfs_lsn_t)-1) {
+		if (!uuid_equal(&mp->m_sb.sb_uuid, uuid))
+			goto recover_immediately;
+		return lsn;
+	}
+
 	/*
 	 * We do individual object checks on dquot and inode buffers as they
 	 * have their own individual LSN records. Also, we could have a stale
-- 
1.8.3.2

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

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

[Mark Tinguely]

On 09/24/13 01:01, Dave Chinner wrote:
> 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>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

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

[Ben Myers]

Hi Dave,

On Tue, Sep 24, 2013 at 04:01:12PM +1000, Dave Chinner wrote:
> 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

Here you're showing an assert reported through an xfsbdstrat codepath...

>  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)

...but it looks like your fix is in xfs_bdstrat_cb, which wouldn't have been
involved in the stack you posted above.  What am I missing?

Thanks,
	Ben

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

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

[Mark Tinguely]

On 09/24/13 01:01, Dave Chinner wrote:
> 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>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 4/5] xfs: fix XFS_IOC_FREE_EOFBLOCKS definition

[Mark Tinguely]

On 09/24/13 01:01, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> It uses a kernel internal structure in it's definition rather than
> the user visible structure that is passed to the ioctl.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 5/5] xfs: log recovery lsn ordering needs uuid check

[Ben Myers]

On Tue, Sep 24, 2013 at 04:01:16PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> After a fair number of xfstests runs, xfs/182 started to fail
> regularly with a corrupted directory - a directory read verifier was
> failing after recovery because it found a block with a XARM magic
> number (remote attribute block) rather than a directory data block.
> 
> The first time I saw this repeated failure I did /something/ and the
> problem went away, so I was never able to find the underlying
> problem. Test xfs/182 failed again today, and I found the root
> cause before I did /something else/ that made it go away.
> 
> Tracing indicated that the block in question was being correctly
> logged, the log was being flushed by sync, but the buffer was not
> being written back before the shutdown occurred. Tracing also
> indicated that log recovery was also reading the block, but then
> never writing it before log recovery invalidated the cache,
> indicating that it was not modified by log recovery.
> 
> More detailed analysis of the corpse indicated that the filesystem
> had a uuid of "a4131074-1872-4cac-9323-2229adbcb886" but the XARM
> block had a uuid of "8f32f043-c3c9-e7f8-f947-4e7f989c05d3", which
> indicated it was a block from an older filesystem. The reason that
> log recovery didn't replay it was that the LSN in the XARM block was
> larger than the LSN of the transaction being replayed, and so the
> block was not overwritten by log recovery.
> 
> Hence, log recovery cant blindly trust the magic number and LSN in
> the block - it must verify that it belongs to the filesystem being
> recovered before using the LSN. i.e. if the UUIDs don't match, we
> need to unconditionally recovery the change held in the log.
			  recover
 
> This patch was first tested on a block device that was repeatedly
> causing xfs/182 to fail with the same failure on the same block with
> the same directory read corruption signature (i.e. XARM block). It
> did not fail, and hasn't failed since.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good to me.
Reviewed-by: Ben Myers <bpm@sgi.com>

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

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

[Mark Tinguely]

On 09/24/13 01:01, Dave Chinner wrote:
> 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>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 0/5] xfs: fixes for 3.12-rc3

[Ben Myers]

On Tue, Sep 24, 2013 at 04:01:11PM +1000, Dave Chinner wrote:
> These are fixes needed for regressions introduced in the 3.12 merge.
> I posted the first 3 patches a week ago, and sinve then I've found a
> couple more issues in working through all the xfstests failures I've
> been seeing since 3.12-rc1 was released.
> 
> The last patch in the series fixes a filesystem corruption bug, so
> getting these into -rc3 should be considered a priority....

Applied 2-5.

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

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

[Dave Chinner]

On Tue, Sep 24, 2013 at 10:33:24AM -0500, Ben Myers wrote:
> Hi Dave,
> 
> On Tue, Sep 24, 2013 at 04:01:12PM +1000, Dave Chinner wrote:
> > 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
> 
> Here you're showing an assert reported through an xfsbdstrat codepath...
> 
> >  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)
> 
> ...but it looks like your fix is in xfs_bdstrat_cb, which wouldn't have been
> involved in the stack you posted above.  What am I missing?

That the first hunk that changes xfs_bioerror_relse() fixes the bug
that caused the assert failure through xfsbdstrat().

However, if we issue the uncached IO through bwrite() rather than
xfsbdstrat() directly, we need to fix xfs_bdstrat_cb() to handle
uncached buffers appropriately.

i.e. there are multiple IO path call-chain and they all need to call
the correct error handler for uncached buffers....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Ben Myers]

Hi Dave,

On Wed, Sep 25, 2013 at 06:32:32AM +1000, Dave Chinner wrote:
> On Tue, Sep 24, 2013 at 10:33:24AM -0500, Ben Myers wrote:
> > Hi Dave,
> > 
> > On Tue, Sep 24, 2013 at 04:01:12PM +1000, Dave Chinner wrote:
> > > 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
> > 
> > Here you're showing an assert reported through an xfsbdstrat codepath...
> > 
> > >  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)
							 ^^^^^^^^^^^^^
> > 
> > ...but it looks like your fix is in xfs_bdstrat_cb, which wouldn't have been
> > involved in the stack you posted above.  What am I missing?
> 
> That the first hunk that changes xfs_bioerror_relse() fixes the bug
> that caused the assert failure through xfsbdstrat().

*blink*

All I see in that first hunk are changes to comments.  The second hunk seems to
contain the only functional change, highlighted above.

Thanks,
	Ben

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

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

[Dave Chinner]

On Tue, Sep 24, 2013 at 03:59:49PM -0500, Ben Myers wrote:
> Hi Dave,
> 
> On Wed, Sep 25, 2013 at 06:32:32AM +1000, Dave Chinner wrote:
> > On Tue, Sep 24, 2013 at 10:33:24AM -0500, Ben Myers wrote:
> > > ...but it looks like your fix is in xfs_bdstrat_cb, which wouldn't have been
> > > involved in the stack you posted above.  What am I missing?
> > 
> > That the first hunk that changes xfs_bioerror_relse() fixes the bug
> > that caused the assert failure through xfsbdstrat().
> 
> *blink*
> 
> All I see in that first hunk are changes to comments.  The second hunk seems to
> contain the only functional change, highlighted above.

You are right - it should not be reliably fixing the failure I'm
seeing. I'm sure there was something else in this patch originally -
I'll go back and check.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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