[PATCH RFC 0/5] xfs: Remove redundant btree insert code

[PATCH RFC 0/5] xfs: Remove redundant btree insert code

[Eric Sandeen]

This depends on the prior changes
([PATCH 0/2] xfs: add missing cursor status check after btree ops)

The btree insert code returns both an error and a status var, but
they are redundant as described in the first patch here.

Every caller handles the error first, and never gets to the status
check, so all that can be removed.

RFC to see if this direction and the patch splitup/granularity seems
ok.

If so the opportunity exists on the btree delete side as well.

 libxfs/xfs_alloc.c    |   32 +++------------------
 libxfs/xfs_bmap.c     |   76 +++++++-----------------------------------------
 libxfs/xfs_btree.c    |    4 --
 libxfs/xfs_btree.h    |    2 -
 libxfs/xfs_ialloc.c   |   28 +++---------------
 libxfs/xfs_ialloc.h   |    3 -
 libxfs/xfs_refcount.c |   45 +++++------------------------
 libxfs/xfs_refcount.h |    2 -
 libxfs/xfs_rmap.c     |   46 ++++--------------------------
 scrub/rcbag.c         |    6 ---
 scrub/rcbag_btree.c   |    5 +--
 scrub/rcbag_btree.h   |    3 -
 12 files changed, 46 insertions(+), 206 deletions(-)

[PATCH 1/5] xfs: remove redundant stat checks after xfs_btree_insert()

[Eric Sandeen]

xfs_btree_insert() already validates its own status internally; it
checks the result of xfs_btree_insrec(). If it succeeds, it returns
error == 0 and status == 1. If that fails, it returns a nonzero error
and never even updates stat.

This makes every instance of the 2nd test in this idiom:

	if ((error = xfs_btree_insert(cnt_cur, &i)))
		return error;
	if (XFS_IS_CORRUPT(mp, i != 1)) {
		xfs_btree_mark_sick(cnt_cur);
		return -EFSCORRUPTED;
	}

duplicate/unreachable code at best, which is good because on an error
return it would have tested a stale variable.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c    | 22 ---------------
 fs/xfs/libxfs/xfs_bmap.c     | 54 ------------------------------------
 fs/xfs/libxfs/xfs_refcount.c |  9 ------
 fs/xfs/libxfs/xfs_rmap.c     | 32 ---------------------
 4 files changed, 117 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index ae221a158764..b40c51a0f651 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -694,10 +694,6 @@ xfs_alloc_fixup_trees(
 		}
 		if ((error = xfs_btree_insert(cnt_cur, &i)))
 			return error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cnt_cur);
-			return -EFSCORRUPTED;
-		}
 	}
 	if (nfbno2 != NULLAGBLOCK) {
 		if ((error = xfs_alloc_lookup_eq(cnt_cur, nfbno2, nflen2, &i)))
@@ -708,10 +704,6 @@ xfs_alloc_fixup_trees(
 		}
 		if ((error = xfs_btree_insert(cnt_cur, &i)))
 			return error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cnt_cur);
-			return -EFSCORRUPTED;
-		}
 	}
 	/*
 	 * Fix up the by-block btree entry(s).
@@ -745,10 +737,6 @@ xfs_alloc_fixup_trees(
 		}
 		if ((error = xfs_btree_insert(bno_cur, &i)))
 			return error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(bno_cur);
-			return -EFSCORRUPTED;
-		}
 	}
 
 	if (fixup_longest)
@@ -2314,11 +2302,6 @@ xfs_free_ag_extent(
 		nlen = len;
 		if ((error = xfs_btree_insert(bno_cur, &i)))
 			goto error0;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(bno_cur);
-			error = -EFSCORRUPTED;
-			goto error0;
-		}
 	}
 	xfs_btree_del_cursor(bno_cur, XFS_BTREE_NOERROR);
 	bno_cur = NULL;
@@ -2341,11 +2324,6 @@ xfs_free_ag_extent(
 		fixup_longest = true;
 	if ((error = xfs_btree_insert(cnt_cur, &i)))
 		goto error0;
-	if (XFS_IS_CORRUPT(mp, i != 1)) {
-		xfs_btree_mark_sick(cnt_cur);
-		error = -EFSCORRUPTED;
-		goto error0;
-	}
 	if (fixup_longest) {
 		error = xfs_alloc_fixup_longest(cnt_cur);
 		if (error)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9bffde1484a7..a7e548078d18 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1674,11 +1674,6 @@ xfs_bmap_add_extent_delay_real(
 			error = xfs_btree_insert(bma->cur, &i);
 			if (error)
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(bma->cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 		ASSERT(da_new <= da_old);
 		break;
@@ -1745,11 +1740,6 @@ xfs_bmap_add_extent_delay_real(
 			error = xfs_btree_insert(bma->cur, &i);
 			if (error)
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(bma->cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 
 		if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
@@ -1836,11 +1826,6 @@ xfs_bmap_add_extent_delay_real(
 			error = xfs_btree_insert(bma->cur, &i);
 			if (error)
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(bma->cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 
 		if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
@@ -1925,11 +1910,6 @@ xfs_bmap_add_extent_delay_real(
 			error = xfs_btree_insert(bma->cur, &i);
 			if (error)
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(bma->cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 
 		if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
@@ -2350,11 +2330,6 @@ xfs_bmap_add_extent_unwritten_real(
 			cur->bc_rec.b = *new;
 			if ((error = xfs_btree_insert(cur, &i)))
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 		break;
 
@@ -2436,11 +2411,6 @@ xfs_bmap_add_extent_unwritten_real(
 			}
 			if ((error = xfs_btree_insert(cur, &i)))
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 		break;
 
@@ -2486,11 +2456,6 @@ xfs_bmap_add_extent_unwritten_real(
 			cur->bc_rec.b = PREV;
 			if ((error = xfs_btree_insert(cur, &i)))
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 			/*
 			 * Reset the cursor to the position of the new extent
 			 * we are about to insert as we can't trust it after
@@ -2507,11 +2472,6 @@ xfs_bmap_add_extent_unwritten_real(
 			/* new middle extent - newext */
 			if ((error = xfs_btree_insert(cur, &i)))
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 		break;
 
