[PATCHSET v4.1] xfs: cleanups for inode rooted btree code

[PATCHSET v4.1] xfs: cleanups for inode rooted btree code

[Darrick J. Wong]

Hi all,

This series prepares the btree code to support realtime reverse mapping btrees
by refactoring xfs_ifork_realloc to be fed a per-btree ops structure so that it
can handle multiple types of inode-rooted btrees.  It moves on to refactoring
the btree code to use the new realloc routines.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been running on the djcloud for months with no problems.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=btree-cleanups

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=btree-cleanups
---
Commits in this patchset:
 * xfs: fix C++ compilation errors in xfs_fs.h
 * xfs: fix FITRIM reporting again
 * xfs: fix a sloppy memory handling bug in xfs_iroot_realloc
 * xfs: replace shouty XFS_BM{BT,DR} macros
 * xfs: move the zero records logic into xfs_bmap_broot_space_calc
 * xfs: refactor the allocation and freeing of incore inode fork btree roots
 * xfs: refactor creation of bmap btree roots
 * xfs: hoist the code that moves the incore inode fork broot memory
 * xfs: rearrange xfs_iroot_realloc a bit
 * xfs: standardize the btree maxrecs function parameters
---
 fs/xfs/libxfs/xfs_alloc_btree.c    |    6 +
 fs/xfs/libxfs/xfs_alloc_btree.h    |    3 -
 fs/xfs/libxfs/xfs_attr_leaf.c      |    8 +
 fs/xfs/libxfs/xfs_bmap.c           |   62 +++++-----
 fs/xfs/libxfs/xfs_bmap_btree.c     |   37 ++++--
 fs/xfs/libxfs/xfs_bmap_btree.h     |  216 +++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_fs.h             |    4 -
 fs/xfs/libxfs/xfs_ialloc.c         |    4 -
 fs/xfs/libxfs/xfs_ialloc_btree.c   |    6 +
 fs/xfs/libxfs/xfs_ialloc_btree.h   |    3 -
 fs/xfs/libxfs/xfs_inode_fork.c     |  192 ++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_inode_fork.h     |    3 +
 fs/xfs/libxfs/xfs_refcount_btree.c |    5 +
 fs/xfs/libxfs/xfs_refcount_btree.h |    3 -
 fs/xfs/libxfs/xfs_rmap_btree.c     |    7 +
 fs/xfs/libxfs/xfs_rmap_btree.h     |    3 -
 fs/xfs/libxfs/xfs_sb.c             |   16 +--
 fs/xfs/libxfs/xfs_trans_resv.c     |    2 
 fs/xfs/scrub/bmap_repair.c         |    2 
 fs/xfs/scrub/inode_repair.c        |   12 +-
 fs/xfs/xfs_bmap_util.c             |    4 -
 fs/xfs/xfs_discard.c               |    2 
 22 files changed, 363 insertions(+), 237 deletions(-)

[PATCH 01/10] xfs: fix C++ compilation errors in xfs_fs.h

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Several people reported C++ compilation errors due to things that C
compilers allow but C++ compilers do not.  Fix both of these problems,
and hope there aren't more of these brown paper bags in 2 months when we
finally get these fixes through the process into a released xfsprogs.

Reported-by: kernel@mattwhitlock.name
Reported-by: sam@gentoo.org
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219203
Fixes: 233f4e12bbb2c ("xfs: add parent pointer ioctls")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_fs.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index c85c8077fac39..6a63634547ca9 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -930,13 +930,13 @@ static inline struct xfs_getparents_rec *
 xfs_getparents_next_rec(struct xfs_getparents *gp,
 			struct xfs_getparents_rec *gpr)
 {
-	void *next = ((void *)gpr + gpr->gpr_reclen);
+	void *next = ((char *)gpr + gpr->gpr_reclen);
 	void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
 
 	if (next >= end)
 		return NULL;
 
-	return next;
+	return (struct xfs_getparents_rec *)next;
 }
 
 /* Iterate through this file handle's directory parent pointers. */

Re: [PATCH 01/10] xfs: fix C++ compilation errors in xfs_fs.h

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 04:33:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Several people reported C++ compilation errors due to things that C
> compilers allow but C++ compilers do not.  Fix both of these problems,
> and hope there aren't more of these brown paper bags in 2 months when we
> finally get these fixes through the process into a released xfsprogs.

