[PATCH] Btrfs: add support for asserts

[PATCH] Btrfs: add support for asserts

[Josef Bacik]

One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
with this I'm introducing a kconfig option to enable/disable a new ASSERT()
mechanism much like what XFS does.  This will allow us developers to still get
our nice panics but allow users/distros to compile them out.  With this we can
go through and convert any BUG_ON()'s that we have to catch actual programming
mistakes to the new ASSERT() and then fix everybody else to return errors.  This
will also allow developers to leave sanity checks in their new code to make sure
we don't trip over problems while testing stuff and vetting new features.
Thanks,

Signed-off-by: Josef Bacik <jbacik@fusionio.com>
---
 fs/btrfs/Kconfig |    9 +++++++++
 fs/btrfs/ctree.h |   16 ++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
index 2b3b832..398cbd5 100644
--- a/fs/btrfs/Kconfig
+++ b/fs/btrfs/Kconfig
@@ -72,3 +72,12 @@ config BTRFS_DEBUG
 	  performance, or export extra information via sysfs.
 
 	  If unsure, say N.
+
+config BTRFS_ASSERT
+	bool "Btrfs assert support"
+	depends on BTRFS_FS
+	help
+	  Enable run-time assertion checking.  This will result in panics if
+	  any of the assertions trip.  This is meant for btrfs developers only.
+
+	  If unsure, say N.
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index c90be01..8278a3f 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 #define btrfs_debug(fs_info, fmt, args...) \
 	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
 
+#ifdef BTRFS_ASSERT
+
+static inline void assfail(char *expr, char *file, int lin)
+{
+	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
+	       expr, file, line);
+	BUG();
+}
+
+#define ASSERT(expr)	\
+	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
+#else
+#define ASSERT(expr)	((void)0)
+#endif
+
+#define btrfs_assert()
 __printf(5, 6)
 void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
 		     unsigned int line, int errno, const char *fmt, ...);
-- 
1.7.7.6

Re: [PATCH] Btrfs: add support for asserts

[Eric Sandeen]

On 8/26/13 3:56 PM, Josef Bacik wrote:
> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
> mechanism much like what XFS does.  This will allow us developers to still get
> our nice panics but allow users/distros to compile them out.  With this we can
> go through and convert any BUG_ON()'s that we have to catch actual programming
> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
> will also allow developers to leave sanity checks in their new code to make sure
> we don't trip over problems while testing stuff and vetting new features.
> Thanks,
> 
> Signed-off-by: Josef Bacik <jbacik@fusionio.com>

+1000 for inheriting the wildly popular XFS assfail() technology. ;)

I think this is a step in the right direction, it'll make it easier to
clearly mark things which are logic assertions vs. things which are just punts
in more common error-handling paths.

Acked-by: Eric Sandeen <sandeen@redhat.com>

Thanks,

-Eric

> ---
>  fs/btrfs/Kconfig |    9 +++++++++
>  fs/btrfs/ctree.h |   16 ++++++++++++++++
>  2 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
> index 2b3b832..398cbd5 100644
> --- a/fs/btrfs/Kconfig
> +++ b/fs/btrfs/Kconfig
> @@ -72,3 +72,12 @@ config BTRFS_DEBUG
>  	  performance, or export extra information via sysfs.
>  
>  	  If unsure, say N.
> +
> +config BTRFS_ASSERT
> +	bool "Btrfs assert support"
> +	depends on BTRFS_FS
> +	help
> +	  Enable run-time assertion checking.  This will result in panics if
> +	  any of the assertions trip.  This is meant for btrfs developers only.
> +
> +	  If unsure, say N.
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index c90be01..8278a3f 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>  #define btrfs_debug(fs_info, fmt, args...) \
>  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>  
> +#ifdef BTRFS_ASSERT
> +
> +static inline void assfail(char *expr, char *file, int lin)
> +{
> +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
> +	       expr, file, line);
> +	BUG();
> +}
> +
> +#define ASSERT(expr)	\
> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> +#else
> +#define ASSERT(expr)	((void)0)
> +#endif
> +
> +#define btrfs_assert()
>  __printf(5, 6)
>  void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
>  		     unsigned int line, int errno, const char *fmt, ...);
> 