@@ -2767,11 +2727,6 @@ xfs_bmap_add_extent_hole_real(
 			error = xfs_btree_insert(cur, &i);
 			if (error)
 				goto done;
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				error = -EFSCORRUPTED;
-				goto done;
-			}
 		}
 		break;
 	}
@@ -5093,10 +5048,6 @@ xfs_bmap_del_extent_real(
 				*logflagsp = 0;
 				return -ENOSPC;
 			}
-			if (XFS_IS_CORRUPT(mp, i != 1)) {
-				xfs_btree_mark_sick(cur);
-				return -EFSCORRUPTED;
-			}
 		} else
 			*logflagsp |= xfs_ilog_fext(whichfork);
 
@@ -5958,11 +5909,6 @@ xfs_bmap_split_extent(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto del_cursor;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto del_cursor;
-		}
 	}
 
 	/*
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 6b67308fa594..49e9993ce360 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -276,15 +276,6 @@ xfs_refcount_insert(
 	cur->bc_rec.rc.rc_domain = irec->rc_domain;
 
 	error = xfs_btree_insert(cur, i);
-	if (error)
-		goto out_error;
-	if (XFS_IS_CORRUPT(cur->bc_mp, *i != 1)) {
-		xfs_btree_mark_sick(cur);
-		error = -EFSCORRUPTED;
-		goto out_error;
-	}
-
-out_error:
 	if (error)
 		trace_xfs_refcount_insert_error(cur, error, _RET_IP_);
 	return error;
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 14aef87837a9..ff4aef8d30a7 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -146,13 +146,6 @@ xfs_rmap_insert(
 	rcur->bc_rec.r.rm_offset = offset;
 	rcur->bc_rec.r.rm_flags = flags;
 	error = xfs_btree_insert(rcur, &i);
-	if (error)
-		goto done;
-	if (XFS_IS_CORRUPT(rcur->bc_mp, i != 1)) {
-		xfs_btree_mark_sick(rcur);
-		error = -EFSCORRUPTED;
-		goto done;
-	}
 done:
 	if (error)
 		trace_xfs_rmap_insert_error(rcur, error, _RET_IP_);
@@ -1185,11 +1178,6 @@ xfs_rmap_map(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto out_error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto out_error;
-		}
 	}
 
 	trace_xfs_rmap_map_done(cur, bno, len, unwritten, oinfo);
@@ -1592,11 +1580,6 @@ xfs_rmap_convert(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto done;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto done;
-		}
 		break;
 
 	case RMAP_RIGHT_FILLING | RMAP_RIGHT_CONTIG:
@@ -1650,11 +1633,6 @@ xfs_rmap_convert(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto done;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto done;
-		}
 		break;
 
 	case 0:
@@ -1683,11 +1661,6 @@ xfs_rmap_convert(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto done;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto done;
-		}
 		/*
 		 * Reset the cursor to the position of the new extent
 		 * we are about to insert as we can't trust it after
@@ -1709,11 +1682,6 @@ xfs_rmap_convert(
 		error = xfs_btree_insert(cur, &i);
 		if (error)
 			goto done;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto done;
-		}
 		break;
 
 	case RMAP_LEFT_FILLING | RMAP_LEFT_CONTIG | RMAP_RIGHT_CONTIG:
-- 
2.55.0

[PATCH 2/5] xfs: remove redundant stat checks after xfs_refcount_insert()

[Eric Sandeen]

xfs_refcount_insert() simply returns the error and status from
xfs_btree_insert, which has already validated the insert or returned
an error.

This makes every instance of the 2nd test in this idiom:

	if ((error = xfs_refcount_insert(cnt_cur, &i)))
		return error;
	if (XFS_IS_CORRUPT(mp, i != 1)) {
		xfs_btree_mark_sick(cnt_cur);
		return -EFSCORRUPTED;
	}

duplicate/unreachable code. Remove them.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_refcount.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 49e9993ce360..aec6c839220a 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -457,12 +457,8 @@ xfs_refcount_split_extent(
 	error = xfs_refcount_insert(cur, &tmp, &found_rec);
 	if (error)
 		goto out_error;
-	if (XFS_IS_CORRUPT(cur->bc_mp, found_rec != 1)) {
-		xfs_btree_mark_sick(cur);
-		error = -EFSCORRUPTED;
-		goto out_error;
-	}
-	return error;
+
+	return 0;
 
 out_error:
 	trace_xfs_refcount_split_extent_error(cur, error, _RET_IP_);
@@ -1191,12 +1187,6 @@ xfs_refcount_adjust_extents(
 						&found_tmp);
 				if (error)
 					goto out_error;
-				if (XFS_IS_CORRUPT(cur->bc_mp,
-						   found_tmp != 1)) {
-					xfs_btree_mark_sick(cur);
-					error = -EFSCORRUPTED;
-					goto out_error;
-				}
 			} else {
 				error = xrefc_free_extent(cur, &tmp);
 				if (error)
@@ -1841,11 +1831,6 @@ xfs_refcount_adjust_cow_extents(
 				&found_tmp);
 		if (error)
 			goto out_error;
-		if (XFS_IS_CORRUPT(cur->bc_mp, found_tmp != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto out_error;
-		}
 		break;
 	case XFS_REFCOUNT_ADJUST_COW_FREE:
 		/* Removing a CoW reservation, there should be one extent. */
