[PATCHSET] xfs: LLM-inspired bug fixes, part 1

[PATCHSET] xfs: LLM-inspired bug fixes, part 1

[Darrick J. Wong]

Hi all,

Someone gave me a pretty generous license for a large language model, so
I pointed it at every single file under fs/xfs and told it to find bugs.
After personally filtering out the errors and garbage, here are fixes
for the real bugs that I think it found.

Most of these fixes target the online fsck code because I'm most
familiar with it, but the LLM will read other parts of the xfs codebase
so there are miscellaneous bug fixes.

I'm sending these out as smallish bundles of bug fixes.  There's no
particular order or grouping.

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

With a bit of luck, this should all go splendidly.
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=llm-fixes-1
---
Commits in this patchset:
 * xfs: don't replace the wrong part of the cow fork
 * xfs: don't wrap around quota ids in dqiterate
 * xfs: use rtrefcount btree cursor in xchk_xref_is_rt_cow_staging
 * xfs: use the rt version of the cow staging checker
 * xfs: write the rg superblock when fixing it
 * xfs: grab rtrmap btree when checking rgsuper
---
 fs/xfs/scrub/trace.h      |   28 ++++--
 fs/xfs/scrub/cow_repair.c |  203 +++++++++++++++++++++++++++++----------------
 fs/xfs/scrub/dqiterate.c  |    2 
 fs/xfs/scrub/rgsuper.c    |   14 +++
 fs/xfs/scrub/rtrefcount.c |    2 
 fs/xfs/scrub/rtrmap.c     |    2 
 6 files changed, 164 insertions(+), 87 deletions(-)

[PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Darrick J. Wong]

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

LOLLM points out that xfs_iext_lookup_extent can return a @got where
got->br_startoff < startoff.  In this case, xrep_cow_replace_range
replaces the entire mapping instead of just the part that had been
marked bad in the bitmap, but advances the bitmap cursor in
xrep_cow_replace by the amount replaced.  As a result, we fail to
replace the end of the bad range, and replace part of the good range.

Fix this by rewriting the replace method to handle replacing the middle
of a cow fork mapping.  This we do by returning both the current mapping
as @got, and the subset of the mapping that we want to replace as @rep,
using @rep to store the results of the new allocation, and comparing
@rep to @got to figure out the exact transformations needed.

Cc: <stable@vger.kernel.org> # v6.8
Fixes: dbbdbd0086320a ("xfs: repair problems in CoW forks")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/trace.h      |   28 ++++--
 fs/xfs/scrub/cow_repair.c |  203 +++++++++++++++++++++++++++++----------------
 2 files changed, 148 insertions(+), 83 deletions(-)


diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
index 1b7d9e07a27d3d..d48a6db5b5f308 100644
--- a/fs/xfs/scrub/trace.h
+++ b/fs/xfs/scrub/trace.h
@@ -2672,9 +2672,9 @@ TRACE_EVENT(xrep_cow_mark_file_range,
 );
 
 TRACE_EVENT(xrep_cow_replace_mapping,
-	TP_PROTO(struct xfs_inode *ip, const struct xfs_bmbt_irec *irec,
-		 xfs_fsblock_t new_startblock, xfs_extlen_t new_blockcount),
-	TP_ARGS(ip, irec, new_startblock, new_blockcount),
+	TP_PROTO(struct xfs_inode *ip, const struct xfs_bmbt_irec *got,
+		 const struct xfs_bmbt_irec *rep),
+	TP_ARGS(ip, got, rep),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
 		__field(xfs_ino_t, ino)
@@ -2682,28 +2682,34 @@ TRACE_EVENT(xrep_cow_replace_mapping,
 		__field(xfs_fileoff_t, startoff)
 		__field(xfs_filblks_t, blockcount)
 		__field(xfs_exntst_t, state)
+		__field(xfs_fileoff_t, new_startoff)
 		__field(xfs_fsblock_t, new_startblock)
 		__field(xfs_extlen_t, new_blockcount)
+		__field(xfs_exntst_t, new_state)
 	),
 	TP_fast_assign(
 		__entry->dev = ip->i_mount->m_super->s_dev;
 		__entry->ino = I_INO(ip);
-		__entry->startoff = irec->br_startoff;
-		__entry->startblock = irec->br_startblock;
-		__entry->blockcount = irec->br_blockcount;
-		__entry->state = irec->br_state;
-		__entry->new_startblock = new_startblock;
-		__entry->new_blockcount = new_blockcount;
+		__entry->startoff = got->br_startoff;
+		__entry->startblock = got->br_startblock;
+		__entry->blockcount = got->br_blockcount;
+		__entry->state = got->br_state;
+		__entry->new_startoff = rep->br_startoff;
+		__entry->new_startblock = rep->br_startblock;
+		__entry->new_blockcount = rep->br_blockcount;
+		__entry->new_state = rep->br_state;
 	),
-	TP_printk("dev %d:%d ino 0x%llx startoff 0x%llx startblock 0x%llx fsbcount 0x%llx state 0x%x new_startblock 0x%llx new_fsbcount 0x%x",
+	TP_printk("dev %d:%d ino 0x%llx startoff 0x%llx startblock 0x%llx fsbcount 0x%llx state 0x%x new_startoff 0x%llx new_startblock 0x%llx new_fsbcount 0x%x new_state 0x%x",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __entry->startoff,
 		  __entry->startblock,
 		  __entry->blockcount,
 		  __entry->state,
+		  __entry->new_startoff,
 		  __entry->new_startblock,
-		  __entry->new_blockcount)
+		  __entry->new_blockcount,
+		  __entry->new_state)
 );
 
 TRACE_EVENT(xrep_cow_free_staging,
diff --git a/fs/xfs/scrub/cow_repair.c b/fs/xfs/scrub/cow_repair.c
index 0075b6d5a1b5ff..d2f1ae74dd8013 100644
--- a/fs/xfs/scrub/cow_repair.c
+++ b/fs/xfs/scrub/cow_repair.c
@@ -80,12 +80,6 @@ struct xrep_cow {
 	unsigned int		next_bno;
 };
 
-/* CoW staging extent. */
-struct xrep_cow_extent {
-	xfs_fsblock_t		fsbno;
-	xfs_extlen_t		len;
-};
-
 /*
  * Mark the part of the file range that corresponds to the given physical
  * space.  Caller must ensure that the physical range is within xc->irec.
@@ -401,22 +395,21 @@ xrep_cow_find_bad_rt(
 STATIC int
 xrep_cow_alloc(
 	struct xfs_scrub	*sc,
-	xfs_extlen_t		maxlen,
-	struct xrep_cow_extent	*repl)
+	struct xfs_bmbt_irec	*del)
 {
 	struct xfs_alloc_arg	args = {
 		.tp		= sc->tp,
 		.mp		= sc->mp,
 		.oinfo		= XFS_RMAP_OINFO_SKIP_UPDATE,
 		.minlen		= 1,
-		.maxlen		= maxlen,
+		.maxlen		= del->br_blockcount,
 		.prod		= 1,
 		.resv		= XFS_AG_RESV_NONE,
 		.datatype	= XFS_ALLOC_USERDATA,
 	};
 	int			error;
 
-	error = xfs_trans_reserve_more(sc->tp, maxlen, 0);
+	error = xfs_trans_reserve_more(sc->tp, del->br_blockcount, 0);
 	if (error)
 		return error;
 
@@ -428,8 +421,8 @@ xrep_cow_alloc(
 
 	xfs_refcount_alloc_cow_extent(sc->tp, false, args.fsbno, args.len);
 
-	repl->fsbno = args.fsbno;
-	repl->len = args.len;
+	del->br_startblock = args.fsbno;
+	del->br_blockcount = args.len;
 	return 0;
 }
 
@@ -440,10 +433,12 @@ xrep_cow_alloc(
 STATIC int
 xrep_cow_alloc_rt(
 	struct xfs_scrub	*sc,
-	xfs_extlen_t		maxlen,
-	struct xrep_cow_extent	*repl)
+	struct xfs_bmbt_irec	*del)
 {
-	xfs_rtxlen_t		maxrtx = xfs_rtb_to_rtx(sc->mp, maxlen);
+	xfs_fsblock_t		fsbno;
+	xfs_rtxlen_t		maxrtx = xfs_rtb_to_rtx(sc->mp,
+							del->br_blockcount);
+	xfs_extlen_t		len;
 	int			error;
 
 	error = xfs_trans_reserve_more(sc->tp, 0, maxrtx);
@@ -451,11 +446,14 @@ xrep_cow_alloc_rt(
 		return error;
 
 	error = xfs_rtallocate_rtgs(sc->tp, NULLRTBLOCK, 1, maxrtx, 1, false,
-			false, &repl->fsbno, &repl->len);
+			false, &fsbno, &len);
 	if (error)
 		return error;
 
-	xfs_refcount_alloc_cow_extent(sc->tp, true, repl->fsbno, repl->len);
+	xfs_refcount_alloc_cow_extent(sc->tp, true, fsbno, len);
+
+	del->br_startblock = fsbno;
+	del->br_blockcount = len;
 	return 0;
 }
 
@@ -469,19 +467,19 @@ static inline int
 xrep_cow_find_mapping(
 	struct xrep_cow		*xc,
 	struct xfs_iext_cursor	*icur,
-	xfs_fileoff_t		startoff,
-	struct xfs_bmbt_irec	*got)
+	xfs_fileoff_t		badoff,
+	xfs_extlen_t		badlen,
+	struct xfs_bmbt_irec	*got,
+	struct xfs_bmbt_irec	*rep)
 {
 	struct xfs_inode	*ip = xc->sc->ip;
 	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, XFS_COW_FORK);
 
-	if (!xfs_iext_lookup_extent(ip, ifp, startoff, icur, got))
+	if (!xfs_iext_lookup_extent(ip, ifp, badoff, icur, got))
 		goto bad;
+	memcpy(rep, got, sizeof(*rep));
 
-	if (got->br_startoff > startoff)
-		goto bad;
-
-	if (got->br_blockcount == 0)
+	if (got->br_startoff > badoff)
 		goto bad;
 
 	if (isnullstartblock(got->br_startblock))
@@ -490,6 +488,24 @@ xrep_cow_find_mapping(
 	if (xfs_bmap_is_written_extent(got))
 		goto bad;
 
+	if (got->br_startoff < badoff) {
+		const int64_t	delta = badoff - got->br_startoff;
+
+		rep->br_blockcount -= delta;
+		rep->br_startoff += delta;
+		rep->br_startblock += delta;
+	}
+
+	if (got->br_startoff + got->br_blockcount > badoff + badlen) {
+		const int64_t	delta = (got->br_startoff + got->br_blockcount) -
+					(badoff + badlen);
+
+		rep->br_blockcount -= delta;
+	}
+
+	if (got->br_blockcount == 0)
+		goto bad;
+
 	return 0;
 bad:
 	ASSERT(0);
@@ -500,46 +516,92 @@ xrep_cow_find_mapping(
 #define REPLACE_RIGHT_SIDE	(1U << 1)
 
 /*
- * Given a CoW fork mapping @got and a replacement mapping @repl, remap the
- * beginning of @got with the space described by @rep.
+ * Given a CoW fork mapping @got and a replacement mapping @rep, map the space
+ * described by @rep into the cow fork, pushing aside @got as necessary.  @icur
+ * must point to iext tree leaf containing @got.
  */
 static inline void
 xrep_cow_replace_mapping(
-	struct xfs_inode		*ip,
-	struct xfs_iext_cursor		*icur,
-	const struct xfs_bmbt_irec	*got,
-	const struct xrep_cow_extent	*repl)
+	struct xfs_inode	*ip,
+	struct xfs_iext_cursor	*icur,
+	struct xfs_bmbt_irec	*got,
+	struct xfs_bmbt_irec	*rep)
 {
-	struct xfs_bmbt_irec		new = *got; /* struct copy */
+	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, XFS_COW_FORK);
+	xfs_fileoff_t		rep_endoff =
+			rep->br_startoff + rep->br_blockcount;
+	xfs_fileoff_t		got_endoff =
+			got->br_startoff + got->br_blockcount;
+	uint32_t		state = BMAP_COWFORK;
 
