Re: [PATCH 22/26] move xfs_bmbt_killroot to common code

[Christoph Hellwig <hch@infradead.org>]

On Tue, Aug 05, 2008 at 11:14:37AM +1000, Dave Chinner wrote:
> On Mon, Aug 04, 2008 at 03:35:42AM +0200, Christoph Hellwig wrote:
> > xfs_bmbt_killroot is a mostly generic implementation of moving from
> > a real block based root to an inode based root.  So move it to xfs_btree.c
> > where it can use all the nice infrastructure there and make it pointer
> > size agnostic
> > 
> > The new name for it is xfs_btree_root_to_iroot which is not very
> > nice but at least slightly more descriptive than the old name.
> .....
> > +
> > +	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> > +	level = cur->bc_nlevels - 1;
> > +	ASSERT(level >= 1);
> 
> probably should assert root in inode is set here.

We have that check just before the call, so this might be a little
overkill..

> 
> > +	cblock = xfs_btree_get_block(cur, level - 1, &cbp);
> > +	numrecs = xfs_btree_get_numrecs(cblock);
> > +
> > +	if (numrecs > cur->bc_ops->get_dmaxrecs(cur, level))
> > +	    	goto out0;
>         ^^^^
> Stray whitespace.

Already fixed before your mail after I ran checkpatch.pl
over all patches - there were a few spread over the various patches.

> > +	// XXX(hch): this assert is bmap btree specific
> > + 	ASSERT(cur->bc_ops->get_maxrecs(cur, level) ==
>    ^
> > +	       XFS_BMAP_BROOT_MAXRECS(ifp->if_broot_bytes));
> 
> Stray whitespace. As to the assert - what is it really trying to
> check? That the btree root space in the inode is large enough to
> fit the max number of records? If so, does it really need to be
> checked here (i.e. could the caller do it?)

I don't think it makes much sense at all.  My preference would
be to simply kill it.