-- 
2.55.0

[PATCH 3/5] xfs: remove redundant stat checks after xfs_inobt_insert_rec()

[Eric Sandeen]

xfs_inobt_insert_rec() simply returns the error and status from
xfs_btree_insert, which has already validated the insert or returned
an error.

This makes every instance of the 2nd test in this idiom:

        if ((error = xfs_inobt_insert_rec(cnt_cur, &i)))
                return error;
        if (XFS_IS_CORRUPT(mp, i != 1)) {
                xfs_btree_mark_sick(cnt_cur);
                return -EFSCORRUPTED;
        }

duplicate/unreachable code. Remove them.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index bb6e684f5467..fd916a2a005b 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -236,7 +236,6 @@ xfs_inobt_insert(
 			xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
 			return error;
 		}
-		ASSERT(i == 1);
 	}
 
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
@@ -572,12 +571,6 @@ xfs_inobt_insert_sprec(
 					     nrec->ir_free, &i);
 		if (error)
 			goto error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto error;
-		}
-
 		goto out;
 	}
 
@@ -647,7 +640,6 @@ xfs_finobt_insert_sprec(
 	struct xfs_buf			*agbp,
 	struct xfs_inobt_rec_incore	*nrec)	/* in/out: new rec. */
 {
-	struct xfs_mount		*mp = pag_mount(pag);
 	struct xfs_btree_cur		*cur;
 	int				error;
 	int				i;
@@ -665,11 +657,6 @@ xfs_finobt_insert_sprec(
 					     nrec->ir_free, &i);
 		if (error)
 			goto error;
-		if (XFS_IS_CORRUPT(mp, i != 1)) {
-			xfs_btree_mark_sick(cur);
-			error = -EFSCORRUPTED;
-			goto error;
-		}
 	} else {
 		error = xfs_inobt_update(cur, nrec);
 		if (error)
@@ -2254,8 +2241,6 @@ xfs_difree_finobt(
 					     ibtrec->ir_free, &i);
 		if (error)
 			goto error;
-		ASSERT(i == 1);
-
 		goto out;
 	}
 
-- 
2.55.0

[PATCH 4/5] xfs: remove redundant stat check after rcbagbt_insert()

[Eric Sandeen]

rcbagbt_insert() simply returns the error and status from
xfs_btree_insert, which has already validated the insert
or returned an error.

This makes this instance of this idiom:

	error = rcbagbt_insert(cur, &bagrec, &has);
	if (error)
		goto out_cur;
	if (!has) {
		error = -EFSCORRUPTED;
		goto out_cur;
	}

duplicate/unreachable code. Remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/scrub/rcbag.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/scrub/rcbag.c b/fs/xfs/scrub/rcbag.c
index 8f3a10f7c908..fd9ccb205725 100644
--- a/fs/xfs/scrub/rcbag.c
+++ b/fs/xfs/scrub/rcbag.c
@@ -105,10 +105,6 @@ rcbag_add(
 		error = rcbagbt_insert(cur, &bagrec, &has);
 		if (error)
 			goto out_cur;
-		if (!has) {
-			error = -EFSCORRUPTED;
-			goto out_cur;
-		}
 	}
 
 	xfs_btree_del_cursor(cur, 0);
-- 
2.55.0

[PATCH 5/5] xfs: remove status parameter from several btree insert functions

[Eric Sandeen]

xfs_btree_insert and its callers now have no need for the status
parameter; either they return error = 0 / status = 1, or nonzero error
and status 0. Every caller now handles the error and never reaches
the duplicate status variable check. Remove the status params.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c    | 10 +++++-----
 fs/xfs/libxfs/xfs_bmap.c     | 22 +++++++++++-----------
 fs/xfs/libxfs/xfs_btree.c    |  4 +---
 fs/xfs/libxfs/xfs_btree.h    |  2 +-
 fs/xfs/libxfs/xfs_ialloc.c   | 13 ++++++-------
 fs/xfs/libxfs/xfs_ialloc.h   |  3 +--
 fs/xfs/libxfs/xfs_refcount.c | 17 +++++++----------
 fs/xfs/libxfs/xfs_refcount.h |  2 +-
 fs/xfs/libxfs/xfs_rmap.c     | 14 +++++++-------
 fs/xfs/scrub/rcbag.c         |  2 +-
 fs/xfs/scrub/rcbag_btree.c   |  5 ++---
 fs/xfs/scrub/rcbag_btree.h   |  3 +--
 12 files changed, 44 insertions(+), 53 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index b40c51a0f651..13a61144a2c8 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -692,7 +692,7 @@ xfs_alloc_fixup_trees(
 			xfs_btree_mark_sick(cnt_cur);
 			return -EFSCORRUPTED;
 		}
-		if ((error = xfs_btree_insert(cnt_cur, &i)))
+		if ((error = xfs_btree_insert(cnt_cur)))
 			return error;
 	}
 	if (nfbno2 != NULLAGBLOCK) {
@@ -702,7 +702,7 @@ xfs_alloc_fixup_trees(
 			xfs_btree_mark_sick(cnt_cur);
 			return -EFSCORRUPTED;
 		}
-		if ((error = xfs_btree_insert(cnt_cur, &i)))
+		if ((error = xfs_btree_insert(cnt_cur)))
 			return error;
 	}
 	/*
@@ -735,7 +735,7 @@ xfs_alloc_fixup_trees(
 			xfs_btree_mark_sick(bno_cur);
 			return -EFSCORRUPTED;
 		}
-		if ((error = xfs_btree_insert(bno_cur, &i)))
+		if ((error = xfs_btree_insert(bno_cur)))
 			return error;
 	}
 
@@ -2300,7 +2300,7 @@ xfs_free_ag_extent(
 	else {
 		nbno = bno;
 		nlen = len;
-		if ((error = xfs_btree_insert(bno_cur, &i)))
+		if ((error = xfs_btree_insert(bno_cur)))
 			goto error0;
 	}
 	xfs_btree_del_cursor(bno_cur, XFS_BTREE_NOERROR);
@@ -2322,7 +2322,7 @@ xfs_free_ag_extent(
 	}
 	if (xfs_alloc_cursor_at_lastrec(cnt_cur))
 		fixup_longest = true;
-	if ((error = xfs_btree_insert(cnt_cur, &i)))
+	if ((error = xfs_btree_insert(cnt_cur)))
 		goto error0;
 	if (fixup_longest) {
 		error = xfs_alloc_fixup_longest(cnt_cur);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index a7e548078d18..a2f46037e0c3 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1671,7 +1671,7 @@ xfs_bmap_add_extent_delay_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			error = xfs_btree_insert(bma->cur, &i);
+			error = xfs_btree_insert(bma->cur);
 			if (error)
 				goto done;
 		}
@@ -1737,7 +1737,7 @@ xfs_bmap_add_extent_delay_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			error = xfs_btree_insert(bma->cur, &i);
+			error = xfs_btree_insert(bma->cur);
 			if (error)
 				goto done;
 		}
@@ -1823,7 +1823,7 @@ xfs_bmap_add_extent_delay_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			error = xfs_btree_insert(bma->cur, &i);
+			error = xfs_btree_insert(bma->cur);
 			if (error)
 				goto done;
 		}
@@ -1907,7 +1907,7 @@ xfs_bmap_add_extent_delay_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			error = xfs_btree_insert(bma->cur, &i);
+			error = xfs_btree_insert(bma->cur);
 			if (error)
 				goto done;
 		}
@@ -2328,7 +2328,7 @@ xfs_bmap_add_extent_unwritten_real(
 			if (error)
 				goto done;
 			cur->bc_rec.b = *new;
-			if ((error = xfs_btree_insert(cur, &i)))
+			if ((error = xfs_btree_insert(cur)))
 				goto done;
 		}
 		break;
@@ -2409,7 +2409,7 @@ xfs_bmap_add_extent_unwritten_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			if ((error = xfs_btree_insert(cur, &i)))
+			if ((error = xfs_btree_insert(cur)))
 				goto done;
 		}
 		break;
@@ -2454,7 +2454,7 @@ xfs_bmap_add_extent_unwritten_real(
 				goto done;
 			/* new left extent - oldext */
 			cur->bc_rec.b = PREV;
