[PATCHSET 1/4] xfs_scrub: codex-inspired bug fixes, part 4

[PATCHSET 1/4] xfs_scrub: codex-inspired bug fixes, part 4

[Darrick J. Wong]

Hi all,

Here's a fourth batch of xfs_scrub fixes resulting from Codex reviews.
This is actually just the two from the last drop that haven't yet
passed review.

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

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=scrub-codex-fixes4
---
Commits in this patchset:
 * xfs_scrub: fix spacemap external log device scan dev key
 * xfs_scrub: fix estimate of work items for phase 4
---
 scrub/spacemap.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 scrub/phase4.c   |   17 +++++++++++++++--
 scrub/phase6.c   |   42 ------------------------------------------
 scrub/phase7.c   |   11 +----------
 scrub/spacemap.c |   17 +++++------------
 5 files changed, 63 insertions(+), 66 deletions(-)

[PATCH 1/2] xfs_scrub: fix spacemap external log device scan dev key

[Darrick J. Wong]

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

Codex bizarrely complains about the rtstart>0 logic in scan_log_rmaps,
because it gets confused about the difference between internal logs
(which are scanned by scan_ag_rmaps) and external logs, which this
function handles.  I'm ignoring the report, but we shouldn't open-code
the synthetic device keys so let's fix that by hoisting the fsmap device
helpers from phase6.c to spacemap.h and using them everywhere.

Cc: <linux-xfs@vger.kernel.org> # v6.15.0
Fixes: 37591ef3f4f14c ("xfs_scrub: support internal RT device")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 scrub/spacemap.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 scrub/phase6.c   |   42 ------------------------------------------
 scrub/phase7.c   |   11 +----------
 scrub/spacemap.c |   17 +++++------------
 4 files changed, 48 insertions(+), 64 deletions(-)


diff --git a/scrub/spacemap.h b/scrub/spacemap.h
index 759d0f89089a23..0d85285f29ede9 100644
--- a/scrub/spacemap.h
+++ b/scrub/spacemap.h
@@ -23,4 +23,46 @@ static inline unsigned int scrub_scan_spacemaps_nproc(struct scrub_ctx *ctx)
 	return scrub_nproc(ctx);
 }
 
+/* Return XFS device index from fsmap device. */
+static inline enum xfs_device
+from_fsmap_dev(
+	struct scrub_ctx	*ctx,
+	dev_t			dev)
+{
+	if (ctx->mnt.fsgeom.rtstart) {
+		if (dev < XFS_DEV_DATA || dev > XFS_DEV_RT)
+			abort();
+		return dev;
+	}
+
+	if (dev == ctx->fsinfo.fs_datadev)
+		return XFS_DEV_DATA;
+	if (dev == ctx->fsinfo.fs_logdev)
+		return XFS_DEV_LOG;
+	if (dev == ctx->fsinfo.fs_rtdev)
+		return XFS_DEV_RT;
+	abort();
+}
+
+/* Return fsmap device for XFS device index. */
+static inline uint32_t
+to_fsmap_dev(
+	struct scrub_ctx	*ctx,
+	enum xfs_device		dev)
+{
+	if (ctx->mnt.fsgeom.rtstart)
+		return dev;
+
+	switch (dev) {
+	case XFS_DEV_DATA:
+		return ctx->fsinfo.fs_datadev;
+	case XFS_DEV_LOG:
+		return ctx->fsinfo.fs_logdev;
+	case XFS_DEV_RT:
+		return ctx->fsinfo.fs_rtdev;
+	default:
+		abort();
+	}
+}
+
 #endif /* XFS_SCRUB_SPACEMAP_H_ */
diff --git a/scrub/phase6.c b/scrub/phase6.c
index aef817add4157b..c562a07666516b 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -45,48 +45,6 @@ struct media_verify_state {
 	struct read_verify_pool	*rvp[XFS_DEV_RT + 1];
 };
 
