Re: [PATCH v2] btrfs: make ASSERT no-op in release builds

[David Sterba <dsterba@suse.cz>]

On Tue, Nov 04, 2025 at 02:37:30PM +0300, Gladyshev Ilya wrote:
> On 11/4/25 03:18, David Sterba wrote:
> > On Sun, Nov 02, 2025 at 10:38:52AM +0300, Gladyshev Ilya wrote:
> >> The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant,
> >> since these checks have no side effects and don't affect code logic.
> > 
> > Have you checked that none of the assert expressions really don't have
> > side effects other than touching the memory?
> 
> Yes, but visually only. Most checks are plain C comparisons, and some 
> call folio/btrfs/refcount _check/test_ functions where I didn't find 
> side effects.
> 
> However, fs/btrfs/ has ~880 asserts, so if you know more robust 
> verification methods, I'd be glad to try them.

Good, thanks. I tried the same, with some random grep filters for
possible function calls but nothing out of scope found so I guess this
is sufficient.

> >> However, some checks contain READ_ONCE or other compiler-unfriendly
> >> constructs. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode
> >> was compiled to a redundant mov instruction due to this issue.
> >>
> >> This patch defines ASSERT as BUILD_BUG_ON_INVALID for !CONFIG_BTRFS_ASSERT
> >> builds. It also marks `full_page_sectors_uptodate` as __maybe_unused to
> >> suppress "unneeded declaration" warning (it's needed in compile time)
> >>
> >> Signed-off-by: Gladyshev Ilya <foxido@foxido.dev>
> >> ---
> >> Changes from v1:
> >> - Annotate full_page_sectors_uptodate as __maybe_unused to avoid
> >>    compiler warning
> >>
> >> Link to v1: https://lore.kernel.org/linux-btrfs/20251030182322.4085697-1-foxido@foxido.dev/
> >> ---
> >>   fs/btrfs/messages.h | 2 +-
> >>   fs/btrfs/raid56.c   | 4 ++--
> >>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
> >> index 4416c165644f..f80fe40a2c2b 100644
> >> --- a/fs/btrfs/messages.h
> >> +++ b/fs/btrfs/messages.h
> >> @@ -168,7 +168,7 @@ do {										\
> >>   #endif
> >>   
> >>   #else
> >> -#define ASSERT(cond, args...)			(void)(cond)
> >> +#define ASSERT(cond, args...)			BUILD_BUG_ON_INVALID(cond)
> > 
> > I'd rather have the expression open coded rather than using
> > BUILD_BUG_ON_INVALID, the name is confusing as it's not checking build
> > time condtitons.
> 
> The name kinda indicates that it triggers on invalid conditions, not 
> false ones, but I understand that it can be confusing. While we could 
> use direct sizeof() magic here, I prefer reusing the same infrastructure 
> as VM_BUG_ON(), VFS_*_ON() and others.
> 
> Maybe adding a comment about its semantics above ASSERT definition will 
> help clarify the usage? But if you prefer the sizeof() approach, I can 
> change it - it's not a big deal.

A comment for ASSERT works too. The BUILD_BUG_ON_INVALID is indeed
widely used so I don't expect any sudden change in semantics. As adding
the comment is simple I'll do that, no need to resend the patch. Thanks.