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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
