xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: kill XBF_FS_MANAGED buffers

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: kill XBF_FS_MANAGED buffers
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 10 Sep 2010 13:10:04 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100909013312.GB29825@xxxxxxxxxxxxx>
References: <1283958778-28610-1-git-send-email-david@xxxxxxxxxxxxx> <1283958778-28610-2-git-send-email-david@xxxxxxxxxxxxx> <20100909013312.GB29825@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Sep 08, 2010 at 09:33:13PM -0400, Christoph Hellwig wrote:
> Looks good, but a few comments below:
> 
> > +   bp = xfs_buf_get_noaddr(sector_size, mp->m_ddev_targp);
> > +
> >     if (!bp || XFS_BUF_ISERROR(bp)) {
> 
> xfs_buf_get_noaddr will never return a buffer with an error set.

OK, will fix.

> > -   ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
> > +
> > +   /* set up the buffer for a read IO */
> > +   xfs_buf_lock(bp);
> > +   XFS_BUF_SET_ADDR(bp, XFS_SB_DADDR);
> > +        XFS_BUF_READ(bp);
> > +        XFS_BUF_BUSY(bp);
> 
> Various indentation problems.

I'll fix all these by putting the uncached read function factoring
into an initial patch.

> > +   /* grab a reference for caching the buffer */
> > +        XFS_BUF_HOLD(bp);
> >     mp->m_sb_bp = bp;
> > +
> >     xfs_buf_relse(bp);
> 
> Grabbing the reference just to drop it three lines later is rather
> pointless, just remove both.

Except that xfs_buf_relse(bp) also unlocks bp. maybe I coul djust
make it do an explicit unlock....

> >     ASSERT(XFS_BUF_VALUSEMA(bp) > 0);
> 
> Given that we took the lock a few lines above this one also feels rather
> poinless.

That's checking the buffer is unlocked, which could be removed if
there is an explicit unlock call. I'll do that.

> 
> > +fail:
> > +   if (bp)
> >             xfs_buf_relse(bp);
> > -   }
> >     return error;
> 
> I'd rather see this split into a fail_buf_relese label that puts the
> buffer, and a fail label that just returns the error.
> 
> >      * when we call xfs_buf_relse().
> >      */
> >     bp = xfs_getsb(mp, 0);
> > -   XFS_BUF_UNMANAGE(bp);
> > -   xfs_buf_relse(bp);
> >     mp->m_sb_bp = NULL;
> > +
> > +   /*
> > +    * need to release the buffer twice to free it because we hold an extra
> > +    * reference count on it.
> > +    */
> > +   xfs_buf_relse(bp);
> > +   xfs_buf_relse(bp);
> 
> I'd rather rewrite xfs_freesb to not use xfs_getsb and thus avoid taking
> the superflous reference:
> 
> void
> xfs_freesb(
>       struct xfs_mount        *mp);
> 
>       struct xfs_buf          *bp = mp->m_sb_bp;
> 
>       mp->m_sb_bp = NULL;
>       if (xfs_buf_cond_lock(bp)
>               BUG();
>       xfs_buf_relse(bp);
> }

Yeah, that's better. I'll do that.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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