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

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 05, 2008 at 03:45:24AM +0200, Christoph Hellwig wrote:
> 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 was going to come back to that - I'd noticed that difference but
hadn't looked deeply into the change that it would cause.

It looks like:

	a) it releases the old buffer
	b) clears the readahead status in the cursor

a) will increase CPU overhead, but if the buffer is dirty won't
affect behaviour at all as it will be held till transaction commit.
b) will have a significant impact on the performance of btree
traversals. That will show up most in small block size
filesystems...

So it shouldn't be a corruption causing change, only a performance
degrading change.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com