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