[PATCH v4 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push

[PATCH v4 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push

[Brian Foster]

Hi all,

Here is v4 of the spinlock recursion fix. The only update is to the comment in
patch 1. These changes have been run through the reproducer over the weekend
without a failure.

I've also run another xfstests run while including the following patch from Dave:

xfs: xfs_bmap_add_attrfork_local is too generic

... due to seeing the same issue Lukas reproduced in test 013 (I saw it in 070 as
well). That problem no longer occurs with this patch included, but I have hit
an error in test 178 that didn't fire in my baseline run. It persists if I revert
back to TOT, so I'm posting this set as is and I'll see if I can trace where that
one came from...

Brian

v4:
- Fix comment in patch 1.
v3:
- Patches reordered and revised to:
  1.) Add the race detection in xfs_buf_item_push().
  2.) Remove the log force from xfs_buf_trylock() entirely to fix the recursion
      and rely on xfsaild to issue the log force.
v2:
- Patch 2 is reworked to detect the potential race and defer the log force to
  xfsaild by returning XFS_ITEM_PINNED.

Brian Foster (2):
  xfs: recheck buffer pinned status after push trylock failure
  xfs: remove log force from xfs_buf_trylock()

 fs/xfs/xfs_buf.c      |    2 --
 fs/xfs/xfs_buf_item.c |   12 +++++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
1.7.7.6

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

[PATCH v4 1/2] xfs: recheck buffer pinned status after push trylock failure

[Brian Foster]

The buffer pinned check and trylock sequence in xfs_buf_item_push()
can race with an active transaction on marking the buffer pinned.
This can result in the buffer becoming pinned and stale after the
initial check and the trylock failure, but before the check in
xfs_buf_trylock() that issues a log force. If the log force is
issued from this context, a spinlock recursion occurs on xa_lock.

Prepare xfs_buf_item_push() to handle the race by detecting a
pinned buffer after the trylock failure so xfsaild issues a log
force from a safe context. This, along with various previous fixes,
renders the log force in xfs_buf_trylock() redundant.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_buf_item.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 9c4c050..cf26347 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -469,8 +469,18 @@ xfs_buf_item_push(
 
 	if (xfs_buf_ispinned(bp))
 		return XFS_ITEM_PINNED;
-	if (!xfs_buf_trylock(bp))
+	if (!xfs_buf_trylock(bp)) {
+		/*
+		 * If we have just raced with a buffer being pinned and it has
+		 * been marked stale, we could end up stalling until someone else
+		 * issues a log force to unpin the stale buffer. Check for the
+		 * race condition here so xfsaild recognizes the buffer is pinned
+		 * and queues a log force to move it along.
+		 */
+		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

[PATCH v4 2/2] xfs: remove log force from xfs_buf_trylock()

[Brian Foster]

The trylock log force invoked via xfs_buf_item_push() can attempt
to acquire xa_lock, thus leading to a recursion bug when called
with xa_lock held.

This log force was originally added to xfs_buf_trylock() to address
xfsaild stalls due to pinned and stale buffers. Since the addition
of this behavior, the log item pushing code had been reworked to
detect and track pinned items to inform xfsaild to issue a log
force itself when necessary. As such, the log force on trylock
failure is redundant and safe to remove.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_buf.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index fbbb9eb..4e8f0df 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -951,8 +951,6 @@ xfs_buf_trylock(
 	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))
-		xfs_log_force(bp->b_target->bt_mount, 0);
 
 	trace_xfs_buf_trylock(bp, _RET_IP_);
 	return locked;
-- 
1.7.7.6

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

Re: [PATCH v4 1/2] xfs: recheck buffer pinned status after push trylock failure

[Dave Chinner]

On Mon, Feb 11, 2013 at 10:08:21AM -0500, Brian Foster wrote:
> The buffer pinned check and trylock sequence in xfs_buf_item_push()
> can race with an active transaction on marking the buffer pinned.
> This can result in the buffer becoming pinned and stale after the
> initial check and the trylock failure, but before the check in
> xfs_buf_trylock() that issues a log force. If the log force is
> issued from this context, a spinlock recursion occurs on xa_lock.
> 
> Prepare xfs_buf_item_push() to handle the race by detecting a
> pinned buffer after the trylock failure so xfsaild issues a log
> force from a safe context. This, along with various previous fixes,
> renders the log force in xfs_buf_trylock() redundant.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good, doesn't seem to cause any problems I can see here.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v4 2/2] xfs: remove log force from xfs_buf_trylock()

[Dave Chinner]

On Mon, Feb 11, 2013 at 10:08:22AM -0500, Brian Foster wrote:
> The trylock log force invoked via xfs_buf_item_push() can attempt
> to acquire xa_lock, thus leading to a recursion bug when called
> with xa_lock held.
> 
> This log force was originally added to xfs_buf_trylock() to address
> xfsaild stalls due to pinned and stale buffers. Since the addition
> of this behavior, the log item pushing code had been reworked to
> detect and track pinned items to inform xfsaild to issue a log
> force itself when necessary. As such, the log force on trylock
> failure is redundant and safe to remove.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v4 0/2] fix spinlock recursion on xa_lock in xfs_buf_item_push

[Ben Myers]

On Mon, Feb 11, 2013 at 10:08:20AM -0500, Brian Foster wrote:
> Hi all,
> 
> Here is v4 of the spinlock recursion fix. The only update is to the comment in
> patch 1. These changes have been run through the reproducer over the weekend
> without a failure.
> 
> I've also run another xfstests run while including the following patch from Dave:
> 
> xfs: xfs_bmap_add_attrfork_local is too generic
> 
> ... due to seeing the same issue Lukas reproduced in test 013 (I saw it in 070 as
> well). That problem no longer occurs with this patch included, but I have hit
> an error in test 178 that didn't fire in my baseline run. It persists if I revert
> back to TOT, so I'm posting this set as is and I'll see if I can trace where that
> one came from...

Applied these two.

Regards,
	Ben

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