-	ASSERT(repl->len > 0);
+	ASSERT(rep->br_blockcount > 0);
 	ASSERT(!isnullstartblock(got->br_startblock));
+	ASSERT(got->br_startoff <= rep->br_startoff);
+	ASSERT(got_endoff >= rep_endoff);
 
-	trace_xrep_cow_replace_mapping(ip, got, repl->fsbno, repl->len);
+	trace_xrep_cow_replace_mapping(ip, got, rep);
 
-	if (got->br_blockcount == repl->len) {
+	if (got->br_startoff == rep->br_startoff)
+		state |= BMAP_LEFT_FILLING;
+	if (got_endoff == rep_endoff)
+		state |= BMAP_RIGHT_FILLING;
+
+	switch (state & (BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING)) {
+	case BMAP_LEFT_FILLING | BMAP_RIGHT_FILLING:
+		/*
+		 * Replacement matches the whole mapping, update the record.
+		 */
+		xfs_iext_update_extent(ip, state, icur, rep);
+		break;
+	case BMAP_LEFT_FILLING:
+		/*
+		 * Replace the first part of the mapping: Update the cursor
+		 * position with the new mapping, then add a record with the
+		 * tail of the old mapping.
+		 */
+		got->br_startoff = rep_endoff;
+		got->br_blockcount -= rep->br_blockcount;
+		got->br_startblock += rep->br_blockcount;
+
+		xfs_iext_update_extent(ip, state, icur, rep);
+		xfs_iext_next(ifp, icur);
+		xfs_iext_insert(ip, icur, got, state);
+		break;
+	case BMAP_RIGHT_FILLING:
 		/*
-		 * The new extent is a complete replacement for the existing
-		 * extent.  Update the COW fork record.
+		 * Replacing the last part of the mapping.  Shorten the current
+		 * mapping then add a record with the new mapping.
 		 */
-		new.br_startblock = repl->fsbno;
-		xfs_iext_update_extent(ip, BMAP_COWFORK, icur, &new);
-		return;
+		got->br_blockcount -= rep->br_blockcount;
+
+		xfs_iext_update_extent(ip, state, icur, got);
+		xfs_iext_next(ifp, icur);
+		xfs_iext_insert(ip, icur, rep, state);
+		break;
+	case 0:
+		/*
+		 * Replacing the middle of the extent.  Shorten the current
+		 * mapping, add a new record with the new mapping, and add a
+		 * second new record with the tail of the old mapping.
+		 */
+		got->br_blockcount = rep->br_startoff - got->br_startoff;
+
+		struct xfs_bmbt_irec	new = {
+			.br_startoff	= rep_endoff,
+			.br_blockcount	= got_endoff - rep_endoff,
+			.br_state	= got->br_state,
+			.br_startblock	= got->br_startblock +
+						rep->br_blockcount +
+						got->br_blockcount,
+		};
+
+		xfs_iext_update_extent(ip, state, icur, got);
+		xfs_iext_next(ifp, icur);
+		xfs_iext_insert(ip, icur, rep, state);
+		xfs_iext_next(ifp, icur);
+		xfs_iext_insert(ip, icur, &new, state);
+		break;
 	}
-
-	/*
-	 * The new extent can replace the beginning of the COW fork record.
-	 * Move the left side of @got upwards, then insert the new record.
-	 */
-	new.br_startoff += repl->len;
-	new.br_startblock += repl->len;
-	new.br_blockcount -= repl->len;
-	xfs_iext_update_extent(ip, BMAP_COWFORK, icur, &new);
-
-	new.br_startoff = got->br_startoff;
-	new.br_startblock = repl->fsbno;
-	new.br_blockcount = repl->len;
-	xfs_iext_insert(ip, icur, &new, BMAP_COWFORK);
 }
 
 /*
@@ -553,33 +615,30 @@ xrep_cow_replace_range(
 	xfs_extlen_t		*blockcount)
 {
 	struct xfs_iext_cursor	icur;
-	struct xrep_cow_extent	repl;
-	struct xfs_bmbt_irec	got;
+	struct xfs_bmbt_irec	got, rep;
 	struct xfs_scrub	*sc = xc->sc;
-	xfs_fileoff_t		nextoff;
-	xfs_extlen_t		alloc_len;
+	xfs_fsblock_t		old_fsbno;
 	int			error;
 
 	/*
-	 * Put the existing CoW fork mapping in @got.  If @got ends before
-	 * @rep, truncate @rep so we only replace one extent mapping at a time.
+	 * Put the existing CoW fork mapping in @got, and put in @rep the
+	 * contents of @got trimmed to @startoff/@blockcount.  We only want
+	 * to replace the bad region, and only one mapping at a time.
 	 */