-			if ((error = xfs_btree_insert(cur, &i)))
+			if ((error = xfs_btree_insert(cur)))
 				goto done;
 			/*
 			 * Reset the cursor to the position of the new extent
@@ -2470,7 +2470,7 @@ xfs_bmap_add_extent_unwritten_real(
 				goto done;
 			}
 			/* new middle extent - newext */
-			if ((error = xfs_btree_insert(cur, &i)))
+			if ((error = xfs_btree_insert(cur)))
 				goto done;
 		}
 		break;
@@ -2724,7 +2724,7 @@ xfs_bmap_add_extent_hole_real(
 				error = -EFSCORRUPTED;
 				goto done;
 			}
-			error = xfs_btree_insert(cur, &i);
+			error = xfs_btree_insert(cur);
 			if (error)
 				goto done;
 		}
@@ -5013,7 +5013,7 @@ xfs_bmap_del_extent_real(
 			if (error)
 				return error;
 			cur->bc_rec.b = new;
-			error = xfs_btree_insert(cur, &i);
+			error = xfs_btree_insert(cur);
 			if (error && error != -ENOSPC)
 				return error;
 			/*
@@ -5906,7 +5906,7 @@ xfs_bmap_split_extent(
 			error = -EFSCORRUPTED;
 			goto del_cursor;
 		}
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto del_cursor;
 	}
diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 7effebe297ea..e2e698596c21 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -3728,8 +3728,7 @@ xfs_btree_insrec(
  */
 int
 xfs_btree_insert(
-	struct xfs_btree_cur	*cur,
-	int			*stat)
+	struct xfs_btree_cur	*cur)
 {
 	int			error;	/* error return value */
 	int			i;	/* result value, 0 for failure */
@@ -3798,7 +3797,6 @@ xfs_btree_insert(
 		}
 	} while (!xfs_btree_ptr_is_null(cur, &nptr));
 
-	*stat = i;
 	return 0;
 error0:
 	return error;
diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
index 60e78572e725..0f82f3106bac 100644
--- a/fs/xfs/libxfs/xfs_btree.h
+++ b/fs/xfs/libxfs/xfs_btree.h
@@ -405,7 +405,7 @@ int xfs_btree_decrement(struct xfs_btree_cur *, int, int *);
 int xfs_btree_lookup(struct xfs_btree_cur *, xfs_lookup_t, int *);
 int xfs_btree_update(struct xfs_btree_cur *, union xfs_btree_rec *);
 int xfs_btree_new_iroot(struct xfs_btree_cur *, int *, int *);
-int xfs_btree_insert(struct xfs_btree_cur *, int *);
+int xfs_btree_insert(struct xfs_btree_cur *);
 int xfs_btree_delete(struct xfs_btree_cur *, int *);
 int xfs_btree_get_rec(struct xfs_btree_cur *, union xfs_btree_rec **, int *);
 int xfs_btree_change_owner(struct xfs_btree_cur *cur, uint64_t new_owner,
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index fd916a2a005b..bd7b0a24225f 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -186,14 +186,13 @@ xfs_inobt_insert_rec(
 	uint16_t		holemask,
 	uint8_t			count,
 	int32_t			freecount,
-	xfs_inofree_t		free,
-	int			*stat)
+	xfs_inofree_t		free)
 {
 	cur->bc_rec.i.ir_holemask = holemask;
 	cur->bc_rec.i.ir_count = count;
 	cur->bc_rec.i.ir_freecount = freecount;
 	cur->bc_rec.i.ir_free = free;
-	return xfs_btree_insert(cur, stat);
+	return xfs_btree_insert(cur);
 }
 
 /*
@@ -231,7 +230,7 @@ xfs_inobt_insert(
 		error = xfs_inobt_insert_rec(cur, XFS_INOBT_HOLEMASK_FULL,
 					     XFS_INODES_PER_CHUNK,
 					     XFS_INODES_PER_CHUNK,
-					     XFS_INOBT_ALL_FREE, &i);
+					     XFS_INOBT_ALL_FREE);
 		if (error) {
 			xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
 			return error;
@@ -568,7 +567,7 @@ xfs_inobt_insert_sprec(
 	if (i == 0) {
 		error = xfs_inobt_insert_rec(cur, nrec->ir_holemask,
 					     nrec->ir_count, nrec->ir_freecount,
-					     nrec->ir_free, &i);
+					     nrec->ir_free);
 		if (error)
 			goto error;
 		goto out;
@@ -654,7 +653,7 @@ xfs_finobt_insert_sprec(
 	if (i == 0) {
 		error = xfs_inobt_insert_rec(cur, nrec->ir_holemask,
 					     nrec->ir_count, nrec->ir_freecount,
-					     nrec->ir_free, &i);
+					     nrec->ir_free);
 		if (error)
 			goto error;
 	} else {
@@ -2238,7 +2237,7 @@ xfs_difree_finobt(
 		error = xfs_inobt_insert_rec(cur, ibtrec->ir_holemask,
 					     ibtrec->ir_count,
 					     ibtrec->ir_freecount,
-					     ibtrec->ir_free, &i);
+					     ibtrec->ir_free);
 		if (error)
 			goto error;
 		goto out;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 3a1323155a45..49c3d4c513c9 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -105,8 +105,7 @@ int xfs_ialloc_has_inodes_at_extent(struct xfs_btree_cur *cur,
 int xfs_ialloc_count_inodes(struct xfs_btree_cur *cur, xfs_agino_t *count,
 		xfs_agino_t *freecount);
 int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask,
-		uint8_t count, int32_t freecount, xfs_inofree_t free,
-		int *stat);
+		uint8_t count, int32_t freecount, xfs_inofree_t free);
 
 int xfs_ialloc_cluster_alignment(struct xfs_mount *mp);
 void xfs_ialloc_setup_geometry(struct xfs_mount *mp);
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index aec6c839220a..574a5e7c0cdf 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -263,8 +263,7 @@ xfs_refcount_update(
 int
 xfs_refcount_insert(
 	struct xfs_btree_cur		*cur,
-	struct xfs_refcount_irec	*irec,
-	int				*i)
+	struct xfs_refcount_irec	*irec)
 {
 	int				error;
 
@@ -275,7 +274,7 @@ xfs_refcount_insert(
 	cur->bc_rec.rc.rc_refcount = irec->rc_refcount;
 	cur->bc_rec.rc.rc_domain = irec->rc_domain;
 
-	error = xfs_btree_insert(cur, i);
+	error = xfs_btree_insert(cur);
 	if (error)
 		trace_xfs_refcount_insert_error(cur, error, _RET_IP_);
 	return error;
@@ -454,7 +453,7 @@ xfs_refcount_split_extent(
 	/* Insert the left extent. */
 	tmp = rcext;
 	tmp.rc_blockcount = agbno - rcext.rc_startblock;
