| Message ID | 40cbbef14229eaa34df0cdc576f02a1bd4ba6809.1644469146.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: mark relocation as writing | expand |
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.
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@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); > } 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.
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@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); > > } > > 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. Cheers, Dave.
On 2/15/22 09:05, 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@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); >>> } >> >> 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. > > Cheers, > > Dave.
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@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); > > > } > > > > 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@fromorbit.com
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); +} + /** * sb_end_write - drop write access to a superblock * @sb: the super we wrote to @@ -1885,6 +1890,11 @@ static inline bool sb_start_write_trylock(struct super_block *sb) return __sb_start_write_trylock(sb, SB_FREEZE_WRITE); } +static inline void sb_assert_write_started(struct super_block *sb) +{ + __sb_assert_write_started(sb, SB_FREEZE_WRITE); +} + /** * sb_start_pagefault - get write access to a superblock from a page fault * @sb: the super we write to @@ -1909,6 +1919,11 @@ static inline void sb_start_pagefault(struct super_block *sb) __sb_start_write(sb, SB_FREEZE_PAGEFAULT); } +static inline void sb_assert_pagefault_started(struct super_block *sb) +{ + __sb_assert_write_started(sb, SB_FREEZE_PAGEFAULT); +} + /** * sb_start_intwrite - get write access to a superblock for internal fs purposes * @sb: the super we write to @@ -1932,6 +1947,11 @@ static inline bool sb_start_intwrite_trylock(struct super_block *sb) return __sb_start_write_trylock(sb, SB_FREEZE_FS); } +static inline void sb_assert_intwrite_started(struct super_block *sb) +{ + __sb_assert_write_started(sb, SB_FREEZE_FS); +} + bool inode_owner_or_capable(struct user_namespace *mnt_userns, const struct inode *inode);
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(+)