xfs
[Top] [All Lists]

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

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 03/12] xfs: modify buffer item reference counting
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 20 May 2010 08:37:25 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1274294158.2140.35.camel@doink>
References: <1274138668-1662-1-git-send-email-david@xxxxxxxxxxxxx> <1274138668-1662-4-git-send-email-david@xxxxxxxxxxxxx> <1274294158.2140.35.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, May 19, 2010 at 01:35:58PM -0500, Alex Elder wrote:
> 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?

A buffer that has been marked stale is on that has been invalidated;
they shouldn't get held across rolling transactions because they
reference free space and cannot be reused without being reallocated.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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