On 2020-02-07 06:00, Dakshaja Uppalapati wrote:
> While testing iser with kernel 5.5, I see attached soft lockups.
> Logged in and ran iozone on mounted disks. While the traffic is running, toggled the initiators interface down to up for 13 secs.
> I see the attached hung task warnings in the target dmesg.
> # iozone -a -I -+d -g 32m
>
> The same test with kernel-5.4 runs fine. I have suspected the following three commits which are all correlated, so removed these commits from 5.5 kernel, rebuilt the kernel and ran the same test then no issue is seen. Iozone is running successfully.
>
> 80647a89eaf3f2: scsi: target: core: Release SPC-2 reservations when closing a session
> e9d3009cb936b: scsi: target: iscsi: Wait for all commands to finish before freeing a session
> 04060db41178c: scsi: RDMA/isert: Fix a recently introduced regression related to logout
>
> I am trying to root cause the issue. Please do suggest me if you want me to check some thing specific.
Does the attached patch help?
Thanks,
Bart.
>From 855af6a1c94f8319867ac944d2e0d97c2c06742c Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@xxxxxxx>
Date: Fri, 7 Feb 2020 19:57:35 -0800
Subject: [PATCH] RDMA/isert: Really fix logout
Commit 04060db41178 moved the isert_put_unsol_pending_cmds() call from
before target_wait_for_sess_cmds() to after that call. However, a comment
above isert_put_unsol_pending_cmds() is as follows: "We might still have
commands that are waiting for unsolicited dataouts messages. We must put
the extra reference on those before blocking on the
target_wait_for_session_cmds()". Make sure that
isert_put_unsol_pending_cmds() is again called before
target_wait_for_sess_cmds().
Cc: Sagi Grimberg <sagi@xxxxxxxxxxx>
Cc: Rahul Kundu <rahul.kundu@xxxxxxxxxxx>
Cc: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
Reported-by: Dakshaja Uppalapati <dakshaja@xxxxxxxxxxx>
Fixes: 04060db41178 ("scsi: RDMA/isert: Fix a recently introduced regression related to logout")
---
drivers/infiniband/ulp/isert/ib_isert.c | 15 ++++++++++++---
drivers/target/iscsi/iscsi_target.c | 3 +++
include/target/iscsi/iscsi_transport.h | 1 +
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index b273e421e910..65111025cd87 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2582,7 +2582,7 @@ isert_wait4logout(struct isert_conn *isert_conn)
*
* We might still have commands that are waiting for unsolicited
* dataouts messages. We must put the extra reference on those
- * before blocking on the target_wait_for_session_cmds
+ * before blocking on the target_wait_for_sess_cmds().
*/
static void
isert_put_unsol_pending_cmds(struct iscsi_conn *conn)
@@ -2610,11 +2610,11 @@ isert_put_unsol_pending_cmds(struct iscsi_conn *conn)
}
}
-static void isert_wait_conn(struct iscsi_conn *conn)
+static void isert_close_conn(struct iscsi_conn *conn)
{
struct isert_conn *isert_conn = conn->context;
- isert_info("Starting conn %p\n", isert_conn);
+ isert_info("Closing conn %p\n", isert_conn);
mutex_lock(&isert_conn->mutex);
isert_conn_terminate(isert_conn);
@@ -2622,6 +2622,14 @@ static void isert_wait_conn(struct iscsi_conn *conn)
ib_drain_qp(isert_conn->qp);
isert_put_unsol_pending_cmds(conn);
+}
+
+static void isert_wait_conn(struct iscsi_conn *conn)
+{
+ struct isert_conn *isert_conn = conn->context;
+
+ isert_info("Waiting for logout on conn %p\n", isert_conn);
+
isert_wait4logout(isert_conn);
queue_work(isert_release_wq, &isert_conn->release_work);
@@ -2653,6 +2661,7 @@ static struct iscsit_transport iser_target_transport = {
.iscsit_setup_np = isert_setup_np,
.iscsit_accept_np = isert_accept_np,
.iscsit_free_np = isert_free_np,
+ .iscsit_close_conn = isert_close_conn,
.iscsit_wait_conn = isert_wait_conn,
.iscsit_free_conn = isert_free_conn,
.iscsit_get_login_rx = isert_get_login_rx,
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index b94ed4e30770..0e6449c87924 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4226,6 +4226,9 @@ int iscsit_close_connection(
atomic_set(&conn->connection_reinstatement, 1);
spin_unlock_bh(&conn->state_lock);
+ if (conn->conn_transport->iscsit_close_conn)
+ conn->conn_transport->iscsit_close_conn(conn);
+
/*
* If any other processes are accessing this connection pointer we
* must wait until they have completed.
diff --git a/include/target/iscsi/iscsi_transport.h b/include/target/iscsi/iscsi_transport.h
index 75bee29fd7dd..f5a370b9e5b3 100644
--- a/include/target/iscsi/iscsi_transport.h
+++ b/include/target/iscsi/iscsi_transport.h
@@ -14,6 +14,7 @@ struct iscsit_transport {
int (*iscsit_setup_np)(struct iscsi_np *, struct sockaddr_storage *);
int (*iscsit_accept_np)(struct iscsi_np *, struct iscsi_conn *);
void (*iscsit_free_np)(struct iscsi_np *);
+ void (*iscsit_close_conn)(struct iscsi_conn *);
void (*iscsit_wait_conn)(struct iscsi_conn *);
void (*iscsit_free_conn)(struct iscsi_conn *);
int (*iscsit_get_login_rx)(struct iscsi_conn *, struct iscsi_login *);
