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

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

[Brian Foster]

Hi all,

Here is v3 of the spinlock recursion fix, based on Dave's comments here:

http://oss.sgi.com/archives/xfs/2013-02/msg00106.html

Note that I'm testing and posting this for review in parallel, given that v2
had seen several hours of testing with the log force bypassed in aild context.
This implementation removes the trylock log force entirely, so continued testing
will determine whether any regressions crop up and I'll post an update after a
few days.

Brian

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 |    9 ++++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
1.7.7.6

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

[PATCH v3 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 |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 9c4c050..4e3a059 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -469,8 +469,15 @@ xfs_buf_item_push(
 
 	if (xfs_buf_ispinned(bp))
 		return XFS_ITEM_PINNED;
-	if (!xfs_buf_trylock(bp))
+	if (!xfs_buf_trylock(bp)) {
+		/*
+		 * Check whether we've raced with the buffer being pinned so
+		 * xfsaild will pend up a log force.
+		 */
+		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

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

[Dave Chinner]

On Fri, Feb 08, 2013 at 09:51:06AM -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>
> ---
>  fs/xfs/xfs_buf_item.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 9c4c050..4e3a059 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -469,8 +469,15 @@ xfs_buf_item_push(
>  
>  	if (xfs_buf_ispinned(bp))
>  		return XFS_ITEM_PINNED;
> -	if (!xfs_buf_trylock(bp))
> +	if (!xfs_buf_trylock(bp)) {
> +		/*
> +		 * Check whether we've raced with the buffer being pinned so
> +		 * xfsaild will pend up a log force.

"pend up": never heard that one before. ;P

Perhaps "queue up" would be better?

As it is, the comment describes what the code is doing, not why we
are doing this check. i.e. "if we just raced with a buffer being
pinned and the buffer has been marked stale, we could end up
stalling until someone else issues a log force to unpin the stale
buffer. Hence, do an optimistic check for the race condition to get
this buffer moving along quickly if we hit it..."


-- 
Dave Chinner
david@fromorbit.com

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

[PATCH v3 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