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

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

Posted by Gladyshev Ilya 3 months, 1 week ago

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

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

Posted by David Sterba 3 months ago

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.

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

Posted by Gladyshev Ilya 3 months ago

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.

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

Posted by David Sterba 3 months ago

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.