[PATCH v3 0/2] xfs: make fatal assert failures conditional in debug mode

[PATCH v3 0/2] xfs: make fatal assert failures conditional in debug mode

[Brian Foster]

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

-- 
2.7.4

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

[Brian Foster]

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.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/xfs_globals.c b/fs/xfs/xfs_globals.c
index 687a4b0..673adf0 100644
--- a/fs/xfs/xfs_globals.c
+++ b/fs/xfs/xfs_globals.c
@@ -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 */
 };
diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
index 11792d8..e68bd10 100644
--- a/fs/xfs/xfs_message.c
+++ b/fs/xfs/xfs_message.c
@@ -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
diff --git a/fs/xfs/xfs_sysctl.h b/fs/xfs/xfs_sysctl.h
index 984a349..82afee0 100644
--- a/fs/xfs/xfs_sysctl.h
+++ b/fs/xfs/xfs_sysctl.h
@@ -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;
 
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index 80ac15f..ec6e0e2 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -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,
 };
-- 
2.7.4

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

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

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/Kconfig       | 13 +++++++++++++
 fs/xfs/xfs.h         |  4 ++++
 fs/xfs/xfs_globals.c |  7 +++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
index 35faf12..1b98cfa 100644
--- a/fs/xfs/Kconfig
+++ b/fs/xfs/Kconfig
@@ -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.
diff --git a/fs/xfs/xfs.h b/fs/xfs/xfs.h
index a742c47..80cd0fd 100644
--- a/fs/xfs/xfs.h
+++ b/fs/xfs/xfs.h
@@ -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
diff --git a/fs/xfs/xfs_globals.c b/fs/xfs/xfs_globals.c
index 673adf0..3e1cc30 100644
--- a/fs/xfs/xfs_globals.c
+++ b/fs/xfs/xfs_globals.c
@@ -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
 };
-- 
2.7.4

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

[Darrick J. Wong]

On Wed, May 10, 2017 at 10:31:51AM -0400, Brian Foster wrote:
> 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.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/Kconfig       | 13 +++++++++++++
>  fs/xfs/xfs.h         |  4 ++++
>  fs/xfs/xfs_globals.c |  7 +++++--
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> index 35faf12..1b98cfa 100644
> --- a/fs/xfs/Kconfig
> +++ b/fs/xfs/Kconfig
> @@ -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.
> diff --git a/fs/xfs/xfs.h b/fs/xfs/xfs.h
> index a742c47..80cd0fd 100644
> --- a/fs/xfs/xfs.h
> +++ b/fs/xfs/xfs.h
> @@ -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
> diff --git a/fs/xfs/xfs_globals.c b/fs/xfs/xfs_globals.c
> index 673adf0..3e1cc30 100644
> --- a/fs/xfs/xfs_globals.c
> +++ b/fs/xfs/xfs_globals.c
> @@ -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.

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

--D

> +	.bug_on_assert		=	true,	/* assert failures BUG() */
> +#else
> +	.bug_on_assert		=	false,	/* assert failures WARN() */
> +#endif
>  };
> -- 
> 2.7.4
> 
> --
> 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