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