[PATCH 0/2] xfs: more shutdown-related fixes

[PATCH 0/2] xfs: more shutdown-related fixes

[Dave Chinner]

Hi folks,

More fixes as a result of forced shutdown testing. The first is
fixing yet another hole in the buf log item freein logic when a
transaction is aborted, and the other removes the asserts from the
inode buffer checking so that verifiers return errors rather than
crashing the system.

Cheers,

Dave.

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

[PATCH 1/2] xfs: aborted buf items can be in the AIL.

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Saw this on generic/270 after a DQALLOC transaction overrun
shutdown:

XFS: Assertion failed: !(bip->bli_item.li_flags & XFS_LI_IN_AIL), file: fs/xfs/xfs_buf_item.c, line: 952
.....
 xfs_buf_item_relse+0x4f/0xd0
 xfs_buf_item_unlock+0x1b4/0x1e0
 xfs_trans_free_items+0x7d/0xb0
 xfs_trans_cancel+0x13c/0x1b0
 xfs_symlink+0x37e/0xa60
....

When a transaction abort occured.

If we are aborting a transaction and trigger this code path, then
the item may be dirty. If the item is dirty, then it may be in the
AIL. Hence if we are aborting, we need to check if the item is in
the AIL and remove it before freeing it.

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

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 3a944b1..88c5ea7 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -613,13 +613,27 @@ xfs_buf_item_unlock(
 			}
 		}
 	}
-	if (clean || aborted) {
-		if (atomic_dec_and_test(&bip->bli_refcount)) {
-			ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
+
+	/*
+	 * Clean buffers, by definition, cannot be in the AIL. However, aborted
+	 * buffers may be dirty and hence in the AIL. Therefore if we are
+	 * aborting a buffer and we've just taken the last refernce away, we
+	 * have to check if it is in the AIL before freeing it. We need to free
+	 * it in this case, because an aborted transaction has already shut the
+	 * filesystem down and this is the last chance we will have to do so.
+	 */
+	if (atomic_dec_and_test(&bip->bli_refcount)) {
+		if (clean)
+			xfs_buf_item_relse(bp);
+		else if (aborted) {
+			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
+			if (lip->li_flags & XFS_LI_IN_AIL) {
+				xfs_trans_ail_delete(lip->li_ailp, lip,
+						     SHUTDOWN_LOG_IO_ERROR);
+			}
 			xfs_buf_item_relse(bp);
 		}
-	} else
-		atomic_dec(&bip->bli_refcount);
+	}
 
 	if (!(flags & XFS_BLI_HOLD))
 		xfs_buf_relse(bp);
-- 
1.8.3.2

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

[PATCH 2/2] xfs: don't assert fail on bad inode numbers

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Let the inode verifier do it's work by returning an error when we
fail to find correct magic numbers in an inode buffer.

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

diff --git a/fs/xfs/xfs_inode_buf.c b/fs/xfs/xfs_inode_buf.c
index 3d25c9a..63382d3 100644
--- a/fs/xfs/xfs_inode_buf.c
+++ b/fs/xfs/xfs_inode_buf.c
@@ -53,9 +53,8 @@ xfs_inobp_check(
 					i * mp->m_sb.sb_inodesize);
 		if (!dip->di_next_unlinked)  {
 			xfs_alert(mp,
-	"Detected bogus zero next_unlinked field in incore inode buffer 0x%p.",
-				bp);
-			ASSERT(dip->di_next_unlinked);
+	"Detected bogus zero next_unlinked field in inode %d buffer 0x%llx.",
+				i, (long long)bp->b_bn);
 		}
 	}
 }
@@ -106,11 +105,10 @@ xfs_inode_buf_verify(
 			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_HIGH,
 					     mp, dip);
 #ifdef DEBUG
-			xfs_emerg(mp,
+			xfs_alert(mp,
 				"bad inode magic/vsn daddr %lld #%d (magic=%x)",
 				(unsigned long long)bp->b_bn, i,
 				be16_to_cpu(dip->di_magic));
-			ASSERT(0);
 #endif
 		}
 	}
-- 
1.8.3.2

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

Re: [PATCH 0/2] xfs: more shutdown-related fixes

[Christoph Hellwig]

