On Tue, Aug 05, 2008 at 11:36:25AM +1000, Dave Chinner wrote:
> > +#ifdef DEBUG
> > + if (cur->bc_btnum == XFS_BTNUM_BNO || cur->bc_btnum == XFS_BTNUM_CNT) {
> > + struct xfs_buf *agbp = cur->bc_private.a.agbp;
> > + struct xfs_agf *agf = XFS_BUF_TO_AGF(agbp);
> > +
> > + ASSERT(be32_to_cpu(agf->agf_roots[cur->bc_btnum]) ==
> > + XFS_DADDR_TO_AGBNO(cur->bc_mp, XFS_BUF_ADDR(bp)));
> > + } else if (cur->bc_btnum == XFS_BTNUM_INO) {
> > + struct xfs_buf *agbp = cur->bc_private.a.agbp;
> > + struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
> > + xfs_fsblock_t fsbno;
> > +
> > + fsbno = XFS_AGB_TO_FSB(cur->bc_mp, cur->bc_private.a.agno,
> > + be32_to_cpu(agi->agi_root));
> > +
> > + ASSERT(fsbno == XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)));
> > + }
> > +#endif
>
> That's kind of messy - can it be pushed out to a separate debug only
> function? Also, why do we check the inobt against a long pointer,
> and the allocbt against a short pointer? both are short pointer
> btrees, so it doesn't really make sense to me to have different
> checks on them...
The old inobt code built a long pointer to pass it to xfs_free_extent,
and this assert validates we get the same block. The right thing to
do here is to just remove this crap, it was just there to make sure
I didn't add a really bad thinko.
> Otherwise looks pretty good.
Unfortunately it's buggy, though. The cur->bc_bufs[level] = NULL; vs
xfs_btree_setbuf(cur, level, NULL); does make an enormous difference
for 512byte block size filesystems. I'll have to find out what's
going on in this area, and to make things worse I remember spotting
a similar different between the ialloc and alloc btrees somewhere
earlier in the series.
|