Re: [PATCH] Btrfs: add support for asserts

[Zach Brown]

> With this we can
> go through and convert any BUG_ON()'s that we have to catch actual programming
> mistakes to the new ASSERT() and then fix everybody else to return errors.

I like the sound of that!

> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>  #define btrfs_debug(fs_info, fmt, args...) \
>  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>  
> +#ifdef BTRFS_ASSERT
> +
> +static inline void assfail(char *expr, char *file, int lin)
> +{
> +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
> +	       expr, file, line);
> +	BUG();
> +}

I'm not sure why this is needed.

> +#define ASSERT(expr)	\
> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))

(Passing the assertion is unlikely()?  I know, this is from xfs...
still.)

> +#else
> +#define ASSERT(expr)	((void)0)
> +#endif

Anyway, if you're going to do it this way, why not:

	#ifdef BTRFS_ASSERT
	#define btrfs_assert(cond)  BUG_ON(!(cond))
	#else
	#define btrfs_assert(cond)  do { if (cond) ; } while (0)
	#endif

?

- z

Re: [PATCH] Btrfs: add support for asserts

[Eric Sandeen]

On 8/26/13 4:53 PM, Zach Brown wrote:
>> With this we can
>> go through and convert any BUG_ON()'s that we have to catch actual programming
>> mistakes to the new ASSERT() and then fix everybody else to return errors.
> 
> I like the sound of that!
> 
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>>  #define btrfs_debug(fs_info, fmt, args...) \
>>  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>>  
>> +#ifdef BTRFS_ASSERT
>> +
>> +static inline void assfail(char *expr, char *file, int lin)
>> +{
>> +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
>> +	       expr, file, line);
>> +	BUG();
>> +}
> 
> I'm not sure why this is needed.

I think it's because we'd like to see the assertion that failed in plain text,
which then would need a function as above, but we'd rather not see that
_every_ ASSERT() failure was at the line of the BUG() in the helper function...

i.e. when xfs trips it does this:

XFS: Assertion failed: first <= last && last < BBTOB(bp->b_length),
file: fs/xfs/xfs_trans_buf.c, line: 568
------------[ cut here ]------------
kernel BUG at fs/xfs/xfs_message.c:108!

note the 2 different line numbers; 568 is what's relevant, 108 is not.

>> +#define ASSERT(expr)	\
>> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> 
> (Passing the assertion is unlikely()?  I know, this is from xfs...
> still.)

hah, that's great.

>> +#else
>> +#define ASSERT(expr)	((void)0)
>> +#endif
> 
> Anyway, if you're going to do it this way, why not:
> 
> 	#ifdef BTRFS_ASSERT
> 	#define btrfs_assert(cond)  BUG_ON(!(cond))
> 	#else
> 	#define btrfs_assert(cond)  do { if (cond) ; } while (0)
> 	#endif

I think the only downside is that the BUG_ON() won't print the
conditional that failed, IIRC.

-Eric

> ?
> 
> - z

Re: [PATCH] Btrfs: add support for asserts

[Zach Brown]

> > 	#ifdef BTRFS_ASSERT
> > 	#define btrfs_assert(cond)  BUG_ON(!(cond))
> > 	#else
> > 	#define btrfs_assert(cond)  do { if (cond) ; } while (0)
> > 	#endif
> 
> I think the only downside is that the BUG_ON() won't print the
> conditional that failed, IIRC.

Sure, if you wanted to go the heavier informative route.  I might also
add format and args in that case.

- z

Re: [PATCH] Btrfs: add support for asserts

[Josef Bacik]

On Mon, Aug 26, 2013 at 02:53:26PM -0700, Zach Brown wrote:
> > With this we can
> > go through and convert any BUG_ON()'s that we have to catch actual programming
> > mistakes to the new ASSERT() and then fix everybody else to return errors.
> 
> I like the sound of that!
> 
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
> >  #define btrfs_debug(fs_info, fmt, args...) \
> >  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
> >  
> > +#ifdef BTRFS_ASSERT
> > +
> > +static inline void assfail(char *expr, char *file, int lin)
> > +{
> > +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
> > +	       expr, file, line);
> > +	BUG();
> > +}
> 
> I'm not sure why this is needed.
> 
> > +#define ASSERT(expr)	\
> > +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> 
> (Passing the assertion is unlikely()?  I know, this is from xfs...
> still.)
>

Yeah I copy+pasted and then thought about it after I sent it.  I will fix it up.
 
> > +#else
> > +#define ASSERT(expr)	((void)0)
> > +#endif
> 
> Anyway, if you're going to do it this way, why not:
> 
> 	#ifdef BTRFS_ASSERT
> 	#define btrfs_assert(cond)  BUG_ON(!(cond))
> 	#else
> 	#define btrfs_assert(cond)  do { if (cond) ; } while (0)
> 	#endif
>

I like the verbosity, especially with random kernel versions and such, it will
help me figure out where we BUG_ON()'ed without having to checkout a particular
version and go hunting.  Thanks,

Josef 

Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

On 8/27/13 9:47 AM, Josef Bacik wrote:
> On Mon, Aug 26, 2013 at 02:53:26PM -0700, Zach Brown wrote:
>>> With this we can
>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>> mistakes to the new ASSERT() and then fix everybody else to return errors.
>>
>> I like the sound of that!
>>
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>>>  #define btrfs_debug(fs_info, fmt, args...) \
>>>  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>>>  
>>> +#ifdef BTRFS_ASSERT
>>> +
>>> +static inline void assfail(char *expr, char *file, int lin)
>>> +{
>>> +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
>>> +	       expr, file, line);
>>> +	BUG();
>>> +}
>>
>> I'm not sure why this is needed.
>>
>>> +#define ASSERT(expr)	\
>>> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
>>
>> (Passing the assertion is unlikely()?  I know, this is from xfs...
>> still.)
>>
> 
> Yeah I copy+pasted and then thought about it after I sent it.  I will fix it up.
>  
>>> +#else
>>> +#define ASSERT(expr)	((void)0)
>>> +#endif
>>
>> Anyway, if you're going to do it this way, why not:
>>
>> 	#ifdef BTRFS_ASSERT
>> 	#define btrfs_assert(cond)  BUG_ON(!(cond))
>> 	#else
>> 	#define btrfs_assert(cond)  do { if (cond) ; } while (0)
>> 	#endif
>>
> 
> I like the verbosity, especially with random kernel versions and such, it will
> help me figure out where we BUG_ON()'ed without having to checkout a particular
> version and go hunting.  Thanks,

Agreed. One of the positives of the obnoxious reiserfs warning IDs is
that it uniquely identifies a call site across kernel versions. You can
tell at a glance that it's the same failure you may have been chasing
for a while. Anything to make the ID-at-a-glance easy is worth it.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 3226 bytes --]

On 8/26/13 4:56 PM, Josef Bacik wrote:
> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
> mechanism much like what XFS does.  This will allow us developers to still get
> our nice panics but allow users/distros to compile them out.  With this we can
> go through and convert any BUG_ON()'s that we have to catch actual programming
> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
> will also allow developers to leave sanity checks in their new code to make sure
> we don't trip over problems while testing stuff and vetting new features.
> Thanks,

I don't think the complaint is so much about the number of BUG_ONs, but
that there's no distinction between something that is supposed to be
impossible and something that is improbable. The BUG_ONs to keep code
correctness are good and are littered all over the kernel with positive
results. The BUG_ONs that are there in place of real error handling
served their purpose and need to be replaced.

