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

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

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);
> 
> But either ways is fine with me:

This got me thinking about the rtrmap and refcount btrees -- could we
save 72 bytes per inode when the btree is completely empty by returning
0 from xfs_{rtrmap,rtrefcount}_broot_space_calc?  The answer to
that was a bunch of null pointer dereferences because there's a fair
amount of code in the rtrmap/rtrefcount/btree code that assumes that
if_broot isn't null if you've created a btree cursor.

OTOH if you're really going to have 130000 zns rtgroups then maybe we
actually want this savings?  That's 9MB of memory that we don't have to
waste on an empty device -- for rtrmaps the savings are minimal because
eventually you'll write *something*; for rtrefcounts, this might be
meaningful because you format with reflink but don't necessarily use it
right away.

Thoughts?

--D

> Reviewed-by: Christoph Hellwig <hch@lst.de>
>