Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Boqun Feng <boqun.feng@gmail.com>
To:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Cc:     Boqun Feng <boqun.feng@gmail.com>,
        Filipe Manana <fdmanana@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>, Jan Kara <jack@suse.cz>,
        David Sterba <dsterba@suse.com>,
        Nikolay Borisov <nborisov@suse.com>,
        "Darrick J. Wong" <darrick.wong@oracle.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Ingo Molnar <mingo@kernel.org>
Subject: [RFC] fs: Avoid to use lockdep information if it's turned off
Date:   Tue, 10 Nov 2020 09:37:37 +0800
Message-Id: <20201110013739.686731-1-boqun.feng@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
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@suse.com/

Reported-by: Filipe Manana <fdmanana@gmail.com>
Fixes: 4d004099a668 ("lockdep: Fix lockdep recursion")
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: David Sterba <dsterba@suse.com>
Cc: Nikolay Borisov <nborisov@suse.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
---
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