Re: [PATCH 17/21] implement generic xfs_btree_split

[Dave Chinner <david@fromorbit.com>]

On Tue, Jul 29, 2008 at 09:31:37PM +0200, Christoph Hellwig wrote:
> Make the btree split 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> +STATIC void
> +xfs_btree_init_block(
> +	struct xfs_btree_cur	*cur,
> +	int			level,
> +	int			numrecs,
> +	struct xfs_btree_block	*new)	/* new block */
> +{
> +	new->bb_h.bb_magic = cpu_to_be32(xfs_magics[cur->bc_btnum]);
> +	new->bb_h.bb_level = cpu_to_be16(level);
> +	new->bb_h.bb_numrecs = cpu_to_be16(numrecs);
> +
> +	if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> +		new->bb_u.l.bb_leftsib = cpu_to_be64(NULLFSBLOCK);
> +		new->bb_u.l.bb_rightsib = cpu_to_be64(NULLFSBLOCK);
> +	} else {
> +		new->bb_u.s.bb_leftsib = cpu_to_be32(NULLAGBLOCK);
> +		new->bb_u.s.bb_rightsib = cpu_to_be32(NULLAGBLOCK);
> +	}
> +}

This is where I begin to question this approach (i.e. using
helpers like this rather than specific ops like I did). It's
taken me 4 ŏr 5 patches to put my finger on it.

The intent of this factorisation is to make implementing new btree
structures easy, not making the current code better or more
managable. The first thing we need is is btrees with different
header blocks (self describing information, CRCs, etc). This above
function will suddenly have four combinations to deal with - long and
short, version 1 and version 2 header formats. The more we change,
the more this complicates these helpers. That is why I pushed
seemingly trivial stuff out to operations vectors - because of the
future flexibility it allowed in implementation of new btrees.....

I don't see this a problem for this patch series, but I can see that
some of this work will end up being converted back to ops vectors
as soon as we start modifying between structures....


> +STATIC int
> +xfs_btree_get_buf_block(
> +	struct xfs_btree_cur	*cur,
> +	union xfs_btree_ptr	*ptr,
> +	int			flags,
> +	struct xfs_btree_block	**block,
> +	struct xfs_buf		**bpp)
> +{
> +	struct xfs_mount	*mp = cur->bc_mp;
> +	xfs_daddr_t		d;
> +
> +	/* need to sort out how callers deal with failures first */
> +	ASSERT(!(flags & XFS_BUF_TRYLOCK));
> +
> +	d = xfs_btree_ptr_to_daddr(cur, ptr);
> +	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
> +				 mp->m_bsize, flags);
> +
> +	ASSERT(*bpp);
> +	ASSERT(!XFS_BUF_GETERROR(*bpp));

xfs_trans_get_buf() can return NULL, right?

> +	/* block allocation / freeing */
> +	int	(*alloc_block)(struct xfs_btree_cur *cur,
> +			       union xfs_btree_ptr *sbno,
> +			       union xfs_btree_ptr *nbno,

start_bno, new_bno.

Otherwise looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com