xfs: scrub tweaks

PATCH In review Eric Sandeen 3 participants 16 messages Dec 1, 2017 – Feb 1, 2018 XFS filesystem

lore

Share & tools

lore

Eric Sandeen

author

2017-12-01 22:12 UTC #0 lore raw

cover xfs: scrub tweaks

A handful of random stuff.

Eric Sandeen

2017-12-01 22:12 UTC #1 lore raw

PATCH 1/4 xfs: add scrub to XFS_BUILD_OPTIONS

Advertise this config option along with the others.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_super.h +7 -0

index fcc5dfc..8cee8e8 100644
@@ -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;

Darrick J. Wong

2017-12-01 22:29 UTC #2 lore raw

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

Quoted 10 lines · Eric Sandeen

Advertise this config option along with the others.
Signed-off-by Eric Sandeen <[email protected]>
---
fs/xfs/xfs_super.h +0 -0
index fcc5dfc..8cee8e8 100644

fs/xfs/xfs_super.h line 51

Quoted 8 lines

fs/xfs/xfs_super.h +5 -0

@@ -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

Quoted 17 lines

fs/xfs/xfs_super.h +2 -1
+
 #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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

author

2017-12-01 22:31 UTC #3 lore raw

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

Quoted 14 lines · Darrick J. Wong

Quoted 13 lines · Eric Sandeen
Advertise this config option along with the others.
Signed-off-by Eric Sandeen <[email protected]>
---
fs/xfs/xfs_super.h +0 -0
index fcc5dfc..8cee8e8 100644
@@ -44,6 +44,12 @@
 # define XFS_REALTIME_STRING
 #endif
 

Quoted 9 lines

fs/xfs/xfs_super.h line 51
Quoted 5 lines
fs/xfs/xfs_super.h +5 -0
+#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.

Quoted 5 lines

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

Darrick J. Wong

Reviewed-by

2018-02-01 00:03 UTC #4 lore raw

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

Quoted 29 lines · Eric Sandeen

Quoted 14 lines · Darrick J. Wong
Quoted 13 lines · Eric Sandeen
Advertise this config option along with the others.
Signed-off-by Eric Sandeen <[email protected]>
---
fs/xfs/xfs_super.h +0 -0
index fcc5dfc..8cee8e8 100644
@@ -44,6 +44,12 @@
 # define XFS_REALTIME_STRING
 #endif
 
Quoted 9 lines
fs/xfs/xfs_super.h line 51
Quoted 5 lines
fs/xfs/xfs_super.h +5 -0
+#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.

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>

--D

Quoted 7 lines

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

Eric Sandeen

2017-12-01 22:13 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---

fs/xfs/scrub/scrub.c +28 ~~-25~~

index 9c42c4e..dc1f33a 100644
@@ -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;
 
 	/*

Darrick J. Wong

Reviewed-by on v1 2/4

2017-12-01 22:26 UTC #6 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Looks ok,

Reviewed-by Darrick J. Wong <[email protected]>

Quoted 160 lines

---

fs/xfs/scrub/scrub.c +28 ~~-26~~

index 9c42c4e..dc1f33a 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2017-12-01 22:14 UTC #7 lore raw

PATCH 3/4 xfs: factor out scrub input checking

In reply to Eric Sandeen

Do this before adding more core checks.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/scrub/scrub.c +44 ~~-31~~

index dc1f33a..16932f9 100644
@@ -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);

Darrick J. Wong

2017-12-01 22:25 UTC #8 lore raw

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

Quoted 105 lines · Eric Sandeen

Do this before adding more core checks.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/scrub/scrub.c +30 ~~-30~~

index dc1f33a..16932f9 100644
@@ -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);

fs/xfs/scrub/scrub.c line 361

Quoted 10 lines

fs/xfs/scrub/scrub.c +10 -0

+
+	/* 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.

fs/xfs/scrub/scrub.c line 372

Quoted 12 lines

fs/xfs/scrub/scrub.c +4 -1

+	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

Quoted 9 lines

fs/xfs/scrub/scrub.c +0 -1
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

2017-12-01 23:23 UTC #9 lore raw

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

In reply to Eric Sandeen

From: Eric Sandeen <[email protected]>

Do this before adding more core checks.

Signed-off-by Eric Sandeen <[email protected]>

[darrick: rename input validation function]

Reviewed-by Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/scrub/scrub.c |   75 +++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

fs/xfs/scrub/scrub.c +44 ~~-31~~

index dc1f33a..182d2fe 100644
@@ -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);

Eric Sandeen

2017-12-01 22:15 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---

enum scrub_type should probably go somewhere shared ...?

fs/xfs/scrub/agheader.c +0 ~~-17~~

index 2a9b4f9..b599358 100644
@@ -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(

fs/xfs/scrub/common.c +1 -9

index ac95fe9..98452ad 100644
@@ -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;

fs/xfs/scrub/common.h +0 -2

index 5c04385..fe12053 100644
@@ -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,

fs/xfs/scrub/quota.c +0 -7

index 2eac160..4587853 100644
@@ -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;

fs/xfs/scrub/rtbitmap.c +2 ~~-10~~

index c6fedb6..6860d5d 100644
@@ -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;

fs/xfs/scrub/scrub.c +56 ~~-13~~

183 lines not on this page · raw message

fs/xfs/scrub/scrub.h +10 -0

index e9ec041..47f098c 100644
@@ -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 *);

Darrick J. Wong

2017-12-01 22:24 UTC #11 lore raw

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

Quoted 323 lines · 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 <[email protected]>

---

enum scrub_type should probably go somewhere shared ...?

fs/xfs/scrub/agheader.c +0 ~~-17~~

index 2a9b4f9..b599358 100644
@@ -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(

fs/xfs/scrub/common.c +1 -9

index ac95fe9..98452ad 100644
@@ -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;

fs/xfs/scrub/common.h +0 -2

index 5c04385..fe12053 100644
@@ -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,

fs/xfs/scrub/quota.c +0 -7

index 2eac160..4587853 100644
@@ -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;

fs/xfs/scrub/rtbitmap.c +2 ~~-10~~

index c6fedb6..6860d5d 100644
@@ -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;

fs/xfs/scrub/scrub.c +56 ~~-13~~

index 16932f9..6899126 100644
@@ -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? */

