| Message ID | 20230127133202.16220-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: restore assertion failure to the code line where it happens | expand |
On 1/27/23 21:32, David Sterba wrote: > In commit 083bd7e54e8e ("btrfs: move the printk and assert helpers to > messages.c") btrfs_assertfail got un-inlined. This means that assertion > failures would all report as messages.c:259 as below, so make it inline > again. > > [403.246730] assertion failed: refcount_read(&block_group->refs) == 1, in fs/btrfs/block-group.c:4259 > [403.247935] ------------[ cut here ]------------ > [403.248405] kernel BUG at fs/btrfs/messages.c:259! Hmm. We have the line number shown from the assert as block-group.c:4259 here. messages.c:259 is from the BUG() called by btrfs_assertfail(). Commit 083bd7e54e8e didn't introduce it. Here is some random example of calling the ASSERT() from 2015. ------------------------ commit 67c5e7d464bc466471b05e027abe8a6b29687ebd <snap> [181631.208236] BTRFS: assertion failed: 0, file: fs/btrfs/volumes.c, line: 2622 [181631.220591] ------------[ cut here ]------------ [181631.222959] kernel BUG at fs/btrfs/ctree.h:4062! ------------------------ > #ifdef CONFIG_BTRFS_ASSERT > -void __cold btrfs_assertfail(const char *expr, const char *file, int line); > +static inline void __cold __noreturn btrfs_assertfail(const char *expr, Further, this won't make all the calls to btrfs_assertfail() as inline unless __always_inline is used. Thanks, Anand
On Tue, Jan 31, 2023 at 07:11:43PM +0800, Anand Jain wrote: > On 1/27/23 21:32, David Sterba wrote: > > In commit 083bd7e54e8e ("btrfs: move the printk and assert helpers to > > messages.c") btrfs_assertfail got un-inlined. This means that assertion > > failures would all report as messages.c:259 as below, so make it inline > > again. > > > > [403.246730] assertion failed: refcount_read(&block_group->refs) == 1, in fs/btrfs/block-group.c:4259 > > [403.247935] ------------[ cut here ]------------ > > [403.248405] kernel BUG at fs/btrfs/messages.c:259! > > > Hmm. We have the line number shown from the assert as block-group.c:4259 > here. > > messages.c:259 is from the BUG() called by btrfs_assertfail(). > > Commit 083bd7e54e8e didn't introduce it. Here is some random example of > calling the ASSERT() from 2015. Right, after double checking the code only got moved, not uninlined. > ------------------------ > commit 67c5e7d464bc466471b05e027abe8a6b29687ebd > <snap> > [181631.208236] BTRFS: assertion failed: 0, file: > fs/btrfs/volumes.c, line: 2622 > [181631.220591] ------------[ cut here ]------------ > [181631.222959] kernel BUG at fs/btrfs/ctree.h:4062! > ------------------------ > > > > #ifdef CONFIG_BTRFS_ASSERT > > -void __cold btrfs_assertfail(const char *expr, const char *file, int line); > > > +static inline void __cold __noreturn btrfs_assertfail(const char *expr, > > Further, this won't make all the calls to btrfs_assertfail() as inline > unless __always_inline is used. The always_inline has a bit stronger semantics and it would be safer to use it here though the function is short enough to be considered for inlining. If the inlining or not is useful would need to be measured, inlining grows the function code vs just a function call. I'll may be do that but for now the code can stay as is.
Hi, > On Tue, Jan 31, 2023 at 07:11:43PM +0800, Anand Jain wrote: > > On 1/27/23 21:32, David Sterba wrote: > > > In commit 083bd7e54e8e ("btrfs: move the printk and assert helpers to > > > messages.c") btrfs_assertfail got un-inlined. This means that assertion > > > failures would all report as messages.c:259 as below, so make it inline > > > again. > > > > > > [403.246730] assertion failed: refcount_read(&block_group->refs) == 1, in fs/btrfs/block-group.c:4259 > > > [403.247935] ------------[ cut here ]------------ > > > [403.248405] kernel BUG at fs/btrfs/messages.c:259! > > > > > > Hmm. We have the line number shown from the assert as block-group.c:4259 > > here. > > > > messages.c:259 is from the BUG() called by btrfs_assertfail(). > > > > Commit 083bd7e54e8e didn't introduce it. Here is some random example of > > calling the ASSERT() from 2015. > > Right, after double checking the code only got moved, not uninlined. > > > ------------------------ > > commit 67c5e7d464bc466471b05e027abe8a6b29687ebd > > <snap> > > [181631.208236] BTRFS: assertion failed: 0, file: > > fs/btrfs/volumes.c, line: 2622 > > [181631.220591] ------------[ cut here ]------------ > > [181631.222959] kernel BUG at fs/btrfs/ctree.h:4062! > > ------------------------ > > > > > > > #ifdef CONFIG_BTRFS_ASSERT > > > -void __cold btrfs_assertfail(const char *expr, const char *file, int line); > > > > > +static inline void __cold __noreturn btrfs_assertfail(const char *expr, > > > > Further, this won't make all the calls to btrfs_assertfail() as inline > > unless __always_inline is used. > > The always_inline has a bit stronger semantics and it would be safer to > use it here though the function is short enough to be considered for > inlining. > > If the inlining or not is useful would need to be measured, inlining > grows the function code vs just a function call. I'll may be do that but > for now the code can stay as is. This patch is yet not in misc-next? By the way , '__cold' is meanless for inline? Best Regards Wang Yugui (wangyugui@e16-tech.com) 2023/02/21
On Tue, Feb 21, 2023 at 06:28:11PM +0800, Wang Yugui wrote: > Hi, > > > On Tue, Jan 31, 2023 at 07:11:43PM +0800, Anand Jain wrote: > > > On 1/27/23 21:32, David Sterba wrote: > > > > In commit 083bd7e54e8e ("btrfs: move the printk and assert helpers to > > > > messages.c") btrfs_assertfail got un-inlined. This means that assertion > > > > failures would all report as messages.c:259 as below, so make it inline > > > > again. > > > > > > > > [403.246730] assertion failed: refcount_read(&block_group->refs) == 1, in fs/btrfs/block-group.c:4259 > > > > [403.247935] ------------[ cut here ]------------ > > > > [403.248405] kernel BUG at fs/btrfs/messages.c:259! > > > > > > > > > Hmm. We have the line number shown from the assert as block-group.c:4259 > > > here. > > > > > > messages.c:259 is from the BUG() called by btrfs_assertfail(). > > > > > > Commit 083bd7e54e8e didn't introduce it. Here is some random example of > > > calling the ASSERT() from 2015. > > > > Right, after double checking the code only got moved, not uninlined. > > > > > ------------------------ > > > commit 67c5e7d464bc466471b05e027abe8a6b29687ebd > > > <snap> > > > [181631.208236] BTRFS: assertion failed: 0, file: > > > fs/btrfs/volumes.c, line: 2622 > > > [181631.220591] ------------[ cut here ]------------ > > > [181631.222959] kernel BUG at fs/btrfs/ctree.h:4062! > > > ------------------------ > > > > > > > > > > #ifdef CONFIG_BTRFS_ASSERT > > > > -void __cold btrfs_assertfail(const char *expr, const char *file, int line); > > > > > > > +static inline void __cold __noreturn btrfs_assertfail(const char *expr, > > > > > > Further, this won't make all the calls to btrfs_assertfail() as inline > > > unless __always_inline is used. > > > > The always_inline has a bit stronger semantics and it would be safer to > > use it here though the function is short enough to be considered for > > inlining. > > > > If the inlining or not is useful would need to be measured, inlining > > grows the function code vs just a function call. I'll may be do that but > > for now the code can stay as is. > > This patch is yet not in misc-next? It was not woarking as I though as it does not change how the assertion line is printed. Inlining would also inline the place of BUG() so that could make a difference but otherwise needs to be measure how the inlining bloats the object files. > By the way , '__cold' is meanless for inline? Right.
diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c index fde5aaa6e7c9..23fc11af498a 100644 --- a/fs/btrfs/messages.c +++ b/fs/btrfs/messages.c @@ -252,14 +252,6 @@ void __cold _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, } #endif -#ifdef CONFIG_BTRFS_ASSERT -void __cold btrfs_assertfail(const char *expr, const char *file, int line) -{ - pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); - BUG(); -} -#endif - void __cold btrfs_print_v0_err(struct btrfs_fs_info *fs_info) { btrfs_err(fs_info, diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h index 8c516ee58ff9..9e8e7741ab76 100644 --- a/fs/btrfs/messages.h +++ b/fs/btrfs/messages.h @@ -4,6 +4,8 @@ #define BTRFS_MESSAGES_H #include <linux/types.h> +#include <linux/printk.h> +#include <linux/bug.h> struct btrfs_fs_info; @@ -160,7 +162,12 @@ do { \ } while (0) #ifdef CONFIG_BTRFS_ASSERT -void __cold btrfs_assertfail(const char *expr, const char *file, int line); +static inline void __cold __noreturn btrfs_assertfail(const char *expr, + const char *file, int line) +{ + pr_err("assertion failed: %s, in %s:%d\n", expr, file, line); + BUG(); +} #define ASSERT(expr) \ (likely(expr) ? (void)0 : btrfs_assertfail(#expr, __FILE__, __LINE__))
In commit 083bd7e54e8e ("btrfs: move the printk and assert helpers to messages.c") btrfs_assertfail got un-inlined. This means that assertion failures would all report as messages.c:259 as below, so make it inline again. [403.246730] assertion failed: refcount_read(&block_group->refs) == 1, in fs/btrfs/block-group.c:4259 [403.247935] ------------[ cut here ]------------ [403.248405] kernel BUG at fs/btrfs/messages.c:259! [403.248879] invalid opcode: 0000 [#1] PREEMPT SMP KASAN [403.249363] CPU: 2 PID: 23202 Comm: umount Not tainted 6.2.0-rc4-default+ #67 [403.249986] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 [403.250931] RIP: 0010:btrfs_assertfail+0x19/0x1b [btrfs] ... [403.259517] Call Trace: [403.259840] <TASK> [403.260134] btrfs_free_block_groups.cold+0x52/0xae [btrfs] [403.260824] close_ctree+0x6c2/0x761 [btrfs] [403.261395] ? __wait_for_common+0x2b8/0x360 [403.261899] ? btrfs_cleanup_one_transaction.cold+0x7a/0x7a [btrfs] [403.262632] ? mark_held_locks+0x6b/0x90 [403.263084] ? lockdep_hardirqs_on_prepare+0x13d/0x200 [403.263628] ? __call_rcu_common.constprop.0+0x1ea/0x3d0 [403.264213] ? trace_hardirqs_on+0x2d/0x110 [403.264699] ? __call_rcu_common.constprop.0+0x1ea/0x3d0 [403.265279] generic_shutdown_super+0xb0/0x1c0 [403.265794] kill_anon_super+0x1e/0x40 [403.266241] btrfs_kill_super+0x25/0x30 [btrfs] [403.266836] deactivate_locked_super+0x4c/0xc0 Fixes: 083bd7e54e8e ("btrfs: move the printk and assert helpers to messages.c") Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/messages.c | 8 -------- fs/btrfs/messages.h | 9 ++++++++- 2 files changed, 8 insertions(+), 9 deletions(-)