Re: [PATCH 21/26] implement generic xfs_btree_inѕert/insrec

[Dave Chinner <david@fromorbit.com>]

On Mon, Aug 04, 2008 at 03:35:35AM +0200, Christoph Hellwig wrote:
> Make the btree insert 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.
....
> +	if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) {
> +		if (numrecs < cur->bc_ops->get_dmaxrecs(cur, level)) {
> +			/* A resizeable root block that can be made bigger. */
> +			cur->bc_ops->realloc_root(cur, 1);
> +			return 0;
> +		}

I think that ->get_dmaxrecs is probably badly named. I called it
that originally because it matched the macro name it was wrapping.
Realisitically it should be ->get_root_maxrecs....

> +	/* Make a key out of the record data to be inserted, and save it. */
> +	cur->bc_ops->init_key_from_rec(cur, &key, recp);
> +
> +	/* If we're off the left edge, return failure. */
> +	ptr = cur->bc_ptrs[level];
> +	if (ptr == 0) {
> +		XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
> +		*stat = 0;
> +		return 0;
> +	}

You can probably move the key initialisation till after then initial
'in this block' checks.

> +	/*
> +	 * If the block is full, we can't insert the new entry until we
> +	 * make the block un-full.
> +	 */
> +	xfs_btree_set_ptr_null(cur, &nptr);
> +	if (numrecs == cur->bc_ops->get_maxrecs(cur, level)) {
> +		error = xfs_btree_make_block_unfull(cur, level, numrecs,
> +					&optr, &ptr, &nptr, &ncur, &nrec, stat);
> +		if (error || *stat == 0)
> +			goto error0;
> +	}
> +
> +	/* The current block may have changed during the split. */
> +	block = xfs_btree_get_block(cur, level, &bp);
> +	numrecs = xfs_btree_get_numrecs(block);

The comment here should probably refer to the unfull call, not a
'split'. i.e.:

	/*
	 * the current block may have changed if the block was
	 * previously full and we have just made space in it.
	 */


Otherwise looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com