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@xxxxxxxxxxxxx
|