fs/xfs/scrub/scrub.h +0 -0

index e9ec041..47f098c 100644

fs/xfs/scrub/scrub.h line 26

Quoted 5 lines

fs/xfs/scrub/scrub.h +2 -0

@@ -22,7 +22,17 @@
 
 struct xfs_scrub_context;
 
+/* Type info and names for the scrub types. */
+enum scrub_type {

enum xfs_scrub_type ?

fs/xfs/scrub/scrub.h line 35

Quoted 9 lines

fs/xfs/scrub/scrub.h +8 -0

+	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

Quoted 8 lines

fs/xfs/scrub/scrub.h +0 -1
 	/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2017-12-01 22:25 UTC #12 lore raw

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

Quoted 6 lines · Darrick J. Wong

Quoted 6 lines
fs/xfs/scrub/scrub.h +0 -0
index e9ec041..47f098c 100644
@@ -22,7 +22,17 @@
 

Quoted 6 lines

fs/xfs/scrub/scrub.h line 26
Quoted 4 lines
fs/xfs/scrub/scrub.h +2 -0
 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

Darrick J. Wong

2017-12-01 23:02 UTC #13 lore raw

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

In reply to Eric Sandeen

Quoted 282 lines · 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 <[email protected]>

---

enum scrub_type should probably go somewhere shared ...?

fs/xfs/scrub/agheader.c +0 ~~-17~~

index 2a9b4f9..b599358 100644
@@ -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(

fs/xfs/scrub/common.c +1 -9

index ac95fe9..98452ad 100644
@@ -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;

fs/xfs/scrub/common.h +0 -2

index 5c04385..fe12053 100644
@@ -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,

fs/xfs/scrub/quota.c +0 -7

index 2eac160..4587853 100644
@@ -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;

fs/xfs/scrub/rtbitmap.c +2 ~~-10~~

index c6fedb6..6860d5d 100644
@@ -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;

fs/xfs/scrub/scrub.c +34 ~~-12~~

index 16932f9..6899126 100644
@@ -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,
 	},
 };

fs/xfs/scrub/scrub.c line 327

Quoted 8 lines

fs/xfs/scrub/scrub.c +2 -0

@@ -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

Quoted 55 lines

fs/xfs/scrub/scrub.c +20 -1

+	/* 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? */

fs/xfs/scrub/scrub.h +10 -1

index e9ec041..47f098c 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

2017-12-01 23:24 UTC #14 lore raw

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

In reply to Eric Sandeen

From: Eric Sandeen <[email protected]>

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 <[email protected]>

[darrick: add xfs_ prefix to enum, check scrub args after checking type]

Reviewed-by Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 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(-)

fs/xfs/scrub/agheader.c +0 ~~-17~~

index 2a9b4f9..b599358 100644
@@ -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(

fs/xfs/scrub/common.c +1 -9

index ac95fe9..98452ad 100644
@@ -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;

fs/xfs/scrub/common.h +0 -2

index 5c04385..fe12053 100644
@@ -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,

fs/xfs/scrub/quota.c +0 -7

index 8e58ba8..d43b654 100644
@@ -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;

fs/xfs/scrub/rtbitmap.c +2 ~~-10~~

index c6fedb6..6860d5d 100644
@@ -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;

fs/xfs/scrub/scrub.c +56 ~~-12~~

181 lines not on this page · raw message

fs/xfs/scrub/scrub.h +11 -0

index e9ec041..2a79614 100644
@@ -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. */

Darrick J. Wong

Reviewed-by on the v1 series

2017-12-01 22:39 UTC #15 lore raw

In reply to Eric Sandeen

Quoted 1 line · Eric Sandeen

A handful of random stuff.

For the whole set:
Looks ok, will test...

Reviewed-by Darrick J. Wong <[email protected]>

Quoted 4 lines

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