[PATCHSET 0/3] xfs_repair: various small fixes

[PATCHSET 0/3] xfs_repair: various small fixes

[Darrick J. Wong]

Hi all,

Bug fixes for 5.16 include:

 - checking V5 feature bits in secondary superblocks against whatever we
   decide is the primary
 - consistently warning if repair can't check existing rmap and refcount
   btrees
 - checking the ftype of dot and dotdot entries

Enjoy!

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=repair-fixes
---
 repair/agheader.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 repair/phase4.c   |   20 +++---------
 repair/phase6.c   |   79 +++++++++++++++++++++++++++++++++---------------
 repair/rmap.c     |   65 +++++++++++++++++++++++++++------------
 repair/rmap.h     |    4 +-
 5 files changed, 194 insertions(+), 62 deletions(-)

[PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

[Darrick J. Wong]

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

Make sure we detect and correct mismatches between the V5 features
described in the primary and the secondary superblocks.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/agheader.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)


diff --git a/repair/agheader.c b/repair/agheader.c
index d8f912f2..650ffb2e 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c
@@ -220,6 +220,92 @@ compare_sb(xfs_mount_t *mp, xfs_sb_t *sb)
 	return(XR_OK);
 }
 
+/*
+ * If the fs feature bits on a secondary superblock don't match the
+ * primary, we need to update them.
+ */
+static inline int
+check_v5_feature_mismatch(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	struct xfs_sb		*sb)
+{
+	bool			dirty = false;
+
+	if (!xfs_has_crc(mp) || agno == 0)
+		return 0;
+
+	if (mp->m_sb.sb_features_compat != sb->sb_features_compat) {
+		if (no_modify) {
+			do_warn(
+	_("would fix compat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_compat,
+					sb->sb_features_compat);
+		} else {
+			do_warn(
+	_("will fix compat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_compat,
+					sb->sb_features_compat);
+			dirty = true;
+		}
+	}
+
+	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
+			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {
+		if (no_modify) {
+			do_warn(
+	_("would fix incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_incompat,
+					sb->sb_features_incompat);
+		} else {
+			do_warn(
+	_("will fix incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_incompat,
+					sb->sb_features_incompat);
+			dirty = true;
+		}
+	}
+
+	if (mp->m_sb.sb_features_ro_compat != sb->sb_features_ro_compat) {
+		if (no_modify) {
+			do_warn(
+	_("would fix ro compat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_ro_compat,
+					sb->sb_features_ro_compat);
+		} else {
+			do_warn(
+	_("will fix ro compat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_ro_compat,
+					sb->sb_features_ro_compat);
+			dirty = true;
+		}
+	}
+
+	if (mp->m_sb.sb_features_log_incompat != sb->sb_features_log_incompat) {
+		if (no_modify) {
+			do_warn(
+	_("would fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_log_incompat,
+					sb->sb_features_log_incompat);
+		} else {
+			do_warn(
+	_("will fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"),
+					agno, mp->m_sb.sb_features_log_incompat,
+					sb->sb_features_log_incompat);
+			dirty = true;
+		}
+	}
+
+	if (!dirty)
+		return 0;
+
+	sb->sb_features_compat = mp->m_sb.sb_features_compat;
+	sb->sb_features_ro_compat = mp->m_sb.sb_features_ro_compat;
+	sb->sb_features_incompat = mp->m_sb.sb_features_incompat;
+	sb->sb_features_log_incompat = mp->m_sb.sb_features_log_incompat;
+	return XR_AG_SB_SEC;
+}
+
 /*
  * Possible fields that may have been set at mkfs time,
  * sb_inoalignmt, sb_unit, sb_width and sb_dirblklog.
@@ -452,6 +538,8 @@ secondary_sb_whack(
 			rval |= XR_AG_SB_SEC;
 	}
 
+	rval |= check_v5_feature_mismatch(mp, i, sb);
+
 	if (xfs_sb_version_needsrepair(sb)) {
 		if (i == 0) {
 			if (!no_modify)

[PATCH 2/3] xfs_repair: improve checking of existing rmap and refcount btrees

[Darrick J. Wong]

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

There are a few deficiencies in the xfs_repair functions that check the
existing reverse mapping and reference count btrees.  First of all, we
don't report corruption or IO errors if we can't read the ondisk
metadata.  Second, we don't consistently warn if we cannot allocate
memory to perform the check.

Add the missing warnings, and simplify the calling convention since we
don't need the extra layer of do_error logging in phase4.c.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase4.c |   20 ++++-------------
 repair/rmap.c   |   65 ++++++++++++++++++++++++++++++++++++++-----------------
 repair/rmap.h   |    4 ++-
 3 files changed, 52 insertions(+), 37 deletions(-)


diff --git a/repair/phase4.c b/repair/phase4.c
index 2260f6a3..746e0ccd 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -173,11 +173,7 @@ _("unable to add AG %u metadata reverse-mapping data.\n"), agno);
 		do_error(
 _("unable to merge AG %u metadata reverse-mapping data.\n"), agno);
 
-	error = rmaps_verify_btree(wq->wq_ctx, agno);
-	if (error)
-		do_error(
-_("%s while checking reverse-mappings"),
-			 strerror(-error));
+	rmaps_verify_btree(wq->wq_ctx, agno);
 }
 
 static void
@@ -212,17 +208,11 @@ _("%s while fixing inode reflink flags.\n"),
 
 static void
 check_refcount_btrees(
-	struct workqueue*wq,
-	xfs_agnumber_t	agno,
-	void		*arg)
+	struct workqueue	*wq,
+	xfs_agnumber_t		agno,
+	void			*arg)
 {
-	int		error;
-
-	error = check_refcounts(wq->wq_ctx, agno);
-	if (error)
-		do_error(
-_("%s while checking reference counts"),
-			 strerror(-error));
+	check_refcounts(wq->wq_ctx, agno);
 }
 
 static void
diff --git a/repair/rmap.c b/repair/rmap.c
index e48f6c1e..b76a149d 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -974,7 +974,7 @@ rmap_is_good(
 /*
  * Compare the observed reverse mappings against what's in the ag btree.
  */
-int
+void
 rmaps_verify_btree(
 	struct xfs_mount	*mp,
 	xfs_agnumber_t		agno)
@@ -989,21 +989,26 @@ rmaps_verify_btree(
 	int			error;
 
 	if (!xfs_has_rmapbt(mp))
-		return 0;
+		return;
 	if (rmapbt_suspect) {
 		if (no_modify && agno == 0)
 			do_warn(_("would rebuild corrupt rmap btrees.\n"));
-		return 0;
+		return;
 	}
 
 	/* Create cursors to refcount structures */
 	error = rmap_init_cursor(agno, &rm_cur);
-	if (error)
-		return error;
+	if (error) {
+		do_warn(_("Not enough memory to check reverse mappings.\n"));
+		return;
+	}
 
 	error = -libxfs_alloc_read_agf(mp, NULL, agno, 0, &agbp);
-	if (error)
+	if (error) {
+		do_warn(_("Could not read AGF %u to check rmap btree.\n"),
+				agno);
 		goto err;
+	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
 	pag = libxfs_perag_get(mp, agno);
@@ -1011,15 +1016,20 @@ rmaps_verify_btree(
 
 	bt_cur = libxfs_rmapbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
-		error = -ENOMEM;
+		do_warn(_("Not enough memory to check reverse mappings.\n"));
 		goto err;
 	}
 
 	rm_rec = pop_slab_cursor(rm_cur);
 	while (rm_rec) {
 		error = rmap_lookup(bt_cur, rm_rec, &tmp, &have);
-		if (error)
+		if (error) {
+			do_warn(
+_("Could not read reverse-mapping record for (%u/%u).\n"),
+					agno, rm_rec->rm_startblock);
 			goto err;
+		}
+
 		/*
 		 * Using the range query is expensive, so only do it if
 		 * the regular lookup doesn't find anything or if it doesn't
@@ -1029,8 +1039,12 @@ rmaps_verify_btree(
 				(!have || !rmap_is_good(rm_rec, &tmp))) {
 			error = rmap_lookup_overlapped(bt_cur, rm_rec,
 					&tmp, &have);
-			if (error)
+			if (error) {
+				do_warn(
+_("Could not read reverse-mapping record for (%u/%u).\n"),
+						agno, rm_rec->rm_startblock);
 				goto err;
+			}
 		}
 		if (!have) {
 			do_warn(
@@ -1088,7 +1102,6 @@ _("Incorrect reverse-mapping: saw (%u/%u) %slen %u owner %"PRId64" %s%soff \
 	if (agbp)
 		libxfs_buf_relse(agbp);
 	free_slab_cursor(&rm_cur);
-	return 0;
 }
 
 /*
@@ -1335,7 +1348,7 @@ refcount_avoid_check(void)
 /*
  * Compare the observed reference counts against what's in the ag btree.
  */
-int
+void
 check_refcounts(
 	struct xfs_mount		*mp,
 	xfs_agnumber_t			agno)
@@ -1351,21 +1364,26 @@ check_refcounts(
 	int				error;
 
 	if (!xfs_has_reflink(mp))
-		return 0;
+		return;
 	if (refcbt_suspect) {
 		if (no_modify && agno == 0)
 			do_warn(_("would rebuild corrupt refcount btrees.\n"));
-		return 0;
+		return;
 	}
 
 	/* Create cursors to refcount structures */
 	error = init_refcount_cursor(agno, &rl_cur);
-	if (error)
-		return error;
+	if (error) {
+		do_warn(_("Not enough memory to check refcount data.\n"));
+		return;
+	}
 
 	error = -libxfs_alloc_read_agf(mp, NULL, agno, 0, &agbp);
-	if (error)
+	if (error) {
+		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
+				agno);
 		goto err;
+	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
 	pag = libxfs_perag_get(mp, agno);
@@ -1373,7 +1391,7 @@ check_refcounts(
 
 	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
-		error = -ENOMEM;
+		do_warn(_("Not enough memory to check refcount data.\n"));
 		goto err;
 	}
 
@@ -1382,8 +1400,12 @@ check_refcounts(
 		/* Look for a refcount record in the btree */
 		error = -libxfs_refcount_lookup_le(bt_cur,
 				rl_rec->rc_startblock, &have);
-		if (error)
+		if (error) {
+			do_warn(
+_("Could not read reference count record for (%u/%u).\n"),
+					agno, rl_rec->rc_startblock);
 			goto err;
+		}
 		if (!have) {
 			do_warn(
 _("Missing reference count record for (%u/%u) len %u count %u\n"),
@@ -1393,8 +1415,12 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
 		}
 
 		error = -libxfs_refcount_get_rec(bt_cur, &tmp, &i);
-		if (error)
+		if (error) {
+			do_warn(
+_("Could not read reference count record for (%u/%u).\n"),
+					agno, rl_rec->rc_startblock);
 			goto err;
+		}
 		if (!i) {
 			do_warn(
 _("Missing reference count record for (%u/%u) len %u count %u\n"),
@@ -1425,7 +1451,6 @@ _("Incorrect reference count: saw (%u/%u) len %u nlinks %u; should be (%u/%u) le
 	if (agbp)
 		libxfs_buf_relse(agbp);
 	free_slab_cursor(&rl_cur);
-	return 0;
 }
 
 /*
diff --git a/repair/rmap.h b/repair/rmap.h
index e5a6a3b4..8d176cb3 100644
--- a/repair/rmap.h
+++ b/repair/rmap.h
@@ -28,7 +28,7 @@ extern int rmap_store_ag_btree_rec(struct xfs_mount *, xfs_agnumber_t);
 extern size_t rmap_record_count(struct xfs_mount *, xfs_agnumber_t);
 extern int rmap_init_cursor(xfs_agnumber_t, struct xfs_slab_cursor **);
 extern void rmap_avoid_check(void);
-extern int rmaps_verify_btree(struct xfs_mount *, xfs_agnumber_t);
+void rmaps_verify_btree(struct xfs_mount *mp, xfs_agnumber_t agno);
 
 extern int64_t rmap_diffkeys(struct xfs_rmap_irec *kp1,
 		struct xfs_rmap_irec *kp2);
@@ -39,7 +39,7 @@ extern int compute_refcounts(struct xfs_mount *, xfs_agnumber_t);
 extern size_t refcount_record_count(struct xfs_mount *, xfs_agnumber_t);
 extern int init_refcount_cursor(xfs_agnumber_t, struct xfs_slab_cursor **);
 extern void refcount_avoid_check(void);
-extern int check_refcounts(struct xfs_mount *, xfs_agnumber_t);
+void check_refcounts(struct xfs_mount *mp, xfs_agnumber_t agno);
 
 extern void record_inode_reflink_flag(struct xfs_mount *, struct xfs_dinode *,
 	xfs_agnumber_t, xfs_agino_t, xfs_ino_t);

[PATCH 3/3] xfs_repair: check the ftype of dot and dotdot directory entries

[Darrick J. Wong]

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

The long-format directory block checking code skips the filetype check
for the '.' and '..' entries, even though they're part of the ondisk
format.  This leads to repair failing to catch subtle corruption at the
start of a directory.

Found by fuzzing bu[0].filetype = zeroes in xfs/386.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/phase6.c |   79 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 25 deletions(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 8fcd4d36..c04b2e09 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1400,6 +1400,48 @@ dir2_kill_block(
 _("directory shrink failed (%d)\n"), error);
 }
 
+static inline void
+check_longform_ftype(
+	struct xfs_mount	*mp,
+	struct xfs_inode	*ip,
+	xfs_dir2_data_entry_t	*dep,
+	ino_tree_node_t		*irec,
+	int			ino_offset,
+	struct dir_hash_tab	*hashtab,
+	xfs_dir2_dataptr_t	addr,
+	struct xfs_da_args	*da,
+	struct xfs_buf		*bp)
+{
+	xfs_ino_t		inum = be64_to_cpu(dep->inumber);
+	uint8_t			dir_ftype;
+	uint8_t			ino_ftype;
+
+	if (!xfs_has_ftype(mp))
+		return;
+
+	dir_ftype = libxfs_dir2_data_get_ftype(mp, dep);
+	ino_ftype = get_inode_ftype(irec, ino_offset);
+
+	if (dir_ftype == ino_ftype)
+		return;
+
+	if (no_modify) {
+		do_warn(
+_("would fix ftype mismatch (%d/%d) in directory/child inode %" PRIu64 "/%" PRIu64 "\n"),
+			dir_ftype, ino_ftype,
+			ip->i_ino, inum);
+		return;
+	}
+
+	do_warn(
+_("fixing ftype mismatch (%d/%d) in directory/child inode %" PRIu64 "/%" PRIu64 "\n"),
+		dir_ftype, ino_ftype,
+		ip->i_ino, inum);
+	libxfs_dir2_data_put_ftype(mp, dep, ino_ftype);
+	libxfs_dir2_data_log_entry(da, bp, dep);
+	dir_hash_update_ftype(hashtab, addr, ino_ftype);
+}
+
 /*
  * process a data block, also checks for .. entry
  * and corrects it to match what we think .. should be
@@ -1737,6 +1779,11 @@ longform_dir2_entry_check_data(
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
 			}
+
+			if (!nbad)
+				check_longform_ftype(mp, ip, dep, irec,
+						ino_offset, hashtab, addr, &da,
+						bp);
 			continue;
 		}
 		ASSERT(no_modify || libxfs_verify_dir_ino(mp, inum));
@@ -1765,6 +1812,11 @@ longform_dir2_entry_check_data(
 					libxfs_dir2_data_log_entry(&da, bp, dep);
 				}
 			}
+
+			if (!nbad)
+				check_longform_ftype(mp, ip, dep, irec,
+						ino_offset, hashtab, addr, &da,
+						bp);
 			*need_dot = 0;
 			continue;
 		}
@@ -1775,31 +1827,8 @@ longform_dir2_entry_check_data(
 			continue;
 
 		/* validate ftype field if supported */
-		if (xfs_has_ftype(mp)) {
-			uint8_t dir_ftype;
-			uint8_t ino_ftype;
-
-			dir_ftype = libxfs_dir2_data_get_ftype(mp, dep);
-			ino_ftype = get_inode_ftype(irec, ino_offset);
-
-			if (dir_ftype != ino_ftype) {
-				if (no_modify) {
-					do_warn(
-	_("would fix ftype mismatch (%d/%d) in directory/child inode %" PRIu64 "/%" PRIu64 "\n"),
-						dir_ftype, ino_ftype,
-						ip->i_ino, inum);
-				} else {
-					do_warn(
-	_("fixing ftype mismatch (%d/%d) in directory/child inode %" PRIu64 "/%" PRIu64 "\n"),
-						dir_ftype, ino_ftype,
-						ip->i_ino, inum);
-					libxfs_dir2_data_put_ftype(mp, dep, ino_ftype);
-					libxfs_dir2_data_log_entry(&da, bp, dep);
-					dir_hash_update_ftype(hashtab, addr,
-							      ino_ftype);
-				}
-			}
-		}
+		check_longform_ftype(mp, ip, dep, irec, ino_offset, hashtab,
+				addr, &da, bp);
 
 		/*
 		 * check easy case first, regular inode, just bump

Re: [PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

[Eric Sandeen]

On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we detect and correct mismatches between the V5 features
> described in the primary and the secondary superblocks.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---


> +	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
> +			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {

I'd like to add a comment about why XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR is special.
(Why is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR special? Just because userspace doesn't
bother to set it on all superblocks in the upgrade paths, right?)

-Eric

Re: [PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

[Darrick J. Wong]

On Thu, May 12, 2022 at 02:02:33PM -0500, Eric Sandeen wrote:
> On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure we detect and correct mismatches between the V5 features
> > described in the primary and the secondary superblocks.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> 
> > +	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
> > +			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {
> 
> I'd like to add a comment about why XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR is special.
> (Why is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR special? Just because userspace doesn't
> bother to set it on all superblocks in the upgrade paths, right?)

Right -- we only set it on the primary super to force users to run
xfs_repair.  Repair will clear NEEDSREPAIR on the primary and all
secondaries before it exits, so there's no point in complaining about
discrepancies in that particular feature bit.

--D

> -Eric
> 

Re: [PATCH 2/3] xfs_repair: improve checking of existing rmap and refcount btrees

[Eric Sandeen]

On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few deficiencies in the xfs_repair functions that check the
> existing reverse mapping and reference count btrees.  First of all, we
> don't report corruption or IO errors if we can't read the ondisk
> metadata.  Second, we don't consistently warn if we cannot allocate
> memory to perform the check.

Well, the caller used to report those, right? i.e.

        error = check_refcounts(wq->wq_ctx, agno);
        if (error)
                do_error(
_("%s while checking reference counts"),
                         strerror(-error));

So I think this patch is just changing from reporting strerror(-error)
when these things return, to hand-crafted error messages in the functions
that get called?

AFAICT this patch changes

"Cannot allocate memory while checking reverse-mappings"
to
"Not enough memory to check reverse mappings"

etc.

Granted, strerror(EFSCORRUPTED) isn't very pretty, and your messages are
probably more clear. But I just wanted to be sure I'm not missing something;
I think every error is actually reported today, and this is just changing
the error messages.

Thanks,
-Eric

Re: [PATCH 3/3] xfs_repair: check the ftype of dot and dotdot directory entries

[Eric Sandeen]

On 5/5/22 11:06 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The long-format directory block checking code skips the filetype check
> for the '.' and '..' entries, even though they're part of the ondisk
> format.  This leads to repair failing to catch subtle corruption at the
> start of a directory.
> 
> Found by fuzzing bu[0].filetype = zeroes in xfs/386.

Ok, mostly a refactoring into a check_longform_ftype, then adding that check
to .. and .

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

Re: [PATCH 2/3] xfs_repair: improve checking of existing rmap and refcount btrees

[Darrick J. Wong]

On Thu, May 12, 2022 at 02:39:13PM -0500, Eric Sandeen wrote:
> On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a few deficiencies in the xfs_repair functions that check the
> > existing reverse mapping and reference count btrees.  First of all, we
> > don't report corruption or IO errors if we can't read the ondisk
> > metadata.  Second, we don't consistently warn if we cannot allocate
> > memory to perform the check.
> 
> Well, the caller used to report those, right? i.e.
> 
>         error = check_refcounts(wq->wq_ctx, agno);
>         if (error)
>                 do_error(
> _("%s while checking reference counts"),
>                          strerror(-error));
> 
> So I think this patch is just changing from reporting strerror(-error)
> when these things return, to hand-crafted error messages in the functions
> that get called?
> 
> AFAICT this patch changes
> 
> "Cannot allocate memory while checking reverse-mappings"
> to
> "Not enough memory to check reverse mappings"
> 
> etc.
> 
> Granted, strerror(EFSCORRUPTED) isn't very pretty, and your messages are
> probably more clear. But I just wanted to be sure I'm not missing something;
> I think every error is actually reported today, and this is just changing
> the error messages.

Yep, that's correct.  I guess I should have (re)written the commit
message like this:

"When we're checking the rmap and refcount btrees, push error reporting
down to the exact locations of failures so that the user sees both a
message specific to the failure that occurred and what repair was doing
at the time.  This also removes quite a bit of return code handling,
since all the errors were fatal anyway."

--D

> Thanks,
> -Eric
> 

Re: [PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

[Dave Chinner]

On Thu, May 12, 2022 at 12:13:29PM -0700, Darrick J. Wong wrote:
> On Thu, May 12, 2022 at 02:02:33PM -0500, Eric Sandeen wrote:
> > On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Make sure we detect and correct mismatches between the V5 features
> > > described in the primary and the secondary superblocks.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > 
> > > +	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
> > > +			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {
> > 
> > I'd like to add a comment about why XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR is special.
> > (Why is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR special? Just because userspace doesn't
> > bother to set it on all superblocks in the upgrade paths, right?)
> 
> Right -- we only set it on the primary super to force users to run
> xfs_repair.  Repair will clear NEEDSREPAIR on the primary and all
> secondaries before it exits

Oh no it doesn't! :)

I just debugged the persistent xfs/158 failure down to repair only
writing the secondary superblocks with "features_changed" is true.

xfs/158 trashes the repair process after setting the inobtcnt and
needrepair feature bits in the primary superblock. That's the only
code that sets the internal "features_changed" flag that triggers
secondary superblock writeback. If repair then dies after this it
won't get set on the next run without the upgrade options set on the
command line. Hence we re-run repair without the upgrade feature
being enabled to check that it still recovers correctly.

The result is that repair does the right thing in completing the
feature upgrade because it sees the feature flag in the primary
superblock, but it *never sets "features_changed"* and so the
secondary superblocks are not updated when it is done. It then goes
on to check NEEDSREPAIR in the primary superblock and clears it,
allowing the fileystem to mount again.

This results in secondary superblocks with in-progress set and
mis-matched feature bits, resulting in xfs_scrub reporting corrupt
secondary superblocks and so failing the test.

This change will probably mask that specific bug - it'll still be
lying their waiting to pounce on some unsuspecting bystander, but it
will be masked by other code detecting the feature mismatch that it
causes and correcting it that way...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

[Darrick J. Wong]

On Mon, May 16, 2022 at 06:11:53PM +1000, Dave Chinner wrote:
> On Thu, May 12, 2022 at 12:13:29PM -0700, Darrick J. Wong wrote:
> > On Thu, May 12, 2022 at 02:02:33PM -0500, Eric Sandeen wrote:
> > > On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Make sure we detect and correct mismatches between the V5 features
> > > > described in the primary and the secondary superblocks.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > 
> > > > +	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
> > > > +			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {
> > > 
> > > I'd like to add a comment about why XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR is special.
> > > (Why is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR special? Just because userspace doesn't
> > > bother to set it on all superblocks in the upgrade paths, right?)
> > 
> > Right -- we only set it on the primary super to force users to run
> > xfs_repair.  Repair will clear NEEDSREPAIR on the primary and all
> > secondaries before it exits
> 
> Oh no it doesn't! :)
> 
> I just debugged the persistent xfs/158 failure down to repair only
> writing the secondary superblocks with "features_changed" is true.
> 
> xfs/158 trashes the repair process after setting the inobtcnt and
> needrepair feature bits in the primary superblock. That's the only
> code that sets the internal "features_changed" flag that triggers
> secondary superblock writeback. If repair then dies after this it
> won't get set on the next run without the upgrade options set on the
> command line. Hence we re-run repair without the upgrade feature
> being enabled to check that it still recovers correctly.
> 
> The result is that repair does the right thing in completing the
> feature upgrade because it sees the feature flag in the primary
> superblock, but it *never sets "features_changed"* and so the
> secondary superblocks are not updated when it is done. It then goes
> on to check NEEDSREPAIR in the primary superblock and clears it,
> allowing the fileystem to mount again.
> 
> This results in secondary superblocks with in-progress set and
> mis-matched feature bits, resulting in xfs_scrub reporting corrupt
> secondary superblocks and so failing the test.
> 
> This change will probably mask that specific bug - it'll still be
> lying their waiting to pounce on some unsuspecting bystander, but it
> will be masked by other code detecting the feature mismatch that it
> causes and correcting it that way...

Ah, ok.  You're pointing out that repair needs to set features_changed
right after printing "clearing needsrepair flag and regenerating
metadata", and that this patch sort of papers over the underlying issue.

The genesis of this patch wasn't xfs/158 failing, it was the secondary
sb fuzz testing noticing that xfs_scrub reported a failure whereas
xfs_repair didn't.

So, I'll go add an extra patch to fix the upgrade case, and I think we
can let Eric add this one to his tree.

--D

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

[PATCH 4/3] xfs_repair: always rewrite secondary supers when needsrepair is set

[Darrick J. Wong]

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

Dave Chinner complained about xfs_scrub failures coming from xfs/158.
That test induces xfs_repair to fail while upgrading a filesystem to
have the inobtcount feature, and then restarts xfs_repair to finish the
upgrade.  When the second xfs_repair run starts, it will find that the
primary super has NEEDSREPAIR set, along with whatever new feature that
we were trying to add to the filesystem.

From there, repair completes the upgrade in much the same manner as the
first repair run would have, with one big exception -- it forgets to set
features_changed to trigger rewriting of the secondary supers at the end
of repair.  This results in discrepancies between the supers:

# XFS_REPAIR_FAIL_AFTER_PHASE=2 xfs_repair -c inobtcount=1 /dev/sdf
Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
        - scan filesystem freespace and inode maps...
        - found root inode chunk
Adding inode btree counts to filesystem.
Killed
# xfs_repair /dev/sdf
Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
        - scan filesystem freespace and inode maps...
clearing needsrepair flag and regenerating metadata
bad inobt block count 0, saw 1
bad finobt block count 0, saw 1
bad inobt block count 0, saw 1
bad finobt block count 0, saw 1
bad inobt block count 0, saw 1
bad finobt block count 0, saw 1
bad inobt block count 0, saw 1
bad finobt block count 0, saw 1
        - found root inode chunk
Phase 3 - for each AG...
        - scan and clear agi unlinked lists...
        - process known inodes and perform inode discovery...
        - agno = 0
        - agno = 1
        - agno = 2
        - agno = 3
        - process newly discovered inodes...
Phase 4 - check for duplicate blocks...
        - setting up duplicate extent list...
        - check for inodes claiming duplicate blocks...
        - agno = 1
        - agno = 2
        - agno = 0
        - agno = 3
Phase 5 - rebuild AG headers and trees...
        - reset superblock...
Phase 6 - check inode connectivity...
        - resetting contents of realtime bitmap and summary inodes
        - traversing filesystem ...
        - traversal finished ...
        - moving disconnected inodes to lost+found ...
Phase 7 - verify and correct link counts...
done
# xfs_db -c 'sb 0' -c 'print' -c 'sb 1' -c 'print' /dev/sdf | \
	egrep '(features_ro_compat|features_incompat)'
features_ro_compat = 0xd
features_incompat = 0xb
features_ro_compat = 0x5
features_incompat = 0xb

Curiously, re-running xfs_repair will not trigger any warnings about the
featureset mismatch between the primary and secondary supers.  xfs_scrub
immediately notices, which is what causes xfs/158 to fail.

This discrepancy doesn't happen when the upgrade completes successfully
in a single repair run, so we need to teach repair to rewrite the
secondaries at the end of repair any time needsrepair was set.

Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/agheader.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/repair/agheader.c b/repair/agheader.c
index d8f912f2..7da5641b 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c
@@ -460,6 +460,14 @@ secondary_sb_whack(
 			else
 				do_warn(
 	_("would clear needsrepair flag and regenerate metadata\n"));
+			/*
+			 * If needsrepair is set on the primary super, there's
+			 * a possibility that repair crashed during an upgrade.
+			 * Set features_changed to ensure that the secondary
+			 * supers are rewritten with the new feature bits once
+			 * we've finished the upgrade.
+			 */
+			features_changed = true;
 		} else {
 			/*
 			 * Quietly clear needsrepair on the secondary supers as