[PATCHSET 0/2] xfsprogs: various 5.15 fixes

[PATCHSET 0/2] xfsprogs: various 5.15 fixes

[Darrick J. Wong]

Hi all,

Here's the last couple of bug fixes for 5.15 -- merely a couple of
omissions from xfs-scrub.

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=xfsprogs-5.15-fixes
---
 scrub/scrub.c |   33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs_scrub: fix xfrog_scrub_metadata error reporting

[Darrick J. Wong]

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

Commit de5d20ec converted xfrog_scrub_metadata to return negative error
codes directly, but forgot to fix up the str_errno calls to use
str_liberror.  This doesn't result in incorrect error reporting
currently, but (a) the calls in the switch statement are inconsistent,
and (b) this will matter in future patches where we can call library
functions in between xfrog_scrub_metadata and str_liberror.

Fixes: de5d20ec ("libfrog: convert scrub.c functions to negative error codes")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/scrub.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/scrub/scrub.c b/scrub/scrub.c
index a4b7084e..07ae0673 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -153,7 +153,7 @@ _("Filesystem is shut down, aborting."));
 	case EIO:
 	case ENOMEM:
 		/* Abort on I/O errors or insufficient memory. */
-		str_errno(ctx, descr_render(&dsc));
+		str_liberror(ctx, error, descr_render(&dsc));
 		return CHECK_ABORT;
 	case EDEADLOCK:
 	case EBUSY:
@@ -164,10 +164,10 @@ _("Filesystem is shut down, aborting."));
 		 * and the other two should be reported via sm_flags.
 		 */
 		str_liberror(ctx, error, _("Kernel bug"));
-		fallthrough;
+		return CHECK_DONE;
 	default:
 		/* Operational error. */
-		str_errno(ctx, descr_render(&dsc));
+		str_liberror(ctx, error, descr_render(&dsc));
 		return CHECK_DONE;
 	}
 

[PATCH 2/2] xfs_scrub: retry scrub (and repair) of items that are ok except for XFAIL

[Darrick J. Wong]

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

Sometimes a metadata object will pass all of the obvious scrubber
checks, but we won't be able to cross-reference the object's records
with other metadata objects (e.g. a file data fork and a free space
btree both claim ownership of an extent).  When this happens during the
checking phase, we should queue the object for a repair, which means
that phase 4 will keep re-evaluating the object as repairs proceed.
Eventually, the hope is that we'll fix the filesystem and everything
will scrub cleanly; if not, we recommend running xfs_repair as a second
attempt to fix the inconsistency.

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


diff --git a/scrub/scrub.c b/scrub/scrub.c
index 07ae0673..4ef19656 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -223,6 +223,16 @@ _("Optimization is possible."));
 		return CHECK_REPAIR;
 	}
 
+	/*
+	 * This metadata object itself looks ok, but we noticed inconsistencies
+	 * when comparing it with the other filesystem metadata.  If we're in
+	 * repair mode we need to queue it for a "repair" so that phase 4 will
+	 * re-examine the object as repairs progress to see if the kernel will
+	 * deem it completely consistent at some point.
+	 */
+	if (xref_failed(meta) && ctx->mode == SCRUB_MODE_REPAIR)
+		return CHECK_REPAIR;
+
 	/* Everything is ok. */
 	return CHECK_DONE;
 }
@@ -787,6 +797,23 @@ _("Read-only filesystem; cannot make changes."));
 			return CHECK_RETRY;
 		str_corrupt(ctx, descr_render(&dsc),
 _("Repair unsuccessful; offline repair required."));
+	} else if (xref_failed(&meta)) {
+		/*
+		 * This metadata object itself looks ok, but we still noticed
+		 * inconsistencies when comparing it with the other filesystem
+		 * metadata.  If we're in "final warning" mode, advise the
+		 * caller to run xfs_repair; otherwise, we'll keep trying to
+		 * reverify the cross-referencing as repairs progress.
+		 */
+		if (repair_flags & XRM_COMPLAIN_IF_UNFIXED) {
+			str_info(ctx, descr_render(&dsc),
+ _("Seems correct but cross-referencing failed; offline repair recommended."));
+		} else {
+			if (verbose)
+				str_info(ctx, descr_render(&dsc),
+ _("Seems correct but cross-referencing failed; will keep checking."));
+			return CHECK_RETRY;
+		}
 	} else {
 		/* Clean operation, no corruption detected. */
 		if (needs_repair(&oldm))

Re: [PATCH 1/2] xfs_scrub: fix xfrog_scrub_metadata error reporting

[Eric Sandeen]

On 3/15/22 6:23 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Commit de5d20ec converted xfrog_scrub_metadata to return negative error
> codes directly, but forgot to fix up the str_errno calls to use
> str_liberror.  This doesn't result in incorrect error reporting
> currently, but (a) the calls in the switch statement are inconsistent,
> and (b) this will matter in future patches where we can call library
> functions in between xfrog_scrub_metadata and str_liberror.
> 
> Fixes: de5d20ec ("libfrog: convert scrub.c functions to negative error codes")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

wow how could you possibly have forgotten the difference between
str_errno, str_liberror, and str_error? ;)

So the net effect here is sending our own error, not errno. Looks right.

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

Re: [PATCH 2/2] xfs_scrub: retry scrub (and repair) of items that are ok except for XFAIL

[Eric Sandeen]

On 3/15/22 6:23 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Sometimes a metadata object will pass all of the obvious scrubber
> checks, but we won't be able to cross-reference the object's records
> with other metadata objects (e.g. a file data fork and a free space
> btree both claim ownership of an extent).  When this happens during the
> checking phase, we should queue the object for a repair, which means
> that phase 4 will keep re-evaluating the object as repairs proceed.
> Eventually, the hope is that we'll fix the filesystem and everything
> will scrub cleanly; if not, we recommend running xfs_repair as a second
> attempt to fix the inconsistency.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

I'm not a scrub-master, but this seems to make sense.

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