* [PATCH v2 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push
@ 2013-02-06 12:44 Brian Foster
2013-02-06 12:44 ` [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf Brian Foster
2013-02-06 12:44 ` [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion Brian Foster
0 siblings, 2 replies; 6+ messages in thread
From: Brian Foster @ 2013-02-06 12:44 UTC (permalink / raw)
To: xfs
Hi all,
Here is v2 of the xa_lock recursion fix[1]. This has survived several days of the
reproduction workload and an xfstests run shows no regressions. Thoughts
appreciated.
Brian
v2:
- Patch 2 is reworked to detect the potential race and defer the log force to
xfsaild by returning XFS_ITEM_PINNED.
[1] - recursion stack trace
BUG: spinlock recursion on CPU#5, xfsaild/dm-3/2690
Pid: 2690, comm: xfsaild/dm-3 Not tainted 3.8.0-rc1+ #46
Call Trace:
[<ffffffff8163211c>] spin_dump+0x8a/0x8f
[<ffffffff81632142>] spin_bug+0x21/0x26
[<ffffffff812f66a1>] do_raw_spin_lock+0x101/0x150
[<ffffffff816378ce>] _raw_spin_lock+0xe/0x10
[<ffffffffa0522c85>] xlog_assign_tail_lsn+0x25/0x50 [xfs]
[<ffffffffa0523286>] xlog_state_release_iclog+0x86/0xd0 [xfs]
[<ffffffffa0523c89>] xlog_write+0x569/0x710 [xfs]
[<ffffffffa052555c>] xlog_cil_push+0x29c/0x3c0 [xfs]
[<ffffffffa04cbfe2>] ? xfs_buf_get_map+0xf2/0x1b0 [xfs]
[<ffffffffa0525d17>] xlog_cil_force_lsn+0x157/0x160 [xfs]
[<ffffffffa04cced1>] ? xfs_buf_read_map+0x31/0x130 [xfs]
[<ffffffffa0529e99>] ? xfs_trans_read_buf_map+0x279/0x4b0 [xfs]
[<ffffffff8117e45d>] ? __kmalloc+0x15d/0x1b0
[<ffffffffa0523f7d>] _xfs_log_force+0x6d/0x290 [xfs]
[<ffffffffa051450f>] ? xfs_iflush_cluster+0x25f/0x3d0 [xfs]
[<ffffffffa05241d9>] xfs_log_force+0x39/0xc0 [xfs]
[<ffffffffa04cbaa0>] xfs_buf_trylock+0xd0/0xe0 [xfs]
[<ffffffffa0526369>] xfs_buf_item_push+0x39/0xd0 [xfs]
[<ffffffffa0527bdf>] ? xfs_inode_item_push+0x8f/0x140 [xfs]
[<ffffffffa0528c01>] xfsaild+0x2e1/0x6e0 [xfs]
[<ffffffff8108aa08>] ? __wake_up_common+0x58/0x90
[<ffffffffa0528920>] ? xfs_trans_ail_cursor_first+0xc0/0xc0 [xfs]
[<ffffffff81081708>] kthread+0xd8/0xe0
[<ffffffff81081630>] ? flush_kthread_work+0x150/0x150
[<ffffffff816400ac>] ret_from_fork+0x7c/0xb0
[<ffffffff81081630>] ? flush_kthread_work+0x150/0x150
Brian Foster (2):
xfs: conditionally force log on trylock failure of pinned/stale buf
xfs: disable log force in xfs_buf_item_push() to avoid xa_lock
recursion
fs/xfs/xfs_buf.c | 8 +++++---
fs/xfs/xfs_buf.h | 3 ++-
fs/xfs/xfs_buf_item.c | 14 +++++++++++++-
3 files changed, 20 insertions(+), 5 deletions(-)
--
1.7.7.6
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf 2013-02-06 12:44 [PATCH v2 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push Brian Foster @ 2013-02-06 12:44 ` Brian Foster 2013-02-06 20:24 ` Mark Tinguely 2013-02-07 0:57 ` Dave Chinner 2013-02-06 12:44 ` [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion Brian Foster 1 sibling, 2 replies; 6+ messages in thread From: Brian Foster @ 2013-02-06 12:44 UTC (permalink / raw) To: xfs xfs_force_log() is not safe from all contexts. Add a flag parameter to xfs_buf_trylock() to specify when the force is appropriate and create a macro to preserve current behavior. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_buf.c | 8 +++++--- fs/xfs/xfs_buf.h | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index fbbb9eb..2e04a44 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -943,15 +943,17 @@ xfs_buf_rele( * to push on stale inode buffers. */ int -xfs_buf_trylock( - struct xfs_buf *bp) +__xfs_buf_trylock( + struct xfs_buf *bp, + bool force_log) { int locked; locked = down_trylock(&bp->b_sema) == 0; if (locked) XB_SET_OWNER(bp); - else if (atomic_read(&bp->b_pin_count) && (bp->b_flags & XBF_STALE)) + else if (force_log && + atomic_read(&bp->b_pin_count) && (bp->b_flags & XBF_STALE)) xfs_log_force(bp->b_target->bt_mount, 0); trace_xfs_buf_trylock(bp, _RET_IP_); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index 433a12e..667b723 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -258,7 +258,8 @@ extern void xfs_buf_free(xfs_buf_t *); extern void xfs_buf_rele(xfs_buf_t *); /* Locking and Unlocking Buffers */ -extern int xfs_buf_trylock(xfs_buf_t *); +#define xfs_buf_trylock(bp) __xfs_buf_trylock(bp, true) +extern int __xfs_buf_trylock(xfs_buf_t *, bool); extern void xfs_buf_lock(xfs_buf_t *); extern void xfs_buf_unlock(xfs_buf_t *); #define xfs_buf_islocked(bp) \ -- 1.7.7.6 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf 2013-02-06 12:44 ` [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf Brian Foster @ 2013-02-06 20:24 ` Mark Tinguely 2013-02-07 0:57 ` Dave Chinner 1 sibling, 0 replies; 6+ messages in thread From: Mark Tinguely @ 2013-02-06 20:24 UTC (permalink / raw) To: Brian Foster; +Cc: xfs On 02/06/13 06:44, Brian Foster wrote: > xfs_force_log() is not safe from all contexts. Add a flag parameter > to xfs_buf_trylock() to specify when the force is appropriate and > create a macro to preserve current behavior. > > Signed-off-by: Brian Foster<bfoster@redhat.com> > --- > fs/xfs/xfs_buf.c | 8 +++++--- > fs/xfs/xfs_buf.h | 3 ++- > 2 files changed, 7 insertions(+), 4 deletions(-) > Looks good. 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] 6+ messages in thread
* Re: [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf 2013-02-06 12:44 ` [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf Brian Foster 2013-02-06 20:24 ` Mark Tinguely @ 2013-02-07 0:57 ` Dave Chinner 1 sibling, 0 replies; 6+ messages in thread From: Dave Chinner @ 2013-02-07 0:57 UTC (permalink / raw) To: Brian Foster; +Cc: xfs On Wed, Feb 06, 2013 at 07:44:40AM -0500, Brian Foster wrote: > xfs_force_log() is not safe from all contexts. Add a flag parameter > to xfs_buf_trylock() to specify when the force is appropriate and > create a macro to preserve current behavior. Static inline functions are preferred over macros because they provide type checking..... However, here's what happens when I actually have time to think about the problem at hand - I suggest a completely different fix. Sorry, Brain, I'm not doing this intentionally to make your life harder than it needs to be... :/ My logic (goes back to the history of changes that lead to the log force being in xfs_buf_trylock()) is as follows: 1. xfs_buf_lock() had the log force added originally because _xfs_buf_find was getting stuck on locked, pinned stale buffers. This was a result of the rework of the busy extent handling to avoid synchronous transactions and/or log forces when reusing recently freed blocks. Somewhere a log force was needed to get the freeing transaction on disk before reuse occurred, and that was done on demand in xfs_buf_lock() when such a stale buffer was being used after a subsequent allocation. 2a. xfs_inode_item_push() used to flush inodes to the the underlying buffer and would skip the flush if the underlying buffer was locked. Hence pinned inodes could not be flushed if the underlying buffer was locked (i.e. when pinned and stale). 2b. xfs_buf_item_push() would fail to lock the buffer if it raced with pinning and do nothing. 3. this resulted in neither xfs_inode_item_push() or xfs_buf_item_push() telling the xfsaild to issue a log force, the AIL would stop doing work as finished it's traverse, and hence tranaction reservations stalled until something else issued a log force and unpinned the tail of the log. 4. the simple and obvious fix at the time was to have xfs_buf_trylock() do the same as xfs_buf_lock() and force the log when a buffer in the state that caused the stall was detected. <time passes> 5a. delayed write buffers were reworked, completely changing how inode flushing and hence xfs_inode_item_push() worked. 5b. delayed write buffers were reworked, changing how xfs_buf_item_push() worked. 6. several bugs in pinned/stale item handling were fixed, and now both xfs_inode_item_push() and xfs_buf_item_push() detect pinned inodes and buffers and tell the xfsaild to force the log appropriately. This means xfs_buf_trylock() no longer needs to force the log to provide a get-out-of-xfsaild-stall-free mechanism. 7. This log force while holding the ail lock bug is discovered. So what I'm thinking is that as a result of 5+6, we have no need for the log force in the xfs_buf_trylock code any more. The only place we actually care about locking latency any more is in _xfs_buf_find(), and when the trylock fails there we immediately run xfs_buf_lock() (unless XBF_TRYLOCK was set) and it will do the log force for us. IOWs, I think the log force code in xfs_buf_trylock() is simply redundant and we should just remove it. With the second patch in this series, racing on the buffer being pinned will now do the right thing (i.e. trigger a log force via the xfsaild) and so we end up both simplifying the code and fixing the bug.... 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] 6+ messages in thread
* [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion 2013-02-06 12:44 [PATCH v2 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push Brian Foster 2013-02-06 12:44 ` [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf Brian Foster @ 2013-02-06 12:44 ` Brian Foster 2013-02-06 20:25 ` Mark Tinguely 1 sibling, 1 reply; 6+ messages in thread From: Brian Foster @ 2013-02-06 12:44 UTC (permalink / raw) To: xfs If the trylock fails and the buffer is pinned and stale, the resulting xfs_log_force() can lead to lock recursion on the ailp xa_lock. Call __xfs_buf_trylock() to attempt the buf lock with xa_lock held, but to avoid the log force. If the trylock had raced with the buffer being pinned, return XFS_ITEM_PINNED such that xfsaild will pend up a log force on the next scan. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_buf_item.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-) diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index 9c4c050..23a328e 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -469,8 +469,20 @@ xfs_buf_item_push( if (xfs_buf_ispinned(bp)) return XFS_ITEM_PINNED; - if (!xfs_buf_trylock(bp)) + + if (!__xfs_buf_trylock(bp, false)) { + /* + * We disable the log force via trylock because the buffer can + * become pinned and stale after the trylock fails and the log + * force is unsafe with xa_lock held. + * + * Check the buffer state once more and return pinned such that + * xfsaild pends up the log force when it drops xa_lock. + */ + if (xfs_buf_ispinned(bp)) + return XFS_ITEM_PINNED; return XFS_ITEM_LOCKED; + } ASSERT(!(bip->bli_flags & XFS_BLI_STALE)); -- 1.7.7.6 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion 2013-02-06 12:44 ` [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion Brian Foster @ 2013-02-06 20:25 ` Mark Tinguely 0 siblings, 0 replies; 6+ messages in thread From: Mark Tinguely @ 2013-02-06 20:25 UTC (permalink / raw) To: Brian Foster; +Cc: xfs On 02/06/13 06:44, Brian Foster wrote: > If the trylock fails and the buffer is pinned and stale, the > resulting xfs_log_force() can lead to lock recursion on the ailp > xa_lock. Call __xfs_buf_trylock() to attempt the buf lock with > xa_lock held, but to avoid the log force. If the trylock had raced > with the buffer being pinned, return XFS_ITEM_PINNED such that > xfsaild will pend up a log force on the next scan. > > Signed-off-by: Brian Foster<bfoster@redhat.com> > --- > fs/xfs/xfs_buf_item.c | 14 +++++++++++++- > 1 files changed, 13 insertions(+), 1 deletions(-) Looks good. 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] 6+ messages in thread
end of thread, other threads:[~2013-02-07 0:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-06 12:44 [PATCH v2 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push Brian Foster 2013-02-06 12:44 ` [PATCH v2 1/2] xfs: conditionally force log on trylock failure of pinned/stale buf Brian Foster 2013-02-06 20:24 ` Mark Tinguely 2013-02-07 0:57 ` Dave Chinner 2013-02-06 12:44 ` [PATCH v2 2/2] xfs: disable log force in xfs_buf_item_push() to avoid xa_lock recursion Brian Foster 2013-02-06 20:25 ` Mark Tinguely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox