Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney <jeffm@suse.com>]

[-- 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 --]