Re: [PATCH 05/10] xfs: move the zero records logic into xfs_bmap_broot_space_calc

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Aug 29, 2024 at 12:05:06PM +1000, Dave Chinner wrote:
> On Wed, Aug 28, 2024 at 06:14:24AM +0200, Christoph Hellwig wrote:
> > On Tue, Aug 27, 2024 at 04:35:01PM -0700, Darrick J. Wong wrote:
> > > This helps us remove a level of indentation in xfs_iroot_realloc because
> > > we can handle the zero-size case in a single place instead of repeatedly
> > > checking it.  We'll refactor further in the next patch.
> > 
> > I think we can do the same cleanup in xfs_iroot_realloc without this
> > special case:
> > 
> > This:
> > 
> > > +	new_size = xfs_bmap_broot_space_calc(mp, new_max);
> > > +	if (new_size == 0) {
> > > +		kfree(ifp->if_broot);
> > > +		ifp->if_broot = NULL;
> > > +		ifp->if_broot_bytes = 0;
> > > +		return;
> > 
> > becomes:
> > 
> > 	if (new_max == 0) {
> > 		kfree(ifp->if_broot);
> > 		ifp->if_broot = NULL;
> > 		ifp->if_broot_bytes = 0;
> > 		return;
> > 	}
> > 	new_size = xfs_bmap_broot_space_calc(mp, new_max);
> 
> I kinda prefer this version; I noticed the code could be cleaned
> up the way looking at some other patch earlier this morning...

As I pointed out to Christoph in this thread already, this change won't
age well because the rt rmap and refcount btrees will want to create
incore btree root blocks with zero records and then create btree cursors
around that.  This refactoring series, incidentally, comes from the
rtrmap series.  Cursor initialization will try to access ifp->if_broot,
which results in null pointer deref whackamole all over xfs_btree.c if
we do that.

I'm working on a better solution than that, which might be pointing
if_broot to a zeroed out rodata xfs_btree_block object and amending
xfs_iroot_free not to delete anything that's not a heap pointer.

We don't need that here yet because the bmap btree code only sets
new_max==0 when it's tearing down the ondisk btree prior to converting
to FMT_EXTENTS, but I'd rather not change this patch now just to revert
it a month from now back to what I originally posted.

--D

> -Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
>