So, I don't know if it's a net win to compile the "good" BUG_ONs out of
the code. Especially if a user runs into something strange yet familiar
and the first response is "oh, huh, can you rebuild with asserts enabled?"

For the call sites that are unimplemented error handling, something to
annotate those so that we can keep track of them and gradually eliminate
those too would be good, though.

-Jeff

> Signed-off-by: Josef Bacik <jbacik@fusionio.com>
> ---
>  fs/btrfs/Kconfig |    9 +++++++++
>  fs/btrfs/ctree.h |   16 ++++++++++++++++
>  2 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
> index 2b3b832..398cbd5 100644
> --- a/fs/btrfs/Kconfig
> +++ b/fs/btrfs/Kconfig
> @@ -72,3 +72,12 @@ config BTRFS_DEBUG
>  	  performance, or export extra information via sysfs.
>  
>  	  If unsure, say N.
> +
> +config BTRFS_ASSERT
> +	bool "Btrfs assert support"
> +	depends on BTRFS_FS
> +	help
> +	  Enable run-time assertion checking.  This will result in panics if
> +	  any of the assertions trip.  This is meant for btrfs developers only.
> +
> +	  If unsure, say N.
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index c90be01..8278a3f 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>  #define btrfs_debug(fs_info, fmt, args...) \
>  	btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>  
> +#ifdef BTRFS_ASSERT
> +
> +static inline void assfail(char *expr, char *file, int lin)
> +{
> +	printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
> +	       expr, file, line);
> +	BUG();
> +}
> +
> +#define ASSERT(expr)	\
> +	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
> +#else
> +#define ASSERT(expr)	((void)0)
> +#endif
> +
> +#define btrfs_assert()
>  __printf(5, 6)
>  void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
>  		     unsigned int line, int errno, const char *fmt, ...);
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Btrfs: add support for asserts

[Josef Bacik]

On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
> On 8/26/13 4:56 PM, Josef Bacik wrote:
> > One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
> > with this I'm introducing a kconfig option to enable/disable a new ASSERT()
> > mechanism much like what XFS does.  This will allow us developers to still get
> > our nice panics but allow users/distros to compile them out.  With this we can
> > go through and convert any BUG_ON()'s that we have to catch actual programming
> > mistakes to the new ASSERT() and then fix everybody else to return errors.  This
> > will also allow developers to leave sanity checks in their new code to make sure
> > we don't trip over problems while testing stuff and vetting new features.
> > Thanks,
> 
> I don't think the complaint is so much about the number of BUG_ONs, but
> that there's no distinction between something that is supposed to be
> impossible and something that is improbable. The BUG_ONs to keep code
> correctness are good and are littered all over the kernel with positive
> results. The BUG_ONs that are there in place of real error handling
> served their purpose and need to be replaced.
> 
> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
> the code. Especially if a user runs into something strange yet familiar
> and the first response is "oh, huh, can you rebuild with asserts enabled?"
> 

Either I provide an option for it or distros do it themselves, this cuts out the
middle man.  I'd really rather they just be on all the time since they aren't
things we should hit anyway, but at least this way people have a choice.
Thanks,

Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On 8/27/13 4:56 PM, Josef Bacik wrote:
> On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
>> On 8/26/13 4:56 PM, Josef Bacik wrote:
>>> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
>>> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
>>> mechanism much like what XFS does.  This will allow us developers to still get
>>> our nice panics but allow users/distros to compile them out.  With this we can
>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
>>> will also allow developers to leave sanity checks in their new code to make sure
>>> we don't trip over problems while testing stuff and vetting new features.
>>> Thanks,
>>
>> I don't think the complaint is so much about the number of BUG_ONs, but
>> that there's no distinction between something that is supposed to be
>> impossible and something that is improbable. The BUG_ONs to keep code
>> correctness are good and are littered all over the kernel with positive
>> results. The BUG_ONs that are there in place of real error handling
>> served their purpose and need to be replaced.
>>
>> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
>> the code. Especially if a user runs into something strange yet familiar
>> and the first response is "oh, huh, can you rebuild with asserts enabled?"
>>
> 
> Either I provide an option for it or distros do it themselves, this cuts out the
> middle man.  I'd really rather they just be on all the time since they aren't
> things we should hit anyway, but at least this way people have a choice.

