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.
> - 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.
> + /* 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.
> ASSERT(XFS_BUF_VALUSEMA(bp) > 0);
Given that we took the lock a few lines above this one also feels rather
poinless.
> +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);
}
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
