Re: [PATCH v2 15/18] btrfs: disk-io: introduce subpage metadata validation check

[Nikolay Borisov <nborisov@suse.com>]

On 14.12.20 г. 13:32 ч., Qu Wenruo wrote:
> 
> 
> On 2020/12/14 下午7:17, Nikolay Borisov wrote:
>>
>>
>> On 14.12.20 г. 12:50 ч., Qu Wenruo wrote:
>>>
>>> IIRC ASSERT() won't execute whatever in it for non debug build.
>>> Thus ASSERT(atomic_*) would cause non-debug kernel not to decrease the
>>> io_pages and hangs the system.
>>
>> Nope:
>>
>>    3362 #ifdef CONFIG_BTRFS_ASSERT
>>       1 __cold __noreturn
>>       2 static inline void assertfail(const char *expr, const char
>> *file, int line)
>>       3 {
>>       4         pr_err("assertion failed: %s, in %s:%d\n", expr, file,
>> line);
>>       5         BUG();
>>       6 }
>>       7
>>       8 #define ASSERT(expr)                                            \
>>       9         (likely(expr) ? (void)0 : assertfail(#expr, __FILE__,
>> __LINE__))
>>      10
>>      11 #else
>>      12 static inline void assertfail(const char *expr, const char*
>> file, int line) { }
>>      13 #define ASSERT(expr)    (void)(expr)               <--
>> expression is evaluated.
>>      14 #endif
>>
> Wow, that's too tricky and maybe that's the reason why Josef is
> complaining about the ASSERT()s slows down the system.
> 
> In fact, from the assert(3) man page, we're doing things differently
> than user space at least:
> 
>   If the macro NDEBUG is defined at the moment <assert.h> was last
>   included, the macro assert() generates no code, and hence does nothing
>   at all.
> 
> So I'm confused, what's the proper way to do ASSERT()?

Well as it stands now, what I suggested would work. OTOH this really
puts forward the question why do we leave code around (well, the
compiler should really eliminate those redundant checks). Hm, David?



> 
> Thanks,
> Qu
>