Ok. With my distro hat on, I can tell you I'll be leaving them on. :)

-Jeff


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Btrfs: add support for asserts

[Eric Sandeen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/27/13 4:07 PM, Jeff Mahoney wrote:
> On 8/27/13 4:56 PM, Josef Bacik wrote:
>> On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
>>> On 8/26/13 4:56 PM, Josef Bacik wrote:
>>>> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
>>>> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
>>>> mechanism much like what XFS does.  This will allow us developers to still get
>>>> our nice panics but allow users/distros to compile them out.  With this we can
>>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>>> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
>>>> will also allow developers to leave sanity checks in their new code to make sure
>>>> we don't trip over problems while testing stuff and vetting new features.
>>>> Thanks,
>>>
>>> I don't think the complaint is so much about the number of BUG_ONs, but
>>> that there's no distinction between something that is supposed to be
>>> impossible and something that is improbable. The BUG_ONs to keep code
>>> correctness are good and are littered all over the kernel with positive
>>> results. The BUG_ONs that are there in place of real error handling
>>> served their purpose and need to be replaced.
>>>
>>> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
>>> the code. Especially if a user runs into something strange yet familiar
>>> and the first response is "oh, huh, can you rebuild with asserts enabled?"
>>>
>>
>> Either I provide an option for it or distros do it themselves, this cuts out the
>> middle man.  I'd really rather they just be on all the time since they aren't
>> things we should hit anyway, but at least this way people have a choice.
> 
> Ok. With my distro hat on, I can tell you I'll be leaving them on. :)
> 
> -Jeff

XFS also has XFS_WARN as a config option, which keeps all the assertions
in place, but printk's & backtraces w/o the icky BUG().  That might be
good to add as well, and perhaps best for a shipping distro (vs. a developer
debugging who might want to drop a core file when the assert trips).

- -Eric

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSHRhOAAoJECCuFpLhPd7gYbcP/034ADG3dwTa83FaAWuAurg7
byKWG4EwRqt3PYjUgruxBJAc426O7tz6j1NNTrAwZys9/GJOsisPShA8gO0f+W/A
+bQZJlXoUMbbwVPMcCqsnKMKlXNyKoqgME9AUQOrzMB/SgDtC9Y/OgdqgWF/58UV
X1KC3OOtcfQr/1t19AZuNhJ5oHfytoscv3nnnW5872t1JtL8daomak4fyDuRKgRV
45kQ726nafUlXNmi1TG8GadlcmKxxbBm0vt2ui6RtZWVauPE4Gej+iEUux9WtwSc
48eOQ5iqbFVzC8v++Rc1eT28mBIjSetr+O/Tk+VL4TvYCKA2trMAltNAFinv9AB0
Q+Z9F1K26aFe/Z/gcM57j+c0VOkv1tvSElF1iJcVHPuRvV7k+548g+KVzbXNDPBP
vuV2fnUCpw/XHQlrI+efYLs7Ies0TuV2eGPhmbKWjhossPwOeng71zxuiXSbNMBE
gVcHg6idXjCdaCCIYuJr8+5K4ngnpTEbAUs4C2x6iHzuHZcXScEHWYU/nHvizElL
bCZ162QSeQZAd+NgSzoZSmv4XqFMj6c4q60XvhAuu3fpkVPVY4GshcFhT14Onhfl
/054HqdQIXjUGOdbeuUwmXoaqzpSKDhBmZ0G+ykarD1KRCaEW61JrnFepRPO7G69
Q3oiPIbvdvCw3BZAJaGL
=7vvX
-----END PGP SIGNATURE-----

Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]