Meh.  I hate these stupid constrains C++ places on but which we need
to care for :(

Maybe also put a comment into xfs_fs.h that it needs to be C++-clean?

Otherwise looks good:

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

Re: [PATCH 01/10] xfs: fix C++ compilation errors in xfs_fs.h

[Dave Chinner]

On Tue, Aug 27, 2024 at 04:33:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Several people reported C++ compilation errors due to things that C
> compilers allow but C++ compilers do not.  Fix both of these problems,
> and hope there aren't more of these brown paper bags in 2 months when we
> finally get these fixes through the process into a released xfsprogs.
> 
> Reported-by: kernel@mattwhitlock.name
> Reported-by: sam@gentoo.org
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219203
> Fixes: 233f4e12bbb2c ("xfs: add parent pointer ioctls")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_fs.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index c85c8077fac39..6a63634547ca9 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -930,13 +930,13 @@ static inline struct xfs_getparents_rec *
>  xfs_getparents_next_rec(struct xfs_getparents *gp,
>  			struct xfs_getparents_rec *gpr)
>  {
> -	void *next = ((void *)gpr + gpr->gpr_reclen);
> +	void *next = ((char *)gpr + gpr->gpr_reclen);
>  	void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
>  
>  	if (next >= end)
>  		return NULL;
>  
> -	return next;
> +	return (struct xfs_getparents_rec *)next;
>  }

Please move this code completely out of the xfs_fs.h header. It is
not part of the kernel UAPI, and we have always tried to keep code
out of public header files like this because it tends to cause
unexpected build problems for users and 3rd party applications....

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

Re: [PATCH 01/10] xfs: fix C++ compilation errors in xfs_fs.h

[Carlos Maiolino]

On Tue, Aug 27, 2024 at 04:33:58PM GMT, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Several people reported C++ compilation errors due to things that C
> compilers allow but C++ compilers do not.  Fix both of these problems,
> and hope there aren't more of these brown paper bags in 2 months when we
> finally get these fixes through the process into a released xfsprogs.
> 
> Reported-by: kernel@mattwhitlock.name
> Reported-by: sam@gentoo.org
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219203
> Fixes: 233f4e12bbb2c ("xfs: add parent pointer ioctls")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_fs.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index c85c8077fac39..6a63634547ca9 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -930,13 +930,13 @@ static inline struct xfs_getparents_rec *
>  xfs_getparents_next_rec(struct xfs_getparents *gp,
>  			struct xfs_getparents_rec *gpr)
>  {
> -	void *next = ((void *)gpr + gpr->gpr_reclen);
> +	void *next = ((char *)gpr + gpr->gpr_reclen);
>  	void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
> 
>  	if (next >= end)
>  		return NULL;
> 
> -	return next;
> +	return (struct xfs_getparents_rec *)next;
>  }
> 
>  /* Iterate through this file handle's directory parent pointers. */

I'm taking this patch alone from this series, so we can fix 6.10 asap, we can
move it out of xfs_fs.h (which I agree with), and pull in the dummy code later.
Getting 6.10.1 out with this fix is priority by now.

[PATCH 02/10] xfs: fix FITRIM reporting again

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Don't report FITRIMming more bytes than possibly exist in the
filesystem.

Fixes: 410e8a18f8e93 ("xfs: don't bother reporting blocks trimmed via FITRIM")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_discard.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index bf1e3f330018d..d8c4a5dcca7ae 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -707,7 +707,7 @@ xfs_ioc_trim(
 		return last_error;
 
 	range.len = min_t(unsigned long long, range.len,
-			  XFS_FSB_TO_B(mp, max_blocks));
+			  XFS_FSB_TO_B(mp, max_blocks) - range.start);
 	if (copy_to_user(urange, &range, sizeof(range)))
 		return -EFAULT;
 	return 0;

Re: [PATCH 02/10] xfs: fix FITRIM reporting again

[Christoph Hellwig]

Looks good:

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

[PATCH 03/10] xfs: fix a sloppy memory handling bug in xfs_iroot_realloc

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While refactoring code, I noticed that when xfs_iroot_realloc tries to
shrink a bmbt root block, it allocates a smaller new block and then
copies "records" and pointers to the new block.  However, bmbt root
blocks cannot ever be leaves, which means that it's not technically
correct to copy records.  We /should/ be copying keys.

Note that this has never resulted in actual memory corruption because
sizeof(bmbt_rec) == (sizeof(bmbt_key) + sizeof(bmbt_ptr)).  However,
this will no longer be true when we start adding realtime rmap stuff,
so fix this now.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_fork.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 9d11ae0159091..6223823009049 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -463,15 +463,15 @@ xfs_iroot_realloc(
 	}
 
 	/*
-	 * Only copy the records and pointers if there are any.
+	 * Only copy the keys and pointers if there are any.
 	 */
 	if (new_max > 0) {
 		/*
-		 * First copy the records.
+		 * First copy the keys.
 		 */
-		op = (char *)XFS_BMBT_REC_ADDR(mp, ifp->if_broot, 1);
-		np = (char *)XFS_BMBT_REC_ADDR(mp, new_broot, 1);
-		memcpy(np, op, new_max * (uint)sizeof(xfs_bmbt_rec_t));
+		op = (char *)XFS_BMBT_KEY_ADDR(mp, ifp->if_broot, 1);
+		np = (char *)XFS_BMBT_KEY_ADDR(mp, new_broot, 1);
+		memcpy(np, op, new_max * (uint)sizeof(xfs_bmbt_key_t));
 
 		/*
 		 * Then copy the pointers.

Re: [PATCH 03/10] xfs: fix a sloppy memory handling bug in xfs_iroot_realloc

[Christoph Hellwig]

Looks good:

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

[PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Replace all the shouty bmap btree and bmap disk root macros with actual
functions, and fix a type handling error in the xattr code that the
macros previously didn't care about.

sed \
 -e 's/XFS_BMBT_BLOCK_LEN/xfs_bmbt_block_len/g' \
 -e 's/XFS_BMBT_REC_ADDR/xfs_bmbt_rec_addr/g' \
 -e 's/XFS_BMBT_KEY_ADDR/xfs_bmbt_key_addr/g' \
 -e 's/XFS_BMBT_PTR_ADDR/xfs_bmbt_ptr_addr/g' \
 -e 's/XFS_BMDR_REC_ADDR/xfs_bmdr_rec_addr/g' \
 -e 's/XFS_BMDR_KEY_ADDR/xfs_bmdr_key_addr/g' \
 -e 's/XFS_BMDR_PTR_ADDR/xfs_bmdr_ptr_addr/g' \
 -e 's/XFS_BMAP_BROOT_PTR_ADDR/xfs_bmap_broot_ptr_addr/g' \
 -e 's/XFS_BMAP_BROOT_SPACE_CALC/xfs_bmap_broot_space_calc/g' \
 -e 's/XFS_BMAP_BROOT_SPACE/xfs_bmap_broot_space/g' \
 -e 's/XFS_BMDR_SPACE_CALC/xfs_bmdr_space_calc/g' \
 -e 's/XFS_BMAP_BMDR_SPACE/xfs_bmap_bmdr_space/g' \
 -i $(git ls-files fs/xfs/*.[ch] fs/xfs/libxfs/*.[ch] fs/xfs/scrub/*.[ch])

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  |    8 +-
 fs/xfs/libxfs/xfs_bmap.c       |   40 ++++----
 fs/xfs/libxfs/xfs_bmap_btree.c |   18 ++--
 fs/xfs/libxfs/xfs_bmap_btree.h |  204 +++++++++++++++++++++++++++-------------
 fs/xfs/libxfs/xfs_inode_fork.c |   30 +++---
 fs/xfs/libxfs/xfs_trans_resv.c |    2 
 fs/xfs/scrub/bmap_repair.c     |    2 
 fs/xfs/scrub/inode_repair.c    |   12 +-
 fs/xfs/xfs_bmap_util.c         |    4 -
 9 files changed, 198 insertions(+), 122 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index b9e98950eb3d8..6aaec1246c950 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -686,7 +686,7 @@ xfs_attr_shortform_bytesfit(
 		 */
 		if (!dp->i_forkoff && dp->i_df.if_bytes >
 		    xfs_default_attroffset(dp))
-			dsize = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
+			dsize = xfs_bmdr_space_calc(MINDBTPTRS);
 		break;
 	case XFS_DINODE_FMT_BTREE:
 		/*
@@ -700,7 +700,7 @@ xfs_attr_shortform_bytesfit(
 				return 0;
 			return dp->i_forkoff;
 		}
-		dsize = XFS_BMAP_BROOT_SPACE(mp, dp->i_df.if_broot);
+		dsize = xfs_bmap_bmdr_space(dp->i_df.if_broot);
 		break;
 	}
 
@@ -708,11 +708,11 @@ xfs_attr_shortform_bytesfit(
 	 * A data fork btree root must have space for at least
 	 * MINDBTPTRS key/ptr pairs if the data fork is small or empty.
 	 */
-	minforkoff = max_t(int64_t, dsize, XFS_BMDR_SPACE_CALC(MINDBTPTRS));
+	minforkoff = max_t(int64_t, dsize, xfs_bmdr_space_calc(MINDBTPTRS));
 	minforkoff = roundup(minforkoff, 8) >> 3;
 
 	/* attr fork btree root can have at least this many key/ptr pairs */
-	maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
+	maxforkoff = XFS_LITINO(mp) - xfs_bmdr_space_calc(MINABTPTRS);
 	maxforkoff = maxforkoff >> 3;	/* rounded down */
 
 	if (offset >= maxforkoff)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 434433ed29dc2..00cac756c9566 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -79,9 +79,9 @@ xfs_bmap_compute_maxlevels(
 	maxleafents = xfs_iext_max_nextents(xfs_has_large_extent_counts(mp),
 				whichfork);
 	if (whichfork == XFS_DATA_FORK)
-		sz = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
+		sz = xfs_bmdr_space_calc(MINDBTPTRS);
 	else
-		sz = XFS_BMDR_SPACE_CALC(MINABTPTRS);
+		sz = xfs_bmdr_space_calc(MINABTPTRS);
 
 	maxrootrecs = xfs_bmdr_maxrecs(sz, 0);
 	minleafrecs = mp->m_bmap_dmnr[0];
@@ -102,8 +102,8 @@ xfs_bmap_compute_attr_offset(
 	struct xfs_mount	*mp)
 {
 	if (mp->m_sb.sb_inodesize == 256)
-		return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
-	return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
+		return XFS_LITINO(mp) - xfs_bmdr_space_calc(MINABTPTRS);
+	return xfs_bmdr_space_calc(6 * MINABTPTRS);
 }
 
 STATIC int				/* error */
@@ -298,7 +298,7 @@ xfs_check_block(
 	prevp = NULL;
 	for( i = 1; i <= xfs_btree_get_numrecs(block); i++) {
 		dmxr = mp->m_bmap_dmxr[0];
-		keyp = XFS_BMBT_KEY_ADDR(mp, block, i);
+		keyp = xfs_bmbt_key_addr(mp, block, i);
 
 		if (prevp) {
 			ASSERT(be64_to_cpu(prevp->br_startoff) <
@@ -310,15 +310,15 @@ xfs_check_block(
 		 * Compare the block numbers to see if there are dups.
 		 */
 		if (root)
-			pp = XFS_BMAP_BROOT_PTR_ADDR(mp, block, i, sz);
+			pp = xfs_bmap_broot_ptr_addr(mp, block, i, sz);
 		else
-			pp = XFS_BMBT_PTR_ADDR(mp, block, i, dmxr);
+			pp = xfs_bmbt_ptr_addr(mp, block, i, dmxr);
 
 		for (j = i+1; j <= be16_to_cpu(block->bb_numrecs); j++) {
 			if (root)
-				thispa = XFS_BMAP_BROOT_PTR_ADDR(mp, block, j, sz);
+				thispa = xfs_bmap_broot_ptr_addr(mp, block, j, sz);
 			else
-				thispa = XFS_BMBT_PTR_ADDR(mp, block, j, dmxr);
+				thispa = xfs_bmbt_ptr_addr(mp, block, j, dmxr);
 			if (*thispa == *pp) {
 				xfs_warn(mp, "%s: thispa(%d) == pp(%d) %lld",
 					__func__, j, i,
@@ -373,7 +373,7 @@ xfs_bmap_check_leaf_extents(
 	level = be16_to_cpu(block->bb_level);
 	ASSERT(level > 0);
 	xfs_check_block(block, mp, 1, ifp->if_broot_bytes);
-	pp = XFS_BMAP_BROOT_PTR_ADDR(mp, block, 1, ifp->if_broot_bytes);
+	pp = xfs_bmap_broot_ptr_addr(mp, block, 1, ifp->if_broot_bytes);
 	bno = be64_to_cpu(*pp);
 
 	ASSERT(bno != NULLFSBLOCK);
@@ -406,7 +406,7 @@ xfs_bmap_check_leaf_extents(
 		 */
 
 		xfs_check_block(block, mp, 0, 0);
-		pp = XFS_BMBT_PTR_ADDR(mp, block, 1, mp->m_bmap_dmxr[1]);
+		pp = xfs_bmbt_ptr_addr(mp, block, 1, mp->m_bmap_dmxr[1]);
 		bno = be64_to_cpu(*pp);
 		if (XFS_IS_CORRUPT(mp, !xfs_verify_fsbno(mp, bno))) {
 			xfs_btree_mark_sick(cur);
@@ -446,14 +446,14 @@ xfs_bmap_check_leaf_extents(
 		 * conform with the first entry in this one.
 		 */
 
-		ep = XFS_BMBT_REC_ADDR(mp, block, 1);
+		ep = xfs_bmbt_rec_addr(mp, block, 1);
 		if (i) {
 			ASSERT(xfs_bmbt_disk_get_startoff(&last) +
 			       xfs_bmbt_disk_get_blockcount(&last) <=
 			       xfs_bmbt_disk_get_startoff(ep));
 		}
 		for (j = 1; j < num_recs; j++) {
-			nextp = XFS_BMBT_REC_ADDR(mp, block, j + 1);
+			nextp = xfs_bmbt_rec_addr(mp, block, j + 1);
 			ASSERT(xfs_bmbt_disk_get_startoff(ep) +
 			       xfs_bmbt_disk_get_blockcount(ep) <=
 			       xfs_bmbt_disk_get_startoff(nextp));
@@ -586,7 +586,7 @@ xfs_bmap_btree_to_extents(
 	ASSERT(be16_to_cpu(rblock->bb_numrecs) == 1);
 	ASSERT(xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0) == 1);
 
-	pp = XFS_BMAP_BROOT_PTR_ADDR(mp, rblock, 1, ifp->if_broot_bytes);
+	pp = xfs_bmap_broot_ptr_addr(mp, rblock, 1, ifp->if_broot_bytes);
 	cbno = be64_to_cpu(*pp);
 #ifdef DEBUG
 	if (XFS_IS_CORRUPT(cur->bc_mp, !xfs_verify_fsbno(mp, cbno))) {
@@ -714,7 +714,7 @@ xfs_bmap_extents_to_btree(
 	for_each_xfs_iext(ifp, &icur, &rec) {
 		if (isnullstartblock(rec.br_startblock))
 			continue;
-		arp = XFS_BMBT_REC_ADDR(mp, ablock, 1 + cnt);
+		arp = xfs_bmbt_rec_addr(mp, ablock, 1 + cnt);
 		xfs_bmbt_disk_set_all(arp, &rec);
 		cnt++;
 	}
@@ -724,10 +724,10 @@ xfs_bmap_extents_to_btree(
 	/*
 	 * Fill in the root key and pointer.
 	 */
-	kp = XFS_BMBT_KEY_ADDR(mp, block, 1);
-	arp = XFS_BMBT_REC_ADDR(mp, ablock, 1);
+	kp = xfs_bmbt_key_addr(mp, block, 1);
+	arp = xfs_bmbt_rec_addr(mp, ablock, 1);
 	kp->br_startoff = cpu_to_be64(xfs_bmbt_disk_get_startoff(arp));
-	pp = XFS_BMBT_PTR_ADDR(mp, block, 1, xfs_bmbt_get_maxrecs(cur,
+	pp = xfs_bmbt_ptr_addr(mp, block, 1, xfs_bmbt_get_maxrecs(cur,
 						be16_to_cpu(block->bb_level)));
 	*pp = cpu_to_be64(args.fsbno);
 
@@ -896,7 +896,7 @@ xfs_bmap_add_attrfork_btree(
 
 	mp = ip->i_mount;
 
-	if (XFS_BMAP_BMDR_SPACE(block) <= xfs_inode_data_fork_size(ip))
+	if (xfs_bmap_bmdr_space(block) <= xfs_inode_data_fork_size(ip))
 		*flags |= XFS_ILOG_DBROOT;
 	else {
 		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
@@ -1160,7 +1160,7 @@ xfs_iread_bmbt_block(
 	}
 
 	/* Copy records into the incore cache. */
-	frp = XFS_BMBT_REC_ADDR(mp, block, 1);
+	frp = xfs_bmbt_rec_addr(mp, block, 1);
 	for (j = 0; j < num_recs; j++, frp++, ir->loaded++) {
 		struct xfs_bmbt_irec	new;
 		xfs_failaddr_t		fa;
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index d1b06ccde19ea..3695b3ad07d4d 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -65,10 +65,10 @@ xfs_bmdr_to_bmbt(
 	ASSERT(be16_to_cpu(rblock->bb_level) > 0);
 	rblock->bb_numrecs = dblock->bb_numrecs;
 	dmxr = xfs_bmdr_maxrecs(dblocklen, 0);
-	fkp = XFS_BMDR_KEY_ADDR(dblock, 1);
-	tkp = XFS_BMBT_KEY_ADDR(mp, rblock, 1);
-	fpp = XFS_BMDR_PTR_ADDR(dblock, 1, dmxr);
-	tpp = XFS_BMAP_BROOT_PTR_ADDR(mp, rblock, 1, rblocklen);
+	fkp = xfs_bmdr_key_addr(dblock, 1);
+	tkp = xfs_bmbt_key_addr(mp, rblock, 1);
+	fpp = xfs_bmdr_ptr_addr(dblock, 1, dmxr);
+	tpp = xfs_bmap_broot_ptr_addr(mp, rblock, 1, rblocklen);
 	dmxr = be16_to_cpu(dblock->bb_numrecs);
 	memcpy(tkp, fkp, sizeof(*fkp) * dmxr);
 	memcpy(tpp, fpp, sizeof(*fpp) * dmxr);
@@ -168,10 +168,10 @@ xfs_bmbt_to_bmdr(
 	dblock->bb_level = rblock->bb_level;
 	dblock->bb_numrecs = rblock->bb_numrecs;
 	dmxr = xfs_bmdr_maxrecs(dblocklen, 0);
-	fkp = XFS_BMBT_KEY_ADDR(mp, rblock, 1);
-	tkp = XFS_BMDR_KEY_ADDR(dblock, 1);
-	fpp = XFS_BMAP_BROOT_PTR_ADDR(mp, rblock, 1, rblocklen);
-	tpp = XFS_BMDR_PTR_ADDR(dblock, 1, dmxr);
+	fkp = xfs_bmbt_key_addr(mp, rblock, 1);
+	tkp = xfs_bmdr_key_addr(dblock, 1);
+	fpp = xfs_bmap_broot_ptr_addr(mp, rblock, 1, rblocklen);
+	tpp = xfs_bmdr_ptr_addr(dblock, 1, dmxr);
 	dmxr = be16_to_cpu(dblock->bb_numrecs);
 	memcpy(tkp, fkp, sizeof(*fkp) * dmxr);
 	memcpy(tpp, fpp, sizeof(*fpp) * dmxr);
@@ -651,7 +651,7 @@ xfs_bmbt_maxrecs(
 	int			blocklen,
 	int			leaf)
 {
-	blocklen -= XFS_BMBT_BLOCK_LEN(mp);
+	blocklen -= xfs_bmbt_block_len(mp);
 	return xfs_bmbt_block_maxrecs(blocklen, leaf);
 }
 
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
index de1b73f1225ca..af47174fca084 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.h
+++ b/fs/xfs/libxfs/xfs_bmap_btree.h
@@ -13,70 +13,6 @@ struct xfs_inode;
 struct xfs_trans;
 struct xbtree_ifakeroot;
 
-/*
- * Btree block header size depends on a superblock flag.
- */
-#define XFS_BMBT_BLOCK_LEN(mp) \
-	(xfs_has_crc(((mp))) ? \
-		XFS_BTREE_LBLOCK_CRC_LEN : XFS_BTREE_LBLOCK_LEN)
-
-#define XFS_BMBT_REC_ADDR(mp, block, index) \
-	((xfs_bmbt_rec_t *) \
-		((char *)(block) + \
-		 XFS_BMBT_BLOCK_LEN(mp) + \
-		 ((index) - 1) * sizeof(xfs_bmbt_rec_t)))
-
-#define XFS_BMBT_KEY_ADDR(mp, block, index) \
-	((xfs_bmbt_key_t *) \
-		((char *)(block) + \
-		 XFS_BMBT_BLOCK_LEN(mp) + \
-		 ((index) - 1) * sizeof(xfs_bmbt_key_t)))
-
-#define XFS_BMBT_PTR_ADDR(mp, block, index, maxrecs) \
-	((xfs_bmbt_ptr_t *) \
-		((char *)(block) + \
-		 XFS_BMBT_BLOCK_LEN(mp) + \
-		 (maxrecs) * sizeof(xfs_bmbt_key_t) + \
-		 ((index) - 1) * sizeof(xfs_bmbt_ptr_t)))
-
-#define XFS_BMDR_REC_ADDR(block, index) \
-	((xfs_bmdr_rec_t *) \
-		((char *)(block) + \
-		 sizeof(struct xfs_bmdr_block) + \
-	         ((index) - 1) * sizeof(xfs_bmdr_rec_t)))
-
-#define XFS_BMDR_KEY_ADDR(block, index) \
-	((xfs_bmdr_key_t *) \
-		((char *)(block) + \
-		 sizeof(struct xfs_bmdr_block) + \
-		 ((index) - 1) * sizeof(xfs_bmdr_key_t)))
-
-#define XFS_BMDR_PTR_ADDR(block, index, maxrecs) \
-	((xfs_bmdr_ptr_t *) \
-		((char *)(block) + \
-		 sizeof(struct xfs_bmdr_block) + \
-		 (maxrecs) * sizeof(xfs_bmdr_key_t) + \
-		 ((index) - 1) * sizeof(xfs_bmdr_ptr_t)))
-
-/*
- * These are to be used when we know the size of the block and
- * we don't have a cursor.
- */
-#define XFS_BMAP_BROOT_PTR_ADDR(mp, bb, i, sz) \
-	XFS_BMBT_PTR_ADDR(mp, bb, i, xfs_bmbt_maxrecs(mp, sz, 0))
-
-#define XFS_BMAP_BROOT_SPACE_CALC(mp, nrecs) \
-	(int)(XFS_BMBT_BLOCK_LEN(mp) + \
-	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
-
-#define XFS_BMAP_BROOT_SPACE(mp, bb) \
-	(XFS_BMAP_BROOT_SPACE_CALC(mp, be16_to_cpu((bb)->bb_numrecs)))
-#define XFS_BMDR_SPACE_CALC(nrecs) \
-	(int)(sizeof(xfs_bmdr_block_t) + \
-	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
-#define XFS_BMAP_BMDR_SPACE(bb) \
-	(XFS_BMDR_SPACE_CALC(be16_to_cpu((bb)->bb_numrecs)))
-
 /*
  * Maximum number of bmap btree levels.
  */
@@ -121,4 +57,144 @@ void xfs_bmbt_destroy_cur_cache(void);
 void xfs_bmbt_init_block(struct xfs_inode *ip, struct xfs_btree_block *buf,
 		struct xfs_buf *bp, __u16 level, __u16 numrecs);
 
+/*
+ * Btree block header size depends on a superblock flag.
+ */
+static inline size_t
+xfs_bmbt_block_len(struct xfs_mount *mp)
+{
+	return xfs_has_crc(mp) ?
+			XFS_BTREE_LBLOCK_CRC_LEN : XFS_BTREE_LBLOCK_LEN;
+}
+
+/* Addresses of key, pointers, and records within an incore bmbt block. */
+
+static inline struct xfs_bmbt_rec *
+xfs_bmbt_rec_addr(
+	struct xfs_mount	*mp,
+	struct xfs_btree_block	*block,
+	unsigned int		index)
+{
+	return (struct xfs_bmbt_rec *)
+		((char *)block + xfs_bmbt_block_len(mp) +
+		 (index - 1) * sizeof(struct xfs_bmbt_rec));
+}
+
+static inline struct xfs_bmbt_key *
+xfs_bmbt_key_addr(
+	struct xfs_mount	*mp,
+	struct xfs_btree_block	*block,
+	unsigned int		index)
+{
+	return (struct xfs_bmbt_key *)
+		((char *)block + xfs_bmbt_block_len(mp) +
+		 (index - 1) * sizeof(struct xfs_bmbt_key *));
+}
+
+static inline xfs_bmbt_ptr_t *
+xfs_bmbt_ptr_addr(
+	struct xfs_mount	*mp,
+	struct xfs_btree_block	*block,
+	unsigned int		index,
+	unsigned int		maxrecs)
+{
+	return (xfs_bmbt_ptr_t *)
+		((char *)block + xfs_bmbt_block_len(mp) +
+		 maxrecs * sizeof(struct xfs_bmbt_key) +
+		 (index - 1) * sizeof(xfs_bmbt_ptr_t));
+}
+
+/* Addresses of key, pointers, and records within an ondisk bmbt block. */
+
+static inline struct xfs_bmbt_rec *
+xfs_bmdr_rec_addr(
+	struct xfs_bmdr_block	*block,
+	unsigned int		index)
+{
+	return (struct xfs_bmbt_rec *)
+		((char *)(block + 1) +
+		 (index - 1) * sizeof(struct xfs_bmbt_rec));
+}
+
+static inline struct xfs_bmbt_key *
+xfs_bmdr_key_addr(
+	struct xfs_bmdr_block	*block,
+	unsigned int		index)
+{
+	return (struct xfs_bmbt_key *)
+		((char *)(block + 1) +
+		 (index - 1) * sizeof(struct xfs_bmbt_key));
+}
+
+static inline xfs_bmbt_ptr_t *
+xfs_bmdr_ptr_addr(
+	struct xfs_bmdr_block	*block,
+	unsigned int		index,
+	unsigned int		maxrecs)
+{
+	return (xfs_bmbt_ptr_t *)
+		((char *)(block + 1) +
+		 maxrecs * sizeof(struct xfs_bmbt_key) +
+		 (index - 1) * sizeof(xfs_bmbt_ptr_t));
+}
+
+/*
+ * Address of pointers within the incore btree root.
+ *
+ * These are to be used when we know the size of the block and
+ * we don't have a cursor.
+ */
+static inline xfs_bmbt_ptr_t *
+xfs_bmap_broot_ptr_addr(
+	struct xfs_mount	*mp,
+	struct xfs_btree_block	*bb,
+	unsigned int		i,
+	unsigned int		sz)
+{
+	return xfs_bmbt_ptr_addr(mp, bb, i, xfs_bmbt_maxrecs(mp, sz, 0));
+}
+
+/*
+ * Compute the space required for the incore btree root containing the given
+ * number of records.
+ */
+static inline size_t
+xfs_bmap_broot_space_calc(
+	struct xfs_mount	*mp,
+	unsigned int		nrecs)
+{
+	return xfs_bmbt_block_len(mp) + \
+	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
+}
+
+/*
+ * Compute the space required for the incore btree root given the ondisk
+ * btree root block.
+ */
+static inline size_t
+xfs_bmap_broot_space(
+	struct xfs_mount	*mp,
+	struct xfs_bmdr_block	*bb)
+{
+	return xfs_bmap_broot_space_calc(mp, be16_to_cpu(bb->bb_numrecs));
+}
+
+/* Compute the space required for the ondisk root block. */
+static inline size_t
+xfs_bmdr_space_calc(unsigned int nrecs)
+{
+	return sizeof(struct xfs_bmdr_block) +
+	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
+}
+
+/*
+ * Compute the space required for the ondisk root block given an incore root
+ * block.
+ */
+static inline size_t
+xfs_bmap_bmdr_space(struct xfs_btree_block *bb)
+{
+	return xfs_bmdr_space_calc(be16_to_cpu(bb->bb_numrecs));
+}
+
 #endif	/* __XFS_BMAP_BTREE_H__ */
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 6223823009049..973e027e3d883 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -185,7 +185,7 @@ xfs_iformat_btree(
 
 	ifp = xfs_ifork_ptr(ip, whichfork);
 	dfp = (xfs_bmdr_block_t *)XFS_DFORK_PTR(dip, whichfork);
-	size = XFS_BMAP_BROOT_SPACE(mp, dfp);
+	size = xfs_bmap_broot_space(mp, dfp);
 	nrecs = be16_to_cpu(dfp->bb_numrecs);
 	level = be16_to_cpu(dfp->bb_level);
 
@@ -198,7 +198,7 @@ xfs_iformat_btree(
 	 */
 	if (unlikely(ifp->if_nextents <= XFS_IFORK_MAXEXT(ip, whichfork) ||
 		     nrecs == 0 ||
-		     XFS_BMDR_SPACE_CALC(nrecs) >
+		     xfs_bmdr_space_calc(nrecs) >
 					XFS_DFORK_SIZE(dip, mp, whichfork) ||
 		     ifp->if_nextents > ip->i_nblocks) ||
 		     level == 0 || level > XFS_BM_MAXLEVELS(mp, whichfork)) {
@@ -409,7 +409,7 @@ xfs_iroot_realloc(
 		 * allocate it now and get out.
 		 */
 		if (ifp->if_broot_bytes == 0) {
-			new_size = XFS_BMAP_BROOT_SPACE_CALC(mp, rec_diff);
+			new_size = xfs_bmap_broot_space_calc(mp, rec_diff);
 			ifp->if_broot = kmalloc(new_size,
 						GFP_KERNEL | __GFP_NOFAIL);
 			ifp->if_broot_bytes = (int)new_size;
@@ -424,15 +424,15 @@ xfs_iroot_realloc(
 		 */
 		cur_max = xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0);
 		new_max = cur_max + rec_diff;
-		new_size = XFS_BMAP_BROOT_SPACE_CALC(mp, new_max);
+		new_size = xfs_bmap_broot_space_calc(mp, new_max);
 		ifp->if_broot = krealloc(ifp->if_broot, new_size,
 					 GFP_KERNEL | __GFP_NOFAIL);
-		op = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, ifp->if_broot, 1,
+		op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
 						     ifp->if_broot_bytes);
-		np = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, ifp->if_broot, 1,
+		np = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
 						     (int)new_size);
 		ifp->if_broot_bytes = (int)new_size;
-		ASSERT(XFS_BMAP_BMDR_SPACE(ifp->if_broot) <=
+		ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
 			xfs_inode_fork_size(ip, whichfork));
 		memmove(np, op, cur_max * (uint)sizeof(xfs_fsblock_t));
 		return;
@@ -448,7 +448,7 @@ xfs_iroot_realloc(
 	new_max = cur_max + rec_diff;
 	ASSERT(new_max >= 0);
 	if (new_max > 0)
-		new_size = XFS_BMAP_BROOT_SPACE_CALC(mp, new_max);
+		new_size = xfs_bmap_broot_space_calc(mp, new_max);
 	else
 		new_size = 0;
 	if (new_size > 0) {
@@ -457,7 +457,7 @@ xfs_iroot_realloc(
 		 * First copy over the btree block header.
 		 */
 		memcpy(new_broot, ifp->if_broot,
-			XFS_BMBT_BLOCK_LEN(ip->i_mount));
+			xfs_bmbt_block_len(ip->i_mount));
 	} else {
 		new_broot = NULL;
 	}
@@ -469,16 +469,16 @@ xfs_iroot_realloc(
 		/*
 		 * First copy the keys.
 		 */
-		op = (char *)XFS_BMBT_KEY_ADDR(mp, ifp->if_broot, 1);
-		np = (char *)XFS_BMBT_KEY_ADDR(mp, new_broot, 1);
+		op = (char *)xfs_bmbt_key_addr(mp, ifp->if_broot, 1);
+		np = (char *)xfs_bmbt_key_addr(mp, new_broot, 1);
 		memcpy(np, op, new_max * (uint)sizeof(xfs_bmbt_key_t));
 
 		/*
 		 * Then copy the pointers.
 		 */
-		op = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, ifp->if_broot, 1,
+		op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
 						     ifp->if_broot_bytes);
-		np = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, new_broot, 1,
+		np = (char *)xfs_bmap_broot_ptr_addr(mp, new_broot, 1,
 						     (int)new_size);
 		memcpy(np, op, new_max * (uint)sizeof(xfs_fsblock_t));
 	}
@@ -486,7 +486,7 @@ xfs_iroot_realloc(
 	ifp->if_broot = new_broot;
 	ifp->if_broot_bytes = (int)new_size;
 	if (ifp->if_broot)
-		ASSERT(XFS_BMAP_BMDR_SPACE(ifp->if_broot) <=
+		ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
 			xfs_inode_fork_size(ip, whichfork));
 	return;
 }
@@ -655,7 +655,7 @@ xfs_iflush_fork(
 		if ((iip->ili_fields & brootflag[whichfork]) &&
 		    (ifp->if_broot_bytes > 0)) {
 			ASSERT(ifp->if_broot != NULL);
-			ASSERT(XFS_BMAP_BMDR_SPACE(ifp->if_broot) <=
+			ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
 			        xfs_inode_fork_size(ip, whichfork));
 			xfs_bmbt_to_bmdr(mp, ifp->if_broot, ifp->if_broot_bytes,
 				(xfs_bmdr_block_t *)cp,
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 2e6d7bb3b5a2f..1a7f95bcf0695 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -130,7 +130,7 @@ xfs_calc_inode_res(
 		(4 * sizeof(struct xlog_op_header) +
 		 sizeof(struct xfs_inode_log_format) +
 		 mp->m_sb.sb_inodesize +
-		 2 * XFS_BMBT_BLOCK_LEN(mp));
+		 2 * xfs_bmbt_block_len(mp));
 }
 
 /*
diff --git a/fs/xfs/scrub/bmap_repair.c b/fs/xfs/scrub/bmap_repair.c
index 1e656fab5e41a..49dc38acc66bf 100644
--- a/fs/xfs/scrub/bmap_repair.c
+++ b/fs/xfs/scrub/bmap_repair.c
@@ -480,7 +480,7 @@ xrep_bmap_iroot_size(
 {
 	ASSERT(level > 0);
 
-	return XFS_BMAP_BROOT_SPACE_CALC(cur->bc_mp, nr_this_level);
+	return xfs_bmap_broot_space_calc(cur->bc_mp, nr_this_level);
 }
 
 /* Update the inode counters. */
diff --git a/fs/xfs/scrub/inode_repair.c b/fs/xfs/scrub/inode_repair.c
index daf9f1ee7c2cb..3e45b9b72312a 100644
--- a/fs/xfs/scrub/inode_repair.c
+++ b/fs/xfs/scrub/inode_repair.c
@@ -846,7 +846,7 @@ xrep_dinode_bad_bmbt_fork(
 	nrecs = be16_to_cpu(dfp->bb_numrecs);
 	level = be16_to_cpu(dfp->bb_level);
 
-	if (nrecs == 0 || XFS_BMDR_SPACE_CALC(nrecs) > dfork_size)
+	if (nrecs == 0 || xfs_bmdr_space_calc(nrecs) > dfork_size)
 		return true;
 	if (level == 0 || level >= XFS_BM_MAXLEVELS(sc->mp, whichfork))
 		return true;
@@ -858,12 +858,12 @@ xrep_dinode_bad_bmbt_fork(
 		xfs_fileoff_t		fileoff;
 		xfs_fsblock_t		fsbno;
 
-		fkp = XFS_BMDR_KEY_ADDR(dfp, i);
+		fkp = xfs_bmdr_key_addr(dfp, i);
 		fileoff = be64_to_cpu(fkp->br_startoff);
 		if (!xfs_verify_fileoff(sc->mp, fileoff))
 			return true;
 
-		fpp = XFS_BMDR_PTR_ADDR(dfp, i, dmxr);
+		fpp = xfs_bmdr_ptr_addr(dfp, i, dmxr);
 		fsbno = be64_to_cpu(*fpp);
 		if (!xfs_verify_fsbno(sc->mp, fsbno))
 			return true;
@@ -1121,7 +1121,7 @@ xrep_dinode_ensure_forkoff(
 	struct xfs_bmdr_block	*bmdr;
 	struct xfs_scrub	*sc = ri->sc;
 	xfs_extnum_t		attr_extents, data_extents;
-	size_t			bmdr_minsz = XFS_BMDR_SPACE_CALC(1);
+	size_t			bmdr_minsz = xfs_bmdr_space_calc(1);
 	unsigned int		lit_sz = XFS_LITINO(sc->mp);
 	unsigned int		afork_min, dfork_min;
 
@@ -1173,7 +1173,7 @@ xrep_dinode_ensure_forkoff(
 	case XFS_DINODE_FMT_BTREE:
 		/* Must have space for btree header and key/pointers. */
 		bmdr = XFS_DFORK_PTR(dip, XFS_ATTR_FORK);
-		afork_min = XFS_BMAP_BROOT_SPACE(sc->mp, bmdr);
+		afork_min = xfs_bmap_broot_space(sc->mp, bmdr);
 		break;
 	default:
 		/* We should never see any other formats. */
@@ -1223,7 +1223,7 @@ xrep_dinode_ensure_forkoff(
 	case XFS_DINODE_FMT_BTREE:
 		/* Must have space for btree header and key/pointers. */
 		bmdr = XFS_DFORK_PTR(dip, XFS_DATA_FORK);
-		dfork_min = XFS_BMAP_BROOT_SPACE(sc->mp, bmdr);
+		dfork_min = xfs_bmap_broot_space(sc->mp, bmdr);
 		break;
 	default:
 		dfork_min = 0;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fe2e2c9309755..a2c8f0dd85d03 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1184,7 +1184,7 @@ xfs_swap_extents_check_format(
 	 */
 	if (tifp->if_format == XFS_DINODE_FMT_BTREE) {
 		if (xfs_inode_has_attr_fork(ip) &&
-		    XFS_BMAP_BMDR_SPACE(tifp->if_broot) > xfs_inode_fork_boff(ip))
+		    xfs_bmap_bmdr_space(tifp->if_broot) > xfs_inode_fork_boff(ip))
 			return -EINVAL;
 		if (tifp->if_nextents <= XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))
 			return -EINVAL;
@@ -1193,7 +1193,7 @@ xfs_swap_extents_check_format(
 	/* Reciprocal target->temp btree format checks */
 	if (ifp->if_format == XFS_DINODE_FMT_BTREE) {
 		if (xfs_inode_has_attr_fork(tip) &&
-		    XFS_BMAP_BMDR_SPACE(ip->i_df.if_broot) > xfs_inode_fork_boff(tip))
+		    xfs_bmap_bmdr_space(ip->i_df.if_broot) > xfs_inode_fork_boff(tip))
 			return -EINVAL;
 		if (ifp->if_nextents <= XFS_IFORK_MAXEXT(tip, XFS_DATA_FORK))
 			return -EINVAL;

Re: [PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

[Dave Chinner]

On Tue, Aug 27, 2024 at 04:34:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Replace all the shouty bmap btree and bmap disk root macros with actual
> functions, and fix a type handling error in the xattr code that the
> macros previously didn't care about.

I don't see a type handling fix in the xattr code in the patch.

If there is one, can you please split it out to make it obvious?

....

> -#define XFS_BMDR_REC_ADDR(block, index) \
> -	((xfs_bmdr_rec_t *) \
> -		((char *)(block) + \
> -		 sizeof(struct xfs_bmdr_block) + \
> -	         ((index) - 1) * sizeof(xfs_bmdr_rec_t)))

> +
> +static inline struct xfs_bmbt_rec *
> +xfs_bmdr_rec_addr(
> +	struct xfs_bmdr_block	*block,
> +	unsigned int		index)
> +{
> +	return (struct xfs_bmbt_rec *)
> +		((char *)(block + 1) +
> +		 (index - 1) * sizeof(struct xfs_bmbt_rec));
> +}

There's a logic change in these BMDR conversions - why does the new
version use (block + 1) and the old one use a (block + sizeof())
calculation?

I *think* they are equivalent, but now as I read the code I have to
think about casts and pointer arithmetic and work out what structure
we are taking the size of in my head rather than it being straight
forward and obvious from the code. 

It doesn't change the code that is generated, so I think that the
existing "+ sizeof()" variants is better than this mechanism because
everyone is familiar with the existing definitions....


> +static inline struct xfs_bmbt_key *
> +xfs_bmdr_key_addr(
> +	struct xfs_bmdr_block	*block,
> +	unsigned int		index)
> +{
> +	return (struct xfs_bmbt_key *)
> +		((char *)(block + 1) +
> +		 (index - 1) * sizeof(struct xfs_bmbt_key));
> +}
> +
> +static inline xfs_bmbt_ptr_t *
> +xfs_bmdr_ptr_addr(
> +	struct xfs_bmdr_block	*block,
> +	unsigned int		index,
> +	unsigned int		maxrecs)
> +{
> +	return (xfs_bmbt_ptr_t *)
> +		((char *)(block + 1) +
> +		 maxrecs * sizeof(struct xfs_bmbt_key) +
> +		 (index - 1) * sizeof(xfs_bmbt_ptr_t));
> +}

Same for these.

> +/*
> + * Compute the space required for the incore btree root containing the given
> + * number of records.
> + */
> +static inline size_t
> +xfs_bmap_broot_space_calc(
> +	struct xfs_mount	*mp,
> +	unsigned int		nrecs)
> +{
> +	return xfs_bmbt_block_len(mp) + \
> +	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
> +}

stray '\' remains in that conversion.

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

Re: [PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

[Darrick J. Wong]

On Thu, Aug 29, 2024 at 12:00:43PM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 04:34:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Replace all the shouty bmap btree and bmap disk root macros with actual
> > functions, and fix a type handling error in the xattr code that the
> > macros previously didn't care about.
> 
> I don't see a type handling fix in the xattr code in the patch.
> 
> If there is one, can you please split it out to make it obvious?

No, I think that's just debris from an old iteration of this years-old
patch.  The xattr fixes got merged in the xattr state machine series
that Allison merged a long time ago.

> ....
> 
> > -#define XFS_BMDR_REC_ADDR(block, index) \
> > -	((xfs_bmdr_rec_t *) \
> > -		((char *)(block) + \
> > -		 sizeof(struct xfs_bmdr_block) + \
> > -	         ((index) - 1) * sizeof(xfs_bmdr_rec_t)))
> 
> > +
> > +static inline struct xfs_bmbt_rec *
> > +xfs_bmdr_rec_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index)
> > +{
> > +	return (struct xfs_bmbt_rec *)
> > +		((char *)(block + 1) +
> > +		 (index - 1) * sizeof(struct xfs_bmbt_rec));
> > +}
> 
> There's a logic change in these BMDR conversions - why does the new
> version use (block + 1) and the old one use a (block + sizeof())
> calculation?

Pointer arithmetic works great inside C functions where you know the
type of the operand.  That's why I used it.

That doesn't work at all in a macro where someone could pass in any
random pointer and then the math is wrong.  That's why the macro casts
to a byte-sized pointer type and then adds exactly the sizeof() bytes,
or at least I assume that's the motivation of the authors.

> I *think* they are equivalent, but now as I read the code I have to
> think about casts and pointer arithmetic and work out what structure
> we are taking the size of in my head rather than it being straight
> forward and obvious from the code.

The function argument declaration is four lines up.

> It doesn't change the code that is generated, so I think that the
> existing "+ sizeof()" variants is better than this mechanism because
> everyone is familiar with the existing definitions....

I disagree.  With your change, to validate this function, everyone must
to check that the argument type matches the sizeof argument to confirm
that the pointer arithmetic is correct.

static inline struct xfs_bmbt_rec *
xfs_bmdr_rec_addr(
	struct xfs_bmdr_block	*block,
	unsigned int		index)
{
	return (struct xfs_bmbt_rec *)
		((char *)(block) +
		 sizeof(struct xfs_btree_block) +
		 (index - 1) * sizeof(struct xfs_bmbt_rec));
}

Oops, this function is broken, when we could have trusted the compiler
to get the types and the math correct for us.

> > +static inline struct xfs_bmbt_key *
> > +xfs_bmdr_key_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index)
> > +{
> > +	return (struct xfs_bmbt_key *)
> > +		((char *)(block + 1) +
> > +		 (index - 1) * sizeof(struct xfs_bmbt_key));
> > +}
> > +
> > +static inline xfs_bmbt_ptr_t *
> > +xfs_bmdr_ptr_addr(
> > +	struct xfs_bmdr_block	*block,
> > +	unsigned int		index,
> > +	unsigned int		maxrecs)
> > +{
> > +	return (xfs_bmbt_ptr_t *)
> > +		((char *)(block + 1) +
> > +		 maxrecs * sizeof(struct xfs_bmbt_key) +
> > +		 (index - 1) * sizeof(xfs_bmbt_ptr_t));
> > +}
> 
> Same for these.
> 
> > +/*
> > + * Compute the space required for the incore btree root containing the given
> > + * number of records.
> > + */
> > +static inline size_t
> > +xfs_bmap_broot_space_calc(
> > +	struct xfs_mount	*mp,
> > +	unsigned int		nrecs)
> > +{
> > +	return xfs_bmbt_block_len(mp) + \
> > +	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
> > +}
> 
> stray '\' remains in that conversion.

Will fix, thanks.

--D

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

Re: [PATCH 04/10] xfs: replace shouty XFS_BM{BT,DR} macros

[Christoph Hellwig]

On Thu, Aug 29, 2024 at 03:10:29PM -0700, Darrick J. Wong wrote:
> I disagree.  With your change, to validate this function, everyone must
> to check that the argument type matches the sizeof argument to confirm
> that the pointer arithmetic is correct.

Agreed.

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

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The bmap btree cannot ever have zero key/pointer records in an incore
btree block.  If the number of records drops to zero, that means we're
converting the fork to extents format and are trying to remove the tree.
This logic won't hold for the future realtime rmap btree, so move the
logic into the bmbt code.

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.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap_btree.h |    7 +++++
 fs/xfs/libxfs/xfs_inode_fork.c |   56 ++++++++++++++--------------------------
 2 files changed, 27 insertions(+), 36 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
index af47174fca084..b7842c3420f04 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.h
+++ b/fs/xfs/libxfs/xfs_bmap_btree.h
@@ -163,6 +163,13 @@ xfs_bmap_broot_space_calc(
 	struct xfs_mount	*mp,
 	unsigned int		nrecs)
 {
+	/*
+	 * If the bmbt root block is empty, we should be converting the fork
+	 * to extents format.  Hence, the size is zero.
+	 */
+	if (nrecs == 0)
+		return 0;
+
 	return xfs_bmbt_block_len(mp) + \
 	       (nrecs * (sizeof(struct xfs_bmbt_key) + sizeof(xfs_bmbt_ptr_t)));
 }
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 973e027e3d883..acb1e9cc45b76 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -447,48 +447,32 @@ xfs_iroot_realloc(
 	cur_max = xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0);
 	new_max = cur_max + rec_diff;
 	ASSERT(new_max >= 0);
-	if (new_max > 0)
-		new_size = xfs_bmap_broot_space_calc(mp, new_max);
-	else
-		new_size = 0;
-	if (new_size > 0) {
-		new_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
-		/*
-		 * First copy over the btree block header.
-		 */
-		memcpy(new_broot, ifp->if_broot,
-			xfs_bmbt_block_len(ip->i_mount));
-	} else {
-		new_broot = NULL;
+
+	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;
 	}
 
-	/*
-	 * Only copy the keys and pointers if there are any.
-	 */
-	if (new_max > 0) {
-		/*
-		 * First copy the keys.
-		 */
-		op = (char *)xfs_bmbt_key_addr(mp, ifp->if_broot, 1);
-		np = (char *)xfs_bmbt_key_addr(mp, new_broot, 1);
-		memcpy(np, op, new_max * (uint)sizeof(xfs_bmbt_key_t));
+	new_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
+	memcpy(new_broot, ifp->if_broot, xfs_bmbt_block_len(ip->i_mount));
+
+	op = (char *)xfs_bmbt_key_addr(mp, ifp->if_broot, 1);
+	np = (char *)xfs_bmbt_key_addr(mp, new_broot, 1);
+	memcpy(np, op, new_max * sizeof(xfs_bmbt_key_t));
+
+	op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
+			ifp->if_broot_bytes);
+	np = (char *)xfs_bmap_broot_ptr_addr(mp, new_broot, 1, (int)new_size);
+	memcpy(np, op, new_max * sizeof(xfs_fsblock_t));
 
-		/*
-		 * Then copy the pointers.
-		 */
-		op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
-						     ifp->if_broot_bytes);
-		np = (char *)xfs_bmap_broot_ptr_addr(mp, new_broot, 1,
-						     (int)new_size);
-		memcpy(np, op, new_max * (uint)sizeof(xfs_fsblock_t));
-	}
 	kfree(ifp->if_broot);
 	ifp->if_broot = new_broot;
 	ifp->if_broot_bytes = (int)new_size;
-	if (ifp->if_broot)
-		ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
-			xfs_inode_fork_size(ip, whichfork));
-	return;
+	ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
+	       xfs_inode_fork_size(ip, whichfork));
 }
 
 

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

[Christoph Hellwig]

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:

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

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

[Darrick J. Wong]

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

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

[Christoph Hellwig]

On Wed, Aug 28, 2024 at 06:15:55PM -0700, Darrick J. Wong wrote:
> 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.

Sounds kinda nice, but also painful.  If the abstraction works out nice
enough it might be worth it.

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

[Dave Chinner]

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

-Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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
> 

[PATCH 06/10] xfs: refactor the allocation and freeing of incore inode fork btree roots

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Refactor the code that allocates and freese the incore inode fork btree
roots.  This will help us disentangle some of the weird logic when we're
creating and tearing down inode-based btrees.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_fork.c |   39 ++++++++++++++++++++++++++++++---------
 fs/xfs/libxfs/xfs_inode_fork.h |    3 +++
 2 files changed, 33 insertions(+), 9 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index acb1e9cc45b76..60646a6c32ec7 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -211,9 +211,7 @@ xfs_iformat_btree(
 		return -EFSCORRUPTED;
 	}
 
-	ifp->if_broot_bytes = size;
-	ifp->if_broot = kmalloc(size,
-				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
+	xfs_iroot_alloc(ip, whichfork, size);
 	ASSERT(ifp->if_broot != NULL);
 	/*
 	 * Copy and convert from the on-disk structure
@@ -362,6 +360,33 @@ xfs_iformat_attr_fork(
 	return error;
 }
 
+/* Allocate a new incore ifork btree root. */
+void
+xfs_iroot_alloc(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	size_t			bytes)
+{
+	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
+
+	ifp->if_broot = kmalloc(bytes,
+				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
+	ifp->if_broot_bytes = bytes;
+}
+
+/* Free all the memory and state associated with an incore ifork btree root. */
+void
+xfs_iroot_free(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
+
+	ifp->if_broot_bytes = 0;
+	kfree(ifp->if_broot);
+	ifp->if_broot = NULL;
+}
+
 /*
  * Reallocate the space for if_broot based on the number of records
  * being added or deleted as indicated in rec_diff.  Move the records
@@ -410,9 +435,7 @@ xfs_iroot_realloc(
 		 */
 		if (ifp->if_broot_bytes == 0) {
 			new_size = xfs_bmap_broot_space_calc(mp, rec_diff);
-			ifp->if_broot = kmalloc(new_size,
-						GFP_KERNEL | __GFP_NOFAIL);
-			ifp->if_broot_bytes = (int)new_size;
+			xfs_iroot_alloc(ip, whichfork, new_size);
 			return;
 		}
 
@@ -450,9 +473,7 @@ xfs_iroot_realloc(
 
 	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;
+		xfs_iroot_free(ip, whichfork);
 		return;
 	}
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 2373d12fd474f..3f228a00b67dd 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -170,6 +170,9 @@ void		xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
 void		xfs_idestroy_fork(struct xfs_ifork *ifp);
 void *		xfs_idata_realloc(struct xfs_inode *ip, int64_t byte_diff,
 				int whichfork);
+void		xfs_iroot_alloc(struct xfs_inode *ip, int whichfork,
+				size_t bytes);
+void		xfs_iroot_free(struct xfs_inode *ip, int whichfork);
 void		xfs_iroot_realloc(struct xfs_inode *, int, int);
 int		xfs_iread_extents(struct xfs_trans *, struct xfs_inode *, int);
 int		xfs_iextents_copy(struct xfs_inode *, struct xfs_bmbt_rec *,

Re: [PATCH 06/10] xfs: refactor the allocation and freeing of incore inode fork btree roots

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 06/10] xfs: refactor the allocation and freeing of incore inode fork btree roots

[Christoph Hellwig]

> +/* Allocate a new incore ifork btree root. */
> +void
> +xfs_iroot_alloc(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	size_t			bytes)
> +{
> +	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
> +
> +	ifp->if_broot = kmalloc(bytes,
> +				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
> +	ifp->if_broot_bytes = bytes;
> +}

.. actually.  Maybe this shuld return ifp->if_broot?  I guess that
would helpful in a few callers to directly assign that to the variable
for the root block.  Similar to what I did with xfs_idata_realloc a
while ago.

Re: [PATCH 06/10] xfs: refactor the allocation and freeing of incore inode fork btree roots

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 06:17:12AM +0200, Christoph Hellwig wrote:
> > +/* Allocate a new incore ifork btree root. */
> > +void
> > +xfs_iroot_alloc(
> > +	struct xfs_inode	*ip,
> > +	int			whichfork,
> > +	size_t			bytes)
> > +{
> > +	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
> > +
> > +	ifp->if_broot = kmalloc(bytes,
> > +				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
> > +	ifp->if_broot_bytes = bytes;
> > +}
> 
> .. actually.  Maybe this shuld return ifp->if_broot?  I guess that
> would helpful in a few callers to directly assign that to the variable
> for the root block.  Similar to what I did with xfs_idata_realloc a
> while ago.

Yeah, that would be useful later on.  I'll change that now.

--D

[PATCH 07/10] xfs: refactor creation of bmap btree roots

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Now that we've created inode fork helpers to allocate and free btree
roots, create a new bmap btree helper to create a new bmbt root, and
refactor the extents <-> btree conversion functions to use our new
helpers.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_bmap.c       |   20 ++++++--------------
 fs/xfs/libxfs/xfs_bmap_btree.c |   13 +++++++++++++
 fs/xfs/libxfs/xfs_bmap_btree.h |    2 ++
 3 files changed, 21 insertions(+), 14 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 00cac756c9566..e3922cf75381c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -614,7 +614,7 @@ xfs_bmap_btree_to_extents(
 	xfs_trans_binval(tp, cbp);
 	if (cur->bc_levels[0].bp == cbp)
 		cur->bc_levels[0].bp = NULL;
-	xfs_iroot_realloc(ip, -1, whichfork);
+	xfs_iroot_free(ip, whichfork);
 	ASSERT(ifp->if_broot == NULL);
 	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
 	*logflagsp |= XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
@@ -655,19 +655,10 @@ xfs_bmap_extents_to_btree(
 	ASSERT(ifp->if_format == XFS_DINODE_FMT_EXTENTS);
 
 	/*
-	 * Make space in the inode incore. This needs to be undone if we fail
-	 * to expand the root.
-	 */
-	xfs_iroot_realloc(ip, 1, whichfork);
-
-	/*
-	 * Fill in the root.
-	 */
-	block = ifp->if_broot;
-	xfs_bmbt_init_block(ip, block, NULL, 1, 1);
-	/*
-	 * Need a cursor.  Can't allocate until bb_level is filled in.
+	 * Fill in the root, create a cursor.  Can't allocate until bb_level is
+	 * filled in.
 	 */
+	xfs_bmbt_iroot_alloc(ip, whichfork);
 	cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork);
 	if (wasdel)
 		cur->bc_flags |= XFS_BTREE_BMBT_WASDEL;
@@ -724,6 +715,7 @@ xfs_bmap_extents_to_btree(
 	/*
 	 * Fill in the root key and pointer.
 	 */
+	block = ifp->if_broot;
 	kp = xfs_bmbt_key_addr(mp, block, 1);
 	arp = xfs_bmbt_rec_addr(mp, ablock, 1);
 	kp->br_startoff = cpu_to_be64(xfs_bmbt_disk_get_startoff(arp));
@@ -745,7 +737,7 @@ xfs_bmap_extents_to_btree(
 out_unreserve_dquot:
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 out_root_realloc:
-	xfs_iroot_realloc(ip, -1, whichfork);
+	xfs_iroot_free(ip, whichfork);
 	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
 	ASSERT(ifp->if_broot == NULL);
 	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index 3695b3ad07d4d..0769644d30412 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -759,3 +759,16 @@ xfs_bmbt_destroy_cur_cache(void)
 	kmem_cache_destroy(xfs_bmbt_cur_cache);
 	xfs_bmbt_cur_cache = NULL;
 }
+
+/* Create an incore bmbt btree root block. */
+void
+xfs_bmbt_iroot_alloc(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
+
+	xfs_iroot_alloc(ip, whichfork, xfs_bmap_broot_space_calc(mp, 1));
+	xfs_bmbt_init_block(ip, ifp->if_broot, NULL, 1, 1);
+}
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
index b7842c3420f04..a187f4b120ea1 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.h
+++ b/fs/xfs/libxfs/xfs_bmap_btree.h
@@ -204,4 +204,6 @@ xfs_bmap_bmdr_space(struct xfs_btree_block *bb)
 	return xfs_bmdr_space_calc(be16_to_cpu(bb->bb_numrecs));
 }
 
+void xfs_bmbt_iroot_alloc(struct xfs_inode *ip, int whichfork);
+
 #endif	/* __XFS_BMAP_BTREE_H__ */

Re: [PATCH 07/10] xfs: refactor creation of bmap btree roots

[Christoph Hellwig]

Loks good:

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

If xfs_bmbt_iroot_alloc return the if_rot value,
xfs_bmap_extents_to_btree could make use of that, though.

Re: [PATCH 07/10] xfs: refactor creation of bmap btree roots

[Dave Chinner]

On Tue, Aug 27, 2024 at 04:35:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that we've created inode fork helpers to allocate and free btree
> roots, create a new bmap btree helper to create a new bmbt root, and
> refactor the extents <-> btree conversion functions to use our new
> helpers.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_bmap.c       |   20 ++++++--------------
>  fs/xfs/libxfs/xfs_bmap_btree.c |   13 +++++++++++++
>  fs/xfs/libxfs/xfs_bmap_btree.h |    2 ++
>  3 files changed, 21 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 00cac756c9566..e3922cf75381c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -614,7 +614,7 @@ xfs_bmap_btree_to_extents(
>  	xfs_trans_binval(tp, cbp);
>  	if (cur->bc_levels[0].bp == cbp)
>  		cur->bc_levels[0].bp = NULL;
> -	xfs_iroot_realloc(ip, -1, whichfork);
> +	xfs_iroot_free(ip, whichfork);

I feel like the "whichfork" interface is unnecessary here. We
already have the ifp in all cases here, and so

	xfs_iroot_free(ifp);

avoids the need to look up the ifp again in xfs_iroot_free().

The same happens with xfs_iroot_alloc() - the callers already have
the ifp in a local variable, so...

>  	ASSERT(ifp->if_broot == NULL);
>  	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
>  	*logflagsp |= XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
> @@ -655,19 +655,10 @@ xfs_bmap_extents_to_btree(
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_EXTENTS);
>  
>  	/*
> -	 * Make space in the inode incore. This needs to be undone if we fail
> -	 * to expand the root.
> -	 */
> -	xfs_iroot_realloc(ip, 1, whichfork);
> -
> -	/*
> -	 * Fill in the root.
> -	 */
> -	block = ifp->if_broot;
> -	xfs_bmbt_init_block(ip, block, NULL, 1, 1);
> -	/*
> -	 * Need a cursor.  Can't allocate until bb_level is filled in.
> +	 * Fill in the root, create a cursor.  Can't allocate until bb_level is
> +	 * filled in.
>  	 */
> +	xfs_bmbt_iroot_alloc(ip, whichfork);

.... this becomes xfs_bmbt_iroot_alloc(ip, ifp);

i.e. once we already have an ifp resolved for the fork, it makes no
sense to pass whichfork down the stack instead of the ifp...

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

Re: [PATCH 07/10] xfs: refactor creation of bmap btree roots

[Darrick J. Wong]

On Thu, Aug 29, 2024 at 12:13:09PM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 04:35:33PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Now that we've created inode fork helpers to allocate and free btree
> > roots, create a new bmap btree helper to create a new bmbt root, and
> > refactor the extents <-> btree conversion functions to use our new
> > helpers.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c       |   20 ++++++--------------
> >  fs/xfs/libxfs/xfs_bmap_btree.c |   13 +++++++++++++
> >  fs/xfs/libxfs/xfs_bmap_btree.h |    2 ++
> >  3 files changed, 21 insertions(+), 14 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 00cac756c9566..e3922cf75381c 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -614,7 +614,7 @@ xfs_bmap_btree_to_extents(
> >  	xfs_trans_binval(tp, cbp);
> >  	if (cur->bc_levels[0].bp == cbp)
> >  		cur->bc_levels[0].bp = NULL;
> > -	xfs_iroot_realloc(ip, -1, whichfork);
> > +	xfs_iroot_free(ip, whichfork);
> 
> I feel like the "whichfork" interface is unnecessary here. We
> already have the ifp in all cases here, and so
> 
> 	xfs_iroot_free(ifp);
> 
> avoids the need to look up the ifp again in xfs_iroot_free().

Yeah, that makes sense.

> The same happens with xfs_iroot_alloc() - the callers already have
> the ifp in a local variable, so...
> 
> >  	ASSERT(ifp->if_broot == NULL);
> >  	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
> >  	*logflagsp |= XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
> > @@ -655,19 +655,10 @@ xfs_bmap_extents_to_btree(
> >  	ASSERT(ifp->if_format == XFS_DINODE_FMT_EXTENTS);
> >  
> >  	/*
> > -	 * Make space in the inode incore. This needs to be undone if we fail
> > -	 * to expand the root.
> > -	 */
> > -	xfs_iroot_realloc(ip, 1, whichfork);
> > -
> > -	/*
> > -	 * Fill in the root.
> > -	 */
> > -	block = ifp->if_broot;
> > -	xfs_bmbt_init_block(ip, block, NULL, 1, 1);
> > -	/*
> > -	 * Need a cursor.  Can't allocate until bb_level is filled in.
> > +	 * Fill in the root, create a cursor.  Can't allocate until bb_level is
> > +	 * filled in.
> >  	 */
> > +	xfs_bmbt_iroot_alloc(ip, whichfork);
> 
> .... this becomes xfs_bmbt_iroot_alloc(ip, ifp);
> 
> i.e. once we already have an ifp resolved for the fork, it makes no
> sense to pass whichfork down the stack instead of the ifp...

Makes sense here too, will change both.

--D

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

[PATCH 08/10] xfs: hoist the code that moves the incore inode fork broot memory

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Whenever we change the size of the memory buffer holding an inode fork
btree root block, we have to copy the contents over.  Refactor all this
into a single function that handles both, in preparation for making
xfs_iroot_realloc more generic.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_fork.c |   87 ++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 60646a6c32ec7..307207473abdb 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -387,6 +387,50 @@ xfs_iroot_free(
 	ifp->if_broot = NULL;
 }
 
+/* Move the bmap btree root from one incore buffer to another. */
+static void
+xfs_ifork_move_broot(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	struct xfs_btree_block	*dst_broot,
+	size_t			dst_bytes,
+	struct xfs_btree_block	*src_broot,
+	size_t			src_bytes,
+	unsigned int		numrecs)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	void			*dptr;
+	void			*sptr;
+
+	ASSERT(xfs_bmap_bmdr_space(src_broot) <= xfs_inode_fork_size(ip, whichfork));
+
+	/*
+	 * We always have to move the pointers because they are not butted
+	 * against the btree block header.
+	 */
+	if (numrecs) {
+		sptr = xfs_bmap_broot_ptr_addr(mp, src_broot, 1, src_bytes);
+		dptr = xfs_bmap_broot_ptr_addr(mp, dst_broot, 1, dst_bytes);
+		memmove(dptr, sptr, numrecs * sizeof(xfs_fsblock_t));
+	}
+
+	if (src_broot == dst_broot)
+		return;
+
+	/*
+	 * If the root is being totally relocated, we have to migrate the block
+	 * header and the keys that come after it.
+	 */
+	memcpy(dst_broot, src_broot, xfs_bmbt_block_len(mp));
+
+	/* Now copy the keys, which come right after the header. */
+	if (numrecs) {
+		sptr = xfs_bmbt_key_addr(mp, src_broot, 1);
+		dptr = xfs_bmbt_key_addr(mp, dst_broot, 1);
+		memcpy(dptr, sptr, numrecs * sizeof(struct xfs_bmbt_key));
+	}
+}
+
 /*
  * Reallocate the space for if_broot based on the number of records
  * being added or deleted as indicated in rec_diff.  Move the records
@@ -413,12 +457,11 @@ xfs_iroot_realloc(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	int			cur_max;
-	struct xfs_ifork	*ifp;
+	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
 	struct xfs_btree_block	*new_broot;
 	int			new_max;
 	size_t			new_size;
-	char			*np;
-	char			*op;
+	size_t			old_size = ifp->if_broot_bytes;
 
 	/*
 	 * Handle the degenerate case quietly.
@@ -427,13 +470,12 @@ xfs_iroot_realloc(
 		return;
 	}
 
-	ifp = xfs_ifork_ptr(ip, whichfork);
 	if (rec_diff > 0) {
 		/*
 		 * If there wasn't any memory allocated before, just
 		 * allocate it now and get out.
 		 */
-		if (ifp->if_broot_bytes == 0) {
+		if (old_size == 0) {
 			new_size = xfs_bmap_broot_space_calc(mp, rec_diff);
 			xfs_iroot_alloc(ip, whichfork, new_size);
 			return;
@@ -442,22 +484,16 @@ xfs_iroot_realloc(
 		/*
 		 * If there is already an existing if_broot, then we need
 		 * to realloc() it and shift the pointers to their new
-		 * location.  The records don't change location because
-		 * they are kept butted up against the btree block header.
+		 * location.
 		 */
-		cur_max = xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0);
+		cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
 		new_max = cur_max + rec_diff;
 		new_size = xfs_bmap_broot_space_calc(mp, new_max);
 		ifp->if_broot = krealloc(ifp->if_broot, new_size,
 					 GFP_KERNEL | __GFP_NOFAIL);
-		op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
-						     ifp->if_broot_bytes);
-		np = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
-						     (int)new_size);
-		ifp->if_broot_bytes = (int)new_size;
-		ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
-			xfs_inode_fork_size(ip, whichfork));
-		memmove(np, op, cur_max * (uint)sizeof(xfs_fsblock_t));
+		ifp->if_broot_bytes = new_size;
+		xfs_ifork_move_broot(ip, whichfork, ifp->if_broot, new_size,
+				ifp->if_broot, old_size, cur_max);
 		return;
 	}
 
@@ -466,8 +502,8 @@ xfs_iroot_realloc(
 	 * if_broot buffer.  It must already exist.  If we go to zero
 	 * records, just get rid of the root and clear the status bit.
 	 */
-	ASSERT((ifp->if_broot != NULL) && (ifp->if_broot_bytes > 0));
-	cur_max = xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0);
+	ASSERT((ifp->if_broot != NULL) && (old_size > 0));
+	cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
 	new_max = cur_max + rec_diff;
 	ASSERT(new_max >= 0);
 
@@ -478,22 +514,11 @@ xfs_iroot_realloc(
 	}
 
 	new_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
-	memcpy(new_broot, ifp->if_broot, xfs_bmbt_block_len(ip->i_mount));
-
-	op = (char *)xfs_bmbt_key_addr(mp, ifp->if_broot, 1);
-	np = (char *)xfs_bmbt_key_addr(mp, new_broot, 1);
-	memcpy(np, op, new_max * sizeof(xfs_bmbt_key_t));
-
-	op = (char *)xfs_bmap_broot_ptr_addr(mp, ifp->if_broot, 1,
-			ifp->if_broot_bytes);
-	np = (char *)xfs_bmap_broot_ptr_addr(mp, new_broot, 1, (int)new_size);
-	memcpy(np, op, new_max * sizeof(xfs_fsblock_t));
-
+	xfs_ifork_move_broot(ip, whichfork, new_broot, new_size, ifp->if_broot,
+			old_size, new_max);
 	kfree(ifp->if_broot);
 	ifp->if_broot = new_broot;
 	ifp->if_broot_bytes = (int)new_size;
-	ASSERT(xfs_bmap_bmdr_space(ifp->if_broot) <=
-	       xfs_inode_fork_size(ip, whichfork));
 }
 
 

Re: [PATCH 08/10] xfs: hoist the code that moves the incore inode fork broot memory

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 04:35:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Whenever we change the size of the memory buffer holding an inode fork
> btree root block, we have to copy the contents over.  Refactor all this
> into a single function that handles both, in preparation for making
> xfs_iroot_realloc more generic.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c |   87 ++++++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 60646a6c32ec7..307207473abdb 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -387,6 +387,50 @@ xfs_iroot_free(
>  	ifp->if_broot = NULL;
>  }
>  
> +/* Move the bmap btree root from one incore buffer to another. */
> +static void
> +xfs_ifork_move_broot(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct xfs_btree_block	*dst_broot,
> +	size_t			dst_bytes,
> +	struct xfs_btree_block	*src_broot,
> +	size_t			src_bytes,
> +	unsigned int		numrecs)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	void			*dptr;
> +	void			*sptr;
> +
> +	ASSERT(xfs_bmap_bmdr_space(src_broot) <= xfs_inode_fork_size(ip, whichfork));

Overly long line.

Otherwise looks good:

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

Re: [PATCH 08/10] xfs: hoist the code that moves the incore inode fork broot memory

[Dave Chinner]

On Tue, Aug 27, 2024 at 04:35:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Whenever we change the size of the memory buffer holding an inode fork
> btree root block, we have to copy the contents over.  Refactor all this
> into a single function that handles both, in preparation for making
> xfs_iroot_realloc more generic.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.c |   87 ++++++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 60646a6c32ec7..307207473abdb 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -387,6 +387,50 @@ xfs_iroot_free(
>  	ifp->if_broot = NULL;
>  }
>  
> +/* Move the bmap btree root from one incore buffer to another. */
> +static void
> +xfs_ifork_move_broot(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct xfs_btree_block	*dst_broot,
> +	size_t			dst_bytes,
> +	struct xfs_btree_block	*src_broot,
> +	size_t			src_bytes,
> +	unsigned int		numrecs)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	void			*dptr;
> +	void			*sptr;
> +
> +	ASSERT(xfs_bmap_bmdr_space(src_broot) <= xfs_inode_fork_size(ip, whichfork));

We pass whichfork just for this debug check. Can you pull this up
to the callers?

> +
> +	/*
> +	 * We always have to move the pointers because they are not butted
> +	 * against the btree block header.
> +	 */
> +	if (numrecs) {
> +		sptr = xfs_bmap_broot_ptr_addr(mp, src_broot, 1, src_bytes);
> +		dptr = xfs_bmap_broot_ptr_addr(mp, dst_broot, 1, dst_bytes);
> +		memmove(dptr, sptr, numrecs * sizeof(xfs_fsblock_t));
> +	}
> +
> +	if (src_broot == dst_broot)
> +		return;

Urk. So this is encoding caller logic directly into this function.
ie. the grow cases uses krealloc() which copies the keys and
pointers but still needs the pointers moved. The buffer is large
enough for that, so it passes src and dst as the same buffer and
this code then jumps out after copying the ptrs (a second time) to
their final resting place.

> +	/*
> +	 * If the root is being totally relocated, we have to migrate the block
> +	 * header and the keys that come after it.
> +	 */
> +	memcpy(dst_broot, src_broot, xfs_bmbt_block_len(mp));
> +
> +	/* Now copy the keys, which come right after the header. */
> +	if (numrecs) {
> +		sptr = xfs_bmbt_key_addr(mp, src_broot, 1);
> +		dptr = xfs_bmbt_key_addr(mp, dst_broot, 1);
> +		memcpy(dptr, sptr, numrecs * sizeof(struct xfs_bmbt_key));
> +	}

And here we do the key copy for the shrink case where we technically
don't need separate buffers but we really want to minimise memory
usage if we can so we reallocate a smaller buffer and free the
original larger one.

Given this, I think this code is more natural by doing all the
allocate/free/copy ourselves instead of using krealloc() and it's
implicit copy for one of the cases.

i.e. rename this function xfs_ifork_realloc_broot() and make it do
this:

{
	struct xfs_btree_block *src = ifp->if_broot;
	struct xfs_btree_block *dst = NULL;

	if (!numrecs)
		goto out_free_src;

	dst = kmalloc(new_size);

	/* copy block header */
	memcpy(dst, src, xfs_bmbt_block_len(mp));

	/* copy records */
	sptr = xfs_bmbt_key_addr(mp, src, 1);
	dptr = xfs_bmbt_key_addr(mp, dst, 1);
	memcpy(dptr, sptr, numrecs * sizeof(struct xfs_bmbt_key));

	/* copy pointers */
	sptr = xfs_bmap_broot_ptr_addr(mp, src_broot, 1, src_bytes);
	dptr = xfs_bmap_broot_ptr_addr(mp, dst_broot, 1, dst_bytes);
	memmove(dptr, sptr, numrecs * sizeof(xfs_fsblock_t));

out_free_src:
	kfree(src);
	ifp->if_broot = dst;
	ifp->if_broot_bytes = new_size;
}

And the callers are now both:

	xfs_ifork_realloc_broot(mp, ifp, new_size, old_size, numrecs);

This also naturally handles the "reduce to zero size" without
needing any special case code, it avoids the double pointer copy on
grow, and the operation logic is simple, obvious and easy to
understand...

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

Re: [PATCH 08/10] xfs: hoist the code that moves the incore inode fork broot memory

[Darrick J. Wong]

On Thu, Aug 29, 2024 at 12:54:32PM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 04:35:48PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Whenever we change the size of the memory buffer holding an inode fork
> > btree root block, we have to copy the contents over.  Refactor all this
> > into a single function that handles both, in preparation for making
> > xfs_iroot_realloc more generic.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.c |   87 ++++++++++++++++++++++++++--------------
> >  1 file changed, 56 insertions(+), 31 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 60646a6c32ec7..307207473abdb 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -387,6 +387,50 @@ xfs_iroot_free(
> >  	ifp->if_broot = NULL;
> >  }
> >  
> > +/* Move the bmap btree root from one incore buffer to another. */
> > +static void
> > +xfs_ifork_move_broot(
> > +	struct xfs_inode	*ip,
> > +	int			whichfork,
> > +	struct xfs_btree_block	*dst_broot,
> > +	size_t			dst_bytes,
> > +	struct xfs_btree_block	*src_broot,
> > +	size_t			src_bytes,
> > +	unsigned int		numrecs)
> > +{
> > +	struct xfs_mount	*mp = ip->i_mount;
> > +	void			*dptr;
> > +	void			*sptr;
> > +
> > +	ASSERT(xfs_bmap_bmdr_space(src_broot) <= xfs_inode_fork_size(ip, whichfork));
> 
> We pass whichfork just for this debug check. Can you pull this up
> to the callers?

I guess I could do that, but the rtrmap patchset adds its own broot
shrink/grow function specific to rtrmap btree inodes:

static void
xfs_rtrmapbt_broot_move(...)
{
	...
	ASSERT(xfs_rtrmap_droot_space(src_broot) <=
	       xfs_inode_fork_size(ip, whichfork));

so I didn't want to add yet another indirect call just for an assertion.

> > +
> > +	/*
> > +	 * We always have to move the pointers because they are not butted
> > +	 * against the btree block header.
> > +	 */
> > +	if (numrecs) {
> > +		sptr = xfs_bmap_broot_ptr_addr(mp, src_broot, 1, src_bytes);
> > +		dptr = xfs_bmap_broot_ptr_addr(mp, dst_broot, 1, dst_bytes);
> > +		memmove(dptr, sptr, numrecs * sizeof(xfs_fsblock_t));
> > +	}
> > +
> > +	if (src_broot == dst_broot)
> > +		return;
> 
> Urk. So this is encoding caller logic directly into this function.
> ie. the grow cases uses krealloc() which copies the keys and
> pointers but still needs the pointers moved. The buffer is large
> enough for that, so it passes src and dst as the same buffer and
> this code then jumps out after copying the ptrs (a second time) to
> their final resting place.

<nod>

> > +	/*
> > +	 * If the root is being totally relocated, we have to migrate the block
> > +	 * header and the keys that come after it.
> > +	 */
> > +	memcpy(dst_broot, src_broot, xfs_bmbt_block_len(mp));
> > +
> > +	/* Now copy the keys, which come right after the header. */
> > +	if (numrecs) {
> > +		sptr = xfs_bmbt_key_addr(mp, src_broot, 1);
> > +		dptr = xfs_bmbt_key_addr(mp, dst_broot, 1);
> > +		memcpy(dptr, sptr, numrecs * sizeof(struct xfs_bmbt_key));
> > +	}
> 
> And here we do the key copy for the shrink case where we technically
> don't need separate buffers but we really want to minimise memory
> usage if we can so we reallocate a smaller buffer and free the
> original larger one.
> 
> Given this, I think this code is more natural by doing all the
> allocate/free/copy ourselves instead of using krealloc() and it's
> implicit copy for one of the cases.
> 
> i.e. rename this function xfs_ifork_realloc_broot() and make it do
> this:
> 
> {
> 	struct xfs_btree_block *src = ifp->if_broot;
> 	struct xfs_btree_block *dst = NULL;
> 
> 	if (!numrecs)
> 		goto out_free_src;
> 
> 	dst = kmalloc(new_size);
> 
> 	/* copy block header */
> 	memcpy(dst, src, xfs_bmbt_block_len(mp));

I'm not sure I like replacing krealloc with kmalloc here.  For a grow
operation, if the new and old object sizes are close enough that we
reuse the existing slab object, then we only have to move the pointers.
In the best case, the object expands, so all the bytes we had before are
still live and we touch fewer cachelines.  In the worst case we get a
new object, but that's roughly exponential.

For a shrink operation, we definitely want the alloc -> copy -> free
logic because there's no way to guarantee that krealloc-down isn't a nop
operation, which wastes memory.

But I see that this function isn't very cohesive and could be split into
separate ->grow and ->shrink functions that do their own allocations.

Or seeing how the only callers of xfs_iroot_realloc are the btree code
itself, maybe I should just refactor this into a single ->broot_realloc
function in the btree ops which will cut out a lot of indirect calls
from the iroot code.

Yeah.  I'm gonna go do that.  Disregard patch 5 onwards.

> 	/* copy records */
> 	sptr = xfs_bmbt_key_addr(mp, src, 1);
> 	dptr = xfs_bmbt_key_addr(mp, dst, 1);
> 	memcpy(dptr, sptr, numrecs * sizeof(struct xfs_bmbt_key));
> 
> 	/* copy pointers */
> 	sptr = xfs_bmap_broot_ptr_addr(mp, src_broot, 1, src_bytes);
> 	dptr = xfs_bmap_broot_ptr_addr(mp, dst_broot, 1, dst_bytes);
> 	memmove(dptr, sptr, numrecs * sizeof(xfs_fsblock_t));
> 
> out_free_src:
> 	kfree(src);
> 	ifp->if_broot = dst;
> 	ifp->if_broot_bytes = new_size;
> }
> 
> And the callers are now both:
> 
> 	xfs_ifork_realloc_broot(mp, ifp, new_size, old_size, numrecs);
> 
> This also naturally handles the "reduce to zero size" without
> needing any special case code, it avoids the double pointer copy on
> grow, and the operation logic is simple, obvious and easy to
> understand...

Hmm.  The rtrmap patchset starts by moving xfs_ifork_move_broot to
xfs_bmap_btree.c and virtualizes the broot grow/shrink operation to
become a per-btree type operation.  The rtreflink series expands this
usage.

--D

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

[PATCH 09/10] xfs: rearrange xfs_iroot_realloc a bit

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Rearrange the innards of xfs_iroot_realloc so that we can reduce
duplicated code prior to genericizing the function.  No functional
changes.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_inode_fork.c |   48 ++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 26 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 307207473abdb..306106b78d088 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -451,44 +451,46 @@ xfs_ifork_move_broot(
  */
 void
 xfs_iroot_realloc(
-	xfs_inode_t		*ip,
+	struct xfs_inode	*ip,
 	int			rec_diff,
 	int			whichfork)
 {
 	struct xfs_mount	*mp = ip->i_mount;
-	int			cur_max;
 	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
 	struct xfs_btree_block	*new_broot;
-	int			new_max;
 	size_t			new_size;
 	size_t			old_size = ifp->if_broot_bytes;
+	int			cur_max;
+	int			new_max;
+
+	/* Handle degenerate cases. */
+	if (rec_diff == 0)
+		return;
 
 	/*
-	 * Handle the degenerate case quietly.
+	 * If there wasn't any memory allocated before, just allocate it now
+	 * and get out.
 	 */
-	if (rec_diff == 0) {
+	if (old_size == 0) {
+		ASSERT(rec_diff > 0);
+
+		xfs_iroot_alloc(ip, whichfork,
+				xfs_bmap_broot_space_calc(mp, rec_diff));
 		return;
 	}
 
+	/* Compute the new and old record count and space requirements. */
+	cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
+	new_max = cur_max + rec_diff;
+	ASSERT(new_max >= 0);
+	new_size = xfs_bmap_broot_space_calc(mp, new_max);
+
 	if (rec_diff > 0) {
-		/*
-		 * If there wasn't any memory allocated before, just
-		 * allocate it now and get out.
-		 */
-		if (old_size == 0) {
-			new_size = xfs_bmap_broot_space_calc(mp, rec_diff);
-			xfs_iroot_alloc(ip, whichfork, new_size);
-			return;
-		}
-
 		/*
 		 * If there is already an existing if_broot, then we need
 		 * to realloc() it and shift the pointers to their new
 		 * location.
 		 */
-		cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
-		new_max = cur_max + rec_diff;
-		new_size = xfs_bmap_broot_space_calc(mp, new_max);
 		ifp->if_broot = krealloc(ifp->if_broot, new_size,
 					 GFP_KERNEL | __GFP_NOFAIL);
 		ifp->if_broot_bytes = new_size;
@@ -500,14 +502,8 @@ xfs_iroot_realloc(
 	/*
 	 * rec_diff is less than 0.  In this case, we are shrinking the
 	 * if_broot buffer.  It must already exist.  If we go to zero
-	 * records, just get rid of the root and clear the status bit.
+	 * bytes, just get rid of the root and clear the status bit.
 	 */
-	ASSERT((ifp->if_broot != NULL) && (old_size > 0));
-	cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
-	new_max = cur_max + rec_diff;
-	ASSERT(new_max >= 0);
-
-	new_size = xfs_bmap_broot_space_calc(mp, new_max);
 	if (new_size == 0) {
 		xfs_iroot_free(ip, whichfork);
 		return;
@@ -518,7 +514,7 @@ xfs_iroot_realloc(
 			old_size, new_max);
 	kfree(ifp->if_broot);
 	ifp->if_broot = new_broot;
-	ifp->if_broot_bytes = (int)new_size;
+	ifp->if_broot_bytes = new_size;
 }
 
 

Re: [PATCH 09/10] xfs: rearrange xfs_iroot_realloc a bit

[Christoph Hellwig]

Looks good:

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

[PATCH 10/10] xfs: standardize the btree maxrecs function parameters

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Standardize the parameters in xfs_{alloc,bm,ino,rmap,refcount}bt_maxrecs
so that we have consistent calling conventions.  This doesn't affect the
kernel that much, but enables us to clean up userspace a bit.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_alloc_btree.c    |    6 +++---
 fs/xfs/libxfs/xfs_alloc_btree.h    |    3 ++-
 fs/xfs/libxfs/xfs_bmap.c           |    2 +-
 fs/xfs/libxfs/xfs_bmap_btree.c     |    6 +++---
 fs/xfs/libxfs/xfs_bmap_btree.h     |    5 +++--
 fs/xfs/libxfs/xfs_ialloc.c         |    4 ++--
 fs/xfs/libxfs/xfs_ialloc_btree.c   |    6 +++---
 fs/xfs/libxfs/xfs_ialloc_btree.h   |    3 ++-
 fs/xfs/libxfs/xfs_inode_fork.c     |    2 +-
 fs/xfs/libxfs/xfs_refcount_btree.c |    5 +++--
 fs/xfs/libxfs/xfs_refcount_btree.h |    3 ++-
 fs/xfs/libxfs/xfs_rmap_btree.c     |    7 ++++---
 fs/xfs/libxfs/xfs_rmap_btree.h     |    3 ++-
 fs/xfs/libxfs/xfs_sb.c             |   16 ++++++++--------
 14 files changed, 39 insertions(+), 32 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 585e98e87ef9e..aada676eee519 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -569,11 +569,11 @@ xfs_allocbt_block_maxrecs(
 /*
  * Calculate number of records in an alloc btree block.
  */
-int
+unsigned int
 xfs_allocbt_maxrecs(
 	struct xfs_mount	*mp,
-	int			blocklen,
-	int			leaf)
+	unsigned int		blocklen,
+	bool			leaf)
 {
 	blocklen -= XFS_ALLOC_BLOCK_LEN(mp);
 	return xfs_allocbt_block_maxrecs(blocklen, leaf);
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.h b/fs/xfs/libxfs/xfs_alloc_btree.h
index 155b47f231ab2..12647f9aaa6d7 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.h
+++ b/fs/xfs/libxfs/xfs_alloc_btree.h
@@ -53,7 +53,8 @@ struct xfs_btree_cur *xfs_bnobt_init_cursor(struct xfs_mount *mp,
 struct xfs_btree_cur *xfs_cntbt_init_cursor(struct xfs_mount *mp,
 		struct xfs_trans *tp, struct xfs_buf *bp,
 		struct xfs_perag *pag);
-extern int xfs_allocbt_maxrecs(struct xfs_mount *, int, int);
+unsigned int xfs_allocbt_maxrecs(struct xfs_mount *mp, unsigned int blocklen,
+		bool leaf);
 extern xfs_extlen_t xfs_allocbt_calc_size(struct xfs_mount *mp,
 		unsigned long long len);
 
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index e3922cf75381c..37a1a1339027c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -584,7 +584,7 @@ xfs_bmap_btree_to_extents(
 	ASSERT(ifp->if_format == XFS_DINODE_FMT_BTREE);
 	ASSERT(be16_to_cpu(rblock->bb_level) == 1);
 	ASSERT(be16_to_cpu(rblock->bb_numrecs) == 1);
-	ASSERT(xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, 0) == 1);
+	ASSERT(xfs_bmbt_maxrecs(mp, ifp->if_broot_bytes, false) == 1);
 
 	pp = xfs_bmap_broot_ptr_addr(mp, rblock, 1, ifp->if_broot_bytes);
 	cbno = be64_to_cpu(*pp);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index 0769644d30412..b811fb38a839a 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -645,11 +645,11 @@ xfs_bmbt_commit_staged_btree(
 /*
  * Calculate number of records in a bmap btree block.
  */
-int
+unsigned int
 xfs_bmbt_maxrecs(
 	struct xfs_mount	*mp,
-	int			blocklen,
-	int			leaf)
+	unsigned int		blocklen,
+	bool			leaf)
 {
 	blocklen -= xfs_bmbt_block_len(mp);
 	return xfs_bmbt_block_maxrecs(blocklen, leaf);
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h
index a187f4b120ea1..df6a60452260e 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.h
+++ b/fs/xfs/libxfs/xfs_bmap_btree.h
@@ -35,7 +35,8 @@ extern void xfs_bmbt_to_bmdr(struct xfs_mount *, struct xfs_btree_block *, int,
 
 extern int xfs_bmbt_get_maxrecs(struct xfs_btree_cur *, int level);
 extern int xfs_bmdr_maxrecs(int blocklen, int leaf);
-extern int xfs_bmbt_maxrecs(struct xfs_mount *, int blocklen, int leaf);
+unsigned int xfs_bmbt_maxrecs(struct xfs_mount *mp, unsigned int blocklen,
+		bool leaf);
 
 extern int xfs_bmbt_change_owner(struct xfs_trans *tp, struct xfs_inode *ip,
 				 int whichfork, xfs_ino_t new_owner,
@@ -151,7 +152,7 @@ xfs_bmap_broot_ptr_addr(
 	unsigned int		i,
 	unsigned int		sz)
 {
-	return xfs_bmbt_ptr_addr(mp, bb, i, xfs_bmbt_maxrecs(mp, sz, 0));
+	return xfs_bmbt_ptr_addr(mp, bb, i, xfs_bmbt_maxrecs(mp, sz, false));
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index fc70601e8d8ee..20bb5ce381345 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2948,8 +2948,8 @@ xfs_ialloc_setup_geometry(
 
 	/* Compute inode btree geometry. */
 	igeo->agino_log = sbp->sb_inopblog + sbp->sb_agblklog;
-	igeo->inobt_mxr[0] = xfs_inobt_maxrecs(mp, sbp->sb_blocksize, 1);
-	igeo->inobt_mxr[1] = xfs_inobt_maxrecs(mp, sbp->sb_blocksize, 0);
+	igeo->inobt_mxr[0] = xfs_inobt_maxrecs(mp, sbp->sb_blocksize, true);
+	igeo->inobt_mxr[1] = xfs_inobt_maxrecs(mp, sbp->sb_blocksize, false);
 	igeo->inobt_mnr[0] = igeo->inobt_mxr[0] / 2;
 	igeo->inobt_mnr[1] = igeo->inobt_mxr[1] / 2;
 
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 797d5b5f7b725..401b42d52af68 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -572,11 +572,11 @@ xfs_inobt_block_maxrecs(
 /*
  * Calculate number of records in an inobt btree block.
  */
-int
+unsigned int
 xfs_inobt_maxrecs(
 	struct xfs_mount	*mp,
-	int			blocklen,
-	int			leaf)
+	unsigned int		blocklen,
+	bool			leaf)
 {
 	blocklen -= XFS_INOBT_BLOCK_LEN(mp);
 	return xfs_inobt_block_maxrecs(blocklen, leaf);
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.h b/fs/xfs/libxfs/xfs_ialloc_btree.h
index 6472ec1ecbb45..300edf5bc0094 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.h
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.h
@@ -50,7 +50,8 @@ struct xfs_btree_cur *xfs_inobt_init_cursor(struct xfs_perag *pag,
 		struct xfs_trans *tp, struct xfs_buf *agbp);
 struct xfs_btree_cur *xfs_finobt_init_cursor(struct xfs_perag *pag,
 		struct xfs_trans *tp, struct xfs_buf *agbp);
-extern int xfs_inobt_maxrecs(struct xfs_mount *, int, int);
+unsigned int xfs_inobt_maxrecs(struct xfs_mount *mp, unsigned int blocklen,
+		bool leaf);
 
 /* ir_holemask to inode allocation bitmap conversion */
 uint64_t xfs_inobt_irec_to_allocmask(const struct xfs_inobt_rec_incore *irec);
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 306106b78d088..847fbfdd87f8c 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -480,7 +480,7 @@ xfs_iroot_realloc(
 	}
 
 	/* Compute the new and old record count and space requirements. */
-	cur_max = xfs_bmbt_maxrecs(mp, old_size, 0);
+	cur_max = xfs_bmbt_maxrecs(mp, old_size, false);
 	new_max = cur_max + rec_diff;
 	ASSERT(new_max >= 0);
 	new_size = xfs_bmap_broot_space_calc(mp, new_max);
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index cb3b1d42ae9a8..795928d1a66d8 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -417,9 +417,10 @@ xfs_refcountbt_block_maxrecs(
 /*
  * Calculate the number of records in a refcount btree block.
  */
-int
+unsigned int
 xfs_refcountbt_maxrecs(
-	int			blocklen,
+	struct xfs_mount	*mp,
+	unsigned int		blocklen,
 	bool			leaf)
 {
 	blocklen -= XFS_REFCOUNT_BLOCK_LEN;
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.h b/fs/xfs/libxfs/xfs_refcount_btree.h
index 1e0ab25f6c680..beb93bef6a814 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.h
+++ b/fs/xfs/libxfs/xfs_refcount_btree.h
@@ -48,7 +48,8 @@ struct xbtree_afakeroot;
 extern struct xfs_btree_cur *xfs_refcountbt_init_cursor(struct xfs_mount *mp,
 		struct xfs_trans *tp, struct xfs_buf *agbp,
 		struct xfs_perag *pag);
-extern int xfs_refcountbt_maxrecs(int blocklen, bool leaf);
+unsigned int xfs_refcountbt_maxrecs(struct xfs_mount *mp, unsigned int blocklen,
+		bool leaf);
 extern void xfs_refcountbt_compute_maxlevels(struct xfs_mount *mp);
 
 extern xfs_extlen_t xfs_refcountbt_calc_size(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index 56fd6c4bd8b41..ac2f1f499b76f 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -731,10 +731,11 @@ xfs_rmapbt_block_maxrecs(
 /*
  * Calculate number of records in an rmap btree block.
  */
-int
+unsigned int
 xfs_rmapbt_maxrecs(
-	int			blocklen,
-	int			leaf)
+	struct xfs_mount	*mp,
+	unsigned int		blocklen,
+	bool			leaf)
 {
 	blocklen -= XFS_RMAP_BLOCK_LEN;
 	return xfs_rmapbt_block_maxrecs(blocklen, leaf);
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.h b/fs/xfs/libxfs/xfs_rmap_btree.h
index eb90d89e80866..119b1567cd0ee 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.h
+++ b/fs/xfs/libxfs/xfs_rmap_btree.h
@@ -47,7 +47,8 @@ struct xfs_btree_cur *xfs_rmapbt_init_cursor(struct xfs_mount *mp,
 				struct xfs_perag *pag);
 void xfs_rmapbt_commit_staged_btree(struct xfs_btree_cur *cur,
 		struct xfs_trans *tp, struct xfs_buf *agbp);
-int xfs_rmapbt_maxrecs(int blocklen, int leaf);
+unsigned int xfs_rmapbt_maxrecs(struct xfs_mount *mp, unsigned int blocklen,
+		bool leaf);
 extern void xfs_rmapbt_compute_maxlevels(struct xfs_mount *mp);
 
 extern xfs_extlen_t xfs_rmapbt_calc_size(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index a6fa9aedb28b0..d95409f3cba66 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1000,23 +1000,23 @@ xfs_sb_mount_common(
 	mp->m_blockwmask = mp->m_blockwsize - 1;
 	xfs_mount_sb_set_rextsize(mp, sbp);
 
-	mp->m_alloc_mxr[0] = xfs_allocbt_maxrecs(mp, sbp->sb_blocksize, 1);
-	mp->m_alloc_mxr[1] = xfs_allocbt_maxrecs(mp, sbp->sb_blocksize, 0);
+	mp->m_alloc_mxr[0] = xfs_allocbt_maxrecs(mp, sbp->sb_blocksize, true);
+	mp->m_alloc_mxr[1] = xfs_allocbt_maxrecs(mp, sbp->sb_blocksize, false);
 	mp->m_alloc_mnr[0] = mp->m_alloc_mxr[0] / 2;
 	mp->m_alloc_mnr[1] = mp->m_alloc_mxr[1] / 2;
 
-	mp->m_bmap_dmxr[0] = xfs_bmbt_maxrecs(mp, sbp->sb_blocksize, 1);
-	mp->m_bmap_dmxr[1] = xfs_bmbt_maxrecs(mp, sbp->sb_blocksize, 0);
+	mp->m_bmap_dmxr[0] = xfs_bmbt_maxrecs(mp, sbp->sb_blocksize, true);
+	mp->m_bmap_dmxr[1] = xfs_bmbt_maxrecs(mp, sbp->sb_blocksize, false);
 	mp->m_bmap_dmnr[0] = mp->m_bmap_dmxr[0] / 2;
 	mp->m_bmap_dmnr[1] = mp->m_bmap_dmxr[1] / 2;
 
-	mp->m_rmap_mxr[0] = xfs_rmapbt_maxrecs(sbp->sb_blocksize, 1);
-	mp->m_rmap_mxr[1] = xfs_rmapbt_maxrecs(sbp->sb_blocksize, 0);
+	mp->m_rmap_mxr[0] = xfs_rmapbt_maxrecs(mp, sbp->sb_blocksize, true);
+	mp->m_rmap_mxr[1] = xfs_rmapbt_maxrecs(mp, sbp->sb_blocksize, false);
 	mp->m_rmap_mnr[0] = mp->m_rmap_mxr[0] / 2;
 	mp->m_rmap_mnr[1] = mp->m_rmap_mxr[1] / 2;
 
-	mp->m_refc_mxr[0] = xfs_refcountbt_maxrecs(sbp->sb_blocksize, true);
-	mp->m_refc_mxr[1] = xfs_refcountbt_maxrecs(sbp->sb_blocksize, false);
+	mp->m_refc_mxr[0] = xfs_refcountbt_maxrecs(mp, sbp->sb_blocksize, true);
+	mp->m_refc_mxr[1] = xfs_refcountbt_maxrecs(mp, sbp->sb_blocksize, false);
 	mp->m_refc_mnr[0] = mp->m_refc_mxr[0] / 2;
 	mp->m_refc_mnr[1] = mp->m_refc_mxr[1] / 2;
 

Re: [PATCH 10/10] xfs: standardize the btree maxrecs function parameters

[Christoph Hellwig]

Looks good:

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

[RFC PATCH] libxfs: compile with a C++ compiler

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Apparently C++ compilers don't like the implicit void* casts that go on
in the system headers.  Compile a dummy program with the C++ compiler to
make sure this works, so Darrick has /some/ chance of figuring these
things out before the users do.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 configure.ac         |    1 +
 include/builddefs.in |    7 +++++++
 libxfs/Makefile      |    8 +++++++-
 libxfs/dummy.cpp     |   15 +++++++++++++++
 4 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 libxfs/dummy.cpp

diff --git a/configure.ac b/configure.ac
index 0ffe2e5dfc53..04544f85395b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -9,6 +9,7 @@ AC_PROG_INSTALL
 LT_INIT
 
 AC_PROG_CC
+AC_PROG_CXX
 AC_ARG_VAR(BUILD_CC, [C compiler for build tools])
 if test "${BUILD_CC+set}" != "set"; then
   if test $cross_compiling = no; then
diff --git a/include/builddefs.in b/include/builddefs.in
index 44f95234d21b..0f312b8b88fe 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -14,6 +14,7 @@ MALLOCLIB = @malloc_lib@
 LOADERFLAGS = @LDFLAGS@
 LTLDFLAGS = @LDFLAGS@
 CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
+CXXFLAGS = @CXXFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
 BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64
 
 # make sure we don't pick up whacky LDFLAGS from the make environment and
@@ -234,9 +235,15 @@ ifeq ($(ENABLE_GETTEXT),yes)
 GCFLAGS += -DENABLE_GETTEXT
 endif
 
+# Override these if C++ needs other options
+SANITIZER_CXXFLAGS = $(SANITIZER_CFLAGS)
+GCXXFLAGS = $(GCFLAGS)
+PCXXFLAGS = $(PCFLAGS)
+
 BUILD_CFLAGS += $(GCFLAGS) $(PCFLAGS)
 # First, Sanitizer, Global, Platform, Local CFLAGS
 CFLAGS += $(FCFLAGS) $(SANITIZER_CFLAGS) $(OPTIMIZER) $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
+CXXFLAGS += $(FCXXFLAGS) $(SANITIZER_CXXFLAGS) $(OPTIMIZER) $(GCXXFLAGS) $(PCXXFLAGS) $(LCXXFLAGS)
 
 include $(TOPDIR)/include/buildmacros
 
diff --git a/libxfs/Makefile b/libxfs/Makefile
index 1185a5e6cb26..bb851ab74204 100644
--- a/libxfs/Makefile
+++ b/libxfs/Makefile
@@ -125,6 +125,8 @@ CFILES = buf_mem.c \
 	xfs_trans_space.c \
 	xfs_types.c
 
+LDIRT += dummy.o
+
 #
 # Tracing flags:
 # -DMEM_DEBUG		all zone memory use
@@ -144,7 +146,11 @@ LTLIBS = $(LIBPTHREAD) $(LIBRT)
 # don't try linking xfs_repair with a debug libxfs.
 DEBUG = -DNDEBUG
 
-default: ltdepend $(LTLIBRARY)
+default: ltdepend $(LTLIBRARY) dummy.o
+
+dummy.o: dummy.cpp
+	@echo "    [CXX]    $@"
+	$(Q)$(CC) $(CXXFLAGS) -c $<
 
 # set up include/xfs header directory
 include $(BUILDRULES)
diff --git a/libxfs/dummy.cpp b/libxfs/dummy.cpp
new file mode 100644
index 000000000000..a872c00ad84b
--- /dev/null
+++ b/libxfs/dummy.cpp
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <djwong@kernel.org>
+ */
+#include "include/xfs.h"
+#include "include/handle.h"
+#include "include/jdm.h"
+
+/* Dummy program to test C++ compilation of user-exported xfs headers */
+
+int main(int argc, char *argv[])
+{
+	return 0;
+}

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

"Darrick J. Wong" <djwong@kernel.org> writes:

> From: Darrick J. Wong <djwong@kernel.org>
>
> Apparently C++ compilers don't like the implicit void* casts that go on
> in the system headers.  Compile a dummy program with the C++ compiler to
> make sure this works, so Darrick has /some/ chance of figuring these
> things out before the users do.

Thanks, this is a good idea. Double thanks for the quick fix.

1) yes, it finds the breakage:
Tested-by: Sam James <sam@gentoo.org>

2) with the fix below (CC -> CXX):
Reviewed-by: Sam James <sam@gentoo.org>

3) another thing to think about is:
* -pedantic?
* maybe do one for a bunch of standards? (I think systemd does every
possible value [1])
* doing the above for C as well

I know that sounds a bit like overkill, but systemd
does it and it's cheap to do it versus the blowup if something goes
wrong. I don't have a strong opinion on this or how far you want to go
with it.

[1] https://github.com/systemd/systemd/blob/3317aedff0901e08a8efc8346ad76b184d5d40ea/src/systemd/meson.build#L60

>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  configure.ac         |    1 +
>  include/builddefs.in |    7 +++++++
>  libxfs/Makefile      |    8 +++++++-
>  libxfs/dummy.cpp     |   15 +++++++++++++++
>  4 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100644 libxfs/dummy.cpp
>
> diff --git a/configure.ac b/configure.ac
> index 0ffe2e5dfc53..04544f85395b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -9,6 +9,7 @@ AC_PROG_INSTALL
>  LT_INIT
>  
>  AC_PROG_CC
> +AC_PROG_CXX
>  AC_ARG_VAR(BUILD_CC, [C compiler for build tools])
>  if test "${BUILD_CC+set}" != "set"; then
>    if test $cross_compiling = no; then
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 44f95234d21b..0f312b8b88fe 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -14,6 +14,7 @@ MALLOCLIB = @malloc_lib@
>  LOADERFLAGS = @LDFLAGS@
>  LTLDFLAGS = @LDFLAGS@
>  CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
> +CXXFLAGS = @CXXFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
>  BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64
>  
>  # make sure we don't pick up whacky LDFLAGS from the make environment and
> @@ -234,9 +235,15 @@ ifeq ($(ENABLE_GETTEXT),yes)
>  GCFLAGS += -DENABLE_GETTEXT
>  endif
>  
> +# Override these if C++ needs other options
> +SANITIZER_CXXFLAGS = $(SANITIZER_CFLAGS)
> +GCXXFLAGS = $(GCFLAGS)
> +PCXXFLAGS = $(PCFLAGS)
> +
>  BUILD_CFLAGS += $(GCFLAGS) $(PCFLAGS)
>  # First, Sanitizer, Global, Platform, Local CFLAGS
>  CFLAGS += $(FCFLAGS) $(SANITIZER_CFLAGS) $(OPTIMIZER) $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
> +CXXFLAGS += $(FCXXFLAGS) $(SANITIZER_CXXFLAGS) $(OPTIMIZER) $(GCXXFLAGS) $(PCXXFLAGS) $(LCXXFLAGS)
>  
>  include $(TOPDIR)/include/buildmacros
>  
> diff --git a/libxfs/Makefile b/libxfs/Makefile
> index 1185a5e6cb26..bb851ab74204 100644
> --- a/libxfs/Makefile
> +++ b/libxfs/Makefile
> @@ -125,6 +125,8 @@ CFILES = buf_mem.c \
>  	xfs_trans_space.c \
>  	xfs_types.c
>  
> +LDIRT += dummy.o
> +
>  #
>  # Tracing flags:
>  # -DMEM_DEBUG		all zone memory use
> @@ -144,7 +146,11 @@ LTLIBS = $(LIBPTHREAD) $(LIBRT)
>  # don't try linking xfs_repair with a debug libxfs.
>  DEBUG = -DNDEBUG
>  
> -default: ltdepend $(LTLIBRARY)
> +default: ltdepend $(LTLIBRARY) dummy.o
> +
> +dummy.o: dummy.cpp
> +	@echo "    [CXX]    $@"
> +	$(Q)$(CC) $(CXXFLAGS) -c $<

$(CXX) ;)

>  
>  # set up include/xfs header directory
>  include $(BUILDRULES)
> diff --git a/libxfs/dummy.cpp b/libxfs/dummy.cpp
> new file mode 100644
> index 000000000000..a872c00ad84b
> --- /dev/null
> +++ b/libxfs/dummy.cpp
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@kernel.org>
> + */
> +#include "include/xfs.h"
> +#include "include/handle.h"
> +#include "include/jdm.h"
> +
> +/* Dummy program to test C++ compilation of user-exported xfs headers */
> +
> +int main(int argc, char *argv[])
> +{
> +	return 0;
> +}

cheers,
sam

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Sam James]

Sam James <sam@gentoo.org> writes:

> "Darrick J. Wong" <djwong@kernel.org> writes:
>
>> From: Darrick J. Wong <djwong@kernel.org>
>>
>> Apparently C++ compilers don't like the implicit void* casts that go on
>> in the system headers.  Compile a dummy program with the C++ compiler to
>> make sure this works, so Darrick has /some/ chance of figuring these
>> things out before the users do.
>
> Thanks, this is a good idea. Double thanks for the quick fix.
>
> 1) yes, it finds the breakage:
> Tested-by: Sam James <sam@gentoo.org>
>
> 2) with the fix below (CC -> CXX):
> Reviewed-by: Sam James <sam@gentoo.org>
>
> 3) another thing to think about is:
> * -pedantic?
> * maybe do one for a bunch of standards? (I think systemd does every
> possible value [1])
> * doing the above for C as well
>
> I know that sounds a bit like overkill, but systemd
> does it and it's cheap to do it versus the blowup if something goes
> wrong. I don't have a strong opinion on this or how far you want to go
> with it.

... thinking about this, it could've helped us with
https://lore.kernel.org/linux-xfs/a216140e-1c8a-4d04-ba46-670646498622@redhat.com/

>
> [1] https://github.com/systemd/systemd/blob/3317aedff0901e08a8efc8346ad76b184d5d40ea/src/systemd/meson.build#L60
>
>>
>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>>  configure.ac         |    1 +
>>  include/builddefs.in |    7 +++++++
>>  libxfs/Makefile      |    8 +++++++-
>>  libxfs/dummy.cpp     |   15 +++++++++++++++
>>  4 files changed, 30 insertions(+), 1 deletion(-)
>>  create mode 100644 libxfs/dummy.cpp
>>
>> diff --git a/configure.ac b/configure.ac
>> index 0ffe2e5dfc53..04544f85395b 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -9,6 +9,7 @@ AC_PROG_INSTALL
>>  LT_INIT
>>  
>>  AC_PROG_CC
>> +AC_PROG_CXX
>>  AC_ARG_VAR(BUILD_CC, [C compiler for build tools])
>>  if test "${BUILD_CC+set}" != "set"; then
>>    if test $cross_compiling = no; then
>> diff --git a/include/builddefs.in b/include/builddefs.in
>> index 44f95234d21b..0f312b8b88fe 100644
>> --- a/include/builddefs.in
>> +++ b/include/builddefs.in
>> @@ -14,6 +14,7 @@ MALLOCLIB = @malloc_lib@
>>  LOADERFLAGS = @LDFLAGS@
>>  LTLDFLAGS = @LDFLAGS@
>>  CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
>> +CXXFLAGS = @CXXFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
>>  BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64
>>  
>>  # make sure we don't pick up whacky LDFLAGS from the make environment and
>> @@ -234,9 +235,15 @@ ifeq ($(ENABLE_GETTEXT),yes)
>>  GCFLAGS += -DENABLE_GETTEXT
>>  endif
>>  
>> +# Override these if C++ needs other options
>> +SANITIZER_CXXFLAGS = $(SANITIZER_CFLAGS)
>> +GCXXFLAGS = $(GCFLAGS)
>> +PCXXFLAGS = $(PCFLAGS)
>> +
>>  BUILD_CFLAGS += $(GCFLAGS) $(PCFLAGS)
>>  # First, Sanitizer, Global, Platform, Local CFLAGS
>>  CFLAGS += $(FCFLAGS) $(SANITIZER_CFLAGS) $(OPTIMIZER) $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
>> +CXXFLAGS += $(FCXXFLAGS) $(SANITIZER_CXXFLAGS) $(OPTIMIZER) $(GCXXFLAGS) $(PCXXFLAGS) $(LCXXFLAGS)
>>  
>>  include $(TOPDIR)/include/buildmacros
>>  
>> diff --git a/libxfs/Makefile b/libxfs/Makefile
>> index 1185a5e6cb26..bb851ab74204 100644
>> --- a/libxfs/Makefile
>> +++ b/libxfs/Makefile
>> @@ -125,6 +125,8 @@ CFILES = buf_mem.c \
>>  	xfs_trans_space.c \
>>  	xfs_types.c
>>  
>> +LDIRT += dummy.o
>> +
>>  #
>>  # Tracing flags:
>>  # -DMEM_DEBUG		all zone memory use
>> @@ -144,7 +146,11 @@ LTLIBS = $(LIBPTHREAD) $(LIBRT)
>>  # don't try linking xfs_repair with a debug libxfs.
>>  DEBUG = -DNDEBUG
>>  
>> -default: ltdepend $(LTLIBRARY)
>> +default: ltdepend $(LTLIBRARY) dummy.o
>> +
>> +dummy.o: dummy.cpp
>> +	@echo "    [CXX]    $@"
>> +	$(Q)$(CC) $(CXXFLAGS) -c $<
>
> $(CXX) ;)
>
>>  
>>  # set up include/xfs header directory
>>  include $(BUILDRULES)
>> diff --git a/libxfs/dummy.cpp b/libxfs/dummy.cpp
>> new file mode 100644
>> index 000000000000..a872c00ad84b
>> --- /dev/null
>> +++ b/libxfs/dummy.cpp
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2024 Oracle.  All Rights Reserved.
>> + * Author: Darrick J. Wong <djwong@kernel.org>
>> + */
>> +#include "include/xfs.h"
>> +#include "include/handle.h"
>> +#include "include/jdm.h"
>> +
>> +/* Dummy program to test C++ compilation of user-exported xfs headers */
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	return 0;
>> +}
>
> cheers,
> sam

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Darrick J. Wong]

On Wed, Aug 28, 2024 at 01:01:31AM +0100, Sam James wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> 
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Apparently C++ compilers don't like the implicit void* casts that go on
> > in the system headers.  Compile a dummy program with the C++ compiler to
> > make sure this works, so Darrick has /some/ chance of figuring these
> > things out before the users do.
> 
> Thanks, this is a good idea. Double thanks for the quick fix.
> 
> 1) yes, it finds the breakage:
> Tested-by: Sam James <sam@gentoo.org>
> 
> 2) with the fix below (CC -> CXX):
> Reviewed-by: Sam James <sam@gentoo.org>
> 
> 3) another thing to think about is:
> * -pedantic?

-pedantic won't build because C++ doesn't support flexarrays:

In file included from ../include/xfs.h:61:
../include/xfs/xfs_fs.h:523:33: error: ISO C++ forbids flexible array member ‘bulkstat’ [-Werror=pedantic]
  523 |         struct xfs_bulkstat     bulkstat[];
      |                                 ^~~~~~~~

even if you wrap it in extern "C" { ... };

> * maybe do one for a bunch of standards? (I think systemd does every
> possible value [1])

That might be overkill since xfsprogs' build system doesn't have a good
mechanism for detecting if a compiler supports a particular standard.
I'm not even sure there's a good "reference" C++ standard to pick here,
since the kernel doesn't require a C++ compiler.

> * doing the above for C as well

Hmm, that's a good idea.

I think the only relevant standard here is C11 (well really gnu11),
because that's what the kernel compiles with since 5.18.  xfsprogs
doesn't specify any particular version of C, but perhaps we should match
the kernel every time they bump that up?

IOWs, should we build xfsprogs with -std=gnu11?  The commit changing the
kernel to gnu11 (e8c07082a810) remarks that gcc 5.1 supports it just
fine.  IIRC RHEL 7 only has 4.8.5 but it's now in extended support so
... who cares?  The oldest supported Debian stable has gcc 8.

--D

> I know that sounds a bit like overkill, but systemd
> does it and it's cheap to do it versus the blowup if something goes
> wrong. I don't have a strong opinion on this or how far you want to go
> with it.
> 
> [1] https://github.com/systemd/systemd/blob/3317aedff0901e08a8efc8346ad76b184d5d40ea/src/systemd/meson.build#L60
> 
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  configure.ac         |    1 +
> >  include/builddefs.in |    7 +++++++
> >  libxfs/Makefile      |    8 +++++++-
> >  libxfs/dummy.cpp     |   15 +++++++++++++++
> >  4 files changed, 30 insertions(+), 1 deletion(-)
> >  create mode 100644 libxfs/dummy.cpp
> >
> > diff --git a/configure.ac b/configure.ac
> > index 0ffe2e5dfc53..04544f85395b 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -9,6 +9,7 @@ AC_PROG_INSTALL
> >  LT_INIT
> >  
> >  AC_PROG_CC
> > +AC_PROG_CXX
> >  AC_ARG_VAR(BUILD_CC, [C compiler for build tools])
> >  if test "${BUILD_CC+set}" != "set"; then
> >    if test $cross_compiling = no; then
> > diff --git a/include/builddefs.in b/include/builddefs.in
> > index 44f95234d21b..0f312b8b88fe 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -14,6 +14,7 @@ MALLOCLIB = @malloc_lib@
> >  LOADERFLAGS = @LDFLAGS@
> >  LTLDFLAGS = @LDFLAGS@
> >  CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
> > +CXXFLAGS = @CXXFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
> >  BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64
> >  
> >  # make sure we don't pick up whacky LDFLAGS from the make environment and
> > @@ -234,9 +235,15 @@ ifeq ($(ENABLE_GETTEXT),yes)
> >  GCFLAGS += -DENABLE_GETTEXT
> >  endif
> >  
> > +# Override these if C++ needs other options
> > +SANITIZER_CXXFLAGS = $(SANITIZER_CFLAGS)
> > +GCXXFLAGS = $(GCFLAGS)
> > +PCXXFLAGS = $(PCFLAGS)
> > +
> >  BUILD_CFLAGS += $(GCFLAGS) $(PCFLAGS)
> >  # First, Sanitizer, Global, Platform, Local CFLAGS
> >  CFLAGS += $(FCFLAGS) $(SANITIZER_CFLAGS) $(OPTIMIZER) $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
> > +CXXFLAGS += $(FCXXFLAGS) $(SANITIZER_CXXFLAGS) $(OPTIMIZER) $(GCXXFLAGS) $(PCXXFLAGS) $(LCXXFLAGS)
> >  
> >  include $(TOPDIR)/include/buildmacros
> >  
> > diff --git a/libxfs/Makefile b/libxfs/Makefile
> > index 1185a5e6cb26..bb851ab74204 100644
> > --- a/libxfs/Makefile
> > +++ b/libxfs/Makefile
> > @@ -125,6 +125,8 @@ CFILES = buf_mem.c \
> >  	xfs_trans_space.c \
> >  	xfs_types.c
> >  
> > +LDIRT += dummy.o
> > +
> >  #
> >  # Tracing flags:
> >  # -DMEM_DEBUG		all zone memory use
> > @@ -144,7 +146,11 @@ LTLIBS = $(LIBPTHREAD) $(LIBRT)
> >  # don't try linking xfs_repair with a debug libxfs.
> >  DEBUG = -DNDEBUG
> >  
> > -default: ltdepend $(LTLIBRARY)
> > +default: ltdepend $(LTLIBRARY) dummy.o
> > +
> > +dummy.o: dummy.cpp
> > +	@echo "    [CXX]    $@"
> > +	$(Q)$(CC) $(CXXFLAGS) -c $<
> 
> $(CXX) ;)
> 
> >  
> >  # set up include/xfs header directory
> >  include $(BUILDRULES)
> > diff --git a/libxfs/dummy.cpp b/libxfs/dummy.cpp
> > new file mode 100644
> > index 000000000000..a872c00ad84b
> > --- /dev/null
> > +++ b/libxfs/dummy.cpp
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 Oracle.  All Rights Reserved.
> > + * Author: Darrick J. Wong <djwong@kernel.org>
> > + */
> > +#include "include/xfs.h"
> > +#include "include/handle.h"
> > +#include "include/jdm.h"
> > +
> > +/* Dummy program to test C++ compilation of user-exported xfs headers */
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	return 0;
> > +}
> 
> cheers,
> sam

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Sam James]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Wed, Aug 28, 2024 at 01:01:31AM +0100, Sam James wrote:
>> "Darrick J. Wong" <djwong@kernel.org> writes:
>> 
>> > From: Darrick J. Wong <djwong@kernel.org>
>> >
>> > Apparently C++ compilers don't like the implicit void* casts that go on
>> > in the system headers.  Compile a dummy program with the C++ compiler to
>> > make sure this works, so Darrick has /some/ chance of figuring these
>> > things out before the users do.
>> 
>> Thanks, this is a good idea. Double thanks for the quick fix.
>> 
>> 1) yes, it finds the breakage:
>> Tested-by: Sam James <sam@gentoo.org>
>> 
>> 2) with the fix below (CC -> CXX):
>> Reviewed-by: Sam James <sam@gentoo.org>
>> 
>> 3) another thing to think about is:
>> * -pedantic?
>
> -pedantic won't build because C++ doesn't support flexarrays:
>
> In file included from ../include/xfs.h:61:
> ../include/xfs/xfs_fs.h:523:33: error: ISO C++ forbids flexible array member ‘bulkstat’ [-Werror=pedantic]
>   523 |         struct xfs_bulkstat     bulkstat[];
>       |                                 ^~~~~~~~
>
> even if you wrap it in extern "C" { ... };

Doh. So the ship has kind of sailed already anyway.

>
>> * maybe do one for a bunch of standards? (I think systemd does every
>> possible value [1])
>
> That might be overkill since xfsprogs' build system doesn't have a good
> mechanism for detecting if a compiler supports a particular standard.
> I'm not even sure there's a good "reference" C++ standard to pick here,
> since the kernel doesn't require a C++ compiler.
>
>> * doing the above for C as well
>
> Hmm, that's a good idea.
>
> I think the only relevant standard here is C11 (well really gnu11),
> because that's what the kernel compiles with since 5.18.  xfsprogs
> doesn't specify any particular version of C, but perhaps we should match
> the kernel every time they bump that up?

Projects are often (IMO far too) conservative with the features they use
in their headers and I don't think it's unreasonable to match the kernel
here.

>
> IOWs, should we build xfsprogs with -std=gnu11?  The commit changing the
> kernel to gnu11 (e8c07082a810) remarks that gcc 5.1 supports it just
> fine.  IIRC RHEL 7 only has 4.8.5 but it's now in extended support so
> ... who cares?  The oldest supported Debian stable has gcc 8.

so, I think we should match whatever linux-headers / the uapi rules are,
and given I've seen flexible array members in there, it's at least C99.

I did some quick greps and am not sure if we're using any C11 features
in uapi at least.

Just don't blame me if someone yells ;)

(kees, any idea if I'm talking rubbish?)

tl;dr: let's try gnu11?

> [...]

sam

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Kees Cook]

On Thu, Aug 29, 2024 at 01:17:53AM +0100, Sam James wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> 
> > On Wed, Aug 28, 2024 at 01:01:31AM +0100, Sam James wrote:
> >> "Darrick J. Wong" <djwong@kernel.org> writes:
> >> 
> >> > From: Darrick J. Wong <djwong@kernel.org>
> >> >
> >> > Apparently C++ compilers don't like the implicit void* casts that go on
> >> > in the system headers.  Compile a dummy program with the C++ compiler to
> >> > make sure this works, so Darrick has /some/ chance of figuring these
> >> > things out before the users do.
> >> 
> >> Thanks, this is a good idea. Double thanks for the quick fix.
> >> 
> >> 1) yes, it finds the breakage:
> >> Tested-by: Sam James <sam@gentoo.org>
> >> 
> >> 2) with the fix below (CC -> CXX):
> >> Reviewed-by: Sam James <sam@gentoo.org>
> >> 
> >> 3) another thing to think about is:
> >> * -pedantic?
> >
> > -pedantic won't build because C++ doesn't support flexarrays:
> >
> > In file included from ../include/xfs.h:61:
> > ../include/xfs/xfs_fs.h:523:33: error: ISO C++ forbids flexible array member ‘bulkstat’ [-Werror=pedantic]
> >   523 |         struct xfs_bulkstat     bulkstat[];
> >       |                                 ^~~~~~~~
> >
> > even if you wrap it in extern "C" { ... };
> 
> Doh. So the ship has kind of sailed already anyway.
> 
> >
> >> * maybe do one for a bunch of standards? (I think systemd does every
> >> possible value [1])
> >
> > That might be overkill since xfsprogs' build system doesn't have a good
> > mechanism for detecting if a compiler supports a particular standard.
> > I'm not even sure there's a good "reference" C++ standard to pick here,
> > since the kernel doesn't require a C++ compiler.
> >
> >> * doing the above for C as well
> >
> > Hmm, that's a good idea.
> >
> > I think the only relevant standard here is C11 (well really gnu11),
> > because that's what the kernel compiles with since 5.18.  xfsprogs
> > doesn't specify any particular version of C, but perhaps we should match
> > the kernel every time they bump that up?
> 
> Projects are often (IMO far too) conservative with the features they use
> in their headers and I don't think it's unreasonable to match the kernel
> here.
> 
> >
> > IOWs, should we build xfsprogs with -std=gnu11?  The commit changing the
> > kernel to gnu11 (e8c07082a810) remarks that gcc 5.1 supports it just
> > fine.  IIRC RHEL 7 only has 4.8.5 but it's now in extended support so
> > ... who cares?  The oldest supported Debian stable has gcc 8.
> 
> so, I think we should match whatever linux-headers / the uapi rules are,
> and given I've seen flexible array members in there, it's at least C99.
> 
> I did some quick greps and am not sure if we're using any C11 features
> in uapi at least.

-std=gnu11 seems like the correct choice, yes.

> Just don't blame me if someone yells ;)
> 
> (kees, any idea if I'm talking rubbish?)
> 
> tl;dr: let's try gnu11?
> 
> > [...]
> 
> sam

In really ugly cases (i.e. MSVC importing headers into a C++ project in
ACPICA) we did things like:

#if defined(__cplusplus)
# define __FLEX_SIZE	0
#else
# define __FLEX_SIZE	/**/
#endif

...
struct ... {
	...
	struct xfs_bulkstat     bulkstat[__FLEX_SIZE];
	...
};

-- 
Kees Cook

Re: [RFC PATCH] libxfs: compile with a C++ compiler

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 04:45:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Apparently C++ compilers don't like the implicit void* casts that go on
> in the system headers.  Compile a dummy program with the C++ compiler to
> make sure this works, so Darrick has /some/ chance of figuring these
> things out before the users do.

I guess if we want to support C++ users of our system headers we'll have
to do something, so ACK.