-	error = xrep_cow_find_mapping(xc, &icur, startoff, &got);
+	error = xrep_cow_find_mapping(xc, &icur, startoff, *blockcount, &got,
+			&rep);
 	if (error)
 		return error;
-	nextoff = min(startoff + *blockcount,
-		      got.br_startoff + got.br_blockcount);
+	old_fsbno = rep.br_startblock;
 
 	/*
 	 * Allocate a replacement extent.  If we don't fill all the blocks,
 	 * shorten the quantity that will be deleted in this step.
 	 */
-	alloc_len = min_t(xfs_fileoff_t, XFS_MAX_BMBT_EXTLEN,
-			  nextoff - startoff);
 	if (XFS_IS_REALTIME_INODE(sc->ip))
-		error = xrep_cow_alloc_rt(sc, alloc_len, &repl);
+		error = xrep_cow_alloc_rt(sc, &rep);
 	else
-		error = xrep_cow_alloc(sc, alloc_len, &repl);
+		error = xrep_cow_alloc(sc, &rep);
 	if (error)
 		return error;
 
@@ -587,7 +646,7 @@ xrep_cow_replace_range(
 	 * Replace the old mapping with the new one, and commit the metadata
 	 * changes made so far.
 	 */
-	xrep_cow_replace_mapping(sc->ip, &icur, &got, &repl);
+	xrep_cow_replace_mapping(sc->ip, &icur, &got, &rep);
 
 	xfs_inode_set_cowblocks_tag(sc->ip);
 	error = xfs_defer_finish(&sc->tp);
@@ -596,15 +655,15 @@ xrep_cow_replace_range(
 
 	/* Note the old CoW staging extents; we'll reap them all later. */
 	if (XFS_IS_REALTIME_INODE(sc->ip))
-		error = xrtb_bitmap_set(&xc->old_cowfork_rtblocks,
-				got.br_startblock, repl.len);
+		error = xrtb_bitmap_set(&xc->old_cowfork_rtblocks, old_fsbno,
+				rep.br_blockcount);
 	else
-		error = xfsb_bitmap_set(&xc->old_cowfork_fsblocks,
-				got.br_startblock, repl.len);
+		error = xfsb_bitmap_set(&xc->old_cowfork_fsblocks, old_fsbno,
+				rep.br_blockcount);
 	if (error)
 		return error;
 
-	*blockcount = repl.len;
+	*blockcount = rep.br_blockcount;
 	return 0;
 }
 

[PATCH 2/6] xfs: don't wrap around quota ids in dqiterate

[Darrick J. Wong]

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

LOLLM noticed that q_id is an unsigned 32-bit variable.  If it happens
to be set to XFS_DQ_ID_MAX due to a filesystem that actually has a dquot
for ID_MAX, then this addition will truncate to zero and the iteration
starts over.  Fix this by casting to u64.

Cc: <stable@vger.kernel.org> # v6.8
Fixes: 21d7500929c8a0 ("xfs: improve dquot iteration for scrub")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/dqiterate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/dqiterate.c b/fs/xfs/scrub/dqiterate.c
index 10950e4bd4c3c0..079dc4e691a01a 100644
--- a/fs/xfs/scrub/dqiterate.c
+++ b/fs/xfs/scrub/dqiterate.c
@@ -205,7 +205,7 @@ xchk_dquot_iter(
 	if (error)
 		return error;
 
-	cursor->id = dq->q_id + 1;
+	cursor->id = (uint64_t)dq->q_id + 1;
 	*dqpp = dq;
 	return 1;
 }

[PATCH 3/6] xfs: use rtrefcount btree cursor in xchk_xref_is_rt_cow_staging

[Darrick J. Wong]

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

LOLLM points out that we pass the wrong btree cursor here.  We want the
rtrefcount btree cursor, not the non-rt one.  This is fairly benign
since it only affects tracing data.

Cc: <stable@vger.kernel.org> # v6.14
Fixes: 91683bb3f264c0 ("xfs: cross-reference checks with the rt refcount btree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/rtrefcount.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
index 0d10ce2910c2cb..4e7c540c8d2307 100644
--- a/fs/xfs/scrub/rtrefcount.c
+++ b/fs/xfs/scrub/rtrefcount.c
@@ -607,7 +607,7 @@ xchk_xref_is_rt_cow_staging(
 
 	/* CoW lookup returned a shared extent record? */
 	if (rc.rc_domain != XFS_REFC_DOMAIN_COW)
-		xchk_btree_xref_set_corrupt(sc, sc->sa.refc_cur, 0);
+		xchk_btree_xref_set_corrupt(sc, sc->sr.refc_cur, 0);
 
 	/* Must be at least as long as what was passed in */
 	if (rc.rc_blockcount < len)

[PATCH 4/6] xfs: use the rt version of the cow staging checker

[Darrick J. Wong]

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

LOLLM also noticed that xchk_rtrmapbt_xref ought to be using the rtdev
version of the "is this a cow extent?" helper function, not the datadev
one.

Cc: <stable@vger.kernel.org> # v6.14
Fixes: 91683bb3f264c0 ("xfs: cross-reference checks with the rt refcount btree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/rtrmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/rtrmap.c b/fs/xfs/scrub/rtrmap.c
index 043be93c714884..b3b2cf17ba2c08 100644
--- a/fs/xfs/scrub/rtrmap.c
+++ b/fs/xfs/scrub/rtrmap.c
@@ -209,7 +209,7 @@ xchk_rtrmapbt_xref(
 			xfs_rgbno_to_rtb(sc->sr.rtg, irec->rm_startblock),
 			irec->rm_blockcount);
 	if (irec->rm_owner == XFS_RMAP_OWN_COW)
-		xchk_xref_is_cow_staging(sc, irec->rm_startblock,
+		xchk_xref_is_rt_cow_staging(sc, irec->rm_startblock,
 				irec->rm_blockcount);
 	else
 		xchk_rtrmapbt_xref_rtrefc(sc, irec);

[PATCH 5/6] xfs: write the rg superblock when fixing it

[Darrick J. Wong]

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

The rtgroup superblock fixer should write the rtgroup superblock.
LOLLM noticed this, oops. :/

Cc: <stable@vger.kernel.org> # v6.13
Fixes: 1433f8f9cead37 ("xfs: repair realtime group superblock")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/rgsuper.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/fs/xfs/scrub/rgsuper.c b/fs/xfs/scrub/rgsuper.c
index 482f899a518a85..a52d37c33ca15a 100644
--- a/fs/xfs/scrub/rgsuper.c
+++ b/fs/xfs/scrub/rgsuper.c
@@ -80,9 +80,13 @@ int
 xrep_rgsuperblock(
 	struct xfs_scrub	*sc)
 {
+	struct xfs_buf		*sb_bp;
+
 	ASSERT(rtg_rgno(sc->sr.rtg) == 0);
 
 	xfs_log_sb(sc->tp);
+	sb_bp = xfs_trans_getsb(sc->tp);
+	xfs_log_rtsb(sc->tp, sb_bp);
 	return 0;
 }
 #endif /* CONFIG_XFS_ONLINE_REPAIR */

[PATCH 6/6] xfs: grab rtrmap btree when checking rgsuper

[Darrick J. Wong]

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

LOLLM noticed that we aren't grabbing the rtrmap btree when we check the
realtime group superblock.  As a result, none of the cross-referencing
checks have ever run.  Fix this.

Cc: <stable@vger.kernel.org> # v6.14
Fixes: 428e4884656db9 ("xfs: allow queued realtime intents to drain before scrubbing")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/rgsuper.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/rgsuper.c b/fs/xfs/scrub/rgsuper.c
index a52d37c33ca15a..c92212d8eb11b0 100644
--- a/fs/xfs/scrub/rgsuper.c
+++ b/fs/xfs/scrub/rgsuper.c
@@ -23,6 +23,8 @@ int
 xchk_setup_rgsuperblock(
 	struct xfs_scrub	*sc)
 {
+	if (xchk_need_intent_drain(sc))
+		xchk_fsgates_enable(sc, XCHK_FSGATES_DRAIN);
 	return xchk_trans_alloc(sc, 0);
 }
 
@@ -43,6 +45,7 @@ xchk_rgsuperblock(
 	struct xfs_scrub	*sc)
 {
 	xfs_rgnumber_t		rgno = sc->sm->sm_agno;
+	unsigned int		flags;
 	int			error;
 
 	/*
@@ -63,7 +66,12 @@ xchk_rgsuperblock(
 	if (!xchk_xref_process_error(sc, 0, 0, &error))
 		return error;
 
-	error = xchk_rtgroup_lock(sc, &sc->sr, XFS_RTGLOCK_BITMAP_SHARED);
+	if (xfs_has_rtrmapbt(sc->mp))
+		flags = XFS_RTGLOCK_BITMAP | XFS_RTGLOCK_RMAP;
+	else
+		flags = XFS_RTGLOCK_BITMAP_SHARED;
+
+	error = xchk_rtgroup_lock(sc, &sc->sr, flags);
 	if (error)
 		return error;
 

Re: [PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:03:24PM -0700, Darrick J. Wong wrote:
>  static inline void
>  xrep_cow_replace_mapping(
> +	struct xfs_inode	*ip,
> +	struct xfs_iext_cursor	*icur,
> +	struct xfs_bmbt_irec	*got,
> +	struct xfs_bmbt_irec	*rep)

This looks like something that should sit in xfs_bmap.c (or at
least xfs_bmap_util.c) and not in random scrub code.

And I really wonder how we can get good test coverage for this, including
for the original bug that this tries to address.

Re: [PATCH 2/6] xfs: don't wrap around quota ids in dqiterate

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:03:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM noticed that q_id is an unsigned 32-bit variable.  If it happens
> to be set to XFS_DQ_ID_MAX due to a filesystem that actually has a dquot
> for ID_MAX, then this addition will truncate to zero and the iteration
> starts over.  Fix this by casting to u64.

Looks good:

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

Re: [PATCH 3/6] xfs: use rtrefcount btree cursor in xchk_xref_is_rt_cow_staging

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:03:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM points out that we pass the wrong btree cursor here.  We want the
> rtrefcount btree cursor, not the non-rt one.  This is fairly benign
> since it only affects tracing data.

Looks good:

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

Re: [PATCH 4/6] xfs: use the rt version of the cow staging checker

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:04:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM also noticed that xchk_rtrmapbt_xref ought to be using the rtdev
> version of the "is this a cow extent?" helper function, not the datadev
> one.

Looks good:

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

Re: [PATCH 6/6] xfs: grab rtrmap btree when checking rgsuper

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:04:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM noticed that we aren't grabbing the rtrmap btree when we check the
> realtime group superblock.  As a result, none of the cross-referencing
> checks have ever run.  Fix this.

Looks good:

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

Re: [PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Darrick J. Wong]

On Wed, Jul 08, 2026 at 11:12:20AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 07, 2026 at 10:03:24PM -0700, Darrick J. Wong wrote:
> >  static inline void
> >  xrep_cow_replace_mapping(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_iext_cursor	*icur,
> > +	struct xfs_bmbt_irec	*got,
> > +	struct xfs_bmbt_irec	*rep)
> 
> This looks like something that should sit in xfs_bmap.c (or at
> least xfs_bmap_util.c) and not in random scrub code.
> 
> And I really wonder how we can get good test coverage for this, including
> for the original bug that this tries to address.

I'll post the debug patch I made that hijacks the errortag knob to make
it randomly shrink the range passed to mark_cow_range instead of the
entire iext record.

The original bug that this tries to address was a pile of bug fixes to
the xfs cow code that you posted for 5.1 that we neglected to backport
to 4.14, resulting in corruption of the cow fork mappings.

--D

Re: [PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Darrick J. Wong]

On Wed, Jul 08, 2026 at 08:25:16AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 08, 2026 at 11:12:20AM +0200, Christoph Hellwig wrote:
> > On Tue, Jul 07, 2026 at 10:03:24PM -0700, Darrick J. Wong wrote:
> > >  static inline void
> > >  xrep_cow_replace_mapping(
> > > +	struct xfs_inode	*ip,
> > > +	struct xfs_iext_cursor	*icur,
> > > +	struct xfs_bmbt_irec	*got,
> > > +	struct xfs_bmbt_irec	*rep)
> > 
> > This looks like something that should sit in xfs_bmap.c (or at
> > least xfs_bmap_util.c) and not in random scrub code.

To push back on this: there's only one user of this, why not keep it
local to the scrub code?  AFAICT there's nowhere else in xfs that needs
to replace a subset of an existing cow fork mapping with another
mapping.

--D

> > And I really wonder how we can get good test coverage for this, including
> > for the original bug that this tries to address.
> 
> I'll post the debug patch I made that hijacks the errortag knob to make
> it randomly shrink the range passed to mark_cow_range instead of the
> entire iext record.
> 
> The original bug that this tries to address was a pile of bug fixes to
> the xfs cow code that you posted for 5.1 that we neglected to backport
> to 4.14, resulting in corruption of the cow fork mappings.
> 
> --D
> 

Re: [PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Christoph Hellwig]

On Wed, Jul 08, 2026 at 10:57:43AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 08, 2026 at 08:25:16AM -0700, Darrick J. Wong wrote:
> > On Wed, Jul 08, 2026 at 11:12:20AM +0200, Christoph Hellwig wrote:
> > > On Tue, Jul 07, 2026 at 10:03:24PM -0700, Darrick J. Wong wrote:
> > > >  static inline void
> > > >  xrep_cow_replace_mapping(
> > > > +	struct xfs_inode	*ip,
> > > > +	struct xfs_iext_cursor	*icur,
> > > > +	struct xfs_bmbt_irec	*got,
> > > > +	struct xfs_bmbt_irec	*rep)
> > > 
> > > This looks like something that should sit in xfs_bmap.c (or at
> > > least xfs_bmap_util.c) and not in random scrub code.
> 
> To push back on this: there's only one user of this, why not keep it
> local to the scrub code?  AFAICT there's nowhere else in xfs that needs
> to replace a subset of an existing cow fork mapping with another
> mapping.

Mostly so that all the code doing hairy operations on subsets of
bmbt_irecs is kept together...

Re: [PATCH 1/6] xfs: don't replace the wrong part of the cow fork

[Darrick J. Wong]

On Thu, Jul 09, 2026 at 06:47:38AM +0200, Christoph Hellwig wrote:
> On Wed, Jul 08, 2026 at 10:57:43AM -0700, Darrick J. Wong wrote:
> > On Wed, Jul 08, 2026 at 08:25:16AM -0700, Darrick J. Wong wrote:
> > > On Wed, Jul 08, 2026 at 11:12:20AM +0200, Christoph Hellwig wrote:
> > > > On Tue, Jul 07, 2026 at 10:03:24PM -0700, Darrick J. Wong wrote:
> > > > >  static inline void
> > > > >  xrep_cow_replace_mapping(
> > > > > +	struct xfs_inode	*ip,
> > > > > +	struct xfs_iext_cursor	*icur,
> > > > > +	struct xfs_bmbt_irec	*got,
> > > > > +	struct xfs_bmbt_irec	*rep)
> > > > 
> > > > This looks like something that should sit in xfs_bmap.c (or at
> > > > least xfs_bmap_util.c) and not in random scrub code.
> > 
> > To push back on this: there's only one user of this, why not keep it
> > local to the scrub code?  AFAICT there's nowhere else in xfs that needs
> > to replace a subset of an existing cow fork mapping with another
> > mapping.
> 
> Mostly so that all the code doing hairy operations on subsets of
> bmbt_irecs is kept together...

Ok.  I'll do the move as a separate patch though.

--D

Re: [PATCH 5/6] xfs: write the rg superblock when fixing it

[Christoph Hellwig]

On Tue, Jul 07, 2026 at 10:04:26PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The rtgroup superblock fixer should write the rtgroup superblock.
> LOLLM noticed this, oops. :/

Looks good:

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

Can we get a testcase to verify this?

Re: [PATCH 5/6] xfs: write the rg superblock when fixing it

[Darrick J. Wong]

On Mon, Jul 13, 2026 at 08:41:05AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 07, 2026 at 10:04:26PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The rtgroup superblock fixer should write the rtgroup superblock.
> > LOLLM noticed this, oops. :/
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Can we get a testcase to verify this?

Hrmm.  Right now both superblock scrubbers don't do much for group 0,
because both buffers are pinned to the xfs_mount, so they assume that
there's no need to check anything.  However, the ondisk super could have
gotten corrupted (or blown away by fdisk), in which case an immediate
crash could render the filesystem unmountable.

So, I could (a) teach the super scrubbers to read the primary / rt
super; and (b) teach them both to log the superblock and bwrite it
immediately to shorten the window in which this could happen.

What do you think?

Also, this patch should be calling xfs_log_sb *after* xfs_trans_getsb,
so I'll fix that for the repost.

--D

Re: [PATCH 5/6] xfs: write the rg superblock when fixing it

[Christoph Hellwig]

On Mon, Jul 13, 2026 at 02:58:57PM -0700, Darrick J. Wong wrote:
> Hrmm.  Right now both superblock scrubbers don't do much for group 0,
> because both buffers are pinned to the xfs_mount, so they assume that
> there's no need to check anything.  However, the ondisk super could have
> gotten corrupted (or blown away by fdisk), in which case an immediate
> crash could render the filesystem unmountable.
> 
> So, I could (a) teach the super scrubbers to read the primary / rt
> super; and (b) teach them both to log the superblock and bwrite it
> immediately to shorten the window in which this could happen.
> 
> What do you think?

I guess the only sensible time to scrub the sb would be early during
mount? 

> Also, this patch should be calling xfs_log_sb *after* xfs_trans_getsb,
> so I'll fix that for the repost.

Oops.