On Tue, Sep 03, 2013 at 09:47:36PM +1000, Dave Chinner wrote:
> Hi folks,
> 
> More fixes as a result of forced shutdown testing. The first is
> fixing yet another hole in the buf log item freein logic when a
> transaction is aborted, and the other removes the asserts from the
> inode buffer checking so that verifiers return errors rather than
> crashing the system.

Btw, I've been wondering for a while if we need a major change to how
the buf item refcounting works.  All these little special cases in there
are utterly non-intuitive.  I've not looked very deep yet, but a normal
scheme where every reference to it increments the refcount, and we
simply free it when that hits zero should work here as well.  We'd
still need flags for the abort and clean conditions, but it would still
be way simpler than what we have now.

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

Re: [PATCH 0/2] xfs: more shutdown-related fixes

[Dave Chinner]

On Tue, Sep 03, 2013 at 12:02:10PM -0700, Christoph Hellwig wrote:
> On Tue, Sep 03, 2013 at 09:47:36PM +1000, Dave Chinner wrote:
> > Hi folks,
> > 
> > More fixes as a result of forced shutdown testing. The first is
> > fixing yet another hole in the buf log item freein logic when a
> > transaction is aborted, and the other removes the asserts from the
> > inode buffer checking so that verifiers return errors rather than
> > crashing the system.
> 
> Btw, I've been wondering for a while if we need a major change to how
> the buf item refcounting works.  All these little special cases in there
> are utterly non-intuitive.  I've not looked very deep yet, but a normal
> scheme where every reference to it increments the refcount, and we
> simply free it when that hits zero should work here as well.  We'd
> still need flags for the abort and clean conditions, but it would still
> be way simpler than what we have now.

Yes, it makes sense to do, but I haven't considered it yet as I have
other things to worry about right now. We'd still need the AIL
removal on abort, though, as that is still the last place we'll see
it on a shutdown...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/2] xfs: aborted buf items can be in the AIL.

[Mark Tinguely]

On 09/03/13 06:47, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Saw this on generic/270 after a DQALLOC transaction overrun
> shutdown:
>
> XFS: Assertion failed: !(bip->bli_item.li_flags&  XFS_LI_IN_AIL), file: fs/xfs/xfs_buf_item.c, line: 952
> .....
>   xfs_buf_item_relse+0x4f/0xd0
>   xfs_buf_item_unlock+0x1b4/0x1e0
>   xfs_trans_free_items+0x7d/0xb0
>   xfs_trans_cancel+0x13c/0x1b0
>   xfs_symlink+0x37e/0xa60
> ....
>
> When a transaction abort occured.
>
> If we are aborting a transaction and trigger this code path, then
> the item may be dirty. If the item is dirty, then it may be in the
> AIL. Hence if we are aborting, we need to check if the item is in
> the AIL and remove it before freeing it.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

This is fine for Linux 3.12. Christoph's reference counting reorg sounds 
interesting.

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

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

Re: [PATCH 2/2] xfs: don't assert fail on bad inode numbers

[Mark Tinguely]

On 09/03/13 06:47, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Let the inode verifier do it's work by returning an error when we
> fail to find correct magic numbers in an inode buffer.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

Looks okay - the xfs_buf_ioerror() sets the error code for the caller to 
detect.

We talked about this in June:
  http://oss.sgi.com/archives/xfs/2013-06/msg00712.html

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/2] xfs: more shutdown-related fixes

[Ben Myers]

On Tue, Sep 03, 2013 at 09:47:36PM +1000, Dave Chinner wrote:
> Hi folks,
> 
> More fixes as a result of forced shutdown testing. The first is
> fixing yet another hole in the buf log item freein logic when a
> transaction is aborted, and the other removes the asserts from the
> inode buffer checking so that verifiers return errors rather than
> crashing the system.

Applied these 2.

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

Re: [PATCH 2/2] xfs: don't assert fail on bad inode numbers

[Rich Johnston]

This has been committed.

Thanks
--Rich

commit ea6a00d47a01bc1df218742784712a12929ab77e
Author: Dave Chinner <dchinner@redhat.com>
Date:   Mon Sep 30 03:15:18 2013 +0000

     xfs: don't assert fail on bad inode numbers

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