[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v3 1/2] xfs: recheck buffer pinned status after push trylock failure
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 11 Feb 2013 10:24:48 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1360335067-59999-2-git-send-email-bfoster@xxxxxxxxxx>
References: <1360335067-59999-1-git-send-email-bfoster@xxxxxxxxxx> <1360335067-59999-2-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> ---
>  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

<Prev in Thread] Current Thread [Next in Thread>