| Message ID | 1493400541-56113-1-git-send-email-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 28, 2017 at 01:29:01PM -0400, Brian Foster wrote: > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > > Hi all, > > Every now and then I find myself wanting to enable DEBUG mode code > without having to BUG the kernel every time an assert fails. Currently, > I end up just commenting out the BUG() call from assfail(). Any thoughts > on something like the below to update our configuration to support the > ability to enable debug mode with assert warnings? > > While this appears as a new option in Kconfig, it just reuses the > existing XFS_WARN definition to convert asserts into warnings regardless > of whether debug mode is enabled or not. There are probably multiple > other ways to do something like this (e.g., a Kconfig 'choice' selection > for various XFS debug modes, dropping the BUG() entirely, etc.). > Thoughts? > > Brian > > fs/xfs/Kconfig | 9 +++++++++ > fs/xfs/xfs_linux.h | 32 +++++++++++++++++++------------- > 2 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig > index 35faf12..db6d830 100644 > --- a/fs/xfs/Kconfig > +++ b/fs/xfs/Kconfig > @@ -96,3 +96,12 @@ 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_WARN > + bool "Non-fatal Asserts" > + default n > + depends on XFS_FS && XFS_DEBUG > + help > + Say Y here to convert DEBUG mode ASSERT failures into warnings. > + Otherwise, ASSERT failures are considered fatal errors and BUG the > + kernel. I'm very confused by this patch. At least my fs/xfs/Kconfig already has a "config XFS_WARN" line. What am I missing? -- 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 Tue, May 02, 2017 at 12:37:30AM -0700, Christoph Hellwig wrote: > On Fri, Apr 28, 2017 at 01:29:01PM -0400, Brian Foster wrote: > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > > > Hi all, > > > > Every now and then I find myself wanting to enable DEBUG mode code > > without having to BUG the kernel every time an assert fails. Currently, > > I end up just commenting out the BUG() call from assfail(). Any thoughts > > on something like the below to update our configuration to support the > > ability to enable debug mode with assert warnings? > > > > While this appears as a new option in Kconfig, it just reuses the > > existing XFS_WARN definition to convert asserts into warnings regardless > > of whether debug mode is enabled or not. There are probably multiple > > other ways to do something like this (e.g., a Kconfig 'choice' selection > > for various XFS debug modes, dropping the BUG() entirely, etc.). > > Thoughts? > > > > Brian > > > > fs/xfs/Kconfig | 9 +++++++++ > > fs/xfs/xfs_linux.h | 32 +++++++++++++++++++------------- > > 2 files changed, 28 insertions(+), 13 deletions(-) > > > > diff --git a/fs/xfs/Kconfig b/fs/xfs/Kconfig > > index 35faf12..db6d830 100644 > > --- a/fs/xfs/Kconfig > > +++ b/fs/xfs/Kconfig > > @@ -96,3 +96,12 @@ 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_WARN > > + bool "Non-fatal Asserts" > > + default n > > + depends on XFS_FS && XFS_DEBUG > > + help > > + Say Y here to convert DEBUG mode ASSERT failures into warnings. > > + Otherwise, ASSERT failures are considered fatal errors and BUG the > > + kernel. > > I'm very confused by this patch. At least my fs/xfs/Kconfig already > has a "config XFS_WARN" line. What am I missing? As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG are currently mutually exclusive. The former enables warnings on assert failures. The latter enables BUG()'s on assert failures and the additional, typical debug mode code. The Kconfig hack above simply pops up a conditional option when debug mode is enabled that effectively allows setting both XFS_WARN and XFS_DEBUG at the same time. The header files interpret this as debug mode enabled with an override for the assert failures to warn rather than BUG(). Note that this is just a hack and we can organize the Kconfig options however we want. For example, we could call this XFS_DEBUG_WARN and continue to consider it a debug mode sub-flag, or we could turn the debug mode option into a multi-mode selector (i.e., Debug modes: "None," "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played around a bit with the latter but it seems like a bit of overkill to me. Brian > -- > 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 Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote: > As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG > are currently mutually exclusive. The former enables warnings on assert > failures. The latter enables BUG()'s on assert failures and the > additional, typical debug mode code. The Kconfig hack above simply pops > up a conditional option when debug mode is enabled that effectively > allows setting both XFS_WARN and XFS_DEBUG at the same time. The header > files interpret this as debug mode enabled with an override for the > assert failures to warn rather than BUG(). Oh, I didn't know Kconfig allows the same symbol to be define twice. But even if that's ok I'd say it's rather odd. > Note that this is just a hack and we can organize the Kconfig options > however we want. For example, we could call this XFS_DEBUG_WARN and > continue to consider it a debug mode sub-flag, or we could turn the > debug mode option into a multi-mode selector (i.e., Debug modes: "None," > "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played > around a bit with the latter but it seems like a bit of overkill to me. Maybe we need something like: XFS_WARN (as-is) XFS_WARN_BUG (XFS_WARN + BUG_ON on assert) XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to ASSERT) -- 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 04:57:25AM -0700, Christoph Hellwig wrote: > On Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote: > > As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG > > are currently mutually exclusive. The former enables warnings on assert > > failures. The latter enables BUG()'s on assert failures and the > > additional, typical debug mode code. The Kconfig hack above simply pops > > up a conditional option when debug mode is enabled that effectively > > allows setting both XFS_WARN and XFS_DEBUG at the same time. The header > > files interpret this as debug mode enabled with an override for the > > assert failures to warn rather than BUG(). > > Oh, I didn't know Kconfig allows the same symbol to be define twice. > But even if that's ok I'd say it's rather odd. > Yes, I just slapped it together quickly to float the idea. > > Note that this is just a hack and we can organize the Kconfig options > > however we want. For example, we could call this XFS_DEBUG_WARN and > > continue to consider it a debug mode sub-flag, or we could turn the > > debug mode option into a multi-mode selector (i.e., Debug modes: "None," > > "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played > > around a bit with the latter but it seems like a bit of overkill to me. > > Maybe we need something like: > > XFS_WARN (as-is) > XFS_WARN_BUG (XFS_WARN + BUG_ON on assert) > XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to > ASSERT) That allows asserts to BUG() on an XFS_WARN kernel, which is not quite what I want to accomplish here (and I don't think that's really needed for an XFS_WARN kernel). I'm not quite following if/how that allows to disable assert BUG()s on an XFS_DEBUG kernel. Would XFS_DEBUG now never BUG() unless XFS_WARN_BUG is defined as well? If so, that sounds reasonable to me. I may just suggest tweaking it to something like this: XFS_WARN (as-is) XFS_DEBUG (DEBUG code modified to warn on assert failure by default) XFS_ASSERT_BUG (depends on XFS_DEBUG, enables BUG() on assert failure) WARN and DEBUG remain mutually exclusive, the default assert behavior for DEBUG changes to a warning rather than BUG(), and the latter is enabled by a new conditional XFS_ASSERT_BUG config option. My only slight concern is that changes default behavior for distros that might create debug builds/packages, but I can see whether we can mitigate that by setting 'default y' for XFS_ASSERT_BUG so long as it is only available in XFS_DEBUG mode. Thoughts? Brian > > -- > 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 Thu, May 04, 2017 at 08:44:26AM -0400, Brian Foster wrote: > On Thu, May 04, 2017 at 04:57:25AM -0700, Christoph Hellwig wrote: > > On Tue, May 02, 2017 at 08:14:25AM -0400, Brian Foster wrote: > > > As noted above, this is just a reuse of the flag. XFS_WARN and XFS_DEBUG > > > are currently mutually exclusive. The former enables warnings on assert > > > failures. The latter enables BUG()'s on assert failures and the > > > additional, typical debug mode code. The Kconfig hack above simply pops > > > up a conditional option when debug mode is enabled that effectively > > > allows setting both XFS_WARN and XFS_DEBUG at the same time. The header > > > files interpret this as debug mode enabled with an override for the > > > assert failures to warn rather than BUG(). > > > > Oh, I didn't know Kconfig allows the same symbol to be define twice. > > But even if that's ok I'd say it's rather odd. > > > > Yes, I just slapped it together quickly to float the idea. > > > > Note that this is just a hack and we can organize the Kconfig options > > > however we want. For example, we could call this XFS_DEBUG_WARN and > > > continue to consider it a debug mode sub-flag, or we could turn the > > > debug mode option into a multi-mode selector (i.e., Debug modes: "None," > > > "Warn only," "Debug mode," "Debug mode w/ non-fatal asserts"). I played > > > around a bit with the latter but it seems like a bit of overkill to me. > > > > Maybe we need something like: > > > > XFS_WARN (as-is) > > XFS_WARN_BUG (XFS_WARN + BUG_ON on assert) > > XFS_DEBUG (everyhing under XFS_DEBUG currently that's not related to > > ASSERT) > > That allows asserts to BUG() on an XFS_WARN kernel, which is not quite > what I want to accomplish here (and I don't think that's really needed > for an XFS_WARN kernel). I'm not quite following if/how that allows to > disable assert BUG()s on an XFS_DEBUG kernel. Would XFS_DEBUG now never > BUG() unless XFS_WARN_BUG is defined as well? > > If so, that sounds reasonable to me. I may just suggest tweaking it to > something like this: > > XFS_WARN (as-is) > XFS_DEBUG (DEBUG code modified to warn on assert failure by default) > XFS_ASSERT_BUG (depends on XFS_DEBUG, enables BUG() on assert failure) > > WARN and DEBUG remain mutually exclusive, the default assert behavior > for DEBUG changes to a warning rather than BUG(), and the latter is > enabled by a new conditional XFS_ASSERT_BUG config option. My only > slight concern is that changes default behavior for distros that might > create debug builds/packages, but I can see whether we can mitigate that > by setting 'default y' for XFS_ASSERT_BUG so long as it is only > available in XFS_DEBUG mode. Thoughts? Sounds reasonable. For years I've been banging around a silly patch that removes the BUG() call so that I can perform more forensic analysis after something goes wrong. (Granted it helps immensely that BUG reports now dump the ftrace buffer though sometimes that just leads to a flood of out of date crap coming over the serial line...) --D > > Brian > > > > > -- > > 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 -- 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..db6d830 100644 --- a/fs/xfs/Kconfig +++ b/fs/xfs/Kconfig @@ -96,3 +96,12 @@ 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_WARN + bool "Non-fatal Asserts" + default n + depends on XFS_FS && XFS_DEBUG + help + Say Y here to convert DEBUG mode ASSERT failures into warnings. + Otherwise, ASSERT failures are considered fatal errors and BUG the + kernel. diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index 044fb0e..3432863 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_WARN) + #define ASSERT(expr) \ (likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) -#ifndef STATIC -# define STATIC noinline -#endif - -#else /* !DEBUG */ - -#ifdef XFS_WARN +#elif 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
Signed-off-by: Brian Foster <bfoster@redhat.com> --- Hi all, Every now and then I find myself wanting to enable DEBUG mode code without having to BUG the kernel every time an assert fails. Currently, I end up just commenting out the BUG() call from assfail(). Any thoughts on something like the below to update our configuration to support the ability to enable debug mode with assert warnings? While this appears as a new option in Kconfig, it just reuses the existing XFS_WARN definition to convert asserts into warnings regardless of whether debug mode is enabled or not. There are probably multiple other ways to do something like this (e.g., a Kconfig 'choice' selection for various XFS debug modes, dropping the BUG() entirely, etc.). Thoughts? Brian fs/xfs/Kconfig | 9 +++++++++ fs/xfs/xfs_linux.h | 32 +++++++++++++++++++------------- 2 files changed, 28 insertions(+), 13 deletions(-)