Skip to contentSkip to conversation

xfs: make fatal assert failures conditional in debug mode

PATCH In review Brian Foster 2 participants 4 messages May 10, 2017 – May 31, 2017 XFS filesystem

lore
Share & tools
lore
#0 lore raw

v3 cover xfs: make fatal assert failures conditional in debug mode

Hi all,

This version makes fatal asserts runtime and build time configurable. Patch 1 creates the runtime tunable and patch 2 creates a Kconfig option to control the default value of the runtime tunable. As before, default DEBUG mode behavior is preserved (i.e., assert failures BUG() the kernel).

Brian

v3:
- Add DEBUG mode sysfs tunable for runtime configuration.
- Refactor compile time flag to set default value of runtime flag.
v2: http://www.spinics.net/lists/linux-xfs/msg06520.html
- Clean up the Kconfig option help text.
v1: http://www.spinics.net/lists/linux-xfs/msg06498.html
- Use a new config option rather than reuse XFS_WARN.
- Disable BUG() in DEBUG mode by default and flip the logic of the new config option.
rfc: http://www.spinics.net/lists/linux-xfs/msg06390.html

Brian Foster (2):
xfs: define bug_on_assert debug mode sysfs tunable
xfs: define fatal assert build time tunable

 fs/xfs/Kconfig       | 13 +++++++++++++
 fs/xfs/xfs.h         |  4 ++++
 fs/xfs/xfs_globals.c |  5 +++++
 fs/xfs/xfs_message.c |  5 ++++-
 fs/xfs/xfs_sysctl.h  |  1 +
 fs/xfs/xfs_sysfs.c   | 33 +++++++++++++++++++++++++++++++++
 6 files changed, 60 insertions(+), 1 deletion(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH v3 1/2 xfs: define bug_on_assert debug mode sysfs tunable

In DEBUG mode, assert failures unconditionally trigger a kernel BUG. This is useful in diagnostic situations to panic a system and collect detailed state information at the time of a failure.

This can also cause problems in cases where DEBUG mode code is desired but it is preferable not trigger kernel BUGs on assert failure. For example, during development of new code or during certain xfstests tests that intentionally cause corruption and test the kernel for survival (but otherwise may expect to trigger assert failures).

To provide additional flexibility, create the <sysfs>/fs/xfs/debug/bug_on_assert tunable to configure assert failure behavior at runtime. This tunable is only available in DEBUG mode and is enabled by default to preserve existing default behavior. When disabled, assert failures in DEBUG mode result in kernel warnings.

---
 fs/xfs/xfs_globals.c |  2 ++
 fs/xfs/xfs_message.c |  5 ++++-
 fs/xfs/xfs_sysctl.h  |  1 +
 fs/xfs/xfs_sysfs.c   | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)