On 8/27/13 5:21 PM, Eric Sandeen wrote:
> On 8/27/13 4:07 PM, Jeff Mahoney wrote:
>> On 8/27/13 4:56 PM, Josef Bacik wrote:
>>> On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
>>>> On 8/26/13 4:56 PM, Josef Bacik wrote:
>>>>> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
>>>>> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
>>>>> mechanism much like what XFS does.  This will allow us developers to still get
>>>>> our nice panics but allow users/distros to compile them out.  With this we can
>>>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>>>> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
>>>>> will also allow developers to leave sanity checks in their new code to make sure
>>>>> we don't trip over problems while testing stuff and vetting new features.
>>>>> Thanks,
>>>>
>>>> I don't think the complaint is so much about the number of BUG_ONs, but
>>>> that there's no distinction between something that is supposed to be
>>>> impossible and something that is improbable. The BUG_ONs to keep code
>>>> correctness are good and are littered all over the kernel with positive
>>>> results. The BUG_ONs that are there in place of real error handling
>>>> served their purpose and need to be replaced.
>>>>
>>>> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
>>>> the code. Especially if a user runs into something strange yet familiar
>>>> and the first response is "oh, huh, can you rebuild with asserts enabled?"
>>>>
>>>
>>> Either I provide an option for it or distros do it themselves, this cuts out the
>>> middle man.  I'd really rather they just be on all the time since they aren't
>>> things we should hit anyway, but at least this way people have a choice.
> 
>> Ok. With my distro hat on, I can tell you I'll be leaving them on. :)
> 
>> -Jeff
> 
> XFS also has XFS_WARN as a config option, which keeps all the assertions
> in place, but printk's & backtraces w/o the icky BUG().  That might be
> good to add as well, and perhaps best for a shipping distro (vs. a developer
> debugging who might want to drop a core file when the assert trips).

Isn't that the distinction between BUG_ON and WARN_ON? If it's worth a
BUG_ON, things should be bad enough (or could result in being bad
enough) that we want to bail out.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Btrfs: add support for asserts

