Re: [PATCH 23/26] implement generic xfs_btree_delete/delrec

[Christoph Hellwig <hch@lst.de>]

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.