Re: [PATCH] xfs: make fatal assert failures conditional in debug mode

[Brian Foster <bfoster@redhat.com>]

On Thu, May 04, 2017 at 01:58:32PM -0500, Eric Sandeen wrote:
> On 5/4/17 1:29 PM, Brian Foster wrote:
> > XFS currently supports two debug modes: XFS_WARN enables assert
> > failure warnings and XFS_DEBUG converts assert failures to fatal
> > errors (via BUG()) and enables additional runtime debug code.
> > 
> > While the behavior to BUG the kernel on assert failure is useful in
> > certain test scenarios, it is also useful for development/debug to
> > enable debug mode code without having to crash the kernel on an
> > assert failure.
> > 
> > To provide this additional flexibility, update XFS debug mode to not
> > BUG() the kernel by default and create a new XFS kernel
> > configuration option to enable fatal assert failures when debug mode
> > is enabled. To provide backwards compatibility with current
> > behavior, enable the fatal asserts option by default when debug mode
> > is enabled.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Hi all,
> > 
> > This tweaks the rfc version based on feedback from Christoph and
> > Darrick. The config option is renamed to minimize confusion and the
> > logic is basically flipped to disable BUG() behavior by default in
> > XFS_DEBUG mode and have the new config option turn it on. This option is
> > dependent on DEBUG mode because I don't believe there is a compelling
> > use case for XFS_WARN mode with fatal asserts and I'd rather not
> > unnecessarily increase the configuration matrix.
> > 
> > Note that the new config option is set 'default y' simply to provide
> > backwards compatibility with current behavior (so users expecting DEBUG
> > mode kernels to BUG() will continue to see that behavior without
> > requiring further config modifications). Thoughts, reviews, flames
> > appreciated.
> > 
> > Brian
> > 
> > v1:
> > - 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
> > 
> >  fs/xfs/Kconfig     |  8 ++++++++
> >  fs/xfs/xfs.h       |  4 ++++
> >  fs/xfs/xfs_linux.h | 32 +++++++++++++++++++-------------
> >  3 files changed, 31 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig
> > index 35faf12..ded25f8 100644
> > --- a/fs/xfs/Kconfig
> > +++ b/fs/xfs/Kconfig
> > @@ -96,3 +96,11 @@ 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
> > +	  Say Y here to convert DEBUG mode ASSERT failures into fatal errors
> > +	  that BUG() the kernel. Otherwise, ASSERT failures result in warnings.
> 
> nitpick, and don't care much either way, but since Y is the default, it's
> a little odd to restate "say Y."  The "Otherwise" isn't crystal clear IMHO.
> 

I think the help text should be independent from the default setting. At
least a couple Kconfig options under fs/Kconfig have "Say Y .... Say N
... If unsure, say ..." with a default of y. (I don't think we really
need "If unsure ..." here...).

> Maybe:
> 
> "The default XFS_DEBUG mode is to BUG() on ASSERT failures.
>  If you say N here, ASSERT failures result in warnings.
> 
> Or:
> 
> "Say Y here to convert DEBUG mode ASSERT failures into fatal errors
>  that BUG() the kernel.
>  If you say N, these ASSERTs will be converted into warnings."
> 
> I'd just like to see "N" behavior /explicitly/ stated (with the text "N") :)
> 

That sounds reasonable. After catching up on some of the chat on irc,
how about the following to avoid using "convert?"

"Say Y here to cause DEBUG mode assert failures to result in fatal
errors that BUG() the kernel. If you say N, assert failures result in
warnings."

Brian

> -Eric
> 
> > 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_linux.h b/fs/xfs/xfs_linux.h
> > index 044fb0e..36c3eab 100644
> > --- a/fs/xfs/xfs_linux.h
> > +++ b/fs/xfs/xfs_linux.h
> > @@ -245,37 +245,43 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
> >  	return x;
> >  }
> >  
> > +/*
> > + * ASSERT() definitions
> > + */
> >  #define ASSERT_ALWAYS(expr)	\
> >  	(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> >  
> > -#ifdef DEBUG
> > +#if defined(DEBUG) && defined(XFS_ASSERT_FATAL)
> > +
> >  #define ASSERT(expr)	\
> >  	(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> >  
> > -#ifndef STATIC
> > -# define STATIC noinline
> > -#endif
> > -
> > -#else	/* !DEBUG */
> > -
> > -#ifdef XFS_WARN
> > +#elif defined(DEBUG) || defined(XFS_WARN)
> >  
> >  #define ASSERT(expr)	\
> >  	(likely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__))
> >  
> > -#ifndef STATIC
> > -# define STATIC static noinline
> > +#else
> > +
> > +#define ASSERT(expr)	((void)0)
> > +
> >  #endif
> >  
> > -#else	/* !DEBUG && !XFS_WARN */
> > +/*
> > + * STATIC definitions
> > + */
> > +#ifdef DEBUG
> > +
> > +#ifndef STATIC
> > +# define STATIC noinline
> > +#endif
> >  
> > -#define ASSERT(expr)	((void)0)
> > +#else	/* !DEBUG */
> >  
> >  #ifndef STATIC
> >  # define STATIC static noinline
> >  #endif
> >  
> > -#endif /* XFS_WARN */
> >  #endif /* DEBUG */
> >  
> >  #ifdef CONFIG_XFS_RT
> > 
> --
> 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