[PATCHSET 0/6] xfs_scrub: small performance tweaks

[PATCHSET 0/6] xfs_scrub: small performance tweaks

[Darrick J. Wong]

Hi all,

Improve the performance of xfs_scrub when scrubbing file metadata by
using the file descriptor to issue scrub ioctls instead of scrubbing by
handle.  This saves us from having to do an untrusted iget (since we
have an open fd, we know the inode number is good) for every single
scrub invocation.  Surprisingly, this adds up to about a 3% reduction in
runtime for phase 3.

Second, if the filesystem doesn't require any repair work and scrub was
invoked in wet run (aka no -n) mode, then the only work for phase 4 is
to run FITRIM.  In this case, phase 4 can skip the precautionary
fscounter scan that we do before running metadata repairs since we'll do
that during phase 7.

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=scrub-performance-tweaks
---
 scrub/phase3.c |  122 ++++++++++++++++++++++++++++++++++++++++----------------
 scrub/phase4.c |   55 ++++++++++++++++++++-----
 scrub/phase7.c |    2 -
 scrub/repair.c |   17 +++++++-
 scrub/repair.h |    6 +++
 scrub/scrub.c  |  120 ++++++++++++-------------------------------------------
 scrub/scrub.h  |   23 ++---------
 7 files changed, 185 insertions(+), 160 deletions(-)

[PATCH 1/6] xfs_scrub: collapse trivial file scrub helpers

[Darrick J. Wong]

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

Remove all these trivial file scrub helper functions since they make
tracing code paths difficult and will become annoying in the patches
that follow.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/phase3.c |   33 +++++--------------
 scrub/scrub.c  |   95 ++++----------------------------------------------------
 scrub/scrub.h  |   18 +----------
 3 files changed, 17 insertions(+), 129 deletions(-)


diff --git a/scrub/phase3.c b/scrub/phase3.c
index c7ce0ada..868f444d 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -20,22 +20,6 @@
 
 /* Phase 3: Scan all inodes. */
 
-/*
- * Run a per-file metadata scanner.  We use the ino/gen interface to
- * ensure that the inode we're checking matches what the inode scan
- * told us to look at.
- */
-static int
-scrub_fd(
-	struct scrub_ctx	*ctx,
-	int			(*fn)(struct scrub_ctx *ctx, uint64_t ino,
-				      uint32_t gen, struct action_list *a),
-	struct xfs_bulkstat	*bs,
-	struct action_list	*alist)
-{
-	return fn(ctx, bs->bs_ino, bs->bs_gen, alist);
-}
-
 struct scrub_inode_ctx {
 	struct ptcounter	*icount;
 	bool			aborted;
@@ -84,7 +68,7 @@ scrub_inode(
 	}
 
 	/* Scrub the inode. */
-	error = scrub_fd(ctx, scrub_inode_fields, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_INODE, &alist);
 	if (error)
 		goto out;
 
@@ -93,13 +77,13 @@ scrub_inode(
 		goto out;
 
 	/* Scrub all block mappings. */
-	error = scrub_fd(ctx, scrub_data_fork, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTD, &alist);
 	if (error)
 		goto out;
-	error = scrub_fd(ctx, scrub_attr_fork, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTA, &alist);
 	if (error)
 		goto out;
-	error = scrub_fd(ctx, scrub_cow_fork, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTC, &alist);
 	if (error)
 		goto out;
 
@@ -109,22 +93,21 @@ scrub_inode(
 
 	if (S_ISLNK(bstat->bs_mode)) {
 		/* Check symlink contents. */
-		error = scrub_symlink(ctx, bstat->bs_ino, bstat->bs_gen,
-				&alist);
+		error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_SYMLINK, &alist);
 	} else if (S_ISDIR(bstat->bs_mode)) {
 		/* Check the directory entries. */
-		error = scrub_fd(ctx, scrub_dir, bstat, &alist);
+		error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_DIR, &alist);
 	}
 	if (error)
 		goto out;
 
 	/* Check all the extended attributes. */
-	error = scrub_fd(ctx, scrub_attr, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_XATTR, &alist);
 	if (error)
 		goto out;
 
 	/* Check parent pointers. */
-	error = scrub_fd(ctx, scrub_parent, bstat, &alist);
+	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_PARENT, &alist);
 	if (error)
 		goto out;
 
diff --git a/scrub/scrub.c b/scrub/scrub.c
index 4ef19656..0034f11d 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -446,14 +446,13 @@ scrub_estimate_ag_work(
 }
 
 /*
- * Scrub inode metadata.  If errors occur, this function will log them and
- * return nonzero.
+ * Scrub file metadata of some sort.  If errors occur, this function will log
+ * them and return nonzero.
  */
