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 11:36:25 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20080804013550.GX8819@lst.de>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20080804013550.GX8819@lst.de>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
On Mon, Aug 04, 2008 at 03:35:50AM +0200, Christoph Hellwig wrote:
> Make the btree delete code generic.  Based on a patch from David Chinner
> with lots of changes to follow the original btree implementations more
> closely.  While this loses some of the generic helper routines for
> inserting/moving/removing records it also solves some of the one off
> bugs in the original code and makes it easier to verify.
.....
> +STATIC int
> +xfs_btree_update_root(
> +     struct xfs_btree_cur    *cur,
> +     struct xfs_buf          *bp,
> +     int                     level,
> +     union xfs_btree_ptr     *newroot)
> +{
> +     int                     error;
> +
> +     XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> +     XFS_BTREE_STATS_INC(cur, killroot);
> +
> +#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...

Otherwise looks pretty good.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx


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