* [PATCH v2] btrfs: make ASSERT no-op in release builds @ 2025-11-02 7:38 Gladyshev Ilya 2025-11-04 0:18 ` David Sterba 0 siblings, 1 reply; 4+ messages in thread From: Gladyshev Ilya @ 2025-11-02 7:38 UTC (permalink / raw) To: foxido; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant, since these checks have no side effects and don't affect code logic. 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) #endif #ifdef CONFIG_BTRFS_DEBUG diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 0135dceb7baa..302f20d8c335 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -299,8 +299,8 @@ static int rbio_bucket(struct btrfs_raid_bio *rbio) return hash_64(num >> 16, BTRFS_STRIPE_HASH_TABLE_BITS); } -static bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio, - unsigned int page_nr) +static __maybe_unused bool full_page_sectors_uptodate(struct btrfs_raid_bio *rbio, + unsigned int page_nr) { const u32 sectorsize = rbio->bioc->fs_info->sectorsize; const u32 sectors_per_page = PAGE_SIZE / sectorsize; base-commit: e53642b87a4f4b03a8d7e5f8507fc3cd0c595ea6 -- 2.51.1.dirty ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] btrfs: make ASSERT no-op in release builds 2025-11-02 7:38 [PATCH v2] btrfs: make ASSERT no-op in release builds Gladyshev Ilya @ 2025-11-04 0:18 ` David Sterba 2025-11-04 11:37 ` Gladyshev Ilya 0 siblings, 1 reply; 4+ messages in thread From: David Sterba @ 2025-11-04 0:18 UTC (permalink / raw) To: Gladyshev Ilya; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel 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? > 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] btrfs: make ASSERT no-op in release builds 2025-11-04 0:18 ` David Sterba @ 2025-11-04 11:37 ` Gladyshev Ilya 2025-11-04 12:10 ` David Sterba 0 siblings, 1 reply; 4+ messages in thread From: Gladyshev Ilya @ 2025-11-04 11:37 UTC (permalink / raw) To: dsterba; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel 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@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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] btrfs: make ASSERT no-op in release builds 2025-11-04 11:37 ` Gladyshev Ilya @ 2025-11-04 12:10 ` David Sterba 0 siblings, 0 replies; 4+ messages in thread From: David Sterba @ 2025-11-04 12:10 UTC (permalink / raw) To: Gladyshev Ilya; +Cc: Chris Mason, David Sterba, linux-btrfs, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-11-04 12:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-02 7:38 [PATCH v2] btrfs: make ASSERT no-op in release builds Gladyshev Ilya 2025-11-04 0:18 ` David Sterba 2025-11-04 11:37 ` Gladyshev Ilya 2025-11-04 12:10 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox