[PATCH 1/2] Revert "nvme-tcp: lockdep: use dynamic lockdep keys per socket instance"

[Shin'ichiro Kawasaki]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>

This reverts commit 19bdb70c77d3b24239a453291299b64040bdba86.

The commit 19bdb70c77d3 ("nvme-tcp: lockdep: use dynamic lockdep keys
per socket instance") addressed the lockdep WARN caused by the circular
lock dependency among six locks:

  set->srcu -> sk_lock -> cpu_hotplug_lock -> fs_reclaim -> q_usage_counter -> elevator_lock -> set->srcu

As its title says, the commit cut the dependency by introducing the
dynamic lockdep keys per socket instance. However, as described in the
Link tag URL, the commit made a wrong assumption: it assumed that
__fput_sync(queue->sock->file) in nvme_tcp_free_queue() would
synchronously destroy the socket. This is wrong: when in-flight packets
cause delayed free of the socket, the prematurely freed lockdep key is
referred to and causes another WARN. The commit is an imperfect fix.
Hence revert it.

To address the circular dependency among the six locks, another solution
is required. It is provided by the commit 0ba6912f7e97 ("Revert "once:
don't use a work queue to reset sleepable static key""). It cuts the
dependency between sk_lock and cpu_hotplug_lock. This solution is
simpler, and reduces the complexity in nvme-tcp.

Link: https://lore.kernel.org/lkml/CANn89i+wnTLC==UnXCpjsS4YxvEfhe5oK0N7fttbqr1zKyqdug@mail.gmail.com/
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>
---
 drivers/nvme/host/tcp.c | 38 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 921934028e0b..0666bb469c0a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -144,11 +144,6 @@ struct nvme_tcp_queue {
 	void (*state_change)(struct sock *);
 	void (*data_ready)(struct sock *);
 	void (*write_space)(struct sock *);
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lock_class_key nvme_tcp_sk_key;
-	struct lock_class_key nvme_tcp_slock_key;
-#endif
 };
 
 static DEFINE_MUTEX(nvme_tcp_ctrl_mutex);
@@ -185,9 +180,12 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue);
  * a separate class prevents lockdep from conflating nvme-tcp socket use with
  * user-space socket API use.
  */
-static void nvme_tcp_reclassify_socket(struct nvme_tcp_queue *queue)
+static struct lock_class_key nvme_tcp_sk_key[2];
+static struct lock_class_key nvme_tcp_slock_key[2];
+
+static void nvme_tcp_reclassify_socket(struct socket *sock)
 {
-	struct sock *sk = queue->sock->sk;
+	struct sock *sk = sock->sk;
 
 	if (WARN_ON_ONCE(!sock_allow_reclassification(sk)))
 		return;
@@ -195,20 +193,22 @@ static void nvme_tcp_reclassify_socket(struct nvme_tcp_queue *queue)
 	switch (sk->sk_family) {
 	case AF_INET:
 		sock_lock_init_class_and_name(sk, "slock-AF_INET-NVME",
-					      &queue->nvme_tcp_slock_key,
+					      &nvme_tcp_slock_key[0],
 					      "sk_lock-AF_INET-NVME",
-					      &queue->nvme_tcp_sk_key);
+					      &nvme_tcp_sk_key[0]);
 		break;
 	case AF_INET6:
 		sock_lock_init_class_and_name(sk, "slock-AF_INET6-NVME",
-					      &queue->nvme_tcp_slock_key,
+					      &nvme_tcp_slock_key[1],
 					      "sk_lock-AF_INET6-NVME",
-					      &queue->nvme_tcp_sk_key);
+					      &nvme_tcp_sk_key[1]);
 		break;
 	default:
 		WARN_ON_ONCE(1);
 	}
 }
+#else
+static void nvme_tcp_reclassify_socket(struct socket *sock) { }
 #endif
 
 static inline struct nvme_tcp_ctrl *to_tcp_ctrl(struct nvme_ctrl *ctrl)
@@ -1523,11 +1523,6 @@ static void nvme_tcp_free_queue(struct nvme_ctrl *nctrl, int qid)
 	mutex_destroy(&queue->send_mutex);
 	mutex_destroy(&queue->queue_lock);
 	mutex_destroy(&queue->pf_cache_lock);
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	lockdep_unregister_key(&queue->nvme_tcp_sk_key);
-	lockdep_unregister_key(&queue->nvme_tcp_slock_key);
-#endif
 }
 
 static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
@@ -1874,12 +1869,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 	}
 
 	sk_net_refcnt_upgrade(queue->sock->sk);
-
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	lockdep_register_key(&queue->nvme_tcp_sk_key);
-	lockdep_register_key(&queue->nvme_tcp_slock_key);
-	nvme_tcp_reclassify_socket(queue);
-#endif
+	nvme_tcp_reclassify_socket(queue->sock);
 
 	/* Single syn retry */
 	tcp_sock_set_syncnt(queue->sock->sk, 1);
@@ -1984,10 +1974,6 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 	/* Use sync variant - see nvme_tcp_free_queue() for explanation */
 	__fput_sync(queue->sock->file);
 	queue->sock = NULL;
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	lockdep_unregister_key(&queue->nvme_tcp_sk_key);
-	lockdep_unregister_key(&queue->nvme_tcp_slock_key);
-#endif
 err_destroy_mutex:
 	mutex_destroy(&queue->send_mutex);
 	mutex_destroy(&queue->queue_lock);
-- 
2.54.0