-static int
-__scrub_file(
+int
+scrub_file(
 	struct scrub_ctx		*ctx,
-	uint64_t			ino,
-	uint32_t			gen,
+	const struct xfs_bulkstat	*bstat,
 	unsigned int			type,
 	struct action_list		*alist)
 {
@@ -464,8 +463,8 @@ __scrub_file(
 	assert(xfrog_scrubbers[type].type == XFROG_SCRUB_TYPE_INODE);
 
 	meta.sm_type = type;
-	meta.sm_ino = ino;
-	meta.sm_gen = gen;
+	meta.sm_ino = bstat->bs_ino;
+	meta.sm_gen = bstat->bs_gen;
 
 	/* Scrub the piece of metadata. */
 	fix = xfs_check_metadata(ctx, &meta, true);
@@ -477,86 +476,6 @@ __scrub_file(
 	return scrub_save_repair(ctx, alist, &meta);
 }
 
-int
-scrub_inode_fields(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_INODE, alist);
-}
-
-int
-scrub_data_fork(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_BMBTD, alist);
-}
-
-int
-scrub_attr_fork(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_BMBTA, alist);
-}
-
-int
-scrub_cow_fork(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_BMBTC, alist);
-}
-
-int
-scrub_dir(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_DIR, alist);
-}
-
-int
-scrub_attr(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_XATTR, alist);
-}
-
-int
-scrub_symlink(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_SYMLINK, alist);
-}
-
-int
-scrub_parent(
-	struct scrub_ctx	*ctx,
-	uint64_t		ino,
-	uint32_t		gen,
-	struct action_list	*alist)
-{
-	return __scrub_file(ctx, ino, gen, XFS_SCRUB_TYPE_PARENT, alist);
-}
-
 /*
  * Test the availability of a kernel scrub command.  If errors occur (or the
  * scrub ioctl is rejected) the errors will be logged and this function will
diff --git a/scrub/scrub.h b/scrub/scrub.h
index 537a2ebe..5b5f6b65 100644
--- a/scrub/scrub.h
+++ b/scrub/scrub.h
@@ -34,22 +34,8 @@ bool can_scrub_symlink(struct scrub_ctx *ctx);
 bool can_scrub_parent(struct scrub_ctx *ctx);
 bool xfs_can_repair(struct scrub_ctx *ctx);
 
-int scrub_inode_fields(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_data_fork(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_attr_fork(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_cow_fork(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_dir(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_attr(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_symlink(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
-int scrub_parent(struct scrub_ctx *ctx, uint64_t ino, uint32_t gen,
-		struct action_list *alist);
+int scrub_file(struct scrub_ctx *ctx, const struct xfs_bulkstat *bstat,
+		unsigned int type, struct action_list *alist);
 
 /* Repair parameters are the scrub inputs and retry count. */
 struct action_item {

Re: [PATCH 1/6] xfs_scrub: collapse trivial file scrub helpers

[Eric Sandeen]

On 5/5/22 11:07 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Remove all these trivial file scrub helper functions since they make
> tracing code paths difficult and will become annoying in the patches
> that follow.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

yay

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

[PATCH 2/6] xfs_scrub: in phase 3, use the opened file descriptor for scrub calls

[Darrick J. Wong]

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

While profiling the performance of xfs_scrub, I noticed that phase3 only
employs the scrub-by-handle interface.  The kernel has had the ability
to skip the untrusted iget lookup if the fd matches the handle data
since the beginning, and using it reduces the phase 3 runtime by 5% on
the author's system.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/phase3.c |   32 ++++++++++++++++++++++----------
 scrub/scrub.c  |   21 ++++++++++++++++++---
 scrub/scrub.h  |    2 +-
 3 files changed, 41 insertions(+), 14 deletions(-)


diff --git a/scrub/phase3.c b/scrub/phase3.c
index 868f444d..7da11299 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -59,16 +59,27 @@ scrub_inode(
 	agno = cvt_ino_to_agno(&ctx->mnt, bstat->bs_ino);
 	background_sleep();
 
-	/* Try to open the inode to pin it. */
+	/*
+	 * Open this regular file to pin it in memory.  Avoiding the use of
+	 * scan-by-handle means that the in-kernel scrubber doesn't pay the
+	 * cost of opening the handle (looking up the inode in the inode btree,
+	 * grabbing the inode, checking the generation) with every scrub call.
+	 *
+	 * Note: We cannot use this same trick for directories because the VFS
+	 * will try to reconnect directory file handles to the root directory
+	 * by walking '..' entries upwards, and loops in the dirent index
+	 * btree will cause livelocks.
+	 *
+	 * ESTALE means we scan the whole cluster again.
+	 */
 	if (S_ISREG(bstat->bs_mode)) {
 		fd = scrub_open_handle(handle);
-		/* Stale inode means we scan the whole cluster again. */
 		if (fd < 0 && errno == ESTALE)
 			return ESTALE;
 	}
 
 	/* Scrub the inode. */
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_INODE, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_INODE, &alist);
 	if (error)
 		goto out;
 
@@ -77,13 +88,13 @@ scrub_inode(
 		goto out;
 
 	/* Scrub all block mappings. */
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTD, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_BMBTD, &alist);
 	if (error)
 		goto out;
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTA, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_BMBTA, &alist);
 	if (error)
 		goto out;
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_BMBTC, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_BMBTC, &alist);
 	if (error)
 		goto out;
 
