[PATCHSET 5/5] xfs: LLM-inspired bug fixes, part 9

[PATCHSET 5/5] xfs: LLM-inspired bug fixes, part 9

[Darrick J. Wong]

Hi all,

Here's a ninth batch of xfs fixes resulting from a LLaMma.  Mwa mwa
mwa...

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-9
---
Commits in this patchset:
 * xfs: signal inode btree xref error if get_rec returns an error
 * xfs: truncate quota file correctly when repairing quota file
 * xfs: compute dquot checksum after resetting dd_lsn in repair
 * xfs: fix backwards skipping logic in xrep_quota_block
 * xfs: fix backwards mergeability logic in refcount scrubber
---
 fs/xfs/scrub/ialloc.c       |    4 ++++
 fs/xfs/scrub/quota_repair.c |    7 +++----
 fs/xfs/scrub/refcount.c     |    2 +-
 fs/xfs/scrub/rtrefcount.c   |    2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

Unreviewed patches in this series are:

[PATCHSET 5/5] xfs: LLM-inspired bug fixes, part 9
  [PATCH 1/5] xfs: signal inode btree xref error if get_rec returns an
  [PATCH 2/5] xfs: truncate quota file correctly when repairing quota
  [PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in
  [PATCH 4/5] xfs: fix backwards skipping logic in xrep_quota_block
  [PATCH 5/5] xfs: fix backwards mergeability logic in refcount

[PATCH 1/5] xfs: signal inode btree xref error if get_rec returns an error

[Darrick J. Wong]

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

LOLLM points out that xchk_finobt_xref_inobt and xchk_inobt_xref_finobt
both ignore errors being returned from the xfs_btree_get_rec function
and proceed with a (possibly stale) "true" value for has_record.  If the
*simple* btree record checks fail during cross-referencing, we can
immediately conclude that there's a cross-referncing error in the other
btree.  On those grounds, we can bubble up the returned error instead of
wasting time cross-referencing with garbage.

Cc: <stable@vger.kernel.org> # v6.4
Fixes: bc0f3b55467e1b ("xfs: directly cross-reference the inode btrees with each other")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/ialloc.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/fs/xfs/scrub/ialloc.c b/fs/xfs/scrub/ialloc.c
index 19c0b1b2a787bb..9270ad075fe0a2 100644
--- a/fs/xfs/scrub/ialloc.c
+++ b/fs/xfs/scrub/ialloc.c
@@ -85,6 +85,8 @@ xchk_inobt_xref_finobt(
 		goto no_record;
 
 	error = xfs_inobt_get_rec(cur, &frec, &has_record);
+	if (error)
+		return error;
 	if (!has_record)
 		return -EFSCORRUPTED;
 
@@ -188,6 +190,8 @@ xchk_finobt_xref_inobt(
 		goto no_record;
 
 	error = xfs_inobt_get_rec(cur, &irec, &has_record);
+	if (error)
+		return error;
 	if (!has_record)
 		return -EFSCORRUPTED;
 

[PATCH 2/5] xfs: truncate quota file correctly when repairing quota file

[Darrick J. Wong]

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

LOLLM noticed that xrep_quota_data_fork screws up the unit handling when
it computes the offset at which to start truncating the quota file.
max_dquid_off is the file block offset containing the highest possible
dquot, and xfs_bunmapi_range takes the starting file block offset.
Therefore, it makes no sense to multiply max_dquid_off by the blocksize;
all we need to do is start truncating at the next block.

Cc: <stable@vger.kernel.org> # v6.8
Fixes: a5b91555403e3a ("xfs: repair quotas")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/quota_repair.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 487bd4f68ebb2a..ca3ac6728339d1 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -455,8 +455,7 @@ xrep_quota_data_fork(
 
 	if (truncate) {
 		/* Erase everything after the block containing the max dquot */
-		error = xfs_bunmapi_range(&sc->tp, sc->ip, 0,
-				max_dqid_off * sc->mp->m_sb.sb_blocksize,
+		error = xfs_bunmapi_range(&sc->tp, sc->ip, 0, max_dqid_off + 1,
 				XFS_MAX_FILEOFF);
 		if (error)
 			goto out;

[PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Darrick J. Wong]

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

LOLLM complains that xrep_quota_block updates dd_lsn after calculating
the crc of the ondisk dquot.  That's clearly broken, so fix that.

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


diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index ca3ac6728339d1..9d0097c5656184 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -364,10 +364,10 @@ xrep_quota_block(
 				defq->rtb.time);
 
 		/* We only support v5 filesystems so always set these. */
+		dqblk->dd_lsn = 0;
 		uuid_copy(&dqblk->dd_uuid, &sc->mp->m_sb.sb_meta_uuid);
 		xfs_update_cksum((char *)dqblk, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
-		dqblk->dd_lsn = 0;
 	}
 	switch (dqtype) {
 	case XFS_DQTYPE_USER:

[PATCH 4/5] xfs: fix backwards skipping logic in xrep_quota_block

[Darrick J. Wong]

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

LOLLM complains about the logic in xrep_quota_block that skips
reinitializing the ondisk dquot if there aren't any problems that would
impede a dqiterate walk later.  I got the type checking logic backwards,
which is the source of the problem.  Fix that.

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


diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index 9d0097c5656184..be825448288ed8 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -325,7 +325,7 @@ xrep_quota_block(
 		 * If there's nothing that would impede a dqiterate, we're
 		 * done.
 		 */
-		if ((ddq->d_type & XFS_DQTYPE_REC_MASK) != dqtype ||
+		if ((ddq->d_type & XFS_DQTYPE_REC_MASK) == dqtype &&
 		    id == be32_to_cpu(ddq->d_id)) {
 			xfs_trans_brelse(sc->tp, bp);
 			return 0;

[PATCH 5/5] xfs: fix backwards mergeability logic in refcount scrubber

[Darrick J. Wong]

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

When we start the refcount or rtrefcount btree scanners, prev_rec is
initialized to all zeroes.  This is done so that the record mergeability
checks skip the first record because you must have two records to
compare.  Unfortunately, I got the logic backwards, so scrub has never
complained about mergeable refcountbt records.  Fix this bug that LOLLM
noticed.

Cc: <stable@vger.kernel.org> # v6.4
Fixes: db0502b39c21d1 ("xfs: flag refcount btree records that could be merged")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
---
 fs/xfs/scrub/refcount.c   |    2 +-
 fs/xfs/scrub/rtrefcount.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
index 4e1bf23e5b8972..f2addaf13c58e4 100644
--- a/fs/xfs/scrub/refcount.c
+++ b/fs/xfs/scrub/refcount.c
@@ -410,7 +410,7 @@ xchk_refcount_mergeable(
 	const struct xfs_refcount_irec	*r1 = &rrc->prev_rec;
 
 	/* Ignore if prev_rec is not yet initialized. */
-	if (r1->rc_blockcount > 0)
+	if (r1->rc_blockcount == 0)
 		return false;
 
 	if (r1->rc_domain != r2->rc_domain)
diff --git a/fs/xfs/scrub/rtrefcount.c b/fs/xfs/scrub/rtrefcount.c
index 4e7c540c8d2307..de100178f41c70 100644
--- a/fs/xfs/scrub/rtrefcount.c
+++ b/fs/xfs/scrub/rtrefcount.c
@@ -375,7 +375,7 @@ xchk_rtrefcount_mergeable(
 	const struct xfs_refcount_irec	*r1 = &rrc->prev_rec;
 
 	/* Ignore if prev_rec is not yet initialized. */
-	if (r1->rc_blockcount > 0)
+	if (r1->rc_blockcount == 0)
 		return false;
 
 	if (r1->rc_startblock + r1->rc_blockcount != r2->rc_startblock)

Re: [PATCH 1/5] xfs: signal inode btree xref error if get_rec returns an error

[Christoph Hellwig]

On Tue, Sep 01, 2026 at 10:48:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM points out that xchk_finobt_xref_inobt and xchk_inobt_xref_finobt
> both ignore errors being returned from the xfs_btree_get_rec function
> and proceed with a (possibly stale) "true" value for has_record.  If the
> *simple* btree record checks fail during cross-referencing, we can
> immediately conclude that there's a cross-referncing error in the other
> btree.  On those grounds, we can bubble up the returned error instead of
> wasting time cross-referencing with garbage.

Looks good:

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

Re: [PATCH 2/5] xfs: truncate quota file correctly when repairing quota file

[Christoph Hellwig]

On Tue, Sep 01, 2026 at 10:49:01PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM noticed that xrep_quota_data_fork screws up the unit handling when
> it computes the offset at which to start truncating the quota file.
> max_dquid_off is the file block offset containing the highest possible
> dquot, and xfs_bunmapi_range takes the starting file block offset.
> Therefore, it makes no sense to multiply max_dquid_off by the blocksize;
> all we need to do is start truncating at the next block.

Aka we never really truncate anything with the current code as the
offset ends up being crazy high...

The fix looks good:

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

Re: [PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Christoph Hellwig]

On Tue, Sep 01, 2026 at 10:49:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM complains that xrep_quota_block updates dd_lsn after calculating
> the crc of the ondisk dquot.  That's clearly broken, so fix that.

Yeah:

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

Also why do we set the lsn to 0, shouldn't that contain a valid value
(probably the current LSN, but I'd have to look deeper at what this
does in detail)

Re: [PATCH 4/5] xfs: fix backwards skipping logic in xrep_quota_block

[Christoph Hellwig]

On Tue, Sep 01, 2026 at 10:49:32PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM complains about the logic in xrep_quota_block that skips
> reinitializing the ondisk dquot if there aren't any problems that would
> impede a dqiterate walk later.  I got the type checking logic backwards,
> which is the source of the problem.  Fix that.

Looks good:

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

Re: [PATCH 5/5] xfs: fix backwards mergeability logic in refcount scrubber

[Christoph Hellwig]

On Tue, Sep 01, 2026 at 10:49:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When we start the refcount or rtrefcount btree scanners, prev_rec is
> initialized to all zeroes.  This is done so that the record mergeability
> checks skip the first record because you must have two records to
> compare.  Unfortunately, I got the logic backwards, so scrub has never
> complained about mergeable refcountbt records.  Fix this bug that LOLLM
> noticed.

Looks good:

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

Re: [PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Darrick J. Wong]

On Wed, Sep 02, 2026 at 09:23:41AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 01, 2026 at 10:49:16PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > LOLLM complains that xrep_quota_block updates dd_lsn after calculating
> > the crc of the ondisk dquot.  That's clearly broken, so fix that.
> 
> Yeah:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Also why do we set the lsn to 0, shouldn't that contain a valid value
> (probably the current LSN, but I'd have to look deeper at what this
> does in detail)

Hmm.  When I wrote this dquot repair code I was mirroring what
xfs_qm_init_dquot_blk does (memsets the new dquot buffer to zeroes,
doesn't explicitly touch dd_lsn).

It probably ought to be set to the current LSN but we don't really have
a dquot log item for determining that, and assigning 0 here means that
any subsequent logged dquot item will always be recovered into the
ondisk dquot that xrep_quota_block writes.

--D

Re: [PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Christoph Hellwig]

On Wed, Sep 02, 2026 at 08:37:03AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 02, 2026 at 09:23:41AM +0200, Christoph Hellwig wrote:
> > On Tue, Sep 01, 2026 at 10:49:16PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > LOLLM complains that xrep_quota_block updates dd_lsn after calculating
> > > the crc of the ondisk dquot.  That's clearly broken, so fix that.
> > 
> > Yeah:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > Also why do we set the lsn to 0, shouldn't that contain a valid value
> > (probably the current LSN, but I'd have to look deeper at what this
> > does in detail)
> 
> Hmm.  When I wrote this dquot repair code I was mirroring what
> xfs_qm_init_dquot_blk does (memsets the new dquot buffer to zeroes,
> doesn't explicitly touch dd_lsn).
> 
> It probably ought to be set to the current LSN but we don't really have
> a dquot log item for determining that, and assigning 0 here means that
> any subsequent logged dquot item will always be recovered into the
> ondisk dquot that xrep_quota_block writes.

Can we capture this for the future in comments somewhere?

Re: [PATCH 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Darrick J. Wong]

On Thu, Sep 03, 2026 at 07:35:17AM +0200, Christoph Hellwig wrote:
> On Wed, Sep 02, 2026 at 08:37:03AM -0700, Darrick J. Wong wrote:
> > On Wed, Sep 02, 2026 at 09:23:41AM +0200, Christoph Hellwig wrote:
> > > On Tue, Sep 01, 2026 at 10:49:16PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > LOLLM complains that xrep_quota_block updates dd_lsn after calculating
> > > > the crc of the ondisk dquot.  That's clearly broken, so fix that.
> > > 
> > > Yeah:
> > > 
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > 
> > > Also why do we set the lsn to 0, shouldn't that contain a valid value
> > > (probably the current LSN, but I'd have to look deeper at what this
> > > does in detail)
> > 
> > Hmm.  When I wrote this dquot repair code I was mirroring what
> > xfs_qm_init_dquot_blk does (memsets the new dquot buffer to zeroes,
> > doesn't explicitly touch dd_lsn).
> > 
> > It probably ought to be set to the current LSN but we don't really have
> > a dquot log item for determining that, and assigning 0 here means that
> > any subsequent logged dquot item will always be recovered into the
> > ondisk dquot that xrep_quota_block writes.
> 
> Can we capture this for the future in comments somewhere?

Will add that and repost.

--D

[PATCH v1.1 3/5] xfs: compute dquot checksum after resetting dd_lsn in repair

[Darrick J. Wong]

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

LOLLM complains that xrep_quota_block updates dd_lsn after calculating
the crc of the ondisk dquot.  That's clearly broken, so fix that.

Cc: <stable@vger.kernel.org> # v6.8
Fixes: a5b91555403e3a ("xfs: repair quotas")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Assisted-by: LOLLM # finding obvious bugs
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/quota_repair.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
index ca3ac6728339d1..5c22bb6ccffa4e 100644
--- a/fs/xfs/scrub/quota_repair.c
+++ b/fs/xfs/scrub/quota_repair.c
@@ -363,11 +363,18 @@ xrep_quota_block(
 				ddq->d_rtbcount, &ddq->d_rtbtimer,
 				defq->rtb.time);
 
+		/*
+		 * This transaction operates on raw disk buffers, so we don't
+		 * have a dquot log item to assign the LSN for us.  Instead,
+		 * set it to zero so that log recovery will always replay any
+		 * logged dquot item atop this buffer.
+		 */
+		dqblk->dd_lsn = 0;
+
 		/* We only support v5 filesystems so always set these. */
 		uuid_copy(&dqblk->dd_uuid, &sc->mp->m_sb.sb_meta_uuid);
 		xfs_update_cksum((char *)dqblk, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
-		dqblk->dd_lsn = 0;
 	}
 	switch (dqtype) {
 	case XFS_DQTYPE_USER: