Re: [RFC] fs: Avoid to use lockdep information if it's turned off

[Boqun Feng]

On Mon, Nov 09, 2020 at 05:49:25PM -0800, Darrick J. Wong wrote:
> On Tue, Nov 10, 2020 at 09:37:37AM +0800, Boqun Feng wrote:
> > Filipe Manana reported a warning followed by task hanging after attempts
> > to freeze a filesystem[1]. The problem happened in a LOCKDEP=y kernel,
> > and percpu_rwsem_is_held() provided incorrect results when
> > debug_locks == 0. Although the behavior is caused by commit 4d004099a668
> > ("lockdep: Fix lockdep recursion"): after that lock_is_held() and its
> > friends always return true if debug_locks == 0. However, one could argue
> 
> ...the silent trylock conversion with no checking of the return value is
> completely broken.  I already sent a patch to tear all this out:
> 
> https://lore.kernel.org/linux-fsdevel/160494580419.772573.9286165021627298770.stgit@magnolia/T/#t
> 

Thanks! That looks good to me. I'm all for removing that piece of code.

While we are at it, I have to ask, when you hit the original problem
(warning after trylock in __start_sb_write()), did you see any lockdep
splat happened previously? Or just like Filipe, you hit that without
seeing any lockdep splat happened before? Thanks! I'm trying to track
down the silent lockdep turn-off.

Regards,
Boqun

> --D
> 
> > that querying the lock holding information regardless if the lockdep
> > turn-off status is inappropriate in the first place. Therefore instead
> > of reverting lock_is_held() and its friends to the previous semantics,
> > add the explicit checking in fs code to avoid use the lock holding
> > information if lockdpe is turned off. And since the original problem
> > also happened with a silent lockdep turn-off, put a warning if
> > debug_locks is 0, which will help us spot the silent lockdep turn-offs.
> > 
> > [1]: https://lore.kernel.org/lkml/a5cf643b-842f-7a60-73c7-85d738a9276f@xxxxxxxx/
> > 
> > Reported-by: Filipe Manana <fdmanana@xxxxxxxxx>
> > Fixes: 4d004099a668 ("lockdep: Fix lockdep recursion")
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: David Sterba <dsterba@xxxxxxxx>
> > Cc: Nikolay Borisov <nborisov@xxxxxxxx>
> > Cc: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
> > ---
> > Hi Filipe,
> > 
> > I use the slightly different approach to fix this problem, and I think
> > it should have the similar effect with my previous fix[2], except that
> > you will hit a warning if the problem happens now. The warning is added
> > on purpose because I don't want to miss a silent lockdep turn-off.
> > 
> > Could you and other fs folks give this a try?
> > 
> > Regards,
> > Boqun
> > 
> > [2]: https://lore.kernel.org/lkml/20201103140828.GA2713762@boqun-archlinux/
> > 
> >  fs/super.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index a51c2083cd6b..1803c8d999e9 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1659,12 +1659,23 @@ int __sb_start_write(struct super_block *sb, int level, bool wait)
> >  	 * twice in some cases, which is OK only because we already hold a
> >  	 * freeze protection also on higher level. Due to these cases we have
> >  	 * to use wait == F (trylock mode) which must not fail.
> > +	 *
> > +	 * Note: lockdep can only prove correct information if debug_locks != 0
> >  	 */
> >  	if (wait) {
> >  		int i;
> >  
> >  		for (i = 0; i < level - 1; i++)
> >  			if (percpu_rwsem_is_held(sb->s_writers.rw_sem + i)) {
> > +				/*
> > +				 * XXX: the WARN_ON_ONCE() here is to help
> > +				 * track down silent lockdep turn-off, i.e.
> > +				 * this warning is triggered, but no lockdep
> > +				 * splat is reported.
> > +				 */
> > +				if (WARN_ON_ONCE(!debug_locks))
> > +					break;
> > +
> >  				force_trylock = true;
> >  				break;
> >  			}
> > -- 
> > 2.29.2
> >