xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 23/26] implement generic xfs_btree_delete/delrec
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Aug 2008 12:18:34 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080805014524.GA6465@lst.de>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20080804013550.GX8819@lst.de> <20080805013625.GN6119@disturbed> <20080805014524.GA6465@lst.de>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
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@xxxxxxxxxxxxx


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