On 2/15/22 06:35, Dave Chinner wrote:
> On Thu, Feb 10, 2022 at 02:59:04PM +0900, Naohiro Aota wrote:
>> Add an assert function sb_assert_write_started() to check if
>> sb_start_write() is properly called. It is used in the next commit.
>>
>> Also, add the assert functions for sb_start_pagefault() and
>> sb_start_intwrite().
>>
>> Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
>> ---
>> include/linux/fs.h | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbf812ce89a8..5d5dc9a276d9 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1820,6 +1820,11 @@ static inline bool __sb_start_write_trylock(struct super_block *sb, int level)
>> #define __sb_writers_release(sb, lev) \
>> percpu_rwsem_release(&(sb)->s_writers.rw_sem[(lev)-1], 1, _THIS_IP_)
>>
>> +static inline void __sb_assert_write_started(struct super_block *sb, int level)
>> +{
>> + lockdep_assert_held_read(sb->s_writers.rw_sem + level - 1);
>> +}
>> +
>
> So this isn't an assert, it's a WARN_ON(). Asserts stop execution
> (i.e. kill the task) rather than just issue a warning, so let's not
> name a function that issues a warning "assert"...
>
> Hence I'd much rather see this implemented as:
>
> static inline bool __sb_write_held(struct super_block *sb, int level)
> {
> return lockdep_is_held_type(sb->s_writers.rw_sem + level - 1, 1);
> }
Since this would be true when called in between __sb_start_write() and
__sb_end_write(), what about calling it __sb_write_started() ? That
disconnects from the fact that the implementation uses a sem.
>
> i.e. named similar to __sb_start_write/__sb_end_write, with similar
> wrappers for pagefault/intwrite, and it just returns a bool status
> that lets the caller do what it wants with the status (warn, bug,
> etc).
>
> Then in the code that needs to check if the right freeze levels are
> held simply need to do:
>
> WARN_ON(!sb_write_held(sb));
>
> in which case it's self documenting in the code that cares about
> this and it's also obvious to anyone debugging such a message where
> it came from and what constraint got violated...
>
> Cheers,
>
> Dave.
--
Damien Le Moal
Western Digital Research
