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

[Christoph Hellwig <hch@infradead.org>]

On Tue, Aug 05, 2008 at 11:05:57AM +1000, Dave Chinner wrote:
> 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....

Yeah.  The name giving macro is already gone in my current tree.

> 
> > +	/* 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.

Done.

> > +	/*
> > +	 * 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.
> 	 */

Updated.