| Message ID | 1493922564-14273-1-git-send-email-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. 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") :) -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
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 -- 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
On 5/5/17 7:20 AM, Brian Foster wrote: >> 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." sounds good to me. sorry for the bikeshed. -Eric -- 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
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. 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
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(-)