All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Jeffery <djeffery@redhat.com>
To: linux-rdma@vger.kernel.org
Cc: target-devel@vger.kernel.org, Sagi Grimberg <sagi@grimberg.me>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Laurence Oberman <loberman@redhat.com>,
	David Jeffery <djeffery@redhat.com>
Subject: [PATCH 2/2] iscsit: increment max_cmd_sn for isert on command release
Date: Fri, 11 Mar 2022 12:57:13 -0500	[thread overview]
Message-ID: <20220311175713.2344960-3-djeffery@redhat.com> (raw)
In-Reply-To: <20220311175713.2344960-1-djeffery@redhat.com>

iscsit with isert currently can suffer a rare deadlock due to how rdma
delays the release of an iscsi_cmd. Because max_cmd_sn is increased and
sent to the initiator before the last rdma completes, iscsit can end up in
a state where all iscsi_cmd structs are active even though the number is
more than double the iscsi window.

Once out of iscsi_cmd structs, isert then deadlocks trying to receive new
commands. It waits for an iscsi_cmd to become available, but the wait also
blocks processing for receiving completion events which would release an
iscsi_cmd waiting on rdma to finish. So neither can advance.

This patch avoids the deadlock state by delaying the increase to max_cmd_sn
until an iscsi_cmd is released. In this way, the number of iscsi_cmd
structs in use for SCSI commands will be limited to the iscsi window size.

An unsolicited NOPIN is then used to inform the initiator of changes to
max_cmd_sn should the difference between the internal value and the value
the initiator has been informed of grow too large (currently set to half
the window).

