On Tue, Feb 15, 2022 at 11:05:15AM +1100, Dave Chinner wrote:
> On Tue, Feb 15, 2022 at 07:49:27AM +0900, Damien Le Moal wrote:
> > 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.
>
> Makes no difference to me; I initially was going to suggest
> *_inprogress() but that seemed a bit verbose. We don't need to
> bikeshed this to death - all I want is it to be a check that can be
> used for generic purposes rather than being an explicit assert.
Agree. I'd like to use __sb_write_started() as it is conforming to
other functions.
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
