xfs
[Top] [All Lists]

Re: [PATCH 03/12] xfs: modify buffer item reference counting

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 03/12] xfs: modify buffer item reference counting
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 19 May 2010 13:35:58 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1274138668-1662-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1274138668-1662-1-git-send-email-david@xxxxxxxxxxxxx> <1274138668-1662-4-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Tue, 2010-05-18 at 09:24 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The buffer log item reference counts used to take referenceѕ for every
> transaction, similar to the pin counting. This is symmetric (like the
> pin/unpin) with respect to transaction completion, but with dleayed logging
> becomes assymetric as the pinning becomes assymetric w.r.t. transaction
> completion.

Doing the reference counts this way is an improvement.

I have a question below.

. . .
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
. . .
>  STATIC void
>  xfs_buf_item_unlock(
> @@ -514,73 +523,54 @@ xfs_buf_item_unlock(
>  
>       bp = bip->bli_buf;
>  
> -     /*
> -      * Clear the buffer's association with this transaction.
> -      */
> +     /* Clear the buffer's association with this transaction. */
>       XFS_BUF_SET_FSPRIVATE2(bp, NULL);
>  
>       /*
> -      * If this is a transaction abort, don't return early.
> -      * Instead, allow the brelse to happen.
> -      * Normally it would be done for stale (cancelled) buffers
> -      * at unpin time, but we'll never go through the pin/unpin
> -      * cycle if we abort inside commit.
> +      * If this is a transaction abort, don't return early.  Instead, allow
> +      * the brelse to happen.  Normally it would be done for stale
> +      * (cancelled) buffers at unpin time, but we'll never go through the
> +      * pin/unpin cycle if we abort inside commit.
>        */
>       aborted = (bip->bli_item.li_flags & XFS_LI_ABORTED) != 0;
>  
>       /*
> -      * If the buf item is marked stale, then don't do anything.
> -      * We'll unlock the buffer and free the buf item when the
> -      * buffer is unpinned for the last time.
> +      * Before possibly freeing the buf item, determine if we should
> +      * release the buffer at the end of this routine.
> +      */
> +     hold = bip->bli_flags & XFS_BLI_HOLD;
> +
> +     /* Clear the per transaction state. */
> +     bip->bli_flags &= ~(XFS_BLI_LOGGED | XFS_BLI_HOLD);
> +
> +     /*
> +      * If the buf item is marked stale, then don't do anything.  We'll
> +      * unlock the buffer and free the buf item when the buffer is unpinned
> +      * for the last time.
>        */
>       if (bip->bli_flags & XFS_BLI_STALE) {
> -             bip->bli_flags &= ~XFS_BLI_LOGGED;
>               trace_xfs_buf_item_unlock_stale(bip);
>               ASSERT(bip->bli_format.blf_flags & XFS_BLI_CANCEL);
> -             if (!aborted)
> +             if (!aborted) {
> +                     atomic_dec(&bip->bli_refcount);

I notice that, unlike before, if you return via this path
the XFS_BLI_HOLD flag will have been turned off.  I guess
I don't know off hand whether this is an issue.  Can you
explain why it is not?

>                       return;
> +             }
>       }
>  
. . .

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