-	error = xfs_refcount_insert(cur, &tmp, &found_rec);
+	error = xfs_refcount_insert(cur, &tmp);
 	if (error)
 		goto out_error;
 
@@ -1141,7 +1140,7 @@ xfs_refcount_adjust_extents(
 {
 	struct xfs_refcount_irec	ext, tmp;
 	int				error;
-	int				found_rec, found_tmp;
+	int				found_rec;
 
 	/* Merging did all the work already. */
 	if (*aglen == 0)
@@ -1183,8 +1182,7 @@ xfs_refcount_adjust_extents(
 			 */
 			cur->bc_refc.nr_ops++;
 			if (tmp.rc_refcount) {
-				error = xfs_refcount_insert(cur, &tmp,
-						&found_tmp);
+				error = xfs_refcount_insert(cur, &tmp);
 				if (error)
 					goto out_error;
 			} else {
@@ -1784,7 +1782,7 @@ xfs_refcount_adjust_cow_extents(
 {
 	struct xfs_refcount_irec	ext, tmp;
 	int				error;
-	int				found_rec, found_tmp;
+	int				found_rec;
 
 	if (aglen == 0)
 		return 0;
@@ -1827,8 +1825,7 @@ xfs_refcount_adjust_cow_extents(
 
 		trace_xfs_refcount_modify_extent(cur, &tmp);
 
-		error = xfs_refcount_insert(cur, &tmp,
-				&found_tmp);
+		error = xfs_refcount_insert(cur, &tmp);
 		if (error)
 			goto out_error;
 		break;
diff --git a/fs/xfs/libxfs/xfs_refcount.h b/fs/xfs/libxfs/xfs_refcount.h
index f2e299a716a4..cf300532bf04 100644
--- a/fs/xfs/libxfs/xfs_refcount.h
+++ b/fs/xfs/libxfs/xfs_refcount.h
@@ -126,7 +126,7 @@ xfs_failaddr_t xfs_refcount_check_irec(struct xfs_perag *pag,
 xfs_failaddr_t xfs_rtrefcount_check_irec(struct xfs_rtgroup *rtg,
 		const struct xfs_refcount_irec *irec);
 extern int xfs_refcount_insert(struct xfs_btree_cur *cur,
-		struct xfs_refcount_irec *irec, int *stat);
+		struct xfs_refcount_irec *irec);
 
 extern struct kmem_cache	*xfs_refcount_intent_cache;
 
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index ff4aef8d30a7..99575ae419a7 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -145,7 +145,7 @@ xfs_rmap_insert(
 	rcur->bc_rec.r.rm_owner = owner;
 	rcur->bc_rec.r.rm_offset = offset;
 	rcur->bc_rec.r.rm_flags = flags;
-	error = xfs_btree_insert(rcur, &i);
+	error = xfs_btree_insert(rcur);
 done:
 	if (error)
 		trace_xfs_rmap_insert_error(rcur, error, _RET_IP_);
@@ -855,7 +855,7 @@ xfs_rmap_unmap(
 				cur->bc_rec.r.rm_owner,
 				cur->bc_rec.r.rm_offset,
 				cur->bc_rec.r.rm_flags);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto out_error;
 	}
@@ -1175,7 +1175,7 @@ xfs_rmap_map(
 		cur->bc_rec.r.rm_offset = offset;
 		cur->bc_rec.r.rm_flags = flags;
 		trace_xfs_rmap_insert(cur, bno, len, owner, offset, flags);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto out_error;
 	}
@@ -1577,7 +1577,7 @@ xfs_rmap_convert(
 		NEW.rm_flags = newext;
 		cur->bc_rec.r = NEW;
 		trace_xfs_rmap_insert(cur, bno, len, owner, offset, newext);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto done;
 		break;
@@ -1630,7 +1630,7 @@ xfs_rmap_convert(
 		NEW.rm_flags = newext;
 		cur->bc_rec.r = NEW;
 		trace_xfs_rmap_insert(cur, bno, len, owner, offset, newext);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto done;
 		break;
@@ -1658,7 +1658,7 @@ xfs_rmap_convert(
 		trace_xfs_rmap_insert(cur, NEW.rm_startblock,
 				NEW.rm_blockcount, NEW.rm_owner, NEW.rm_offset,
 				NEW.rm_flags);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto done;
 		/*
@@ -1679,7 +1679,7 @@ xfs_rmap_convert(
 		cur->bc_rec.r.rm_flags &= ~XFS_RMAP_UNWRITTEN;
 		cur->bc_rec.r.rm_flags |= newext;
 		trace_xfs_rmap_insert(cur, bno, len, owner, offset, newext);
-		error = xfs_btree_insert(cur, &i);
+		error = xfs_btree_insert(cur);
 		if (error)
 			goto done;
 		break;
diff --git a/fs/xfs/scrub/rcbag.c b/fs/xfs/scrub/rcbag.c
index fd9ccb205725..41e798f65c67 100644
--- a/fs/xfs/scrub/rcbag.c
+++ b/fs/xfs/scrub/rcbag.c
@@ -102,7 +102,7 @@ rcbag_add(
 		bagrec.rbg_blockcount = rmap->rm_blockcount;
 		bagrec.rbg_refcount = 1;
 
-		error = rcbagbt_insert(cur, &bagrec, &has);
+		error = rcbagbt_insert(cur, &bagrec);
 		if (error)
 			goto out_cur;
 	}
diff --git a/fs/xfs/scrub/rcbag_btree.c b/fs/xfs/scrub/rcbag_btree.c
index 367f8ccf55c4..790931fa8f06 100644
--- a/fs/xfs/scrub/rcbag_btree.c
+++ b/fs/xfs/scrub/rcbag_btree.c
@@ -342,11 +342,10 @@ rcbagbt_update(
 int
 rcbagbt_insert(
 	struct xfs_btree_cur	*cur,
-	const struct rcbag_rec	*rec,
-	int			*success)
+	const struct rcbag_rec	*rec)
 {
 	struct rcbag_rec	*btrec = (struct rcbag_rec *)&cur->bc_rec;
 
 	memcpy(btrec, rec, sizeof(struct rcbag_rec));
-	return xfs_btree_insert(cur, success);
+	return xfs_btree_insert(cur);
 }
diff --git a/fs/xfs/scrub/rcbag_btree.h b/fs/xfs/scrub/rcbag_btree.h
index 03cadb032552..da45d9e885f8 100644
--- a/fs/xfs/scrub/rcbag_btree.h
+++ b/fs/xfs/scrub/rcbag_btree.h
@@ -70,8 +70,7 @@ int rcbagbt_lookup_eq(struct xfs_btree_cur *cur,
 		const struct xfs_rmap_irec *rmap, int *success);
 int rcbagbt_get_rec(struct xfs_btree_cur *cur, struct rcbag_rec *rec, int *has);
 int rcbagbt_update(struct xfs_btree_cur *cur, const struct rcbag_rec *rec);
-int rcbagbt_insert(struct xfs_btree_cur *cur, const struct rcbag_rec *rec,
-		int *success);
+int rcbagbt_insert(struct xfs_btree_cur *cur, const struct rcbag_rec *rec);
 
 #else
 # define rcbagbt_init_cur_cache()		0
-- 
2.55.0

Re: [PATCH 5/5] xfs: remove status parameter from several btree insert functions

[Dave Chinner]

On Tue, Oct 13, 2026 at 08:30:03PM -0500, Eric Sandeen wrote:
> xfs_btree_insert and its callers now have no need for the status
> parameter; either they return error = 0 / status = 1, or nonzero error
> and status 0. Every caller now handles the error and never reaches
> the duplicate status variable check. Remove the status params.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_alloc.c    | 10 +++++-----
>  fs/xfs/libxfs/xfs_bmap.c     | 22 +++++++++++-----------
>  fs/xfs/libxfs/xfs_btree.c    |  4 +---
>  fs/xfs/libxfs/xfs_btree.h    |  2 +-
>  fs/xfs/libxfs/xfs_ialloc.c   | 13 ++++++-------
>  fs/xfs/libxfs/xfs_ialloc.h   |  3 +--
>  fs/xfs/libxfs/xfs_refcount.c | 17 +++++++----------
>  fs/xfs/libxfs/xfs_refcount.h |  2 +-
>  fs/xfs/libxfs/xfs_rmap.c     | 14 +++++++-------
>  fs/xfs/scrub/rcbag.c         |  2 +-
>  fs/xfs/scrub/rcbag_btree.c   |  5 ++---
>  fs/xfs/scrub/rcbag_btree.h   |  3 +--
>  12 files changed, 44 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index b40c51a0f651..13a61144a2c8 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -692,7 +692,7 @@ xfs_alloc_fixup_trees(
>  			xfs_btree_mark_sick(cnt_cur);
>  			return -EFSCORRUPTED;
>  		}
> -		if ((error = xfs_btree_insert(cnt_cur, &i)))
> +		if ((error = xfs_btree_insert(cnt_cur)))

While you are touching all of these, can you convert them to the
more common pattern of:

	error = xfs_btree_insert(cur);
	if (error)
		return error;

Otherwise the whole series looks good to me.

-Dave.
-- 
Dave Chinner
dgc@kernel.org

Re: [PATCH 5/5] xfs: remove status parameter from several btree insert functions

[Eric Sandeen]

On 10/13/26 10:59 PM, Dave Chinner wrote:
> On Tue, Oct 13, 2026 at 08:30:03PM -0500, Eric Sandeen wrote:
>> xfs_btree_insert and its callers now have no need for the status
>> parameter; either they return error = 0 / status = 1, or nonzero error
>> and status 0. Every caller now handles the error and never reaches
>> the duplicate status variable check. Remove the status params.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/xfs/libxfs/xfs_alloc.c    | 10 +++++-----
>>  fs/xfs/libxfs/xfs_bmap.c     | 22 +++++++++++-----------
>>  fs/xfs/libxfs/xfs_btree.c    |  4 +---
>>  fs/xfs/libxfs/xfs_btree.h    |  2 +-
>>  fs/xfs/libxfs/xfs_ialloc.c   | 13 ++++++-------
>>  fs/xfs/libxfs/xfs_ialloc.h   |  3 +--
>>  fs/xfs/libxfs/xfs_refcount.c | 17 +++++++----------
>>  fs/xfs/libxfs/xfs_refcount.h |  2 +-
>>  fs/xfs/libxfs/xfs_rmap.c     | 14 +++++++-------
>>  fs/xfs/scrub/rcbag.c         |  2 +-
>>  fs/xfs/scrub/rcbag_btree.c   |  5 ++---
>>  fs/xfs/scrub/rcbag_btree.h   |  3 +--
>>  12 files changed, 44 insertions(+), 53 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
>> index b40c51a0f651..13a61144a2c8 100644
>> --- a/fs/xfs/libxfs/xfs_alloc.c
>> +++ b/fs/xfs/libxfs/xfs_alloc.c
>> @@ -692,7 +692,7 @@ xfs_alloc_fixup_trees(
>>  			xfs_btree_mark_sick(cnt_cur);
>>  			return -EFSCORRUPTED;
>>  		}
>> -		if ((error = xfs_btree_insert(cnt_cur, &i)))
>> +		if ((error = xfs_btree_insert(cnt_cur)))
> 
> While you are touching all of these, can you convert them to the
> more common pattern of:
> 
> 	error = xfs_btree_insert(cur);
> 	if (error)
> 		return error;
> 
> Otherwise the whole series looks good to me.
That /adds/ lines, what are you talking about! ;) 

Sure, I can do that if folks prefer.

-Eric

Re: [PATCH 1/5] xfs: remove redundant stat checks after xfs_btree_insert()

[Eric Sandeen]

On 10/13/26 8:29 PM, Eric Sandeen wrote:
> xfs_btree_insert() already validates its own status internally; it
> checks the result of xfs_btree_insrec(). If it succeeds, it returns
> error == 0 and status == 1. If that fails, it returns a nonzero error
> and never even updates stat.
> 
> This makes every instance of the 2nd test in this idiom:
> 
> 	if ((error = xfs_btree_insert(cnt_cur, &i)))
> 		return error;
> 	if (XFS_IS_CORRUPT(mp, i != 1)) {
> 		xfs_btree_mark_sick(cnt_cur);
> 		return -EFSCORRUPTED;
> 	}
> 
> duplicate/unreachable code at best, which is good because on an error
> return it would have tested a stale variable.

rereading this, it's not very clear.

I'll tidy up the message somehow on a proper resend.

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Re: [PATCH 5/5] xfs: remove status parameter from several btree insert functions

[Christoph Hellwig]

On Tue, Oct 13, 2026 at 11:07:41PM -0500, Eric Sandeen wrote:
> > While you are touching all of these, can you convert them to the
> > more common pattern of:
> > 
> > 	error = xfs_btree_insert(cur);
> > 	if (error)
> > 		return error;
> > 
> > Otherwise the whole series looks good to me.
> That /adds/ lines, what are you talking about! ;) 
> 
> Sure, I can do that if folks prefer.

I'd much prefer it, thanks.