@@ -93,21 +104,22 @@ scrub_inode(
 
 	if (S_ISLNK(bstat->bs_mode)) {
 		/* Check symlink contents. */
-		error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_SYMLINK, &alist);
+		error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_SYMLINK,
+				&alist);
 	} else if (S_ISDIR(bstat->bs_mode)) {
 		/* Check the directory entries. */
-		error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_DIR, &alist);
+		error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_DIR, &alist);
 	}
 	if (error)
 		goto out;
 
 	/* Check all the extended attributes. */
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_XATTR, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_XATTR, &alist);
 	if (error)
 		goto out;
 
 	/* Check parent pointers. */
-	error = scrub_file(ctx, bstat, XFS_SCRUB_TYPE_PARENT, &alist);
+	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_PARENT, &alist);
 	if (error)
 		goto out;
 
diff --git a/scrub/scrub.c b/scrub/scrub.c
index 0034f11d..19a0b2d0 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -122,6 +122,7 @@ scrub_warn_incomplete_scrub(
 static enum check_outcome
 xfs_check_metadata(
 	struct scrub_ctx		*ctx,
+	struct xfs_fd			*xfdp,
 	struct xfs_scrub_metadata	*meta,
 	bool				is_inode)
 {
@@ -135,7 +136,7 @@ xfs_check_metadata(
 
 	dbg_printf("check %s flags %xh\n", descr_render(&dsc), meta->sm_flags);
 retry:
-	error = -xfrog_scrub_metadata(&ctx->mnt, meta);
+	error = -xfrog_scrub_metadata(xfdp, meta);
 	if (debug_tweak_on("XFS_SCRUB_FORCE_REPAIR") && !error)
 		meta->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
 	switch (error) {
@@ -316,7 +317,7 @@ scrub_meta_type(
 	background_sleep();
 
 	/* Check the item. */
-	fix = xfs_check_metadata(ctx, &meta, false);
+	fix = xfs_check_metadata(ctx, &ctx->mnt, &meta, false);
 	progress_add(1);
 
 	switch (fix) {
@@ -452,11 +453,14 @@ scrub_estimate_ag_work(
 int
 scrub_file(
 	struct scrub_ctx		*ctx,
+	int				fd,
 	const struct xfs_bulkstat	*bstat,
 	unsigned int			type,
 	struct action_list		*alist)
 {
 	struct xfs_scrub_metadata	meta = {0};
+	struct xfs_fd			xfd;
+	struct xfs_fd			*xfdp = &ctx->mnt;
 	enum check_outcome		fix;
 
 	assert(type < XFS_SCRUB_TYPE_NR);
@@ -466,8 +470,19 @@ scrub_file(
 	meta.sm_ino = bstat->bs_ino;
 	meta.sm_gen = bstat->bs_gen;
 
+	/*
+	 * If the caller passed us a file descriptor for a scrub, use it
+	 * instead of scrub-by-handle because this enables the kernel to skip
+	 * costly inode btree lookups.
+	 */
+	if (fd >= 0) {
+		memcpy(&xfd, xfdp, sizeof(xfd));
+		xfd.fd = fd;
+		xfdp = &xfd;
+	}
+
 	/* Scrub the piece of metadata. */
-	fix = xfs_check_metadata(ctx, &meta, true);
+	fix = xfs_check_metadata(ctx, xfdp, &meta, true);
 	if (fix == CHECK_ABORT)
 		return ECANCELED;
 	if (fix == CHECK_DONE)
diff --git a/scrub/scrub.h b/scrub/scrub.h
index 5b5f6b65..325d8f95 100644
--- a/scrub/scrub.h
+++ b/scrub/scrub.h
@@ -34,7 +34,7 @@ bool can_scrub_symlink(struct scrub_ctx *ctx);
 bool can_scrub_parent(struct scrub_ctx *ctx);
 bool xfs_can_repair(struct scrub_ctx *ctx);
 
-int scrub_file(struct scrub_ctx *ctx, const struct xfs_bulkstat *bstat,
+int scrub_file(struct scrub_ctx *ctx, int fd, const struct xfs_bulkstat *bstat,
 		unsigned int type, struct action_list *alist);
 
 /* Repair parameters are the scrub inputs and retry count. */

Re: [PATCH 2/6] xfs_scrub: in phase 3, use the opened file descriptor for scrub calls

[Eric Sandeen]

On 5/5/22 11:07 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While profiling the performance of xfs_scrub, I noticed that phase3 only
> employs the scrub-by-handle interface.  The kernel has had the ability
> to skip the untrusted iget lookup if the fd matches the handle data
> since the beginning, and using it reduces the phase 3 runtime by 5% on
> the author's system.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

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

[PATCH 3/6] xfs_scrub: fall back to scrub-by-handle if opening handles fails

[Darrick J. Wong]

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

Back when I originally wrote xfs_scrub, I decided that phase 3 (the file
scrubber) should try to open all regular files by handle to pin the file
during the scrub.  At the time, I decided that an ESTALE return value
should cause the entire inode group (aka an inobt record) to be
rescanned for thoroughness reasons.

Over the past four years, I've realized that checking the return value
here isn't necessary.  For most runtime errors, we already fall back to
scrubbing with the file handle, at a fairly small performance cost.

For ESTALE, if the file has been freed and reallocated, its metadata has
been rewritten completely since bulkstat, so it's not necessary to check
it for latent disk errors.  If the file was freed, we can simply move on
to the next file.  If the filesystem is corrupt enough that
open-by-handle fails (this also results in ESTALE), we actually /want/
to fall back to scrubbing that file by handle, not rescanning the entire
inode group.

Therefore, remove the ESTALE check and leave a comment detailing these
findings.

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


diff --git a/scrub/phase3.c b/scrub/phase3.c
index 7da11299..d22758c1 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -65,18 +65,26 @@ scrub_inode(
 	 * cost of opening the handle (looking up the inode in the inode btree,
 	 * grabbing the inode, checking the generation) with every scrub call.
 	 *
+	 * Ignore any runtime or corruption related errors here because we can
+	 * fall back to scrubbing by handle.  ESTALE can be ignored for the
+	 * following reasons:
+	 *
+	 *  - If the file has been deleted since bulkstat, there's nothing to
+	 *    check.  Scrub-by-handle returns ENOENT for such inodes.
+	 *  - If the file has been deleted and reallocated since bulkstat,
+	 *    its ondisk metadata have been rewritten and is assumed to be ok.
+	 *    Scrub-by-handle also returns ENOENT if the generation doesn't
+	 *    match.
+	 *  - The file itself is corrupt and cannot be loaded.  In this case,
+	 *    we fall back to scrub-by-handle.
+	 *
 	 * Note: We cannot use this same trick for directories because the VFS
 	 * will try to reconnect directory file handles to the root directory
 	 * by walking '..' entries upwards, and loops in the dirent index
 	 * btree will cause livelocks.
-	 *
-	 * ESTALE means we scan the whole cluster again.
 	 */
-	if (S_ISREG(bstat->bs_mode)) {
+	if (S_ISREG(bstat->bs_mode))
 		fd = scrub_open_handle(handle);
-		if (fd < 0 && errno == ESTALE)
-			return ESTALE;
-	}
 
 	/* Scrub the inode. */
 	error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_INODE, &alist);

Re: [PATCH 3/6] xfs_scrub: fall back to scrub-by-handle if opening handles fails

[Eric Sandeen]

On 5/5/22 11:07 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Back when I originally wrote xfs_scrub, I decided that phase 3 (the file
> scrubber) should try to open all regular files by handle to pin the file
> during the scrub.  At the time, I decided that an ESTALE return value
> should cause the entire inode group (aka an inobt record) to be
> rescanned for thoroughness reasons.
> 
> Over the past four years, I've realized that checking the return value
> here isn't necessary.  For most runtime errors, we already fall back to
> scrubbing with the file handle, at a fairly small performance cost.
> 
> For ESTALE, if the file has been freed and reallocated, its metadata has
> been rewritten completely since bulkstat, so it's not necessary to check
> it for latent disk errors.  If the file was freed, we can simply move on
> to the next file.  If the filesystem is corrupt enough that
> open-by-handle fails (this also results in ESTALE), we actually /want/
> to fall back to scrubbing that file by handle, not rescanning the entire
> inode group.
> 
> Therefore, remove the ESTALE check and leave a comment detailing these
> findings.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

ohhhkay, tbh mostly trusting you n this but it all; sounds plausible ;)

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

[PATCH 4/6] xfs_scrub: don't try any file repairs during phase 3 if AG metadata bad

[Darrick J. Wong]

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

Currently, phase 3 tries to repair file metadata even after phase 2
tells us that there are problems with the AG metadata.  While this
generally won't cause too many problems since the repair code will bail
out on any obvious corruptions it finds, this isn't totally foolproof.
If the filesystem space metadata are not in good shape, we want to queue
the file repairs to run /after/ the space metadata repairs in phase 4.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/phase3.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
 scrub/repair.h |    6 ++++++
 2 files changed, 51 insertions(+), 5 deletions(-)


diff --git a/scrub/phase3.c b/scrub/phase3.c
index d22758c1..fd8e5419 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -21,8 +21,16 @@
 /* Phase 3: Scan all inodes. */
 
 struct scrub_inode_ctx {
+	struct scrub_ctx	*ctx;
+
+	/* Number of inodes scanned. */
 	struct ptcounter	*icount;
+
+	/* Set to true to abort all threads. */
 	bool			aborted;
+
+	/* Set to true if we want to defer file repairs to phase 4. */
+	bool			always_defer_repairs;
 };
 
 /* Report a filesystem error that the vfs fed us on close. */
@@ -40,6 +48,24 @@ report_close_error(
 	str_errno(ctx, descr);
 }
 
+/* Run repair actions now and defer unfinished items for later. */
+static int
+try_inode_repair(
+	struct scrub_inode_ctx	*ictx,
+	xfs_agnumber_t		agno,
+	struct action_list	*alist)
+{
+	/*
+	 * If at the start of phase 3 we already had ag/rt metadata repairs
+	 * queued up for phase 4, leave the action list untouched so that file
+	 * metadata repairs will be deferred in scan order until phase 4.
+	 */
+	if (ictx->always_defer_repairs)
+		return 0;
+
+	return action_list_process_or_defer(ictx->ctx, agno, alist);
+}
+
 /* Verify the contents, xattrs, and extent maps of an inode. */
 static int
 scrub_inode(
@@ -91,7 +117,7 @@ scrub_inode(
 	if (error)
 		goto out;
 
-	error = action_list_process_or_defer(ctx, agno, &alist);
+	error = try_inode_repair(ictx, agno, &alist);
 	if (error)
 		goto out;
 
@@ -106,7 +132,7 @@ scrub_inode(
 	if (error)
 		goto out;
 
-	error = action_list_process_or_defer(ctx, agno, &alist);
+	error = try_inode_repair(ictx, agno, &alist);
 	if (error)
 		goto out;
 
@@ -132,7 +158,7 @@ scrub_inode(
 		goto out;
 
 	/* Try to repair the file while it's open. */
-	error = action_list_process_or_defer(ctx, agno, &alist);
+	error = try_inode_repair(ictx, agno, &alist);
 	if (error)
 		goto out;
 
@@ -147,7 +173,10 @@ scrub_inode(
 		ictx->aborted = true;
 	}
 	progress_add(1);
-	action_list_defer(ctx, agno, &alist);
+
+	if (!error && !ictx->aborted)
+		action_list_defer(ctx, agno, &alist);
+
 	if (fd >= 0) {
 		int	err2;
 
@@ -168,8 +197,9 @@ int
 phase3_func(
 	struct scrub_ctx	*ctx)
 {
-	struct scrub_inode_ctx	ictx = { NULL };
+	struct scrub_inode_ctx	ictx = { .ctx = ctx };
 	uint64_t		val;
+	xfs_agnumber_t		agno;
 	int			err;
 
 	err = ptcounter_alloc(scrub_nproc(ctx), &ictx.icount);
@@ -178,6 +208,16 @@ phase3_func(
 		return err;
 	}
 
+	/*
+	 * If we already have ag/fs metadata to repair from previous phases,
+	 * we would rather not try to repair file metadata until we've tried
+	 * to repair the space metadata.
+	 */
+	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
+		if (!action_list_empty(&ctx->action_lists[agno]))
+			ictx.always_defer_repairs = true;
+	}
+
 	err = scrub_scan_all_inodes(ctx, scrub_inode, &ictx);
 	if (!err && ictx.aborted)
 		err = ECANCELED;
diff --git a/scrub/repair.h b/scrub/repair.h
index 1994c50a..4261be49 100644
--- a/scrub/repair.h
+++ b/scrub/repair.h
@@ -16,6 +16,12 @@ int action_lists_alloc(size_t nr, struct action_list **listsp);
 void action_lists_free(struct action_list **listsp);
 
 void action_list_init(struct action_list *alist);
+
+static inline bool action_list_empty(const struct action_list *alist)
+{
+	return list_empty(&alist->list);
+}
+
 size_t action_list_length(struct action_list *alist);
 void action_list_add(struct action_list *dest, struct action_item *item);
 void action_list_splice(struct action_list *dest, struct action_list *src);

Re: [PATCH 4/6] xfs_scrub: don't try any file repairs during phase 3 if AG metadata bad

[Eric Sandeen]

On 5/5/22 11:08 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Currently, phase 3 tries to repair file metadata even after phase 2
> tells us that there are problems with the AG metadata.  While this
> generally won't cause too many problems since the repair code will bail
> out on any obvious corruptions it finds, this isn't totally foolproof.
> If the filesystem space metadata are not in good shape, we want to queue
> the file repairs to run /after/ the space metadata repairs in phase 4.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

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

[PATCH 5/6] xfs_scrub: make phase 4 go straight to fstrim if nothing to fix

[Darrick J. Wong]

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

If there's nothing to repair in phase 4, there's no need to hold up the
FITRIM call to do the summary count scan that prepares us to repair
filesystem metadata.  Rearrange this a bit.

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


diff --git a/scrub/phase4.c b/scrub/phase4.c
index 6ed7210f..ad26d9d5 100644
--- a/scrub/phase4.c
+++ b/scrub/phase4.c
@@ -95,15 +95,32 @@ repair_everything(
 	if (aborted)
 		return ECANCELED;
 
-	pthread_mutex_lock(&ctx->lock);
-	if (ctx->corruptions_found == 0 && ctx->unfixable_errors == 0 &&
-	    want_fstrim) {
+	return 0;
+}
+
+/* Decide if we have any repair work to do. */
+static inline bool
+have_action_items(
+	struct scrub_ctx	*ctx)
+{
+	xfs_agnumber_t		agno;
+
+	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
+		if (action_list_length(&ctx->action_lists[agno]) > 0)
+			return true;
+	}
+
+	return false;
+}
+
+/* Trim the unused areas of the filesystem if the caller asked us to. */
+static void
+trim_filesystem(
+	struct scrub_ctx	*ctx)
+{
+	if (want_fstrim)
 		fstrim(ctx);
-		progress_add(1);
-	}
-	pthread_mutex_unlock(&ctx->lock);
-
-	return 0;
+	progress_add(1);
 }
 
 /* Fix everything that needs fixing. */
@@ -113,6 +130,9 @@ phase4_func(
 {
 	int			ret;
 
+	if (!have_action_items(ctx))
+		goto maybe_trim;
+
 	/*
 	 * Check the summary counters early.  Normally we do this during phase
 	 * seven, but some of the cross-referencing requires fairly-accurate
@@ -123,7 +143,20 @@ phase4_func(
 	if (ret)
 		return ret;
 
-	return repair_everything(ctx);
+	ret = repair_everything(ctx);
+	if (ret)
+		return ret;
+
+	/*
+	 * If errors remain on the filesystem, do not trim anything.  We don't
+	 * have any threads running, so it's ok to skip the ctx lock here.
+	 */
+	if (ctx->corruptions_found || ctx->unfixable_errors == 0)
+		return 0;
+
+maybe_trim:
+	trim_filesystem(ctx);
+	return 0;
 }
 
 /* Estimate how much work we're going to do. */

Re: [PATCH 5/6] xfs_scrub: make phase 4 go straight to fstrim if nothing to fix

[Eric Sandeen]

On 5/5/22 11:08 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If there's nothing to repair in phase 4, there's no need to hold up the
> FITRIM call to do the summary count scan that prepares us to repair
> filesystem metadata.  Rearrange this a bit.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>


> +
> +	/*
> +	 * If errors remain on the filesystem, do not trim anything.  We don't
> +	 * have any threads running, so it's ok to skip the ctx lock here.
> +	 */
> +	if (ctx->corruptions_found || ctx->unfixable_errors == 0)
> +		return 0;
> +
> +maybe_trim:
> +	trim_filesystem(ctx);
> +	return 0;
>  }

I'm a little confused by the unfixable_errors test, is that correct?

Why do you bail out if there are 0 unfixable errors?

-Eric

Re: [PATCH 5/6] xfs_scrub: make phase 4 go straight to fstrim if nothing to fix

[Darrick J. Wong]

On Thu, May 12, 2022 at 05:34:37PM -0500, Eric Sandeen wrote:
> On 5/5/22 11:08 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If there's nothing to repair in phase 4, there's no need to hold up the
> > FITRIM call to do the summary count scan that prepares us to repair
> > filesystem metadata.  Rearrange this a bit.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> 
> > +
> > +	/*
> > +	 * If errors remain on the filesystem, do not trim anything.  We don't
> > +	 * have any threads running, so it's ok to skip the ctx lock here.
> > +	 */
> > +	if (ctx->corruptions_found || ctx->unfixable_errors == 0)
> > +		return 0;
> > +
> > +maybe_trim:
> > +	trim_filesystem(ctx);
> > +	return 0;
> >  }
> 
> I'm a little confused by the unfixable_errors test, is that correct?
> 
> Why do you bail out if there are 0 unfixable errors?

Oooops.  That was a logic inversion bug. :(

--D

> -Eric

Re: [PATCH 5/6] xfs_scrub: make phase 4 go straight to fstrim if nothing to fix

[Eric Sandeen]

On 5/12/22 6:12 PM, Darrick J. Wong wrote:
> On Thu, May 12, 2022 at 05:34:37PM -0500, Eric Sandeen wrote:
>> On 5/5/22 11:08 AM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> If there's nothing to repair in phase 4, there's no need to hold up the
>>> FITRIM call to do the summary count scan that prepares us to repair
>>> filesystem metadata.  Rearrange this a bit.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>
>>
>>> +
>>> +	/*
>>> +	 * If errors remain on the filesystem, do not trim anything.  We don't
>>> +	 * have any threads running, so it's ok to skip the ctx lock here.
>>> +	 */
>>> +	if (ctx->corruptions_found || ctx->unfixable_errors == 0)
>>> +		return 0;
>>> +
>>> +maybe_trim:
>>> +	trim_filesystem(ctx);
>>> +	return 0;
>>>  }
>>
>> I'm a little confused by the unfixable_errors test, is that correct?
>>
>> Why do you bail out if there are 0 unfixable errors?
> 
> Oooops.  That was a logic inversion bug. :(

I'll fix it up and commit it with:

+	if (ctx->corruptions_found || ctx->unfixable_errors != 0)
+		return 0;

if you want to confirm that's correct?

-Eric

Re: [PATCH 5/6] xfs_scrub: make phase 4 go straight to fstrim if nothing to fix

[Darrick J. Wong]

On Fri, May 13, 2022 at 08:18:15AM -0500, Eric Sandeen wrote:
> On 5/12/22 6:12 PM, Darrick J. Wong wrote:
> > On Thu, May 12, 2022 at 05:34:37PM -0500, Eric Sandeen wrote:
> >> On 5/5/22 11:08 AM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <djwong@kernel.org>
> >>>
> >>> If there's nothing to repair in phase 4, there's no need to hold up the
> >>> FITRIM call to do the summary count scan that prepares us to repair
> >>> filesystem metadata.  Rearrange this a bit.
> >>>
> >>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> >>
> >>
> >>> +
> >>> +	/*
> >>> +	 * If errors remain on the filesystem, do not trim anything.  We don't
> >>> +	 * have any threads running, so it's ok to skip the ctx lock here.
> >>> +	 */
> >>> +	if (ctx->corruptions_found || ctx->unfixable_errors == 0)
> >>> +		return 0;
> >>> +
> >>> +maybe_trim:
> >>> +	trim_filesystem(ctx);
> >>> +	return 0;
> >>>  }
> >>
> >> I'm a little confused by the unfixable_errors test, is that correct?
> >>
> >> Why do you bail out if there are 0 unfixable errors?
> > 
> > Oooops.  That was a logic inversion bug. :(
> 
> I'll fix it up and commit it with:
> 
> +	if (ctx->corruptions_found || ctx->unfixable_errors != 0)
> +		return 0;
> 
> if you want to confirm that's correct?

Yes, that's correct, thank you.

--D

> -Eric

[PATCH 6/6] xfs_scrub: in phase 3, use the opened file descriptor for repair calls

[Darrick J. Wong]

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

While profiling the performance of xfs_scrub, I noticed that phase3 only
employs the scrub-by-handle interface for repairs.  The kernel has had
the ability to skip the untrusted iget lookup if the fd matches the
handle data since the beginning, and using it reduces the repair runtime
by 5% on the author's system.  Normally, we shouldn't be running that
many repairs or optimizations, but we did this for scrub, so we should
do the same for repair.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 scrub/phase3.c |   17 +++++++++++++----
 scrub/phase4.c |    4 ++--
 scrub/phase7.c |    2 +-
 scrub/repair.c |   17 +++++++++++++++--
 scrub/scrub.c  |    4 ++--
 scrub/scrub.h  |    5 +++--
 6 files changed, 36 insertions(+), 13 deletions(-)


diff --git a/scrub/phase3.c b/scrub/phase3.c
index fd8e5419..d659a779 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -52,9 +52,12 @@ report_close_error(
 static int
 try_inode_repair(
 	struct scrub_inode_ctx	*ictx,
+	int			fd,
 	xfs_agnumber_t		agno,
 	struct action_list	*alist)
 {
+	int			ret;
+
 	/*
 	 * If at the start of phase 3 we already had ag/rt metadata repairs
 	 * queued up for phase 4, leave the action list untouched so that file
@@ -63,7 +66,13 @@ try_inode_repair(
 	if (ictx->always_defer_repairs)
 		return 0;
 
-	return action_list_process_or_defer(ictx->ctx, agno, alist);
+	ret = action_list_process(ictx->ctx, fd, alist,
+			ALP_REPAIR_ONLY | ALP_NOPROGRESS);
+	if (ret)
+		return ret;
+
+	action_list_defer(ictx->ctx, agno, alist);
+	return 0;
 }
 
 /* Verify the contents, xattrs, and extent maps of an inode. */
@@ -117,7 +126,7 @@ scrub_inode(
 	if (error)
 		goto out;
 
-	error = try_inode_repair(ictx, agno, &alist);
+	error = try_inode_repair(ictx, fd, agno, &alist);
 	if (error)
 		goto out;
 
@@ -132,7 +141,7 @@ scrub_inode(
 	if (error)
 		goto out;
 
-	error = try_inode_repair(ictx, agno, &alist);
+	error = try_inode_repair(ictx, fd, agno, &alist);
 	if (error)
 		goto out;
 
@@ -158,7 +167,7 @@ scrub_inode(
 		goto out;
 
 	/* Try to repair the file while it's open. */
-	error = try_inode_repair(ictx, agno, &alist);
+	error = try_inode_repair(ictx, fd, agno, &alist);
 	if (error)
 		goto out;
 
diff --git a/scrub/phase4.c b/scrub/phase4.c
index ad26d9d5..559b2779 100644
--- a/scrub/phase4.c
+++ b/scrub/phase4.c
@@ -40,7 +40,7 @@ repair_ag(
 
 	/* Repair anything broken until we fail to make progress. */
 	do {
-		ret = action_list_process(ctx, ctx->mnt.fd, alist, flags);
+		ret = action_list_process(ctx, -1, alist, flags);
 		if (ret) {
 			*aborted = true;
 			return;
@@ -55,7 +55,7 @@ repair_ag(
 
 	/* Try once more, but this time complain if we can't fix things. */
 	flags |= ALP_COMPLAIN_IF_UNFIXED;
-	ret = action_list_process(ctx, ctx->mnt.fd, alist, flags);
+	ret = action_list_process(ctx, -1, alist, flags);
 	if (ret)
 		*aborted = true;
 }
diff --git a/scrub/phase7.c b/scrub/phase7.c
index 84546b1c..8d8034c3 100644
--- a/scrub/phase7.c
+++ b/scrub/phase7.c
@@ -121,7 +121,7 @@ phase7_func(
 	error = scrub_fs_summary(ctx, &alist);
 	if (error)
 		return error;
-	error = action_list_process(ctx, ctx->mnt.fd, &alist,
+	error = action_list_process(ctx, -1, &alist,
 			ALP_COMPLAIN_IF_UNFIXED | ALP_NOPROGRESS);
 	if (error)
 		return error;
diff --git a/scrub/repair.c b/scrub/repair.c
index 1ef6372e..bb026101 100644
--- a/scrub/repair.c
+++ b/scrub/repair.c
@@ -230,17 +230,30 @@ action_list_process(
 	struct action_list		*alist,
 	unsigned int			repair_flags)
 {
+	struct xfs_fd			xfd;
+	struct xfs_fd			*xfdp = &ctx->mnt;
 	struct action_item		*aitem;
 	struct action_item		*n;
 	enum check_outcome		fix;
 
+	/*
+	 * If the caller passed us a file descriptor for a scrub, use it
+	 * instead of scrub-by-handle because this enables the kernel to skip
+	 * costly inode btree lookups.
+	 */
+	if (fd >= 0) {
+		memcpy(&xfd, xfdp, sizeof(xfd));
+		xfd.fd = fd;
+		xfdp = &xfd;
+	}
+
 	if (!alist->sorted) {
 		list_sort(NULL, &alist->list, xfs_action_item_compare);
 		alist->sorted = true;
 	}
 
 	list_for_each_entry_safe(aitem, n, &alist->list, list) {
-		fix = xfs_repair_metadata(ctx, fd, aitem, repair_flags);
+		fix = xfs_repair_metadata(ctx, xfdp, aitem, repair_flags);
 		switch (fix) {
 		case CHECK_DONE:
 			if (!(repair_flags & ALP_NOPROGRESS))
@@ -284,7 +297,7 @@ action_list_process_or_defer(
 {
 	int				ret;
 
-	ret = action_list_process(ctx, ctx->mnt.fd, alist,
+	ret = action_list_process(ctx, -1, alist,
 			ALP_REPAIR_ONLY | ALP_NOPROGRESS);
 	if (ret)
 		return ret;
diff --git a/scrub/scrub.c b/scrub/scrub.c
index 19a0b2d0..e83d0d9c 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -611,7 +611,7 @@ xfs_can_repair(
 enum check_outcome
 xfs_repair_metadata(
 	struct scrub_ctx		*ctx,
-	int				fd,
+	struct xfs_fd			*xfdp,
 	struct action_item		*aitem,
 	unsigned int			repair_flags)
 {
@@ -649,7 +649,7 @@ xfs_repair_metadata(
 		str_info(ctx, descr_render(&dsc),
 				_("Attempting optimization."));
 
-	error = -xfrog_scrub_metadata(&ctx->mnt, &meta);
+	error = -xfrog_scrub_metadata(xfdp, &meta);
 	switch (error) {
 	case 0:
 		/* No operational errors encountered. */
diff --git a/scrub/scrub.h b/scrub/scrub.h
index 325d8f95..fccd82f2 100644
--- a/scrub/scrub.h
+++ b/scrub/scrub.h
@@ -57,7 +57,8 @@ struct action_item {
 /* Complain if still broken even after fix. */
 #define XRM_COMPLAIN_IF_UNFIXED	(1U << 1)
 
-enum check_outcome xfs_repair_metadata(struct scrub_ctx *ctx, int fd,
-		struct action_item *aitem, unsigned int repair_flags);
+enum check_outcome xfs_repair_metadata(struct scrub_ctx *ctx,
+		struct xfs_fd *xfdp, struct action_item *aitem,
+		unsigned int repair_flags);
 
 #endif /* XFS_SCRUB_SCRUB_H_ */

Re: [PATCH 6/6] xfs_scrub: in phase 3, use the opened file descriptor for repair calls

[Eric Sandeen]

On 5/5/22 11:08 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While profiling the performance of xfs_scrub, I noticed that phase3 only
> employs the scrub-by-handle interface for repairs.  The kernel has had
> the ability to skip the untrusted iget lookup if the fd matches the
> handle data since the beginning, and using it reduces the repair runtime
> by 5% on the author's system.  Normally, we shouldn't be running that
> many repairs or optimizations, but we did this for scrub, so we should
> do the same for repair.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

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