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

[Gladyshev Ilya]

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.

> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/
> > ---
> >   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.