Re: [PATCH v2 1/2] fs: add asserting functions for sb_start_{write,pagefault,intwrite}

[Dave Chinner <david@fromorbit.com>]

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@wdc.com>
> ---
>  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);
}

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.
-- 
Dave Chinner
david@fromorbit.com