-/* Return XFS device index from fsmap device. */
-static enum xfs_device
-from_fsmap_dev(
-	struct scrub_ctx	*ctx,
-	dev_t			dev)
-{
-	if (ctx->mnt.fsgeom.rtstart) {
-		if (dev < XFS_DEV_DATA || dev > XFS_DEV_RT)
-			abort();
-		return dev;
-	}
-
-	if (dev == ctx->fsinfo.fs_datadev)
-		return XFS_DEV_DATA;
-	if (dev == ctx->fsinfo.fs_logdev)
-		return XFS_DEV_LOG;
-	if (dev == ctx->fsinfo.fs_rtdev)
-		return XFS_DEV_RT;
-	abort();
-}
-
-/* Return fsmap device for XFS device index. */
-static uint32_t
-to_fsmap_dev(
-	struct scrub_ctx	*ctx,
-	enum xfs_device		dev)
-{
-	if (ctx->mnt.fsgeom.rtstart)
-		return dev;
-
-	switch (dev) {
-	case XFS_DEV_DATA:
-		return ctx->fsinfo.fs_datadev;
-	case XFS_DEV_LOG:
-		return ctx->fsinfo.fs_logdev;
-	case XFS_DEV_RT:
-		return ctx->fsinfo.fs_rtdev;
-	default:
-		abort();
-	}
-}
-
 struct disk_ioerr_report {
 	struct scrub_ctx	*ctx;
 	enum xfs_device		dev;
diff --git a/scrub/phase7.c b/scrub/phase7.c
index 375ba0632c353b..d741865295d043 100644
--- a/scrub/phase7.c
+++ b/scrub/phase7.c
@@ -70,19 +70,10 @@ count_block_summary(
 	void			*arg)
 {
 	struct summary_counts	*counts;
-	enum xfs_device		dev;
+	enum xfs_device		dev = from_fsmap_dev(ctx, fsmap->fmr_device);
 	unsigned long long	len;
 	int			ret;
 
-	if (ctx->mnt.fsgeom.rtstart)
-		dev = fsmap->fmr_device;
-	else if (fsmap->fmr_device == ctx->fsinfo.fs_logdev)
-		dev = XFS_DEV_LOG;
-	else if (fsmap->fmr_device == ctx->fsinfo.fs_rtdev)
-		dev = XFS_DEV_RT;
-	else
-		dev = XFS_DEV_DATA;
-
 	counts = ptvar_get((struct ptvar *)arg, &ret);
 	if (ret) {
 		str_liberror(ctx, -ret, _("retrieving summary counts"));
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index 8f595ad94c5991..45ac61d0942ccb 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -103,10 +103,7 @@ scan_ag_rmaps(
 	bperag = (off_t)ctx->mnt.fsgeom.agblocks *
 		 (off_t)ctx->mnt.fsgeom.blocksize;
 
-	if (ctx->mnt.fsgeom.rtstart)
-		keys[0].fmr_device = XFS_DEV_DATA;
-	else
-		keys[0].fmr_device = ctx->fsinfo.fs_datadev;
+	keys[0].fmr_device = to_fsmap_dev(ctx, XFS_DEV_DATA);
 	keys[0].fmr_physical = agno * bperag;
 	keys[1].fmr_device = keys[0].fmr_device;
 	keys[1].fmr_physical = ((agno + 1) * bperag) - 1;
@@ -143,10 +140,7 @@ scan_rtg_rmaps(
 	off_t			bperrg = bytes_per_rtgroup(&ctx->mnt.fsgeom);
 	int			ret;
 
-	if (ctx->mnt.fsgeom.rtstart)
-		keys[0].fmr_device = XFS_DEV_RT;
-	else
-		keys[0].fmr_device = ctx->fsinfo.fs_rtdev;
+	keys[0].fmr_device = to_fsmap_dev(ctx, XFS_DEV_RT);
 	keys[0].fmr_physical = (xfs_rtblock_t)rgno * bperrg;
 	keys[1].fmr_device = keys[0].fmr_device;
 	keys[1].fmr_physical = ((rgno + 1) * bperrg) - 1;
@@ -210,10 +204,10 @@ scan_rt_rmaps(
 {
 	struct scrub_ctx	*ctx = (struct scrub_ctx *)wq->wq_ctx;
 
-	scan_dev_rmaps(ctx, ctx->fsinfo.fs_rtdev, arg);
+	scan_dev_rmaps(ctx, to_fsmap_dev(ctx, XFS_DEV_RT), arg);
 }
 
-/* Iterate all the reverse mappings of the log device. */
+/* Iterate all the reverse mappings of the external log device. */
 static void
 scan_log_rmaps(
 	struct workqueue	*wq,
@@ -222,8 +216,7 @@ scan_log_rmaps(
 {
 	struct scrub_ctx	*ctx = (struct scrub_ctx *)wq->wq_ctx;
 
-	scan_dev_rmaps(ctx, ctx->mnt.fsgeom.rtstart ? 2 : ctx->fsinfo.fs_logdev,
-			arg);
+	scan_dev_rmaps(ctx, to_fsmap_dev(ctx, XFS_DEV_LOG), arg);
 }
 
 /*

Re: [PATCH 1/2] xfs_scrub: fix spacemap external log device scan dev key

[Christoph Hellwig]

Looks good:

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

[PATCH 2/2] xfs_scrub: fix estimate of work items for phase 4

[Darrick J. Wong]

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

Codex complains that the number of work items computed when estimating
the amount of work for phase 4 doesn't include the FSCOUNTERS and
QUOTACHECK items.  Add them back in.

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


diff --git a/scrub/phase4.c b/scrub/phase4.c
index 6bd16c23939eb4..95fb42f187fc77 100644
--- a/scrub/phase4.c
+++ b/scrub/phase4.c
@@ -254,7 +254,8 @@ phase4_func(
 	 * phase 7, but some of the cross-referencing requires fairly accurate
 	 * summary counters.  Check and try to repair them now to minimize the
 	 * chance that repairs of primary metadata fail due to secondary
-	 * metadata.  If repairs fails, we'll come back during phase 7.
+	 * metadata or ENOSPC on broken counters.  If repairs fails, we'll come
+	 * back during phase 7.
 	 */
 	scrub_item_init_fs(&sri);
 	scrub_item_schedule(&sri, XFS_SCRUB_TYPE_FSCOUNTERS);
@@ -269,6 +270,10 @@ phase4_func(
 	if (ret)
 		return ret;
 
+	/*
+	 * Try to fix the quota usage counts so that online repair doesn't
+	 * fail with EDQUOT (or worse shut down the fs) due to bad counts.
+	 */
 	if (fsgeom.sick & XFS_FSOP_GEOM_SICK_QUOTACHECK)
 		scrub_item_schedule(&sri, XFS_SCRUB_TYPE_QUOTACHECK);
 
@@ -293,10 +298,18 @@ phase4_estimate(
 {
 	unsigned long long	need_fixing;
 
-	/* Everything on the repair lis. */
+	/* Everything on the repair lists. */
 	need_fixing = action_list_length(ctx->fs_repair_list) +
 		      action_list_length(ctx->file_repair_list);
 
+	/*
+	 * fscounters and quotacheck are run directly by phase4_func
+	 * independent of the repair lists, so put that in the item count.
+	 * See phase4_func for why.
+	 */
+	if (need_fixing)
+		need_fixing += 2;
+
 	*items = need_fixing;
 	*nr_threads = scrub_nproc(ctx) + 1;
 	*rshift = 0;

Re: [PATCH 2/2] xfs_scrub: fix estimate of work items for phase 4

[Christoph Hellwig]

Looks good:

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