Re: [PATCH 17/21] implement generic xfs_btree_split

[Christoph Hellwig <hch@lst.de>]

> 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....

Maybe.  But even when we convert it to ops vectors it should not
be the btree implementation vector, but a btree_block_ops that's
implemented once instead of duplicated for the alloc vs ialloc
btree.  And for now having all this in xfs_btree.c makes reading
and working on the patch series easier, so..

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

Only when XFS_BUF_TRYLOCK is set, which it never is for the
btree code, and the assert above makes sure we catch any new caller
early.

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

Done.