[PATCH 0/4] xfs: scrub tweaks

[PATCH 0/4] xfs: scrub tweaks

[Eric Sandeen]

A handful of random stuff.

[PATCH 1/4] xfs: add scrub to XFS_BUILD_OPTIONS

[Eric Sandeen]

Advertise this config option along with the others.

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

diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index fcc5dfc..8cee8e8 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -44,6 +44,12 @@
 # define XFS_REALTIME_STRING
 #endif
 
+#ifdef CONFIG_XFS_ONLINE_SCRUB
+# define XFS_SCRUB_STRING	"scrub, "
+#else
+# define XFS_SCRUB_STRING
+#endif
+
 #ifdef DEBUG
 # define XFS_DBG_STRING		"debug"
 #else
@@ -54,6 +60,7 @@
 #define XFS_BUILD_OPTIONS	XFS_ACL_STRING \
 				XFS_SECURITY_STRING \
 				XFS_REALTIME_STRING \
+				XFS_SCRUB_STRING \
 				XFS_DBG_STRING /* DBG must be last */
 
 struct xfs_inode;

[PATCH 2/4] xfs: explicitly initialize meta_scrub_ops array by type

[Eric Sandeen]

An implicit mapping to type by order of initialization seems
error-prone, and doesn't lend itself to cscope-ing.

Also add sanity checks about size of array vs. max types,
and a defensive check that ->scrub exists before using it.

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

diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 9c42c4e..dc1f33a 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -168,104 +168,104 @@
 /* Scrubbing dispatch. */
 
 static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
-	{ /* ioctl presence test */
+	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
 		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_probe,
 	},
-	{ /* superblock */
+	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
 		.setup	= xfs_scrub_setup_ag_header,
 		.scrub	= xfs_scrub_superblock,
 	},
-	{ /* agf */
+	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
 		.setup	= xfs_scrub_setup_ag_header,
 		.scrub	= xfs_scrub_agf,
 	},
-	{ /* agfl */
+	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
 		.setup	= xfs_scrub_setup_ag_header,
 		.scrub	= xfs_scrub_agfl,
 	},
-	{ /* agi */
+	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
 		.setup	= xfs_scrub_setup_ag_header,
 		.scrub	= xfs_scrub_agi,
 	},
-	{ /* bnobt */
+	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_bnobt,
 	},
-	{ /* cntbt */
+	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_cntbt,
 	},
-	{ /* inobt */
+	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_inobt,
 	},
-	{ /* finobt */
+	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_finobt,
 		.has	= xfs_sb_version_hasfinobt,
 	},
-	{ /* rmapbt */
+	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
 		.setup	= xfs_scrub_setup_ag_rmapbt,
 		.scrub	= xfs_scrub_rmapbt,
 		.has	= xfs_sb_version_hasrmapbt,
 	},
-	{ /* refcountbt */
+	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
 		.setup	= xfs_scrub_setup_ag_refcountbt,
 		.scrub	= xfs_scrub_refcountbt,
 		.has	= xfs_sb_version_hasreflink,
 	},
-	{ /* inode record */
+	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
 		.setup	= xfs_scrub_setup_inode,
 		.scrub	= xfs_scrub_inode,
 	},
-	{ /* inode data fork */
+	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_data,
 	},
-	{ /* inode attr fork */
+	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_attr,
 	},
-	{ /* inode CoW fork */
+	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_cow,
 	},
-	{ /* directory */
+	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
 		.setup	= xfs_scrub_setup_directory,
 		.scrub	= xfs_scrub_directory,
 	},
-	{ /* extended attributes */
+	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
 		.setup	= xfs_scrub_setup_xattr,
 		.scrub	= xfs_scrub_xattr,
 	},
-	{ /* symbolic link */
+	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
 		.setup	= xfs_scrub_setup_symlink,
 		.scrub	= xfs_scrub_symlink,
 	},
-	{ /* parent pointers */
+	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
 		.setup	= xfs_scrub_setup_parent,
 		.scrub	= xfs_scrub_parent,
 	},
-	{ /* realtime bitmap */
+	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtbitmap,
 		.has	= xfs_sb_version_hasrealtime,
 	},
-	{ /* realtime summary */
+	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtsummary,
 		.has	= xfs_sb_version_hasrealtime,
 	},
-	{ /* user quota */
+	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
 		.setup = xfs_scrub_setup_quota,
 		.scrub = xfs_scrub_quota,
 	},
-	{ /* group quota */
+	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
 		.setup = xfs_scrub_setup_quota,
 		.scrub = xfs_scrub_quota,
 	},
-	{ /* project quota */
+	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
 		.setup = xfs_scrub_setup_quota,
 		.scrub = xfs_scrub_quota,
 	},
@@ -297,6 +297,9 @@
 	bool				try_harder = false;
 	int				error = 0;
 
+	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
+		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
+
 	trace_xfs_scrub_start(ip, sm, error);
 
 	/* Forbidden if we are shut down or mounted norecovery. */
@@ -320,7 +323,7 @@
 	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
 		goto out;
 	ops = &meta_scrub_ops[sm->sm_type];
-	if (ops->scrub == NULL)
+	if (ops->setup == NULL || ops->scrub == NULL)
 		goto out;
 
 	/*

[PATCH 3/4] xfs: factor out scrub input checking

[Eric Sandeen]

Do this before adding more core checks.

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

diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index dc1f33a..16932f9 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -285,47 +285,34 @@
 "EXPERIMENTAL online scrub feature in use. Use at your own risk!");
 }
 
-/* Dispatch metadata scrubbing. */
-int
-xfs_scrub_metadata(
-	struct xfs_inode		*ip,
+static int
+xfs_scrub_validate(
+	struct xfs_mount		*mp,
 	struct xfs_scrub_metadata	*sm)
 {
-	struct xfs_scrub_context	sc;
-	struct xfs_mount		*mp = ip->i_mount;
+	int				error;
 	const struct xfs_scrub_meta_ops	*ops;
-	bool				try_harder = false;
-	int				error = 0;
-
-	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
-		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
-
-	trace_xfs_scrub_start(ip, sm, error);
-
-	/* Forbidden if we are shut down or mounted norecovery. */
-	error = -ESHUTDOWN;
-	if (XFS_FORCED_SHUTDOWN(mp))
-		goto out;
-	error = -ENOTRECOVERABLE;
-	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
-		goto out;
 
-	/* Check our inputs. */
 	error = -EINVAL;
+	/* Check our inputs. */
 	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
 	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
 		goto out;
 	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
 		goto out;
 
-	/* Do we know about this type of metadata? */
 	error = -ENOENT;
+	/* Do we know about this type of metadata? */
 	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
 		goto out;
 	ops = &meta_scrub_ops[sm->sm_type];
 	if (ops->setup == NULL || ops->scrub == NULL)
 		goto out;
+	/* Does this fs even support this type of metadata? */
+	if (ops->has && !ops->has(&mp->m_sb))
+		goto out;
 
+	error = -EOPNOTSUPP;
 	/*
 	 * We won't scrub any filesystem that doesn't have the ability
 	 * to record unwritten extents.  The option was made default in
@@ -335,20 +322,46 @@
 	 * We also don't support v1-v3 filesystems, which aren't
 	 * mountable.
 	 */
-	error = -EOPNOTSUPP;
 	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
 		goto out;
 
-	/* Does this fs even support this type of metadata? */
-	error = -ENOENT;
-	if (ops->has && !ops->has(&mp->m_sb))
-		goto out;
-
 	/* We don't know how to repair anything yet. */
-	error = -EOPNOTSUPP;
 	if (sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR)
 		goto out;
 
+	error = 0;
+out:
+	return error;
+}
+
+/* Dispatch metadata scrubbing. */
+int
+xfs_scrub_metadata(
+	struct xfs_inode		*ip,
+	struct xfs_scrub_metadata	*sm)
+{
+	struct xfs_scrub_context	sc;
+	struct xfs_mount		*mp = ip->i_mount;
+	bool				try_harder = false;
+	int				error = 0;
+
+	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
+		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
+
+	trace_xfs_scrub_start(ip, sm, error);
+
+	/* Forbidden if we are shut down or mounted norecovery. */
+	error = -ESHUTDOWN;
+	if (XFS_FORCED_SHUTDOWN(mp))
+		goto out;
+	error = -ENOTRECOVERABLE;
+	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
+		goto out;
+
+	error = xfs_scrub_validate(mp, sm);
+	if (error)
+		goto out;
+
 	xfs_scrub_experimental_warning(mp);
 
 retry_op:
@@ -356,7 +369,7 @@
 	memset(&sc, 0, sizeof(sc));
 	sc.mp = ip->i_mount;
 	sc.sm = sm;
-	sc.ops = ops;
+	sc.ops = &meta_scrub_ops[sm->sm_type]; 
 	sc.try_harder = try_harder;
 	sc.sa.agno = NULLAGNUMBER;
 	error = sc.ops->setup(&sc, ip);

[PATCH 4/4] xfs: move all scrub input checking to xfs_scrub_validate

[Eric Sandeen]

There were ad-hoc checks for some scrub types but not others;
mark each scrub type with ... it's type, and use that to validate
the allowed and/or required input fields.

Moving these checks out of xfs_scrub_setup_ag_header makes it
a thin wrapper, so unwrap it in the process.

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

enum scrub_type should probably go somewhere shared ...?

diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 2a9b4f9..b599358 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -37,23 +37,6 @@
 #include "scrub/common.h"
 #include "scrub/trace.h"
 
-/*
- * Set up scrub to check all the static metadata in each AG.
- * This means the SB, AGF, AGI, and AGFL headers.
- */
-int
-xfs_scrub_setup_ag_header(
-	struct xfs_scrub_context	*sc,
-	struct xfs_inode		*ip)
-{
-	struct xfs_mount		*mp = sc->mp;
-
-	if (sc->sm->sm_agno >= mp->m_sb.sb_agcount ||
-	    sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
-	return xfs_scrub_setup_fs(sc, ip);
-}
-
 /* Walk all the blocks in the AGFL. */
 int
 xfs_scrub_walk_agfl(
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index ac95fe9..98452ad 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -472,7 +472,7 @@
 			return error;
 	}
 
-	error = xfs_scrub_setup_ag_header(sc, ip);
+	error = xfs_scrub_setup_fs(sc, ip);
 	if (error)
 		return error;
 
@@ -507,14 +507,6 @@
 	struct xfs_inode		*ip = NULL;
 	int				error;
 
-	/*
-	 * If userspace passed us an AG number or a generation number
-	 * without an inode number, they haven't got a clue so bail out
-	 * immediately.
-	 */
-	if (sc->sm->sm_agno || (sc->sm->sm_gen && !sc->sm->sm_ino))
-		return -EINVAL;
-
 	/* We want to scan the inode we already had opened. */
 	if (sc->sm->sm_ino == 0 || sc->sm->sm_ino == ip_in->i_ino) {
 		sc->ip = ip_in;
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 5c04385..fe12053 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -78,8 +78,6 @@ void xfs_scrub_fblock_set_warning(struct xfs_scrub_context *sc, int whichfork,
 
 /* Setup functions */
 int xfs_scrub_setup_fs(struct xfs_scrub_context *sc, struct xfs_inode *ip);
-int xfs_scrub_setup_ag_header(struct xfs_scrub_context *sc,
-			      struct xfs_inode *ip);
 int xfs_scrub_setup_ag_allocbt(struct xfs_scrub_context *sc,
 			       struct xfs_inode *ip);
 int xfs_scrub_setup_ag_iallocbt(struct xfs_scrub_context *sc,
diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 2eac160..4587853 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -67,13 +67,6 @@
 {
 	uint				dqtype;
 
-	/*
-	 * If userspace gave us an AG number or inode data, they don't
-	 * know what they're doing.  Get out.
-	 */
-	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
-
 	dqtype = xfs_scrub_quota_to_dqtype(sc);
 	if (dqtype == 0)
 		return -EINVAL;
diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index c6fedb6..6860d5d 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -43,22 +43,14 @@
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	struct xfs_mount		*mp = sc->mp;
-	int				error = 0;
-
-	/*
-	 * If userspace gave us an AG number or inode data, they don't
-	 * know what they're doing.  Get out.
-	 */
-	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
+	int				error;
 
 	error = xfs_scrub_setup_fs(sc, ip);
 	if (error)
 		return error;
 
 	sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
-	sc->ip = mp->m_rbmip;
+	sc->ip = sc->mp->m_rbmip;
 	xfs_ilock(sc->ip, sc->ilock_flags);
 
 	return 0;
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 16932f9..6899126 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -129,8 +129,6 @@
 {
 	int				error = 0;
 
-	if (sc->sm->sm_ino || sc->sm->sm_agno)
-		return -EINVAL;
 	if (xfs_scrub_should_terminate(sc, &error))
 		return error;
 
@@ -169,105 +167,129 @@
 
 static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
 	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
+		.type	= ST_NONE,
 		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_probe,
 	},
 	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_superblock,
 	},
 	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agf,
 	},
 	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agfl,
 	},
 	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agi,
 	},
 	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_bnobt,
 	},
 	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_cntbt,
 	},
 	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_inobt,
 	},
 	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_finobt,
 		.has	= xfs_sb_version_hasfinobt,
 	},
 	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_rmapbt,
 		.scrub	= xfs_scrub_rmapbt,
 		.has	= xfs_sb_version_hasrmapbt,
 	},
 	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_refcountbt,
 		.scrub	= xfs_scrub_refcountbt,
 		.has	= xfs_sb_version_hasreflink,
 	},
 	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode,
 		.scrub	= xfs_scrub_inode,
 	},
 	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_data,
 	},
 	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_attr,
 	},
 	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_cow,
 	},
 	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_directory,
 		.scrub	= xfs_scrub_directory,
 	},
 	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_xattr,
 		.scrub	= xfs_scrub_xattr,
 	},
 	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_symlink,
 		.scrub	= xfs_scrub_symlink,
 	},
 	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_parent,
 		.scrub	= xfs_scrub_parent,
 	},
 	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
+		.type	= ST_FS,
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtbitmap,
 		.has	= xfs_sb_version_hasrealtime,
 	},
 	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
+		.type	= ST_FS,
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtsummary,
 		.has	= xfs_sb_version_hasrealtime,
 	},
 	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 };
 
@@ -298,14 +320,35 @@
 	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
 	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
 		goto out;
+	/* sm_reserved[] must be zero */
 	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
 		goto out;
 
+	ops = &meta_scrub_ops[sm->sm_type];
+	/* restricting fields must be appropriate for type */
+	switch (ops->type) {
+	case ST_NONE:
+	case ST_FS:
+		if (sm->sm_ino || sm->sm_gen || sm->sm_agno)
+			goto out;
+		break;
+	case ST_PERAG:
+		if (sm->sm_ino || sm->sm_gen ||
+		    sm->sm_agno >= mp->m_sb.sb_agcount)
+			goto out;
+		break;
+	case ST_INODE:
+		if (sm->sm_agno || (sm->sm_gen && !sm->sm_ino))
+			goto out;
+		break;
+	default:
+		goto out;
+	}
+
 	error = -ENOENT;
 	/* Do we know about this type of metadata? */
 	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
 		goto out;
-	ops = &meta_scrub_ops[sm->sm_type];
 	if (ops->setup == NULL || ops->scrub == NULL)
 		goto out;
 	/* Does this fs even support this type of metadata? */
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index e9ec041..47f098c 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -22,7 +22,17 @@
 
 struct xfs_scrub_context;
 
+/* Type info and names for the scrub types. */
+enum scrub_type {
+	ST_NONE = 1,	/* disabled */
+	ST_PERAG,	/* per-AG metadata */
+	ST_FS,		/* per-FS metadata */
+	ST_INODE,	/* per-inode metadata */
+};
+
 struct xfs_scrub_meta_ops {
+	/* type describing required/allowed inputs */
+	int		type;
 	/* Acquire whatever resources are needed for the operation. */
 	int		(*setup)(struct xfs_scrub_context *,
 				 struct xfs_inode *);

Re: [PATCH 4/4] xfs: move all scrub input checking to xfs_scrub_validate

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:15:04PM -0600, Eric Sandeen wrote:
> There were ad-hoc checks for some scrub types but not others;
> mark each scrub type with ... it's type, and use that to validate
> the allowed and/or required input fields.
> 
> Moving these checks out of xfs_scrub_setup_ag_header makes it
> a thin wrapper, so unwrap it in the process.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> enum scrub_type should probably go somewhere shared ...?
> 
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index 2a9b4f9..b599358 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -37,23 +37,6 @@
>  #include "scrub/common.h"
>  #include "scrub/trace.h"
>  
> -/*
> - * Set up scrub to check all the static metadata in each AG.
> - * This means the SB, AGF, AGI, and AGFL headers.
> - */
> -int
> -xfs_scrub_setup_ag_header(
> -	struct xfs_scrub_context	*sc,
> -	struct xfs_inode		*ip)
> -{
> -	struct xfs_mount		*mp = sc->mp;
> -
> -	if (sc->sm->sm_agno >= mp->m_sb.sb_agcount ||
> -	    sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> -	return xfs_scrub_setup_fs(sc, ip);
> -}
> -
>  /* Walk all the blocks in the AGFL. */
>  int
>  xfs_scrub_walk_agfl(
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index ac95fe9..98452ad 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -472,7 +472,7 @@
>  			return error;
>  	}
>  
> -	error = xfs_scrub_setup_ag_header(sc, ip);
> +	error = xfs_scrub_setup_fs(sc, ip);
>  	if (error)
>  		return error;
>  
> @@ -507,14 +507,6 @@
>  	struct xfs_inode		*ip = NULL;
>  	int				error;
>  
> -	/*
> -	 * If userspace passed us an AG number or a generation number
> -	 * without an inode number, they haven't got a clue so bail out
> -	 * immediately.
> -	 */
> -	if (sc->sm->sm_agno || (sc->sm->sm_gen && !sc->sm->sm_ino))
> -		return -EINVAL;
> -
>  	/* We want to scan the inode we already had opened. */
>  	if (sc->sm->sm_ino == 0 || sc->sm->sm_ino == ip_in->i_ino) {
>  		sc->ip = ip_in;
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index 5c04385..fe12053 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -78,8 +78,6 @@ void xfs_scrub_fblock_set_warning(struct xfs_scrub_context *sc, int whichfork,
>  
>  /* Setup functions */
>  int xfs_scrub_setup_fs(struct xfs_scrub_context *sc, struct xfs_inode *ip);
> -int xfs_scrub_setup_ag_header(struct xfs_scrub_context *sc,
> -			      struct xfs_inode *ip);
>  int xfs_scrub_setup_ag_allocbt(struct xfs_scrub_context *sc,
>  			       struct xfs_inode *ip);
>  int xfs_scrub_setup_ag_iallocbt(struct xfs_scrub_context *sc,
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index 2eac160..4587853 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -67,13 +67,6 @@
>  {
>  	uint				dqtype;
>  
> -	/*
> -	 * If userspace gave us an AG number or inode data, they don't
> -	 * know what they're doing.  Get out.
> -	 */
> -	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> -
>  	dqtype = xfs_scrub_quota_to_dqtype(sc);
>  	if (dqtype == 0)
>  		return -EINVAL;
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index c6fedb6..6860d5d 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -43,22 +43,14 @@
>  	struct xfs_scrub_context	*sc,
>  	struct xfs_inode		*ip)
>  {
> -	struct xfs_mount		*mp = sc->mp;
> -	int				error = 0;
> -
> -	/*
> -	 * If userspace gave us an AG number or inode data, they don't
> -	 * know what they're doing.  Get out.
> -	 */
> -	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> +	int				error;
>  
>  	error = xfs_scrub_setup_fs(sc, ip);
>  	if (error)
>  		return error;
>  
>  	sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
> -	sc->ip = mp->m_rbmip;
> +	sc->ip = sc->mp->m_rbmip;
>  	xfs_ilock(sc->ip, sc->ilock_flags);
>  
>  	return 0;
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 16932f9..6899126 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -129,8 +129,6 @@
>  {
>  	int				error = 0;
>  
> -	if (sc->sm->sm_ino || sc->sm->sm_agno)
> -		return -EINVAL;
>  	if (xfs_scrub_should_terminate(sc, &error))
>  		return error;
>  
> @@ -169,105 +167,129 @@
>  
>  static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
>  	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
> +		.type	= ST_NONE,
>  		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_probe,
>  	},
>  	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_superblock,
>  	},
>  	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agf,
>  	},
>  	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agfl,
>  	},
>  	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agi,
>  	},
>  	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_bnobt,
>  	},
>  	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_cntbt,
>  	},
>  	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_inobt,
>  	},
>  	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_finobt,
>  		.has	= xfs_sb_version_hasfinobt,
>  	},
>  	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_rmapbt,
>  		.scrub	= xfs_scrub_rmapbt,
>  		.has	= xfs_sb_version_hasrmapbt,
>  	},
>  	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_refcountbt,
>  		.scrub	= xfs_scrub_refcountbt,
>  		.has	= xfs_sb_version_hasreflink,
>  	},
>  	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode,
>  		.scrub	= xfs_scrub_inode,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_data,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_attr,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_cow,
>  	},
>  	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_directory,
>  		.scrub	= xfs_scrub_directory,
>  	},
>  	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_xattr,
>  		.scrub	= xfs_scrub_xattr,
>  	},
>  	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_symlink,
>  		.scrub	= xfs_scrub_symlink,
>  	},
>  	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_parent,
>  		.scrub	= xfs_scrub_parent,
>  	},
>  	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
> +		.type	= ST_FS,
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtbitmap,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
>  	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
> +		.type	= ST_FS,
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtsummary,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
>  	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  };
>  
> @@ -298,14 +320,35 @@
>  	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
>  	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
>  		goto out;
> +	/* sm_reserved[] must be zero */
>  	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
>  		goto out;
>  
> +	ops = &meta_scrub_ops[sm->sm_type];
> +	/* restricting fields must be appropriate for type */
> +	switch (ops->type) {
> +	case ST_NONE:
> +	case ST_FS:
> +		if (sm->sm_ino || sm->sm_gen || sm->sm_agno)
> +			goto out;
> +		break;
> +	case ST_PERAG:
> +		if (sm->sm_ino || sm->sm_gen ||
> +		    sm->sm_agno >= mp->m_sb.sb_agcount)
> +			goto out;
> +		break;
> +	case ST_INODE:
> +		if (sm->sm_agno || (sm->sm_gen && !sm->sm_ino))
> +			goto out;
> +		break;
> +	default:
> +		goto out;
> +	}
> +
>  	error = -ENOENT;
>  	/* Do we know about this type of metadata? */
>  	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
>  		goto out;
> -	ops = &meta_scrub_ops[sm->sm_type];
>  	if (ops->setup == NULL || ops->scrub == NULL)
>  		goto out;
>  	/* Does this fs even support this type of metadata? */
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index e9ec041..47f098c 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -22,7 +22,17 @@
>  
>  struct xfs_scrub_context;
>  
> +/* Type info and names for the scrub types. */
> +enum scrub_type {

enum xfs_scrub_type ?

> +	ST_NONE = 1,	/* disabled */
> +	ST_PERAG,	/* per-AG metadata */
> +	ST_FS,		/* per-FS metadata */
> +	ST_INODE,	/* per-inode metadata */
> +};
> +
>  struct xfs_scrub_meta_ops {
> +	/* type describing required/allowed inputs */
> +	int		type;

enum xfs_scrub_type	type; ?

--D

>  	/* Acquire whatever resources are needed for the operation. */
>  	int		(*setup)(struct xfs_scrub_context *,
>  				 struct xfs_inode *);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] xfs: factor out scrub input checking

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:14:15PM -0600, Eric Sandeen wrote:
> Do this before adding more core checks.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index dc1f33a..16932f9 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -285,47 +285,34 @@
>  "EXPERIMENTAL online scrub feature in use. Use at your own risk!");
>  }
>  
> -/* Dispatch metadata scrubbing. */
> -int
> -xfs_scrub_metadata(
> -	struct xfs_inode		*ip,
> +static int
> +xfs_scrub_validate(
> +	struct xfs_mount		*mp,
>  	struct xfs_scrub_metadata	*sm)
>  {
> -	struct xfs_scrub_context	sc;
> -	struct xfs_mount		*mp = ip->i_mount;
> +	int				error;
>  	const struct xfs_scrub_meta_ops	*ops;
> -	bool				try_harder = false;
> -	int				error = 0;
> -
> -	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
> -		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
> -
> -	trace_xfs_scrub_start(ip, sm, error);
> -
> -	/* Forbidden if we are shut down or mounted norecovery. */
> -	error = -ESHUTDOWN;
> -	if (XFS_FORCED_SHUTDOWN(mp))
> -		goto out;
> -	error = -ENOTRECOVERABLE;
> -	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
> -		goto out;
>  
> -	/* Check our inputs. */
>  	error = -EINVAL;
> +	/* Check our inputs. */
>  	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
>  	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
>  		goto out;
>  	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
>  		goto out;
>  
> -	/* Do we know about this type of metadata? */
>  	error = -ENOENT;
> +	/* Do we know about this type of metadata? */
>  	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
>  		goto out;
>  	ops = &meta_scrub_ops[sm->sm_type];
>  	if (ops->setup == NULL || ops->scrub == NULL)
>  		goto out;
> +	/* Does this fs even support this type of metadata? */
> +	if (ops->has && !ops->has(&mp->m_sb))
> +		goto out;
>  
> +	error = -EOPNOTSUPP;
>  	/*
>  	 * We won't scrub any filesystem that doesn't have the ability
>  	 * to record unwritten extents.  The option was made default in
> @@ -335,20 +322,46 @@
>  	 * We also don't support v1-v3 filesystems, which aren't
>  	 * mountable.
>  	 */
> -	error = -EOPNOTSUPP;
>  	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
>  		goto out;
>  
> -	/* Does this fs even support this type of metadata? */
> -	error = -ENOENT;
> -	if (ops->has && !ops->has(&mp->m_sb))
> -		goto out;
> -
>  	/* We don't know how to repair anything yet. */
> -	error = -EOPNOTSUPP;
>  	if (sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR)
>  		goto out;
>  
> +	error = 0;
> +out:
> +	return error;
> +}
> +
> +/* Dispatch metadata scrubbing. */
> +int
> +xfs_scrub_metadata(
> +	struct xfs_inode		*ip,
> +	struct xfs_scrub_metadata	*sm)
> +{
> +	struct xfs_scrub_context	sc;
> +	struct xfs_mount		*mp = ip->i_mount;
> +	bool				try_harder = false;
> +	int				error = 0;
> +
> +	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
> +		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
> +
> +	trace_xfs_scrub_start(ip, sm, error);
> +
> +	/* Forbidden if we are shut down or mounted norecovery. */
> +	error = -ESHUTDOWN;
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		goto out;
> +	error = -ENOTRECOVERABLE;
> +	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
> +		goto out;
> +
> +	error = xfs_scrub_validate(mp, sm);

I might've named this xfs_scrub_validate_inputs, but eh.

> +	if (error)
> +		goto out;
> +
>  	xfs_scrub_experimental_warning(mp);
>  
>  retry_op:
> @@ -356,7 +369,7 @@
>  	memset(&sc, 0, sizeof(sc));
>  	sc.mp = ip->i_mount;
>  	sc.sm = sm;
> -	sc.ops = ops;
> +	sc.ops = &meta_scrub_ops[sm->sm_type]; 

Trailing whitespace.

--D

>  	sc.try_harder = try_harder;
>  	sc.sa.agno = NULLAGNUMBER;
>  	error = sc.ops->setup(&sc, ip);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] xfs: move all scrub input checking to xfs_scrub_validate

[Eric Sandeen]

On 12/1/17 4:24 PM, Darrick J. Wong wrote:
>> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
>> index e9ec041..47f098c 100644
>> --- a/fs/xfs/scrub/scrub.h
>> +++ b/fs/xfs/scrub/scrub.h
>> @@ -22,7 +22,17 @@
>>  
>>  struct xfs_scrub_context;
>>  
>> +/* Type info and names for the scrub types. */
>> +enum scrub_type {
> enum xfs_scrub_type ?
> 

Oh, sure.  I can resend or feel free to fix it on the way in if that's
all there is.

-Eric

Re: [PATCH 2/4] xfs: explicitly initialize meta_scrub_ops array by type

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:13:27PM -0600, Eric Sandeen wrote:
> An implicit mapping to type by order of initialization seems
> error-prone, and doesn't lend itself to cscope-ing.
> 
> Also add sanity checks about size of array vs. max types,
> and a defensive check that ->scrub exists before using it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 9c42c4e..dc1f33a 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -168,104 +168,104 @@
>  /* Scrubbing dispatch. */
>  
>  static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
> -	{ /* ioctl presence test */
> +	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
>  		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_probe,
>  	},
> -	{ /* superblock */
> +	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
>  		.setup	= xfs_scrub_setup_ag_header,
>  		.scrub	= xfs_scrub_superblock,
>  	},
> -	{ /* agf */
> +	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
>  		.setup	= xfs_scrub_setup_ag_header,
>  		.scrub	= xfs_scrub_agf,
>  	},
> -	{ /* agfl */
> +	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
>  		.setup	= xfs_scrub_setup_ag_header,
>  		.scrub	= xfs_scrub_agfl,
>  	},
> -	{ /* agi */
> +	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
>  		.setup	= xfs_scrub_setup_ag_header,
>  		.scrub	= xfs_scrub_agi,
>  	},
> -	{ /* bnobt */
> +	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_bnobt,
>  	},
> -	{ /* cntbt */
> +	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_cntbt,
>  	},
> -	{ /* inobt */
> +	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_inobt,
>  	},
> -	{ /* finobt */
> +	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_finobt,
>  		.has	= xfs_sb_version_hasfinobt,
>  	},
> -	{ /* rmapbt */
> +	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
>  		.setup	= xfs_scrub_setup_ag_rmapbt,
>  		.scrub	= xfs_scrub_rmapbt,
>  		.has	= xfs_sb_version_hasrmapbt,
>  	},
> -	{ /* refcountbt */
> +	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
>  		.setup	= xfs_scrub_setup_ag_refcountbt,
>  		.scrub	= xfs_scrub_refcountbt,
>  		.has	= xfs_sb_version_hasreflink,
>  	},
> -	{ /* inode record */
> +	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
>  		.setup	= xfs_scrub_setup_inode,
>  		.scrub	= xfs_scrub_inode,
>  	},
> -	{ /* inode data fork */
> +	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_data,
>  	},
> -	{ /* inode attr fork */
> +	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_attr,
>  	},
> -	{ /* inode CoW fork */
> +	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_cow,
>  	},
> -	{ /* directory */
> +	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
>  		.setup	= xfs_scrub_setup_directory,
>  		.scrub	= xfs_scrub_directory,
>  	},
> -	{ /* extended attributes */
> +	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
>  		.setup	= xfs_scrub_setup_xattr,
>  		.scrub	= xfs_scrub_xattr,
>  	},
> -	{ /* symbolic link */
> +	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
>  		.setup	= xfs_scrub_setup_symlink,
>  		.scrub	= xfs_scrub_symlink,
>  	},
> -	{ /* parent pointers */
> +	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
>  		.setup	= xfs_scrub_setup_parent,
>  		.scrub	= xfs_scrub_parent,
>  	},
> -	{ /* realtime bitmap */
> +	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtbitmap,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
> -	{ /* realtime summary */
> +	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtsummary,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
> -	{ /* user quota */
> +	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
>  		.setup = xfs_scrub_setup_quota,
>  		.scrub = xfs_scrub_quota,
>  	},
> -	{ /* group quota */
> +	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
>  		.setup = xfs_scrub_setup_quota,
>  		.scrub = xfs_scrub_quota,
>  	},
> -	{ /* project quota */
> +	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
>  		.setup = xfs_scrub_setup_quota,
>  		.scrub = xfs_scrub_quota,
>  	},
> @@ -297,6 +297,9 @@
>  	bool				try_harder = false;
>  	int				error = 0;
>  
> +	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
> +		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
> +
>  	trace_xfs_scrub_start(ip, sm, error);
>  
>  	/* Forbidden if we are shut down or mounted norecovery. */
> @@ -320,7 +323,7 @@
>  	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
>  		goto out;
>  	ops = &meta_scrub_ops[sm->sm_type];
> -	if (ops->scrub == NULL)
> +	if (ops->setup == NULL || ops->scrub == NULL)
>  		goto out;
>  
>  	/*
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] xfs: add scrub to XFS_BUILD_OPTIONS

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:12:34PM -0600, Eric Sandeen wrote:
> Advertise this config option along with the others.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index fcc5dfc..8cee8e8 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -44,6 +44,12 @@
>  # define XFS_REALTIME_STRING
>  #endif
>  
> +#ifdef CONFIG_XFS_ONLINE_SCRUB
> +# define XFS_SCRUB_STRING	"scrub, "
> +#else
> +# define XFS_SCRUB_STRING
> +#endif

I don't mind this addition to the build options, though I had pictured
scrub becoming an integral part of xfs some day and not remaining a
configurable option.

That said I haven't evaluated how much scrub bloats up xfs nor do I know
if the kernel tinyfication people would actually want to turn it off??

(For my part I prefer it some day not be Kconfig option so that it won't
bitrot ala CONFIG_XFS_RT=y.)

--D

> +
>  #ifdef DEBUG
>  # define XFS_DBG_STRING		"debug"
>  #else
> @@ -54,6 +60,7 @@
>  #define XFS_BUILD_OPTIONS	XFS_ACL_STRING \
>  				XFS_SECURITY_STRING \
>  				XFS_REALTIME_STRING \
> +				XFS_SCRUB_STRING \
>  				XFS_DBG_STRING /* DBG must be last */
>  
>  struct xfs_inode;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] xfs: add scrub to XFS_BUILD_OPTIONS

[Eric Sandeen]

On 12/1/17 4:29 PM, Darrick J. Wong wrote:
> On Fri, Dec 01, 2017 at 04:12:34PM -0600, Eric Sandeen wrote:
>> Advertise this config option along with the others.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
>> index fcc5dfc..8cee8e8 100644
>> --- a/fs/xfs/xfs_super.h
>> +++ b/fs/xfs/xfs_super.h
>> @@ -44,6 +44,12 @@
>>  # define XFS_REALTIME_STRING
>>  #endif
>>  
>> +#ifdef CONFIG_XFS_ONLINE_SCRUB
>> +# define XFS_SCRUB_STRING	"scrub, "
>> +#else
>> +# define XFS_SCRUB_STRING
>> +#endif
> 
> I don't mind this addition to the build options, though I had pictured
> scrub becoming an integral part of xfs some day and not remaining a
> configurable option.

It could be removed at that time, unless it's considered an API?

I'm not hung up on this, if there's no reason for it that's OK.

> That said I haven't evaluated how much scrub bloats up xfs nor do I know
> if the kernel tinyfication people would actually want to turn it off??
> 
> (For my part I prefer it some day not be Kconfig option so that it won't
> bitrot ala CONFIG_XFS_RT=y.)

I'd imagine that it'd be turned on more often than RT but that's just a guess.

-Eric
 

Re: [PATCH 0/4] xfs: scrub tweaks

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:11:59PM -0600, Eric Sandeen wrote:
> A handful of random stuff.

For the whole set:
Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] xfs: move all scrub input checking to xfs_scrub_validate

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:15:04PM -0600, Eric Sandeen wrote:
> There were ad-hoc checks for some scrub types but not others;
> mark each scrub type with ... it's type, and use that to validate
> the allowed and/or required input fields.
> 
> Moving these checks out of xfs_scrub_setup_ag_header makes it
> a thin wrapper, so unwrap it in the process.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> enum scrub_type should probably go somewhere shared ...?
> 
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index 2a9b4f9..b599358 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -37,23 +37,6 @@
>  #include "scrub/common.h"
>  #include "scrub/trace.h"
>  
> -/*
> - * Set up scrub to check all the static metadata in each AG.
> - * This means the SB, AGF, AGI, and AGFL headers.
> - */
> -int
> -xfs_scrub_setup_ag_header(
> -	struct xfs_scrub_context	*sc,
> -	struct xfs_inode		*ip)
> -{
> -	struct xfs_mount		*mp = sc->mp;
> -
> -	if (sc->sm->sm_agno >= mp->m_sb.sb_agcount ||
> -	    sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> -	return xfs_scrub_setup_fs(sc, ip);
> -}
> -
>  /* Walk all the blocks in the AGFL. */
>  int
>  xfs_scrub_walk_agfl(
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index ac95fe9..98452ad 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -472,7 +472,7 @@
>  			return error;
>  	}
>  
> -	error = xfs_scrub_setup_ag_header(sc, ip);
> +	error = xfs_scrub_setup_fs(sc, ip);
>  	if (error)
>  		return error;
>  
> @@ -507,14 +507,6 @@
>  	struct xfs_inode		*ip = NULL;
>  	int				error;
>  
> -	/*
> -	 * If userspace passed us an AG number or a generation number
> -	 * without an inode number, they haven't got a clue so bail out
> -	 * immediately.
> -	 */
> -	if (sc->sm->sm_agno || (sc->sm->sm_gen && !sc->sm->sm_ino))
> -		return -EINVAL;
> -
>  	/* We want to scan the inode we already had opened. */
>  	if (sc->sm->sm_ino == 0 || sc->sm->sm_ino == ip_in->i_ino) {
>  		sc->ip = ip_in;
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index 5c04385..fe12053 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -78,8 +78,6 @@ void xfs_scrub_fblock_set_warning(struct xfs_scrub_context *sc, int whichfork,
>  
>  /* Setup functions */
>  int xfs_scrub_setup_fs(struct xfs_scrub_context *sc, struct xfs_inode *ip);
> -int xfs_scrub_setup_ag_header(struct xfs_scrub_context *sc,
> -			      struct xfs_inode *ip);
>  int xfs_scrub_setup_ag_allocbt(struct xfs_scrub_context *sc,
>  			       struct xfs_inode *ip);
>  int xfs_scrub_setup_ag_iallocbt(struct xfs_scrub_context *sc,
> diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
> index 2eac160..4587853 100644
> --- a/fs/xfs/scrub/quota.c
> +++ b/fs/xfs/scrub/quota.c
> @@ -67,13 +67,6 @@
>  {
>  	uint				dqtype;
>  
> -	/*
> -	 * If userspace gave us an AG number or inode data, they don't
> -	 * know what they're doing.  Get out.
> -	 */
> -	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> -
>  	dqtype = xfs_scrub_quota_to_dqtype(sc);
>  	if (dqtype == 0)
>  		return -EINVAL;
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index c6fedb6..6860d5d 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -43,22 +43,14 @@
>  	struct xfs_scrub_context	*sc,
>  	struct xfs_inode		*ip)
>  {
> -	struct xfs_mount		*mp = sc->mp;
> -	int				error = 0;
> -
> -	/*
> -	 * If userspace gave us an AG number or inode data, they don't
> -	 * know what they're doing.  Get out.
> -	 */
> -	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> -		return -EINVAL;
> +	int				error;
>  
>  	error = xfs_scrub_setup_fs(sc, ip);
>  	if (error)
>  		return error;
>  
>  	sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
> -	sc->ip = mp->m_rbmip;
> +	sc->ip = sc->mp->m_rbmip;
>  	xfs_ilock(sc->ip, sc->ilock_flags);
>  
>  	return 0;
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 16932f9..6899126 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -129,8 +129,6 @@
>  {
>  	int				error = 0;
>  
> -	if (sc->sm->sm_ino || sc->sm->sm_agno)
> -		return -EINVAL;
>  	if (xfs_scrub_should_terminate(sc, &error))
>  		return error;
>  
> @@ -169,105 +167,129 @@
>  
>  static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
>  	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
> +		.type	= ST_NONE,
>  		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_probe,
>  	},
>  	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_superblock,
>  	},
>  	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agf,
>  	},
>  	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agfl,
>  	},
>  	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
> -		.setup	= xfs_scrub_setup_ag_header,
> +		.type	= ST_PERAG,
> +		.setup	= xfs_scrub_setup_fs,
>  		.scrub	= xfs_scrub_agi,
>  	},
>  	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_bnobt,
>  	},
>  	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_allocbt,
>  		.scrub	= xfs_scrub_cntbt,
>  	},
>  	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_inobt,
>  	},
>  	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_iallocbt,
>  		.scrub	= xfs_scrub_finobt,
>  		.has	= xfs_sb_version_hasfinobt,
>  	},
>  	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_rmapbt,
>  		.scrub	= xfs_scrub_rmapbt,
>  		.has	= xfs_sb_version_hasrmapbt,
>  	},
>  	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
> +		.type	= ST_PERAG,
>  		.setup	= xfs_scrub_setup_ag_refcountbt,
>  		.scrub	= xfs_scrub_refcountbt,
>  		.has	= xfs_sb_version_hasreflink,
>  	},
>  	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode,
>  		.scrub	= xfs_scrub_inode,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_data,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_attr,
>  	},
>  	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_inode_bmap,
>  		.scrub	= xfs_scrub_bmap_cow,
>  	},
>  	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_directory,
>  		.scrub	= xfs_scrub_directory,
>  	},
>  	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_xattr,
>  		.scrub	= xfs_scrub_xattr,
>  	},
>  	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_symlink,
>  		.scrub	= xfs_scrub_symlink,
>  	},
>  	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
> +		.type	= ST_INODE,
>  		.setup	= xfs_scrub_setup_parent,
>  		.scrub	= xfs_scrub_parent,
>  	},
>  	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
> +		.type	= ST_FS,
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtbitmap,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
>  	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
> +		.type	= ST_FS,
>  		.setup	= xfs_scrub_setup_rt,
>  		.scrub	= xfs_scrub_rtsummary,
>  		.has	= xfs_sb_version_hasrealtime,
>  	},
>  	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
> -		.setup = xfs_scrub_setup_quota,
> -		.scrub = xfs_scrub_quota,
> +		.type	= ST_FS,
> +		.setup	= xfs_scrub_setup_quota,
> +		.scrub	= xfs_scrub_quota,
>  	},
>  };
>  
> @@ -298,14 +320,35 @@
>  	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
>  	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
>  		goto out;
> +	/* sm_reserved[] must be zero */
>  	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
>  		goto out;
>  
> +	ops = &meta_scrub_ops[sm->sm_type];

This has to come after we range-check sm_type.

--D

> +	/* restricting fields must be appropriate for type */
> +	switch (ops->type) {
> +	case ST_NONE:
> +	case ST_FS:
> +		if (sm->sm_ino || sm->sm_gen || sm->sm_agno)
> +			goto out;
> +		break;
> +	case ST_PERAG:
> +		if (sm->sm_ino || sm->sm_gen ||
> +		    sm->sm_agno >= mp->m_sb.sb_agcount)
> +			goto out;
> +		break;
> +	case ST_INODE:
> +		if (sm->sm_agno || (sm->sm_gen && !sm->sm_ino))
> +			goto out;
> +		break;
> +	default:
> +		goto out;
> +	}
> +
>  	error = -ENOENT;
>  	/* Do we know about this type of metadata? */
>  	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
>  		goto out;
> -	ops = &meta_scrub_ops[sm->sm_type];
>  	if (ops->setup == NULL || ops->scrub == NULL)
>  		goto out;
>  	/* Does this fs even support this type of metadata? */
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index e9ec041..47f098c 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -22,7 +22,17 @@
>  
>  struct xfs_scrub_context;
>  
> +/* Type info and names for the scrub types. */
> +enum scrub_type {
> +	ST_NONE = 1,	/* disabled */
> +	ST_PERAG,	/* per-AG metadata */
> +	ST_FS,		/* per-FS metadata */
> +	ST_INODE,	/* per-inode metadata */
> +};
> +
>  struct xfs_scrub_meta_ops {
> +	/* type describing required/allowed inputs */
> +	int		type;
>  	/* Acquire whatever resources are needed for the operation. */
>  	int		(*setup)(struct xfs_scrub_context *,
>  				 struct xfs_inode *);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] xfs: factor out scrub input checking

