On Thu, Jan 14, 2021 at 04:39:42AM -0500, Brian Foster wrote:

On Wed, Jan 13, 2021 at 05:05:48PM -0800, Darrick J. Wong wrote:

quoted

On Wed, Jan 13, 2021 at 01:17:49PM -0500, Brian Foster wrote:

quoted

On Fri, Jan 08, 2021 at 10:28:16PM -0800, Darrick J. Wong wrote:

quoted

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

Clear the needsrepair flag, since it's used to prevent mounting of an
inconsistent filesystem.

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

diff --git a/repair/agheader.c b/repair/agheader.c
index 8bb99489..f6174dbf 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c

@@ -452,6 +452,17 @@ secondary_sb_whack(
 			rval |= XR_AG_SB_SEC;
 	}
 
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (!no_modify)
+			sb->sb_features_incompat &=
+					~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		if (!do_bzero) {
+			rval |= XR_AG_SB;
+			do_warn(_("needsrepair flag set in sb %d\n"), i);
+		} else
+			rval |= XR_AG_SB_SEC;
+	}
+

Looks reasonable modulo the questions on the previous patch. When I give
this a test, one thing that stands out is that the needsrepair state
itself sort of presents as corruption. I.e.,

# ./db/xfs_db -x -c "version needsrepair" <dev>
Upgrading V5 filesystem
Upgraded V5 filesystem.  Please run xfs_repair.
versionnum [0xb4a5+0x18a] =
V5,NLINK,DIRV2,ALIGN,LOGV2,EXTFLG,MOREBITS,ATTR2,LAZYSBCOUNT,PROJID32BIT,CRC,FTYPE,FINOBT,SPARSE_INODES,REFLINK,NEEDSREPAIR
# ./repair/xfs_repair <dev>
Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
        - scan filesystem freespace and inode maps...
needsrepair flag set in sb 1
reset bad sb for ag 1
needsrepair flag set in sb 2
reset bad sb for ag 2
needsrepair flag set in sb 0
reset bad sb for ag 0
needsrepair flag set in sb 3
reset bad sb for ag 3
        - found root inode chunk
Phase 3 - for each AG...
...

So nothing was ever done to this fs besides set and clear the bit. Not a
huge deal, but I wonder if we should print something more user friendly
to indicate that repair found and cleared the needsrepair state, or at
least just avoid the "reset bad sb ..." message for the needsrepair
case.

Hm.  For the backup supers I guess there's really not much point in
saying anything about the bit being set, because the only time they get
used is when repair tries to use one to fix a filesystem.

As for AG 0, I guess I could change that to say:

	dbprintf(_("Thank you for running xfs_repair!"));

:D Or maybe there's no need to say anything at all.

Heh. FWIW, the "needsrepair flag set ..." messages didn't seem as
alarming as the "reset bad sb ..." ones, but that's just me. I'd be fine
with making this entirely silent too, or at least starting that way
until some user complains with a good enough reason for a new message..
;)

I changed it so repair says "clearing needsrepair flag and regenerating
metadata" and omits the "reset bad sb" warnings.

--D

Brian

quoted

--D

quoted

Brian

quoted

 	return(rval);
 }