xfs: support debug mode with assert warnings
RFC PATCH 1/1 xfs: support debug mode with assert warnings
---
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(-)
fs/xfs/Kconfig +9 -0
@@ -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.
fs/xfs/xfs_linux.h +19 -13
@@ -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
Signature · 1 line
2.7.4
Quoted 31 lines · Brian Foster
Signed-off-by Brian Foster <[email protected]>---
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 fromassfail(). 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 existingXFS_WARNdefinition 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., aKconfig'choice' selection for various XFS debug modes, dropping theBUG()entirely, etc.). Thoughts?Brian
fs/xfs/Kconfig | 9 +++++++++ fs/xfs/xfs_linux.h | 32 +++++++++++++++++++------------- 2 files changed, 28 insertions(+), 13 deletions(-)fs/xfs/Kconfig +0
-0@@ -96,3 +96,12 @@ config XFS_DEBUG not useful unless you are debugging a particular problem.
fs/xfs/Kconfig config XFS_DEBUG · line 107
Quoted 10 lines
fs/xfs/Kconfig +9
-0Say 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?
Quoted 32 lines · Christoph Hellwig
Quoted 31 lines · Brian Foster
Signed-off-by Brian Foster <[email protected]>---
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 fromassfail(). 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 existingXFS_WARNdefinition 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., aKconfig'choice' selection for various XFS debug modes, dropping theBUG()entirely, etc.). Thoughts?Brian
fs/xfs/Kconfig | 9 +++++++++ fs/xfs/xfs_linux.h | 32 +++++++++++++++++++------------- 2 files changed, 28 insertions(+), 13 deletions(-)fs/xfs/Kconfig +0
-0@@ -96,3 +96,12 @@ config XFS_DEBUG not useful unless you are debugging a particular problem.
Quoted 13 lines
fs/xfs/Kconfig config XFS_DEBUG · line 107
Quoted 10 lines
fs/xfs/Kconfig +9
-0Say 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/Kconfigalready has a "configXFS_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
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines · Brian Foster
As noted above, this is just a reuse of the flag.
XFS_WARNandXFS_DEBUGare 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. TheKconfighack above simply pops up a conditional option when debug mode is enabled that effectively allows setting bothXFS_WARNandXFS_DEBUGat the same time. The header files interpret this as debug mode enabled with an override for the assert failures to warn rather thanBUG().
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.
Quoted 6 lines
Note that this is just a hack and we can organize the
Kconfigoptions however we want. For example, we could call thisXFS_DEBUG_WARNand 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)
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Brian Foster
As noted above, this is just a reuse of the flag.
XFS_WARNandXFS_DEBUGare 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. TheKconfighack above simply pops up a conditional option when debug mode is enabled that effectively allows setting bothXFS_WARNandXFS_DEBUGat the same time. The header files interpret this as debug mode enabled with an override for the assert failures to warn rather thanBUG().
Quoted 3 lines
Oh, I didn't know
Kconfigallows 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.
Quoted 7 lines
Quoted 6 lines
Note that this is just a hack and we can organize the
Kconfigoptions however we want. For example, we could call thisXFS_DEBUG_WARNand 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.
Quoted 6 lines
Maybe we need something like:
XFS_WARN(as-is)
XFS_WARN_BUG(XFS_WARN+BUG_ONon assert)
XFS_DEBUG(everyhing underXFS_DEBUGcurrently 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
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 44 lines · Brian Foster
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Brian Foster
As noted above, this is just a reuse of the flag.
XFS_WARNandXFS_DEBUGare 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. TheKconfighack above simply pops up a conditional option when debug mode is enabled that effectively allows setting bothXFS_WARNandXFS_DEBUGat the same time. The header files interpret this as debug mode enabled with an override for the assert failures to warn rather thanBUG().Quoted 3 lines
Oh, I didn't know
Kconfigallows 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.
Quoted 7 lines
Quoted 6 lines
Note that this is just a hack and we can organize the
Kconfigoptions however we want. For example, we could call thisXFS_DEBUG_WARNand 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.Quoted 6 lines
Maybe we need something like:
XFS_WARN(as-is)
XFS_WARN_BUG(XFS_WARN+BUG_ONon assert)
XFS_DEBUG(everyhing underXFS_DEBUGcurrently that's not related to
ASSERT)That allows asserts to
BUG()on anXFS_WARNkernel, which is not quite what I want to accomplish here (and I don't think that's really needed for anXFS_WARNkernel). I'm not quite following if/how that allows to disable assert BUG()s on anXFS_DEBUGkernel. WouldXFS_DEBUGnow neverBUG()unlessXFS_WARN_BUGis 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 onXFS_DEBUG, enablesBUG()on assert failure)
Quoted 7 lines
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 conditionalXFS_ASSERT_BUGconfig 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' forXFS_ASSERT_BUGso long as it is only available inXFS_DEBUGmode. 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
Quoted 12 lines
Brian
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html