[Darrick J. Wong]

From: Eric Sandeen <sandeen@sandeen.net>

Do this before adding more core checks.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
[darrick: rename input validation function]
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/scrub.c |   75 +++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index dc1f33a..182d2fe 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -285,47 +285,34 @@ xfs_scrub_experimental_warning(
 "EXPERIMENTAL online scrub feature in use. Use at your own risk!");
 }
 
-/* Dispatch metadata scrubbing. */
-int
-xfs_scrub_metadata(
-	struct xfs_inode		*ip,
+static int
+xfs_scrub_validate_inputs(
+	struct xfs_mount		*mp,
 	struct xfs_scrub_metadata	*sm)
 {
-	struct xfs_scrub_context	sc;
-	struct xfs_mount		*mp = ip->i_mount;
+	int				error;
 	const struct xfs_scrub_meta_ops	*ops;
-	bool				try_harder = false;
-	int				error = 0;
-
-	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
-		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
-
-	trace_xfs_scrub_start(ip, sm, error);
-
-	/* Forbidden if we are shut down or mounted norecovery. */
-	error = -ESHUTDOWN;
-	if (XFS_FORCED_SHUTDOWN(mp))
-		goto out;
-	error = -ENOTRECOVERABLE;
-	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
-		goto out;
 
-	/* Check our inputs. */
 	error = -EINVAL;
+	/* Check our inputs. */
 	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
 	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
 		goto out;
 	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
 		goto out;
 
-	/* Do we know about this type of metadata? */
 	error = -ENOENT;
+	/* Do we know about this type of metadata? */
 	if (sm->sm_type >= XFS_SCRUB_TYPE_NR)
 		goto out;
 	ops = &meta_scrub_ops[sm->sm_type];
 	if (ops->setup == NULL || ops->scrub == NULL)
 		goto out;
+	/* Does this fs even support this type of metadata? */
+	if (ops->has && !ops->has(&mp->m_sb))
+		goto out;
 
+	error = -EOPNOTSUPP;
 	/*
 	 * We won't scrub any filesystem that doesn't have the ability
 	 * to record unwritten extents.  The option was made default in
@@ -335,20 +322,46 @@ xfs_scrub_metadata(
 	 * We also don't support v1-v3 filesystems, which aren't
 	 * mountable.
 	 */
-	error = -EOPNOTSUPP;
 	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
 		goto out;
 
-	/* Does this fs even support this type of metadata? */
-	error = -ENOENT;
-	if (ops->has && !ops->has(&mp->m_sb))
-		goto out;
-
 	/* We don't know how to repair anything yet. */
-	error = -EOPNOTSUPP;
 	if (sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR)
 		goto out;
 
+	error = 0;
+out:
+	return error;
+}
+
+/* Dispatch metadata scrubbing. */
+int
+xfs_scrub_metadata(
+	struct xfs_inode		*ip,
+	struct xfs_scrub_metadata	*sm)
+{
+	struct xfs_scrub_context	sc;
+	struct xfs_mount		*mp = ip->i_mount;
+	bool				try_harder = false;
+	int				error = 0;
+
+	BUILD_BUG_ON(sizeof(meta_scrub_ops) !=
+		(sizeof(struct xfs_scrub_meta_ops) * XFS_SCRUB_TYPE_NR));
+
+	trace_xfs_scrub_start(ip, sm, error);
+
+	/* Forbidden if we are shut down or mounted norecovery. */
+	error = -ESHUTDOWN;
+	if (XFS_FORCED_SHUTDOWN(mp))
+		goto out;
+	error = -ENOTRECOVERABLE;
+	if (mp->m_flags & XFS_MOUNT_NORECOVERY)
+		goto out;
+
+	error = xfs_scrub_validate_inputs(mp, sm);
+	if (error)
+		goto out;
+
 	xfs_scrub_experimental_warning(mp);
 
 retry_op:
@@ -356,7 +369,7 @@ xfs_scrub_metadata(
 	memset(&sc, 0, sizeof(sc));
 	sc.mp = ip->i_mount;
 	sc.sm = sm;
-	sc.ops = ops;
+	sc.ops = &meta_scrub_ops[sm->sm_type];
 	sc.try_harder = try_harder;
 	sc.sa.agno = NULLAGNUMBER;
 	error = sc.ops->setup(&sc, ip);

[PATCH v2 4/4] xfs: move all scrub input checking to xfs_scrub_validate

[Darrick J. Wong]

From: Eric Sandeen <sandeen@sandeen.net>

There were ad-hoc checks for some scrub types but not others;
mark each scrub type with ... it's type, and use that to validate
the allowed and/or required input fields.

Moving these checks out of xfs_scrub_setup_ag_header makes it
a thin wrapper, so unwrap it in the process.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
[darrick: add xfs_ prefix to enum, check scrub args after checking type]
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/agheader.c |   17 ------------
 fs/xfs/scrub/common.c   |   10 +------
 fs/xfs/scrub/common.h   |    2 -
 fs/xfs/scrub/quota.c    |    7 -----
 fs/xfs/scrub/rtbitmap.c |   12 +-------
 fs/xfs/scrub/scrub.c    |   68 +++++++++++++++++++++++++++++++++++++++--------
 fs/xfs/scrub/scrub.h    |   11 ++++++++
 7 files changed, 70 insertions(+), 57 deletions(-)

diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 2a9b4f9..b599358 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -37,23 +37,6 @@
 #include "scrub/common.h"
 #include "scrub/trace.h"
 
-/*
- * Set up scrub to check all the static metadata in each AG.
- * This means the SB, AGF, AGI, and AGFL headers.
- */
-int
-xfs_scrub_setup_ag_header(
-	struct xfs_scrub_context	*sc,
-	struct xfs_inode		*ip)
-{
-	struct xfs_mount		*mp = sc->mp;
-
-	if (sc->sm->sm_agno >= mp->m_sb.sb_agcount ||
-	    sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
-	return xfs_scrub_setup_fs(sc, ip);
-}
-
 /* Walk all the blocks in the AGFL. */
 int
 xfs_scrub_walk_agfl(
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index ac95fe9..98452ad 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -472,7 +472,7 @@ xfs_scrub_setup_ag_btree(
 			return error;
 	}
 
-	error = xfs_scrub_setup_ag_header(sc, ip);
+	error = xfs_scrub_setup_fs(sc, ip);
 	if (error)
 		return error;
 
@@ -507,14 +507,6 @@ xfs_scrub_get_inode(
 	struct xfs_inode		*ip = NULL;
 	int				error;
 
-	/*
-	 * If userspace passed us an AG number or a generation number
-	 * without an inode number, they haven't got a clue so bail out
-	 * immediately.
-	 */
-	if (sc->sm->sm_agno || (sc->sm->sm_gen && !sc->sm->sm_ino))
-		return -EINVAL;
-
 	/* We want to scan the inode we already had opened. */
 	if (sc->sm->sm_ino == 0 || sc->sm->sm_ino == ip_in->i_ino) {
 		sc->ip = ip_in;
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 5c04385..fe12053 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -78,8 +78,6 @@ int xfs_scrub_checkpoint_log(struct xfs_mount *mp);
 
 /* Setup functions */
 int xfs_scrub_setup_fs(struct xfs_scrub_context *sc, struct xfs_inode *ip);
-int xfs_scrub_setup_ag_header(struct xfs_scrub_context *sc,
-			      struct xfs_inode *ip);
 int xfs_scrub_setup_ag_allocbt(struct xfs_scrub_context *sc,
 			       struct xfs_inode *ip);
 int xfs_scrub_setup_ag_iallocbt(struct xfs_scrub_context *sc,
diff --git a/fs/xfs/scrub/quota.c b/fs/xfs/scrub/quota.c
index 8e58ba8..d43b654 100644
--- a/fs/xfs/scrub/quota.c
+++ b/fs/xfs/scrub/quota.c
@@ -67,13 +67,6 @@ xfs_scrub_setup_quota(
 {
 	uint				dqtype;
 
-	/*
-	 * If userspace gave us an AG number or inode data, they don't
-	 * know what they're doing.  Get out.
-	 */
-	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
-
 	dqtype = xfs_scrub_quota_to_dqtype(sc);
 	if (dqtype == 0)
 		return -EINVAL;
diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index c6fedb6..6860d5d 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -43,22 +43,14 @@ xfs_scrub_setup_rt(
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	struct xfs_mount		*mp = sc->mp;
-	int				error = 0;
-
-	/*
-	 * If userspace gave us an AG number or inode data, they don't
-	 * know what they're doing.  Get out.
-	 */
-	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
-		return -EINVAL;
+	int				error;
 
 	error = xfs_scrub_setup_fs(sc, ip);
 	if (error)
 		return error;
 
 	sc->ilock_flags = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
-	sc->ip = mp->m_rbmip;
+	sc->ip = sc->mp->m_rbmip;
 	xfs_ilock(sc->ip, sc->ilock_flags);
 
 	return 0;
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 182d2fe..b98084d 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -129,8 +129,6 @@ xfs_scrub_probe(
 {
 	int				error = 0;
 
-	if (sc->sm->sm_ino || sc->sm->sm_agno)
-		return -EINVAL;
 	if (xfs_scrub_should_terminate(sc, &error))
 		return error;
 
@@ -169,105 +167,129 @@ xfs_scrub_teardown(
 
 static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
 	[XFS_SCRUB_TYPE_PROBE] = {	/* ioctl presence test */
+		.type	= ST_NONE,
 		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_probe,
 	},
 	[XFS_SCRUB_TYPE_SB] = {		/* superblock */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_superblock,
 	},
 	[XFS_SCRUB_TYPE_AGF] = {	/* agf */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agf,
 	},
 	[XFS_SCRUB_TYPE_AGFL]= {	/* agfl */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agfl,
 	},
 	[XFS_SCRUB_TYPE_AGI] = {	/* agi */
-		.setup	= xfs_scrub_setup_ag_header,
+		.type	= ST_PERAG,
+		.setup	= xfs_scrub_setup_fs,
 		.scrub	= xfs_scrub_agi,
 	},
 	[XFS_SCRUB_TYPE_BNOBT] = {	/* bnobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_bnobt,
 	},
 	[XFS_SCRUB_TYPE_CNTBT] = {	/* cntbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_allocbt,
 		.scrub	= xfs_scrub_cntbt,
 	},
 	[XFS_SCRUB_TYPE_INOBT] = {	/* inobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_inobt,
 	},
 	[XFS_SCRUB_TYPE_FINOBT] = {	/* finobt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_iallocbt,
 		.scrub	= xfs_scrub_finobt,
 		.has	= xfs_sb_version_hasfinobt,
 	},
 	[XFS_SCRUB_TYPE_RMAPBT] = {	/* rmapbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_rmapbt,
 		.scrub	= xfs_scrub_rmapbt,
 		.has	= xfs_sb_version_hasrmapbt,
 	},
 	[XFS_SCRUB_TYPE_REFCNTBT] = {	/* refcountbt */
+		.type	= ST_PERAG,
 		.setup	= xfs_scrub_setup_ag_refcountbt,
 		.scrub	= xfs_scrub_refcountbt,
 		.has	= xfs_sb_version_hasreflink,
 	},
 	[XFS_SCRUB_TYPE_INODE] = {	/* inode record */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode,
 		.scrub	= xfs_scrub_inode,
 	},
 	[XFS_SCRUB_TYPE_BMBTD] = {	/* inode data fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_data,
 	},
 	[XFS_SCRUB_TYPE_BMBTA] = {	/* inode attr fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_attr,
 	},
 	[XFS_SCRUB_TYPE_BMBTC] = {	/* inode CoW fork */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_inode_bmap,
 		.scrub	= xfs_scrub_bmap_cow,
 	},
 	[XFS_SCRUB_TYPE_DIR] = {	/* directory */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_directory,
 		.scrub	= xfs_scrub_directory,
 	},
 	[XFS_SCRUB_TYPE_XATTR] = {	/* extended attributes */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_xattr,
 		.scrub	= xfs_scrub_xattr,
 	},
 	[XFS_SCRUB_TYPE_SYMLINK] = {	/* symbolic link */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_symlink,
 		.scrub	= xfs_scrub_symlink,
 	},
 	[XFS_SCRUB_TYPE_PARENT] = {	/* parent pointers */
+		.type	= ST_INODE,
 		.setup	= xfs_scrub_setup_parent,
 		.scrub	= xfs_scrub_parent,
 	},
 	[XFS_SCRUB_TYPE_RTBITMAP] = {	/* realtime bitmap */
+		.type	= ST_FS,
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtbitmap,
 		.has	= xfs_sb_version_hasrealtime,
 	},
 	[XFS_SCRUB_TYPE_RTSUM] = {	/* realtime summary */
+		.type	= ST_FS,
 		.setup	= xfs_scrub_setup_rt,
 		.scrub	= xfs_scrub_rtsummary,
 		.has	= xfs_sb_version_hasrealtime,
 	},
 	[XFS_SCRUB_TYPE_UQUOTA] = {	/* user quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 	[XFS_SCRUB_TYPE_GQUOTA] = {	/* group quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 	[XFS_SCRUB_TYPE_PQUOTA] = {	/* project quota */
-		.setup = xfs_scrub_setup_quota,
-		.scrub = xfs_scrub_quota,
+		.type	= ST_FS,
+		.setup	= xfs_scrub_setup_quota,
+		.scrub	= xfs_scrub_quota,
 	},
 };
 
@@ -298,6 +320,7 @@ xfs_scrub_validate_inputs(
 	sm->sm_flags &= ~XFS_SCRUB_FLAGS_OUT;
 	if (sm->sm_flags & ~XFS_SCRUB_FLAGS_IN)
 		goto out;
+	/* sm_reserved[] must be zero */
 	if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
 		goto out;
 
@@ -312,6 +335,27 @@ xfs_scrub_validate_inputs(
 	if (ops->has && !ops->has(&mp->m_sb))
 		goto out;
 
+	error = -EINVAL;
+	/* restricting fields must be appropriate for type */
+	switch (ops->type) {
+	case ST_NONE:
+	case ST_FS:
+		if (sm->sm_ino || sm->sm_gen || sm->sm_agno)
+			goto out;
+		break;
+	case ST_PERAG:
+		if (sm->sm_ino || sm->sm_gen ||
+		    sm->sm_agno >= mp->m_sb.sb_agcount)
+			goto out;
+		break;
+	case ST_INODE:
+		if (sm->sm_agno || (sm->sm_gen && !sm->sm_ino))
+			goto out;
+		break;
+	default:
+		goto out;
+	}
+
 	error = -EOPNOTSUPP;
 	/*
 	 * We won't scrub any filesystem that doesn't have the ability
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index e9ec041..2a79614 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -22,6 +22,14 @@
 
 struct xfs_scrub_context;
 
+/* Type info and names for the scrub types. */
+enum xfs_scrub_type {
+	ST_NONE = 1,	/* disabled */
+	ST_PERAG,	/* per-AG metadata */
+	ST_FS,		/* per-FS metadata */
+	ST_INODE,	/* per-inode metadata */
+};
+
 struct xfs_scrub_meta_ops {
 	/* Acquire whatever resources are needed for the operation. */
 	int		(*setup)(struct xfs_scrub_context *,
@@ -32,6 +40,9 @@ struct xfs_scrub_meta_ops {
 
 	/* Decide if we even have this piece of metadata. */
 	bool		(*has)(struct xfs_sb *);
+
+	/* type describing required/allowed inputs */
+	enum xfs_scrub_type	type;
 };
 
 /* Buffer pointers and btree cursors for an entire AG. */

Re: [PATCH 1/4] xfs: add scrub to XFS_BUILD_OPTIONS

[Darrick J. Wong]

On Fri, Dec 01, 2017 at 04:31:04PM -0600, Eric Sandeen wrote:
> On 12/1/17 4:29 PM, Darrick J. Wong wrote:
> > On Fri, Dec 01, 2017 at 04:12:34PM -0600, Eric Sandeen wrote:
> >> Advertise this config option along with the others.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> >> index fcc5dfc..8cee8e8 100644
> >> --- a/fs/xfs/xfs_super.h
> >> +++ b/fs/xfs/xfs_super.h
> >> @@ -44,6 +44,12 @@
> >>  # define XFS_REALTIME_STRING
> >>  #endif
> >>  
> >> +#ifdef CONFIG_XFS_ONLINE_SCRUB
> >> +# define XFS_SCRUB_STRING	"scrub, "
> >> +#else
> >> +# define XFS_SCRUB_STRING
> >> +#endif
> > 
> > I don't mind this addition to the build options, though I had pictured
> > scrub becoming an integral part of xfs some day and not remaining a
> > configurable option.
> 
> It could be removed at that time, unless it's considered an API?
> 
> I'm not hung up on this, if there's no reason for it that's OK.
> 
> > That said I haven't evaluated how much scrub bloats up xfs nor do I know
> > if the kernel tinyfication people would actually want to turn it off??
> > 
> > (For my part I prefer it some day not be Kconfig option so that it won't
> > bitrot ala CONFIG_XFS_RT=y.)
> 
> I'd imagine that it'd be turned on more often than RT but that's just a guess.

Ok, now that we've merged all this stuff, it looks like disabling the
scrub code alone can reduce the module size by ~8%.  I can imagine the
occasional system builder who wants xfs but doesn't care about online
fsck, so I think I'll keep the kconfig option.  Therefore, it ought to
go in the build string.

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> -Eric
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html