[Eric Sandeen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/27/13 4:25 PM, Jeff Mahoney wrote:
> On 8/27/13 5:21 PM, Eric Sandeen wrote:
>> On 8/27/13 4:07 PM, Jeff Mahoney wrote:
>>> On 8/27/13 4:56 PM, Josef Bacik wrote:
>>>> On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
>>>>> On 8/26/13 4:56 PM, Josef Bacik wrote:
>>>>>> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
>>>>>> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
>>>>>> mechanism much like what XFS does.  This will allow us developers to still get
>>>>>> our nice panics but allow users/distros to compile them out.  With this we can
>>>>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>>>>> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
>>>>>> will also allow developers to leave sanity checks in their new code to make sure
>>>>>> we don't trip over problems while testing stuff and vetting new features.
>>>>>> Thanks,
>>>>>
>>>>> I don't think the complaint is so much about the number of BUG_ONs, but
>>>>> that there's no distinction between something that is supposed to be
>>>>> impossible and something that is improbable. The BUG_ONs to keep code
>>>>> correctness are good and are littered all over the kernel with positive
>>>>> results. The BUG_ONs that are there in place of real error handling
>>>>> served their purpose and need to be replaced.
>>>>>
>>>>> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
>>>>> the code. Especially if a user runs into something strange yet familiar
>>>>> and the first response is "oh, huh, can you rebuild with asserts enabled?"
>>>>>
>>>>
>>>> Either I provide an option for it or distros do it themselves, this cuts out the
>>>> middle man.  I'd really rather they just be on all the time since they aren't
>>>> things we should hit anyway, but at least this way people have a choice.
>>
>>> Ok. With my distro hat on, I can tell you I'll be leaving them on. :)
>>
>>> -Jeff
>>
>> XFS also has XFS_WARN as a config option, which keeps all the assertions
>> in place, but printk's & backtraces w/o the icky BUG().  That might be
>> good to add as well, and perhaps best for a shipping distro (vs. a developer
>> debugging who might want to drop a core file when the assert trips).
> 
> Isn't that the distinction between BUG_ON and WARN_ON? If it's worth a
> BUG_ON, things should be bad enough (or could result in being bad
> enough) that we want to bail out.
> 
> -Jeff

Maybe; just FWIW here was Dave's rationale for xfs.  Right now btrfs
doesn't have the behavior-changing side effect (no BTRFS_DEBUG config)
though, so maybe the distinction is less important...

    xfs: introduce CONFIG_XFS_WARN
    
    Running a CONFIG_XFS_DEBUG kernel in production environments is not
    the best idea as it introduces significant overhead, can change
    the behaviour of algorithms (such as allocation) to improve test
    coverage, and (most importantly) panic the machine on non-fatal
    errors.
    
    There are many cases where all we want to do is run a
    kernel with more bounds checking enabled, such as is provided by the
    ASSERT() statements throughout the code, but without all the
    potential overhead and drawbacks.
    
    This patch converts all the ASSERT statements to evaluate as
    WARN_ON(1) statements and hence if they fail dump a warning and a
    stack trace to the log. This has minimal overhead and does not
    change any algorithms, and will allow us to find strange "out of
    bounds" problems more easily on production machines.
    
    There are a few places where assert statements contain debug only
    code. These are converted to be debug-or-warn only code so that we
    still get all the assert checks in the code.
    
    Signed-off-by: Dave Chinner <dchinner@redhat.com>


-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSHRn5AAoJECCuFpLhPd7gpsQQAJlFGX/t9b/LASxSisiv/wdL
ZoQXHhCzxpzVVdsWstzOY0bVXw8vdsG2E+nmih2S6T7AzkoqPDoEnYE9CqpNQFFy
Ca/kJOcfE1T4mIwKZwLHATkJX0V/S6nY7jPa7xdcseie+1H7ldSPaM5Jb6fkvXg/
8lNPTikGeoRJdUwQN4xxNgsivITfJpl65Z+AVg5UAUqqUKZtYZLfVeAlyQFKvOyl
/am80yLLzhFODtV3GcWkaYcInBaB2AaVlqHrpTnf53gG9JGynyFjnZGlysz0flSs
wstNKLOon+wNBg1Dz0HrUSVma87g5hc1WtaZFC/qI3uHuoatsxOWxG6+LXZlr2CN
Jsq3ZwHHxOs4MLgyEYlSirpgKqn/aKA+J8O0mNlltBj2lpU2hKgPS7dmMw5o8VAM
1uei1er15eBlCY0uBncRXIcLEcXfRXo9b69ErQBIbCN7xrGyWdbZ/DVtElaFeImh
Lw+iBXebBbw6SCqCMZFc3vpYdF+9RP6shImBlsqxTzKs5M1gISrFtCF0GqOuPrWt
7jyrredhpKACAaOpxPW8UWh2vL+q51JWzzZKYE35Gy4M/8E64TQ0rYhLGj7x+TYU
FWYzpONK0x7XbmgtEKTutwi9w+vfSlMzFNpUavwFeTZIh8Dw1tEO3dBn59Rs9Oz8
Widxpe+hqz/qK/0O4rTb
=4nmO
-----END PGP SIGNATURE-----

Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 4567 bytes --]

