xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 23/26] implement generic xfs_btree_delete/delrec
From: Christoph Hellwig <hch@xxxxxx>
Date: Tue, 5 Aug 2008 03:45:24 +0200
In-reply-to: <20080805013625.GN6119@disturbed>
References: <20080804013550.GX8819@lst.de> <20080805013625.GN6119@disturbed>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.3.28i
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.


<Prev in Thread] Current Thread [Next in Thread>