fs/xfs/xfs_globals.c +2 -0
index 687a4b0..673adf0 100644
@@ -47,4 +47,6 @@ xfs_param_t xfs_params = {
 
 struct xfs_globals xfs_globals = {
 	.log_recovery_delay	=	0,	/* no delay by default */
+	.bug_on_assert		=	true,	/* historical default in DEBUG
+						 * mode */
 };
fs/xfs/xfs_message.c +4 -1
index 11792d8..e68bd10 100644
@@ -110,7 +110,10 @@ assfail(char *expr, char *file, int line)
 {
 	xfs_emerg(NULL, "Assertion failed: %s, file: %s, line: %d",
 		expr, file, line);
-	BUG();
+	if (xfs_globals.bug_on_assert)
+		BUG();
+	else
+		WARN_ON(1);
 }
 
 void
fs/xfs/xfs_sysctl.h +1 -0
index 984a349..82afee0 100644
@@ -95,6 +95,7 @@ extern xfs_param_t	xfs_params;
 
 struct xfs_globals {
 	int	log_recovery_delay;	/* log recovery delay (secs) */
+	bool	bug_on_assert;		/* BUG() the kernel on assert failure */
 };
 extern struct xfs_globals	xfs_globals;
 
fs/xfs/xfs_sysfs.c +33 -0
index 80ac15f..ec6e0e2 100644
@@ -146,6 +146,38 @@ struct kobj_type xfs_mp_ktype = {
 /* debug */
 
 STATIC ssize_t
+bug_on_assert_store(
+	struct kobject		*kobject,
+	const char		*buf,
+	size_t			count)
+{
+	int			ret;
+	int			val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val == 1)
+		xfs_globals.bug_on_assert = true;
+	else if (val == 0)
+		xfs_globals.bug_on_assert = false;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+STATIC ssize_t
+bug_on_assert_show(
+	struct kobject		*kobject,
+	char			*buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", xfs_globals.bug_on_assert ? 1 : 0);
+}
+XFS_SYSFS_ATTR_RW(bug_on_assert);
+
+STATIC ssize_t
 log_recovery_delay_store(
 	struct kobject	*kobject,
 	const char	*buf,
@@ -176,6 +208,7 @@ log_recovery_delay_show(
 XFS_SYSFS_ATTR_RW(log_recovery_delay);
 
 static struct attribute *xfs_dbg_attrs[] = {
+	ATTR_LIST(bug_on_assert),
 	ATTR_LIST(log_recovery_delay),
 	NULL,
 };
Signature · 1 line
2.7.4
#2 lore raw

PATCH v3 2/2 xfs: define fatal assert build time tunable

In reply to Brian Foster

While configurable at runtime, the DEBUG mode assert failure behavior is usually either desired or not for a particular situation. For example, developers using kernel modules may prefer for fatal asserts to remain disabled across module reloads while QE engineers doing broad regression testing may prefer to have fatal asserts enabled on boot to facilitate data collection for bug reports.

To provide a compromise/convenience for developers, create a Kconfig option that sets the default value of the DEBUG mode 'bug_on_assert' sysfs tunable. The default behavior remains to trigger kernel BUGs on assert failures to preserve existing behavior across kernel configuration updates with DEBUG mode enabled.

---
 fs/xfs/Kconfig       | 13 +++++++++++++
 fs/xfs/xfs.h         |  4 ++++
 fs/xfs/xfs_globals.c |  7 +++++--
 3 files changed, 22 insertions(+), 2 deletions(-)
fs/xfs/Kconfig +13 -0
index 35faf12..1b98cfa 100644
@@ -96,3 +96,16 @@ config XFS_DEBUG
 	  not useful unless you are debugging a particular problem.
 
 	  Say N unless you are an XFS developer, or you play one on TV.
+
+config XFS_ASSERT_FATAL
+	bool "XFS fatal asserts"
+	default y
+	depends on XFS_FS && XFS_DEBUG
+	help
+	  Set the default DEBUG mode ASSERT failure behavior.
+
+	  Say Y here to cause DEBUG mode ASSERT failures to result in fatal
+	  errors that BUG() the kernel by default. If you say N, ASSERT failures
+	  result in warnings.
+
+	  This behavior can be modified at runtime via sysfs.
fs/xfs/xfs.h +4 -0
index a742c47..80cd0fd 100644
@@ -24,6 +24,10 @@
 #define XFS_BUF_LOCK_TRACKING 1
 #endif
 
+#ifdef CONFIG_XFS_ASSERT_FATAL
+#define XFS_ASSERT_FATAL 1
+#endif
+
 #ifdef CONFIG_XFS_WARN
 #define XFS_WARN 1
 #endif
fs/xfs/xfs_globals.c +5 -2
index 673adf0..3e1cc30 100644
@@ -47,6 +47,9 @@ xfs_param_t xfs_params = {
 
 struct xfs_globals xfs_globals = {
 	.log_recovery_delay	=	0,	/* no delay by default */
-	.bug_on_assert		=	true,	/* historical default in DEBUG
-						 * mode */
+#ifdef XFS_ASSERT_FATAL
+	.bug_on_assert		=	true,	/* assert failures BUG() */
+#else
+	.bug_on_assert		=	false,	/* assert failures WARN() */
+#endif
 };
Signature · 1 line
2.7.4
Reviewed-by on v3 2/2
#3 lore raw

Re: [PATCH v3 2/2] xfs: define fatal assert build time tunable

Quoted 62 lines · Brian Foster

While configurable at runtime, the DEBUG mode assert failure behavior is usually either desired or not for a particular situation. For example, developers using kernel modules may prefer for fatal asserts to remain disabled across module reloads while QE engineers doing broad regression testing may prefer to have fatal asserts enabled on boot to facilitate data collection for bug reports.

To provide a compromise/convenience for developers, create a Kconfig option that sets the default value of the DEBUG mode 'bug_on_assert' sysfs tunable. The default behavior remains to trigger kernel BUGs on assert failures to preserve existing behavior across kernel configuration updates with DEBUG mode enabled.

---
 fs/xfs/Kconfig       | 13 +++++++++++++
 fs/xfs/xfs.h         |  4 ++++
 fs/xfs/xfs_globals.c |  7 +++++--
 3 files changed, 22 insertions(+), 2 deletions(-)
fs/xfs/Kconfig +13 -0
index 35faf12..1b98cfa 100644
@@ -96,3 +96,16 @@ config XFS_DEBUG
 	  not useful unless you are debugging a particular problem.
 
 	  Say N unless you are an XFS developer, or you play one on TV.
+
+config XFS_ASSERT_FATAL
+	bool "XFS fatal asserts"
+	default y
+	depends on XFS_FS && XFS_DEBUG
+	help
+	  Set the default DEBUG mode ASSERT failure behavior.
+
+	  Say Y here to cause DEBUG mode ASSERT failures to result in fatal
+	  errors that BUG() the kernel by default. If you say N, ASSERT failures
+	  result in warnings.
+
+	  This behavior can be modified at runtime via sysfs.
fs/xfs/xfs.h +4 -0
index a742c47..80cd0fd 100644
@@ -24,6 +24,10 @@
 #define XFS_BUF_LOCK_TRACKING 1
 #endif
 
+#ifdef CONFIG_XFS_ASSERT_FATAL
+#define XFS_ASSERT_FATAL 1
+#endif
+
 #ifdef CONFIG_XFS_WARN
 #define XFS_WARN 1
 #endif
fs/xfs/xfs_globals.c +0 -0
index 673adf0..3e1cc30 100644

fs/xfs/xfs_globals.c xfs_param_t xfs_params = { · line 50

Quoted 6 lines
fs/xfs/xfs_globals.c +1 -2
@@ -47,6 +47,9 @@ xfs_param_t xfs_params = {
 
 struct xfs_globals xfs_globals = {
 	.log_recovery_delay	=	0,	/* no delay by default */
-	.bug_on_assert		=	true,	/* historical default in DEBUG
-						 * mode */
+#ifdef XFS_ASSERT_FATAL

Minor nit to pick, but if you're only testing that XFS_ASSERT_FATAL is defined, you could just '#define XFS_ASSERT_FATAL' without providing a value.... or '#if XFS_ASSERT_FATAL' here.

OTOH XFS_WARN also does that, and we reach the same ends regardless of which way we do it. I'm willing to pull this in for 4.13 as-is, if nobody objects.

--D

Quoted 12 lines
fs/xfs/xfs_globals.c +4 -0
+	.bug_on_assert		=	true,	/* assert failures BUG() */
+#else
+	.bug_on_assert		=	false,	/* assert failures WARN() */
+#endif
 };
Signature · 6 lines
2.7.4

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