On 8/27/13 5:28 PM, Eric Sandeen wrote:
> On 8/27/13 4:25 PM, Jeff Mahoney wrote:
>> On 8/27/13 5:21 PM, Eric Sandeen wrote:
>>> On 8/27/13 4:07 PM, Jeff Mahoney wrote:
>>>> On 8/27/13 4:56 PM, Josef Bacik wrote:
>>>>> On Tue, Aug 27, 2013 at 03:28:24PM -0400, Jeff Mahoney wrote:
>>>>>> On 8/26/13 4:56 PM, Josef Bacik wrote:
>>>>>>> One of the complaints we get a lot is how many BUG_ON()'s we have.  So to help
>>>>>>> with this I'm introducing a kconfig option to enable/disable a new ASSERT()
>>>>>>> mechanism much like what XFS does.  This will allow us developers to still get
>>>>>>> our nice panics but allow users/distros to compile them out.  With this we can
>>>>>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>>>>>> mistakes to the new ASSERT() and then fix everybody else to return errors.  This
>>>>>>> will also allow developers to leave sanity checks in their new code to make sure
>>>>>>> we don't trip over problems while testing stuff and vetting new features.
>>>>>>> Thanks,
>>>>>>
>>>>>> I don't think the complaint is so much about the number of BUG_ONs, but
>>>>>> that there's no distinction between something that is supposed to be
>>>>>> impossible and something that is improbable. The BUG_ONs to keep code
>>>>>> correctness are good and are littered all over the kernel with positive
>>>>>> results. The BUG_ONs that are there in place of real error handling
>>>>>> served their purpose and need to be replaced.
>>>>>>
>>>>>> So, I don't know if it's a net win to compile the "good" BUG_ONs out of
>>>>>> the code. Especially if a user runs into something strange yet familiar
>>>>>> and the first response is "oh, huh, can you rebuild with asserts enabled?"
>>>>>>
>>>>>
>>>>> Either I provide an option for it or distros do it themselves, this cuts out the
>>>>> middle man.  I'd really rather they just be on all the time since they aren't
>>>>> things we should hit anyway, but at least this way people have a choice.
>>>
>>>> Ok. With my distro hat on, I can tell you I'll be leaving them on. :)
>>>
>>>> -Jeff
>>>
>>> XFS also has XFS_WARN as a config option, which keeps all the assertions
>>> in place, but printk's & backtraces w/o the icky BUG().  That might be
>>> good to add as well, and perhaps best for a shipping distro (vs. a developer
>>> debugging who might want to drop a core file when the assert trips).
> 
>> Isn't that the distinction between BUG_ON and WARN_ON? If it's worth a
>> BUG_ON, things should be bad enough (or could result in being bad
>> enough) that we want to bail out.
> 
>> -Jeff
> 
> Maybe; just FWIW here was Dave's rationale for xfs.  Right now btrfs
> doesn't have the behavior-changing side effect (no BTRFS_DEBUG config)
> though, so maybe the distinction is less important...

Yeah, I'd agree with the distinction not being there in btrfs (yet).
ReiserFS has a similar mode where there are a ton of checks that are
optionally enabled and does invasive things that can slow things down.
It's disabled pretty much universally AFAIK. One of the things (low) on
my TODO list is to go through all of those and move them into regular
checks since some of them are the types of things fsfuzzer likes to trip
over.

-Jeff

>     xfs: introduce CONFIG_XFS_WARN
>     
>     Running a CONFIG_XFS_DEBUG kernel in production environments is not
>     the best idea as it introduces significant overhead, can change
>     the behaviour of algorithms (such as allocation) to improve test
>     coverage, and (most importantly) panic the machine on non-fatal
>     errors.
>     
>     There are many cases where all we want to do is run a
>     kernel with more bounds checking enabled, such as is provided by the
>     ASSERT() statements throughout the code, but without all the
>     potential overhead and drawbacks.
>     
>     This patch converts all the ASSERT statements to evaluate as
>     WARN_ON(1) statements and hence if they fail dump a warning and a
>     stack trace to the log. This has minimal overhead and does not
>     change any algorithms, and will allow us to find strange "out of
>     bounds" problems more easily on production machines.
>     
>     There are a few places where assert statements contain debug only
>     code. These are converted to be debug-or-warn only code so that we
>     still get all the assert checks in the code.
>     
>     Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> 
> 

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Btrfs: add support for asserts

[David Sterba]

On Mon, Aug 26, 2013 at 04:56:06PM -0400, Josef Bacik wrote:
> +#ifdef BTRFS_ASSERT
> +
> +static inline void assfail(char *expr, char *file, int lin)

typo: lin instead of line