Signed-off-by: David Jeffery <djeffery@redhat.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c        | 18 +++++------
 drivers/target/iscsi/iscsi_target_device.c | 35 +++++++++++++++++++++-
 drivers/target/iscsi/iscsi_target_login.c  |  1 +
 drivers/target/iscsi/iscsi_target_util.c   |  5 +++-
 drivers/target/iscsi/iscsi_target_util.h   |  1 +
 include/target/iscsi/iscsi_target_core.h   |  8 +++++
 include/target/iscsi/iscsi_transport.h     |  1 +
 7 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..f67e909c5546 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -2757,7 +2757,7 @@ static int iscsit_send_conn_drop_async_message(
 	cmd->stat_sn		= conn->stat_sn++;
 	hdr->statsn		= cpu_to_be32(cmd->stat_sn);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 	hdr->async_event	= ISCSI_ASYNC_MSG_DROPPING_CONNECTION;
 	hdr->param1		= cpu_to_be16(cmd->logout_cid);
 	hdr->param2		= cpu_to_be16(conn->sess->sess_ops->DefaultTime2Wait);
@@ -2815,7 +2815,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 		hdr->statsn		= cpu_to_be32(0xFFFFFFFF);
 
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 	hdr->datasn		= cpu_to_be32(datain->data_sn);
 	hdr->offset		= cpu_to_be32(datain->offset);
 
@@ -2970,7 +2970,7 @@ iscsit_build_logout_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
 	iscsit_increment_maxcmdsn(cmd, conn->sess);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 	pr_debug("Built Logout Response ITT: 0x%08x StatSN:"
 		" 0x%08x Response: 0x%02x CID: %hu on CID: %hu\n",
@@ -3013,7 +3013,7 @@ iscsit_build_nopin_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 		iscsit_increment_maxcmdsn(cmd, conn->sess);
 
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 	pr_debug("Built NOPIN %s Response ITT: 0x%08x, TTT: 0x%08x,"
 		" StatSN: 0x%08x, Length %u\n", (nopout_response) ?
@@ -3094,7 +3094,7 @@ static int iscsit_send_r2t(
 	hdr->ttt		= cpu_to_be32(r2t->targ_xfer_tag);
 	hdr->statsn		= cpu_to_be32(conn->stat_sn);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 	hdr->r2tsn		= cpu_to_be32(r2t->r2t_sn);
 	hdr->data_offset	= cpu_to_be32(r2t->offset);
 	hdr->data_length	= cpu_to_be32(r2t->xfer_len);
@@ -3234,7 +3234,7 @@ void iscsit_build_rsp_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
 	iscsit_increment_maxcmdsn(cmd, conn->sess);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 	pr_debug("Built SCSI Response, ITT: 0x%08x, StatSN: 0x%08x,"
 		" Response: 0x%02x, SAM Status: 0x%02x, CID: %hu\n",
@@ -3314,7 +3314,7 @@ iscsit_build_task_mgt_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
 	iscsit_increment_maxcmdsn(cmd, conn->sess);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 	pr_debug("Built Task Management Response ITT: 0x%08x,"
 		" StatSN: 0x%08x, Response: 0x%02x, CID: %hu\n",
@@ -3522,7 +3522,7 @@ iscsit_build_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 	 */
 	cmd->maxcmdsn_inc = 0;
 	hdr->exp_cmdsn = cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn = cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 	pr_debug("Built Text Response: ITT: 0x%08x, TTT: 0x%08x, StatSN: 0x%08x,"
 		" Length: %u, CID: %hu F: %d C: %d\n", cmd->init_task_tag,
@@ -3563,7 +3563,7 @@ iscsit_build_reject(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 	cmd->stat_sn		= conn->stat_sn++;
 	hdr->statsn		= cpu_to_be32(cmd->stat_sn);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
-	hdr->max_cmdsn		= cpu_to_be32((u32) atomic_read(&conn->sess->max_cmd_sn));
+	iscsit_set_max_cmdsn(hdr, conn);
 
 }
 EXPORT_SYMBOL(iscsit_build_reject);
diff --git a/drivers/target/iscsi/iscsi_target_device.c b/drivers/target/iscsi/iscsi_target_device.c
index 8bf36ec86e3f..09b23c133dca 100644
--- a/drivers/target/iscsi/iscsi_target_device.c
+++ b/drivers/target/iscsi/iscsi_target_device.c
@@ -13,10 +13,14 @@
 #include <target/target_core_fabric.h>
 
 #include <target/iscsi/iscsi_target_core.h>
+#include <target/iscsi/iscsi_transport.h>
 #include "iscsi_target_device.h"
 #include "iscsi_target_tpg.h"
 #include "iscsi_target_util.h"
 
+#define iscsit_needs_delayed_maxcmdsn_increment(conn) \
+	(conn->conn_transport->transport_type == ISCSI_INFINIBAND)
+
 void iscsit_determine_maxcmdsn(struct iscsi_session *sess)
 {
 	struct se_node_acl *se_nacl;
@@ -42,7 +46,7 @@ void iscsit_determine_maxcmdsn(struct iscsi_session *sess)
 	atomic_add(se_nacl->queue_depth - 1, &sess->max_cmd_sn);
 }
 
-void iscsit_increment_maxcmdsn(struct iscsi_cmd *cmd, struct iscsi_session *sess)
+void __iscsit_increment_maxcmdsn(struct iscsi_cmd *cmd, struct iscsi_session *sess)
 {
 	u32 max_cmd_sn;
 
@@ -54,4 +58,33 @@ void iscsit_increment_maxcmdsn(struct iscsi_cmd *cmd, struct iscsi_session *sess
 	max_cmd_sn = atomic_inc_return(&sess->max_cmd_sn);
 	pr_debug("Updated MaxCmdSN to 0x%08x\n", max_cmd_sn);
 }
+
+void iscsit_increment_maxcmdsn(struct iscsi_cmd *cmd, struct iscsi_session *sess)
+{
+	if (!iscsit_needs_delayed_maxcmdsn_increment(cmd->conn))
+		__iscsit_increment_maxcmdsn(cmd, sess);
+}
 EXPORT_SYMBOL(iscsit_increment_maxcmdsn);
+
+
+
+void iscsit_increment_maxcmdsn_on_release(struct iscsi_cmd *cmd, struct iscsi_session *sess)
+{
+	if (iscsit_needs_delayed_maxcmdsn_increment(cmd->conn)) {
+		__iscsit_increment_maxcmdsn(cmd, sess);
+		if ((u32)atomic_read(&sess->max_cmd_sn) -
+		     READ_ONCE(sess->last_max_cmd_sn)
+		     > sess->cmdsn_window / 2) {
+			/*
+			 * Prevent nopin races if one may be needed by using
+			 * a lock and rechecking after grabbing the lock
+			 */
+			spin_lock_bh(&cmd->conn->nopin_timer_lock);
+			if ((u32)atomic_read(&sess->max_cmd_sn) -
+			    READ_ONCE(sess->last_max_cmd_sn)
+			    > sess->cmdsn_window / 2)
+				iscsit_add_nopin(cmd->conn, 0);
+			spin_unlock_bh(&cmd->conn->nopin_timer_lock);
+		}
+	}
+}
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 1a9c50401bdb..d97c3792f140 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -307,6 +307,7 @@ static int iscsi_login_zero_tsih_s1(
 	 * Initiator Node's ACL once the login has been successfully completed.
 	 */
 	atomic_set(&sess->max_cmd_sn, be32_to_cpu(pdu->cmdsn));
+	sess->last_max_cmd_sn = be32_to_cpu(pdu->cmdsn);
 
 	sess->sess_ops = kzalloc(sizeof(struct iscsi_sess_ops), GFP_KERNEL);
 	if (!sess->sess_ops) {
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 6dd5810e2af1..18685b23e1d0 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -708,6 +708,8 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd)
 
 	BUG_ON(!sess || !sess->se_sess);
 
+	iscsit_increment_maxcmdsn_on_release(cmd, sess);
+
 	kfree(cmd->buf_ptr);
 	kfree(cmd->pdu_list);
 	kfree(cmd->seq_list);
@@ -867,7 +869,7 @@ void iscsit_inc_conn_usage_count(struct iscsi_conn *conn)
 	spin_unlock_bh(&conn->conn_usage_lock);
 }
 
-static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response)
+int iscsit_add_nopin(struct iscsi_conn *conn, int want_response)
 {
 	u8 state;
 	struct iscsi_cmd *cmd;
@@ -877,6 +879,7 @@ static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response)
 		return -1;
 
 	cmd->iscsi_opcode = ISCSI_OP_NOOP_IN;
+	cmd->immediate_cmd = 1;
 	state = (want_response) ? ISTATE_SEND_NOPIN_WANT_RESPONSE :
 				ISTATE_SEND_NOPIN_NO_RESPONSE;
 	cmd->init_task_tag = RESERVED_ITT;
diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
index 8ee1c133a9b7..c4474943f310 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -68,5 +68,6 @@ extern int tx_data(struct iscsi_conn *, struct kvec *, int, int);
 extern void iscsit_collect_login_stats(struct iscsi_conn *, u8, u8);
 extern struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct iscsi_conn *);
 extern void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *);
+extern int iscsit_add_nopin(struct iscsi_conn *, int);
 
 #endif /*** ISCSI_TARGET_UTIL_H ***/
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..2983d3798432 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -643,6 +643,7 @@ struct iscsi_session {
 	u32			exp_cmd_sn;
 	/* session wide counter: maximum allowed command sequence number */
 	atomic_t		max_cmd_sn;
+	u32			last_max_cmd_sn;
 	struct list_head	sess_ooo_cmdsn_list;
 
 	/* LIO specific session ID */
@@ -923,4 +924,11 @@ static inline void iscsit_thread_check_cpumask(
 	 */
 	set_cpus_allowed_ptr(p, conn->conn_cpumask);
 }
+
+#define iscsit_set_max_cmdsn(hdr, conn) \
+{ \
+	u32 max_cmdsn = (u32) atomic_read(&conn->sess->max_cmd_sn); \
+	hdr->max_cmdsn = cpu_to_be32(max_cmdsn); \
+	conn->sess->last_max_cmd_sn = max_cmdsn; \
+}
 #endif /* ISCSI_TARGET_CORE_H */
diff --git a/include/target/iscsi/iscsi_transport.h b/include/target/iscsi/iscsi_transport.h
index b8feba7ffebc..878733ca584c 100644
--- a/include/target/iscsi/iscsi_transport.h
+++ b/include/target/iscsi/iscsi_transport.h
@@ -106,6 +106,7 @@ extern int iscsit_response_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
  * From iscsi_target_device.c
  */
 extern void iscsit_increment_maxcmdsn(struct iscsi_cmd *, struct iscsi_session *);
+extern void iscsit_increment_maxcmdsn_on_release(struct iscsi_cmd *, struct iscsi_session *);
 /*
  * From iscsi_target_erl0.c
  */
-- 
2.31.1


  parent reply	other threads:[~2022-03-11 17:57 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-11 17:57 [Patch 0/2] iscsit/isert deadlock prevention under heavy I/O David Jeffery
2022-03-11 17:57 ` [PATCH 1/2] isert: support for unsolicited NOPIN with no response David Jeffery
2022-03-11 17:57 ` David Jeffery [this message]
2022-03-11 19:08 ` [Patch 0/2] iscsit/isert deadlock prevention under heavy I/O Laurence Oberman
2022-03-13  9:59 ` Max Gurtovoy
2022-03-14 13:57   ` David Jeffery
2022-03-14 14:52     ` Max Gurtovoy
2022-03-14 15:55       ` David Jeffery
2022-03-14 17:40         ` Laurence Oberman
2022-03-16 10:38           ` Max Gurtovoy
2022-03-16 13:07             ` Laurence Oberman
2022-03-16 14:39               ` Sagi Grimberg
2022-03-16 15:26                 ` Laurence Oberman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220311175713.2344960-3-djeffery@redhat.com \
    --to=djeffery@redhat.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=loberman@redhat.com \
    --cc=mgurtovoy@nvidia.com \
    --cc=sagi@grimberg.me \
    --cc=target-devel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.