* [PATCH 1/2] xfs: aborted buf items can be in the AIL.
2013-09-03 11:47 [PATCH 0/2] xfs: more shutdown-related fixes Dave Chinner
@ 2013-09-03 11:47 ` Dave Chinner
2013-09-06 19:39 ` Mark Tinguely
2013-09-03 11:47 ` [PATCH 2/2] xfs: don't assert fail on bad inode numbers Dave Chinner
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Dave Chinner @ 2013-09-03 11:47 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/2] xfs: aborted buf items can be in the AIL.
2013-09-03 11:47 ` [PATCH 1/2] xfs: aborted buf items can be in the AIL Dave Chinner
@ 2013-09-06 19:39 ` Mark Tinguely
0 siblings, 0 replies; 9+ messages in thread
From: Mark Tinguely @ 2013-09-06 19:39 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] xfs: don't assert fail on bad inode numbers
2013-09-03 11:47 [PATCH 0/2] xfs: more shutdown-related fixes Dave Chinner
2013-09-03 11:47 ` [PATCH 1/2] xfs: aborted buf items can be in the AIL Dave Chinner
@ 2013-09-03 11:47 ` Dave Chinner
2013-09-06 19:54 ` Mark Tinguely
2013-10-18 16:51 ` Rich Johnston
2013-09-03 19:02 ` [PATCH 0/2] xfs: more shutdown-related fixes Christoph Hellwig
2013-09-10 22:39 ` Ben Myers
3 siblings, 2 replies; 9+ messages in thread
From: Dave Chinner @ 2013-09-03 11:47 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/2] xfs: more shutdown-related fixes
2013-09-03 11:47 [PATCH 0/2] xfs: more shutdown-related fixes Dave Chinner
2013-09-03 11:47 ` [PATCH 1/2] xfs: aborted buf items can be in the AIL Dave Chinner
2013-09-03 11:47 ` [PATCH 2/2] xfs: don't assert fail on bad inode numbers Dave Chinner
@ 2013-09-03 19:02 ` Christoph Hellwig
2013-09-03 19:56 ` Dave Chinner
2013-09-10 22:39 ` Ben Myers
3 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2013-09-03 19:02 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] xfs: more shutdown-related fixes
2013-09-03 19:02 ` [PATCH 0/2] xfs: more shutdown-related fixes Christoph Hellwig
@ 2013-09-03 19:56 ` Dave Chinner
0 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2013-09-03 19:56 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] xfs: more shutdown-related fixes
2013-09-03 11:47 [PATCH 0/2] xfs: more shutdown-related fixes Dave Chinner
` (2 preceding siblings ...)
2013-09-03 19:02 ` [PATCH 0/2] xfs: more shutdown-related fixes Christoph Hellwig
@ 2013-09-10 22:39 ` Ben Myers
3 siblings, 0 replies; 9+ messages in thread
From: Ben Myers @ 2013-09-10 22:39 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
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
^ permalink raw reply [flat|nested] 9+ messages in thread