* [PATCH 01/28] scsi: iscsi: Add task completion helper
2021-05-23 17:57 iscsi error handler fixes Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 02/28] scsi: iscsi: Stop queueing during ep_disconnect Mike Christie
` (26 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
This adds a helper to detect if a cmd has completed but not yet freed.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
include/scsi/libiscsi.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 02f966e9358f..8c6d358a8abc 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -145,6 +145,13 @@ static inline void* iscsi_next_hdr(struct iscsi_task *task)
return (void*)task->hdr + task->hdr_len;
}
+static inline bool iscsi_task_is_completed(struct iscsi_task *task)
+{
+ return task->state == ISCSI_TASK_COMPLETED ||
+ task->state == ISCSI_TASK_ABRT_TMF ||
+ task->state == ISCSI_TASK_ABRT_SESS_RECOV;
+}
+
/* Connection's states */
enum {
ISCSI_CONN_INITIAL_STAGE,
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 02/28] scsi: iscsi: Stop queueing during ep_disconnect
2021-05-23 17:57 iscsi error handler fixes Mike Christie
2021-05-23 17:57 ` [PATCH 01/28] scsi: iscsi: Add task completion helper Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 03/28] scsi: iscsi: Drop suspend calls from ep_disconnect Mike Christie
` (25 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
During ep_disconnect we have been doing iscsi_suspend_tx/queue to block
new IO but every driver except cxgbi and iscsi_tcp can still get IO from
__iscsi_conn_send_pdu if we haven't called iscsi_conn_failure before
ep_disconnect. This could happen if we were terminating the session, and
the logout timed out before it was even sent to libiscsi.
This patch fixes the issue by adding a helper which reverses the bind_conn
call that allows new IO to be queued. Drivers implementing ep_disconnect
can use this to make sure new IO is not queued to them when handling the
disconnect.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
drivers/scsi/be2iscsi/be_main.c | 1 +
drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 +
drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 1 +
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 1 +
drivers/scsi/libiscsi.c | 70 +++++++++++++++++++++---
drivers/scsi/qedi/qedi_iscsi.c | 1 +
drivers/scsi/qla4xxx/ql4_os.c | 1 +
drivers/scsi/scsi_transport_iscsi.c | 10 +++-
include/scsi/libiscsi.h | 1 +
include/scsi/scsi_transport_iscsi.h | 1 +
11 files changed, 78 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 8fcaa1136f2c..6baebcb6d14d 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -1002,6 +1002,7 @@ static struct iscsi_transport iscsi_iser_transport = {
/* connection management */
.create_conn = iscsi_iser_conn_create,
.bind_conn = iscsi_iser_conn_bind,
+ .unbind_conn = iscsi_conn_unbind,
.destroy_conn = iscsi_conn_teardown,
.attr_is_visible = iser_attr_is_visible,
.set_param = iscsi_iser_set_param,
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 22cf7f4b8d8c..27c4f1598f76 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -5809,6 +5809,7 @@ struct iscsi_transport beiscsi_iscsi_transport = {
.destroy_session = beiscsi_session_destroy,
.create_conn = beiscsi_conn_create,
.bind_conn = beiscsi_conn_bind,
+ .unbind_conn = iscsi_conn_unbind,
.destroy_conn = iscsi_conn_teardown,
.attr_is_visible = beiscsi_attr_is_visible,
.set_iface_param = beiscsi_iface_set_param,
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 1e6d8f62ea3c..b6c1da46d582 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -2276,6 +2276,7 @@ struct iscsi_transport bnx2i_iscsi_transport = {
.destroy_session = bnx2i_session_destroy,
.create_conn = bnx2i_conn_create,
.bind_conn = bnx2i_conn_bind,
+ .unbind_conn = iscsi_conn_unbind,
.destroy_conn = bnx2i_conn_destroy,
.attr_is_visible = bnx2i_attr_is_visible,
.set_param = iscsi_set_param,
diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
index 203f938fca7e..f949a4e00783 100644
--- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
+++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
@@ -117,6 +117,7 @@ static struct iscsi_transport cxgb3i_iscsi_transport = {
/* connection management */
.create_conn = cxgbi_create_conn,
.bind_conn = cxgbi_bind_conn,
+ .unbind_conn = iscsi_conn_unbind,
.destroy_conn = iscsi_tcp_conn_teardown,
.start_conn = iscsi_conn_start,
.stop_conn = iscsi_conn_stop,
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 2c3491528d42..efb3e2b3398e 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -134,6 +134,7 @@ static struct iscsi_transport cxgb4i_iscsi_transport = {
/* connection management */
.create_conn = cxgbi_create_conn,
.bind_conn = cxgbi_bind_conn,
+ .unbind_conn = iscsi_conn_unbind,
.destroy_conn = iscsi_tcp_conn_teardown,
.start_conn = iscsi_conn_start,
.stop_conn = iscsi_conn_stop,
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 4834219497ee..2aaf83678654 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1387,23 +1387,32 @@ void iscsi_session_failure(struct iscsi_session *session,
}
EXPORT_SYMBOL_GPL(iscsi_session_failure);
-void iscsi_conn_failure(struct iscsi_conn *conn, enum iscsi_err err)
+static bool iscsi_set_conn_failed(struct iscsi_conn *conn)
{
struct iscsi_session *session = conn->session;
- spin_lock_bh(&session->frwd_lock);
- if (session->state == ISCSI_STATE_FAILED) {
- spin_unlock_bh(&session->frwd_lock);
- return;
- }
+ if (session->state == ISCSI_STATE_FAILED)
+ return false;
if (conn->stop_stage == 0)
session->state = ISCSI_STATE_FAILED;
- spin_unlock_bh(&session->frwd_lock);
set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx);
set_bit(ISCSI_SUSPEND_BIT, &conn->suspend_rx);
- iscsi_conn_error_event(conn->cls_conn, err);
+ return true;
+}
+
+void iscsi_conn_failure(struct iscsi_conn *conn, enum iscsi_err err)
+{
+ struct iscsi_session *session = conn->session;
+ bool needs_evt;
+
+ spin_lock_bh(&session->frwd_lock);
+ needs_evt = iscsi_set_conn_failed(conn);
+ spin_unlock_bh(&session->frwd_lock);
+
+ if (needs_evt)
+ iscsi_conn_error_event(conn->cls_conn, err);
}
EXPORT_SYMBOL_GPL(iscsi_conn_failure);
@@ -2180,6 +2189,51 @@ static void iscsi_check_transport_timeouts(struct timer_list *t)
spin_unlock(&session->frwd_lock);
}
+/**
+ * iscsi_conn_unbind - prevent queueing to conn.
+ * @cls_conn: iscsi conn ep is bound to.
+ * @is_active: is the conn in use for boot or is this for EH/termination
+ *
+ * This must be called by drivers implementing the ep_disconnect callout.
+ * It disables queueing to the connection from libiscsi in preparation for
+ * an ep_disconnect call.
+ */
+void iscsi_conn_unbind(struct iscsi_cls_conn *cls_conn, bool is_active)
+{
+ struct iscsi_session *session;
+ struct iscsi_conn *conn;
+
+ if (!cls_conn)
+ return;
+
+ conn = cls_conn->dd_data;
+ session = conn->session;
+ /*
+ * Wait for iscsi_eh calls to exit. We don't wait for the tmf to
+ * complete or timeout. The caller just wants to know what's running
+ * is everything that needs to be cleaned up, and no cmds will be
+ * queued.
+ */
+ mutex_lock(&session->eh_mutex);
+
+ iscsi_suspend_queue(conn);
+ iscsi_suspend_tx(conn);
+
+ spin_lock_bh(&session->frwd_lock);
+ if (!is_active) {
+ /*
+ * if logout timed out before userspace could even send a PDU
+ * the state might still be in ISCSI_STATE_LOGGED_IN and
+ * allowing new cmds and TMFs.
+ */
+ if (session->state == ISCSI_STATE_LOGGED_IN)
+ iscsi_set_conn_failed(conn);
+ }
+ spin_unlock_bh(&session->frwd_lock);
+ mutex_unlock(&session->eh_mutex);
+}
+EXPORT_SYMBOL_GPL(iscsi_conn_unbind);
+
static void iscsi_prep_abort_task_pdu(struct iscsi_task *task,
struct iscsi_tm *hdr)
{
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 08c05403cd72..ef16537c523c 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -1401,6 +1401,7 @@ struct iscsi_transport qedi_iscsi_transport = {
.destroy_session = qedi_session_destroy,
.create_conn = qedi_conn_create,
.bind_conn = qedi_conn_bind,
+ .unbind_conn = iscsi_conn_unbind,
.start_conn = qedi_conn_start,
.stop_conn = iscsi_conn_stop,
.destroy_conn = qedi_conn_destroy,
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index ad3afe30f617..74d0d1bc208d 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -259,6 +259,7 @@ static struct iscsi_transport qla4xxx_iscsi_transport = {
.start_conn = qla4xxx_conn_start,
.create_conn = qla4xxx_conn_create,
.bind_conn = qla4xxx_conn_bind,
+ .unbind_conn = iscsi_conn_unbind,
.stop_conn = iscsi_conn_stop,
.destroy_conn = qla4xxx_conn_destroy,
.set_param = iscsi_set_param,
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 441f0152193f..82491343e94a 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2964,7 +2964,7 @@ static int iscsi_if_ep_connect(struct iscsi_transport *transport,
}
static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
- u64 ep_handle)
+ u64 ep_handle, bool is_active)
{
struct iscsi_cls_conn *conn;
struct iscsi_endpoint *ep;
@@ -2981,6 +2981,8 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
conn->ep = NULL;
mutex_unlock(&conn->ep_mutex);
conn->state = ISCSI_CONN_FAILED;
+
+ transport->unbind_conn(conn, is_active);
}
transport->ep_disconnect(ep);
@@ -3012,7 +3014,8 @@ iscsi_if_transport_ep(struct iscsi_transport *transport,
break;
case ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT:
rc = iscsi_if_ep_disconnect(transport,
- ev->u.ep_disconnect.ep_handle);
+ ev->u.ep_disconnect.ep_handle,
+ false);
break;
}
return rc;
@@ -3737,7 +3740,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
conn = iscsi_conn_lookup(ev->u.b_conn.sid, ev->u.b_conn.cid);
if (conn && conn->ep)
- iscsi_if_ep_disconnect(transport, conn->ep->id);
+ iscsi_if_ep_disconnect(transport, conn->ep->id, true);
if (!session || !conn) {
err = -EINVAL;
@@ -4656,6 +4659,7 @@ iscsi_register_transport(struct iscsi_transport *tt)
int err;
BUG_ON(!tt);
+ WARN_ON(tt->ep_disconnect && !tt->unbind_conn);
priv = iscsi_if_transport_lookup(tt);
if (priv)
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 8c6d358a8abc..13d413a0b8b6 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -431,6 +431,7 @@ extern int iscsi_conn_start(struct iscsi_cls_conn *);
extern void iscsi_conn_stop(struct iscsi_cls_conn *, int);
extern int iscsi_conn_bind(struct iscsi_cls_session *, struct iscsi_cls_conn *,
int);
+extern void iscsi_conn_unbind(struct iscsi_cls_conn *cls_conn, bool is_active);
extern void iscsi_conn_failure(struct iscsi_conn *conn, enum iscsi_err err);
extern void iscsi_session_failure(struct iscsi_session *session,
enum iscsi_err err);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index fc5a39839b4b..8874016b3c9a 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -82,6 +82,7 @@ struct iscsi_transport {
void (*destroy_session) (struct iscsi_cls_session *session);
struct iscsi_cls_conn *(*create_conn) (struct iscsi_cls_session *sess,
uint32_t cid);
+ void (*unbind_conn) (struct iscsi_cls_conn *conn, bool is_active);
int (*bind_conn) (struct iscsi_cls_session *session,
struct iscsi_cls_conn *cls_conn,
uint64_t transport_eph, int is_leading);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 03/28] scsi: iscsi: Drop suspend calls from ep_disconnect
2021-05-23 17:57 iscsi error handler fixes Mike Christie
2021-05-23 17:57 ` [PATCH 01/28] scsi: iscsi: Add task completion helper Mike Christie
2021-05-23 17:57 ` [PATCH 02/28] scsi: iscsi: Stop queueing during ep_disconnect Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 04/28] scsi: iscsi: Force immediate failure during shutdown Mike Christie
` (24 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
libiscsi will now suspend the send/tx queue for the drivers so we can drop
it from the drivers ep_disconnect.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/be2iscsi/be_iscsi.c | 6 ------
drivers/scsi/bnx2i/bnx2i_iscsi.c | 6 +-----
drivers/scsi/cxgbi/libcxgbi.c | 1 -
drivers/scsi/qedi/qedi_iscsi.c | 8 ++++----
4 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 0e935c49b57b..51a7b19bfffe 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -1293,7 +1293,6 @@ static int beiscsi_conn_close(struct beiscsi_endpoint *beiscsi_ep)
void beiscsi_ep_disconnect(struct iscsi_endpoint *ep)
{
struct beiscsi_endpoint *beiscsi_ep;
- struct beiscsi_conn *beiscsi_conn;
struct beiscsi_hba *phba;
uint16_t cri_index;
@@ -1312,11 +1311,6 @@ void beiscsi_ep_disconnect(struct iscsi_endpoint *ep)
return;
}
- if (beiscsi_ep->conn) {
- beiscsi_conn = beiscsi_ep->conn;
- iscsi_suspend_queue(beiscsi_conn->conn);
- }
-
if (!beiscsi_hba_is_online(phba)) {
beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG,
"BS_%d : HBA in error 0x%lx\n", phba->state);
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index b6c1da46d582..9a4f4776a78a 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -2113,7 +2113,6 @@ static void bnx2i_ep_disconnect(struct iscsi_endpoint *ep)
{
struct bnx2i_endpoint *bnx2i_ep;
struct bnx2i_conn *bnx2i_conn = NULL;
- struct iscsi_conn *conn = NULL;
struct bnx2i_hba *hba;
bnx2i_ep = ep->dd_data;
@@ -2126,11 +2125,8 @@ static void bnx2i_ep_disconnect(struct iscsi_endpoint *ep)
!time_after(jiffies, bnx2i_ep->timestamp + (12 * HZ)))
msleep(250);
- if (bnx2i_ep->conn) {
+ if (bnx2i_ep->conn)
bnx2i_conn = bnx2i_ep->conn;
- conn = bnx2i_conn->cls_conn->dd_data;
- iscsi_suspend_queue(conn);
- }
hba = bnx2i_ep->hba;
mutex_lock(&hba->net_dev_lock);
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index f078b3c4e083..215dd0eb3f48 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -2968,7 +2968,6 @@ void cxgbi_ep_disconnect(struct iscsi_endpoint *ep)
ep, cep, cconn, csk, csk->state, csk->flags);
if (cconn && cconn->iconn) {
- iscsi_suspend_tx(cconn->iconn);
write_lock_bh(&csk->callback_lock);
cep->csk->user_data = NULL;
cconn->cep = NULL;
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index ef16537c523c..30dc345b011c 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -988,7 +988,6 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
{
struct qedi_endpoint *qedi_ep;
struct qedi_conn *qedi_conn = NULL;
- struct iscsi_conn *conn = NULL;
struct qedi_ctx *qedi;
int ret = 0;
int wait_delay;
@@ -1007,8 +1006,6 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
if (qedi_ep->conn) {
qedi_conn = qedi_ep->conn;
- conn = qedi_conn->cls_conn->dd_data;
- iscsi_suspend_queue(conn);
abrt_conn = qedi_conn->abrt_conn;
while (count--) {
@@ -1621,8 +1618,11 @@ void qedi_clear_session_ctx(struct iscsi_cls_session *cls_sess)
struct iscsi_conn *conn = session->leadconn;
struct qedi_conn *qedi_conn = conn->dd_data;
- if (iscsi_is_session_online(cls_sess))
+ if (iscsi_is_session_online(cls_sess)) {
+ if (conn)
+ iscsi_suspend_queue(conn);
qedi_ep_disconnect(qedi_conn->iscsi_ep);
+ }
qedi_conn_destroy(qedi_conn->cls_conn);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 04/28] scsi: iscsi: Force immediate failure during shutdown
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (2 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 03/28] scsi: iscsi: Drop suspend calls from ep_disconnect Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 16:00 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 05/28] scsi: iscsi: Use system_unbound_wq for destroy_work Mike Christie
` (23 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If the system is not up, we can just fail immediately since iscsid is not
going to ever answer our netlink events. We are already setting the
recovery_tmo to 0, but by passing stop_conn STOP_CONN_TERM we never will
block the session and start the recovery timer, because for that flag
userspace will do the unbind and destroy events which would remove the
devices and wake up and kill the eh.
Since the conn is dead and the system is going dowm this just has us use
STOP_CONN_RECOVER with recovery_tmo=0 so we fail immediately. However, if
the user has set the recovery_tmo=-1 we let the system hang like they
requested since they might have used that setting for specific reasons
(one known is reason is for buggy cluster software).
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 82491343e94a..d134156d67f0 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2513,11 +2513,17 @@ static void stop_conn_work_fn(struct work_struct *work)
session = iscsi_session_lookup(sid);
if (session) {
if (system_state != SYSTEM_RUNNING) {
- session->recovery_tmo = 0;
- iscsi_if_stop_conn(conn, STOP_CONN_TERM);
- } else {
- iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
+ /*
+ * If the user has set up for the session to
+ * never timeout then hang like they wanted.
+ * For all other cases fail right away since
+ * userspace is not going to relogin.
+ */
+ if (session->recovery_tmo > 0)
+ session->recovery_tmo = 0;
}
+
+ iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
}
list_del_init(&conn->conn_list_err);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 04/28] scsi: iscsi: Force immediate failure during shutdown
2021-05-23 17:57 ` [PATCH 04/28] scsi: iscsi: Force immediate failure during shutdown Mike Christie
@ 2021-05-24 16:00 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 16:00 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> If the system is not up, we can just fail immediately since iscsid is not
> going to ever answer our netlink events. We are already setting the
> recovery_tmo to 0, but by passing stop_conn STOP_CONN_TERM we never will
> block the session and start the recovery timer, because for that flag
> userspace will do the unbind and destroy events which would remove the
> devices and wake up and kill the eh.
>
> Since the conn is dead and the system is going dowm this just has us use
> STOP_CONN_RECOVER with recovery_tmo=0 so we fail immediately. However, if
> the user has set the recovery_tmo=-1 we let the system hang like they
> requested since they might have used that setting for specific reasons
> (one known is reason is for buggy cluster software).
Typo: extra "is" in the above sentence. Not a big deal. :)
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 82491343e94a..d134156d67f0 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2513,11 +2513,17 @@ static void stop_conn_work_fn(struct work_struct *work)
> session = iscsi_session_lookup(sid);
> if (session) {
> if (system_state != SYSTEM_RUNNING) {
> - session->recovery_tmo = 0;
> - iscsi_if_stop_conn(conn, STOP_CONN_TERM);
> - } else {
> - iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
> + /*
> + * If the user has set up for the session to
> + * never timeout then hang like they wanted.
> + * For all other cases fail right away since
> + * userspace is not going to relogin.
> + */
> + if (session->recovery_tmo > 0)
> + session->recovery_tmo = 0;
> }
> +
> + iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
> }
>
> list_del_init(&conn->conn_list_err);
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 05/28] scsi: iscsi: Use system_unbound_wq for destroy_work
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (3 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 04/28] scsi: iscsi: Force immediate failure during shutdown Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 06/28] scsi: iscsi: Rel ref after iscsi_lookup_endpoint Mike Christie
` (22 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Use the system_unbound_wq for async session destruction. We don't need a
dedicated workqueue for async session destruction because:
1. perf does not seem to be an issue since we only allow 1 active work.
2. it does not have deps with other system works and we can run them
in parallel with each other.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index d134156d67f0..2eb77f69fe0c 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -95,8 +95,6 @@ static DECLARE_WORK(stop_conn_work, stop_conn_work_fn);
static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
static struct workqueue_struct *iscsi_eh_timer_workq;
-static struct workqueue_struct *iscsi_destroy_workq;
-
static DEFINE_IDA(iscsi_sess_ida);
/*
* list of registered transports and lock that must
@@ -3724,7 +3722,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
list_del_init(&session->sess_list);
spin_unlock_irqrestore(&sesslock, flags);
- queue_work(iscsi_destroy_workq, &session->destroy_work);
+ queue_work(system_unbound_wq, &session->destroy_work);
}
break;
case ISCSI_UEVENT_UNBIND_SESSION:
@@ -4820,18 +4818,8 @@ static __init int iscsi_transport_init(void)
goto release_nls;
}
- iscsi_destroy_workq = alloc_workqueue("%s",
- WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_UNBOUND,
- 1, "iscsi_destroy");
- if (!iscsi_destroy_workq) {
- err = -ENOMEM;
- goto destroy_wq;
- }
-
return 0;
-destroy_wq:
- destroy_workqueue(iscsi_eh_timer_workq);
release_nls:
netlink_kernel_release(nls);
unregister_flashnode_bus:
@@ -4853,7 +4841,6 @@ static __init int iscsi_transport_init(void)
static void __exit iscsi_transport_exit(void)
{
- destroy_workqueue(iscsi_destroy_workq);
destroy_workqueue(iscsi_eh_timer_workq);
netlink_kernel_release(nls);
bus_unregister(&iscsi_flashnode_bus);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 06/28] scsi: iscsi: Rel ref after iscsi_lookup_endpoint
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (4 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 05/28] scsi: iscsi: Use system_unbound_wq for destroy_work Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 07/28] scsi: iscsi: Fix in-kernel conn failure handling Mike Christie
` (21 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
The next patches allow the kernel to do ep_disconnect. In that case we
will have to get a proper refcount on the ep so one thread does not
delete it from under another.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
drivers/scsi/be2iscsi/be_iscsi.c | 19 ++++++++++++------
drivers/scsi/bnx2i/bnx2i_iscsi.c | 23 +++++++++++++++-------
drivers/scsi/cxgbi/libcxgbi.c | 12 ++++++++----
drivers/scsi/qedi/qedi_iscsi.c | 25 +++++++++++++++++-------
drivers/scsi/qla4xxx/ql4_os.c | 1 +
drivers/scsi/scsi_transport_iscsi.c | 25 ++++++++++++++++--------
include/scsi/scsi_transport_iscsi.h | 1 +
8 files changed, 75 insertions(+), 32 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 6baebcb6d14d..776e46ee95da 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -506,6 +506,7 @@ iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
iser_conn->iscsi_conn = conn;
out:
+ iscsi_put_endpoint(ep);
mutex_unlock(&iser_conn->state_mutex);
return error;
}
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 51a7b19bfffe..8aeaddc93b16 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -182,6 +182,7 @@ int beiscsi_conn_bind(struct iscsi_cls_session *cls_session,
struct beiscsi_endpoint *beiscsi_ep;
struct iscsi_endpoint *ep;
uint16_t cri_index;
+ int rc = 0;
ep = iscsi_lookup_endpoint(transport_fd);
if (!ep)
@@ -189,15 +190,17 @@ int beiscsi_conn_bind(struct iscsi_cls_session *cls_session,
beiscsi_ep = ep->dd_data;
- if (iscsi_conn_bind(cls_session, cls_conn, is_leading))
- return -EINVAL;
+ if (iscsi_conn_bind(cls_session, cls_conn, is_leading)) {
+ rc = -EINVAL;
+ goto put_ep;
+ }
if (beiscsi_ep->phba != phba) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
"BS_%d : beiscsi_ep->hba=%p not equal to phba=%p\n",
beiscsi_ep->phba, phba);
-
- return -EEXIST;
+ rc = -EEXIST;
+ goto put_ep;
}
cri_index = BE_GET_CRI_FROM_CID(beiscsi_ep->ep_cid);
if (phba->conn_table[cri_index]) {
@@ -209,7 +212,8 @@ int beiscsi_conn_bind(struct iscsi_cls_session *cls_session,
beiscsi_ep->ep_cid,
beiscsi_conn,
phba->conn_table[cri_index]);
- return -EINVAL;
+ rc = -EINVAL;
+ goto put_ep;
}
}
@@ -226,7 +230,10 @@ int beiscsi_conn_bind(struct iscsi_cls_session *cls_session,
"BS_%d : cid %d phba->conn_table[%u]=%p\n",
beiscsi_ep->ep_cid, cri_index, beiscsi_conn);
phba->conn_table[cri_index] = beiscsi_conn;
- return 0;
+
+put_ep:
+ iscsi_put_endpoint(ep);
+ return rc;
}
static int beiscsi_iface_create_ipv4(struct beiscsi_hba *phba)
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 9a4f4776a78a..26cb1c6536ce 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1420,17 +1420,23 @@ static int bnx2i_conn_bind(struct iscsi_cls_session *cls_session,
* Forcefully terminate all in progress connection recovery at the
* earliest, either in bind(), send_pdu(LOGIN), or conn_start()
*/
- if (bnx2i_adapter_ready(hba))
- return -EIO;
+ if (bnx2i_adapter_ready(hba)) {
+ ret_code = -EIO;
+ goto put_ep;
+ }
bnx2i_ep = ep->dd_data;
if ((bnx2i_ep->state == EP_STATE_TCP_FIN_RCVD) ||
- (bnx2i_ep->state == EP_STATE_TCP_RST_RCVD))
+ (bnx2i_ep->state == EP_STATE_TCP_RST_RCVD)) {
/* Peer disconnect via' FIN or RST */
- return -EINVAL;
+ ret_code = -EINVAL;
+ goto put_ep;
+ }
- if (iscsi_conn_bind(cls_session, cls_conn, is_leading))
- return -EINVAL;
+ if (iscsi_conn_bind(cls_session, cls_conn, is_leading)) {
+ ret_code = -EINVAL;
+ goto put_ep;
+ }
if (bnx2i_ep->hba != hba) {
/* Error - TCP connection does not belong to this device
@@ -1441,7 +1447,8 @@ static int bnx2i_conn_bind(struct iscsi_cls_session *cls_session,
iscsi_conn_printk(KERN_ALERT, cls_conn->dd_data,
"belong to hba (%s)\n",
hba->netdev->name);
- return -EEXIST;
+ ret_code = -EEXIST;
+ goto put_ep;
}
bnx2i_ep->conn = bnx2i_conn;
bnx2i_conn->ep = bnx2i_ep;
@@ -1458,6 +1465,8 @@ static int bnx2i_conn_bind(struct iscsi_cls_session *cls_session,
bnx2i_put_rq_buf(bnx2i_conn, 0);
bnx2i_arm_cq_event_coalescing(bnx2i_conn->ep, CNIC_ARM_CQE);
+put_ep:
+ iscsi_put_endpoint(ep);
return ret_code;
}
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 215dd0eb3f48..dbe22a7136f3 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -2690,11 +2690,13 @@ int cxgbi_bind_conn(struct iscsi_cls_session *cls_session,
err = csk->cdev->csk_ddp_setup_pgidx(csk, csk->tid,
ppm->tformat.pgsz_idx_dflt);
if (err < 0)
- return err;
+ goto put_ep;
err = iscsi_conn_bind(cls_session, cls_conn, is_leading);
- if (err)
- return -EINVAL;
+ if (err) {
+ err = -EINVAL;
+ goto put_ep;
+ }
/* calculate the tag idx bits needed for this conn based on cmds_max */
cconn->task_idx_bits = (__ilog2_u32(conn->session->cmds_max - 1)) + 1;
@@ -2715,7 +2717,9 @@ int cxgbi_bind_conn(struct iscsi_cls_session *cls_session,
/* init recv engine */
iscsi_tcp_hdr_recv_prep(tcp_conn);
- return 0;
+put_ep:
+ iscsi_put_endpoint(ep);
+ return err;
}
EXPORT_SYMBOL_GPL(cxgbi_bind_conn);
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 30dc345b011c..80f8d35b5900 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -377,6 +377,7 @@ static int qedi_conn_bind(struct iscsi_cls_session *cls_session,
struct qedi_ctx *qedi = iscsi_host_priv(shost);
struct qedi_endpoint *qedi_ep;
struct iscsi_endpoint *ep;
+ int rc = 0;
ep = iscsi_lookup_endpoint(transport_fd);
if (!ep)
@@ -384,11 +385,16 @@ static int qedi_conn_bind(struct iscsi_cls_session *cls_session,
qedi_ep = ep->dd_data;
if ((qedi_ep->state == EP_STATE_TCP_FIN_RCVD) ||
- (qedi_ep->state == EP_STATE_TCP_RST_RCVD))
- return -EINVAL;
+ (qedi_ep->state == EP_STATE_TCP_RST_RCVD)) {
+ rc = -EINVAL;
+ goto put_ep;
+ }
+
+ if (iscsi_conn_bind(cls_session, cls_conn, is_leading)) {
+ rc = -EINVAL;
+ goto put_ep;
+ }
- if (iscsi_conn_bind(cls_session, cls_conn, is_leading))
- return -EINVAL;
qedi_ep->conn = qedi_conn;
qedi_conn->ep = qedi_ep;
@@ -398,13 +404,18 @@ static int qedi_conn_bind(struct iscsi_cls_session *cls_session,
qedi_conn->cmd_cleanup_req = 0;
qedi_conn->cmd_cleanup_cmpl = 0;
- if (qedi_bind_conn_to_iscsi_cid(qedi, qedi_conn))
- return -EINVAL;
+ if (qedi_bind_conn_to_iscsi_cid(qedi, qedi_conn)) {
+ rc = -EINVAL;
+ goto put_ep;
+ }
+
spin_lock_init(&qedi_conn->tmf_work_lock);
INIT_LIST_HEAD(&qedi_conn->tmf_work_list);
init_waitqueue_head(&qedi_conn->wait_queue);
- return 0;
+put_ep:
+ iscsi_put_endpoint(ep);
+ return rc;
}
static int qedi_iscsi_update_conn(struct qedi_ctx *qedi,
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 74d0d1bc208d..0e7a7e82e028 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -3235,6 +3235,7 @@ static int qla4xxx_conn_bind(struct iscsi_cls_session *cls_session,
conn = cls_conn->dd_data;
qla_conn = conn->dd_data;
qla_conn->qla_ep = ep->dd_data;
+ iscsi_put_endpoint(ep);
return 0;
}
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 2eb77f69fe0c..bab6654d8ee9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -266,9 +266,20 @@ void iscsi_destroy_endpoint(struct iscsi_endpoint *ep)
}
EXPORT_SYMBOL_GPL(iscsi_destroy_endpoint);
+void iscsi_put_endpoint(struct iscsi_endpoint *ep)
+{
+ put_device(&ep->dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_put_endpoint);
+
+/**
+ * iscsi_lookup_endpoint - get ep from handle
+ * @handle: endpoint handle
+ *
+ * Caller must do a iscsi_put_endpoint.
+ */
struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)
{
- struct iscsi_endpoint *ep;
struct device *dev;
dev = class_find_device(&iscsi_endpoint_class, NULL, &handle,
@@ -276,13 +287,7 @@ struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle)
if (!dev)
return NULL;
- ep = iscsi_dev_to_endpoint(dev);
- /*
- * we can drop this now because the interface will prevent
- * removals and lookups from racing.
- */
- put_device(dev);
- return ep;
+ return iscsi_dev_to_endpoint(dev);
}
EXPORT_SYMBOL_GPL(iscsi_lookup_endpoint);
@@ -2990,6 +2995,7 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
}
transport->ep_disconnect(ep);
+ iscsi_put_endpoint(ep);
return 0;
}
@@ -3015,6 +3021,7 @@ iscsi_if_transport_ep(struct iscsi_transport *transport,
ev->r.retcode = transport->ep_poll(ep,
ev->u.ep_poll.timeout_ms);
+ iscsi_put_endpoint(ep);
break;
case ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT:
rc = iscsi_if_ep_disconnect(transport,
@@ -3698,6 +3705,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
ev->u.c_bound_session.initial_cmdsn,
ev->u.c_bound_session.cmds_max,
ev->u.c_bound_session.queue_depth);
+ iscsi_put_endpoint(ep);
break;
case ISCSI_UEVENT_DESTROY_SESSION:
session = iscsi_session_lookup(ev->u.d_session.sid);
@@ -3769,6 +3777,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
mutex_lock(&conn->ep_mutex);
conn->ep = ep;
mutex_unlock(&conn->ep_mutex);
+ iscsi_put_endpoint(ep);
} else
iscsi_cls_conn_printk(KERN_ERR, conn,
"Could not set ep conn "
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 8874016b3c9a..d36a72cf049f 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -442,6 +442,7 @@ extern int iscsi_scan_finished(struct Scsi_Host *shost, unsigned long time);
extern struct iscsi_endpoint *iscsi_create_endpoint(int dd_size);
extern void iscsi_destroy_endpoint(struct iscsi_endpoint *ep);
extern struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle);
+extern void iscsi_put_endpoint(struct iscsi_endpoint *ep);
extern int iscsi_block_scsi_eh(struct scsi_cmnd *cmd);
extern struct iscsi_iface *iscsi_create_iface(struct Scsi_Host *shost,
struct iscsi_transport *t,
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 07/28] scsi: iscsi: Fix in-kernel conn failure handling
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (5 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 06/28] scsi: iscsi: Rel ref after iscsi_lookup_endpoint Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:22 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 08/28] scsi: iscsi_tcp: Set no linger Mike Christie
` (20 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
The commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely
in kernel space") has the following regressions/bugs that this patch fixes:
1. It can return cmds to upper layers like dm-multipath where that can
retry them. After they are successful the fs/app can send new IO to the
same sectors, but we've left the cmds running in FW or in the net layer.
We need to be calling ep_disconnect if userspace is not up.
This patch only fixes the issue for offload drivers. iscsi_tcp will be
fixed in separate patch because it doesn't have a ep_disconnect call.
2. The drivers that implement ep_disconnect expect that it's called before
conn_stop. Besides crashes, if the cleanup_task callout is called before
ep_disconnect it might free up driver/card resources for session1 then
they could be allocated for session2. But because the driver's
ep_disconnect is not called it has not cleaned up the firmware so the card
is still using the resources for the original cmd.
3. The stop_conn_work_fn can run after userspace has done it's recovery
and we are happily using the session. We will then end up with various
bugs depending on what is going on at the time.
We may also run stop_conn_work_fn late after userspace has called
stop_conn and ep_disconnect and is now going to call start/bind conn. If
stop_conn_work_fn runs after bind but before start, we would leave the
conn in a unbound but sort of started state where IO might be allowed
even though the drivers have been set in a state where they no longer
expect IO.
4. returning -EAGAIN in iscsi_if_destroy_conn if we haven't yet run the in
kernel stop_conn function is breaking userspace. We should have been doing
this for the caller.
Fixes: 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
kernel space")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 471 ++++++++++++++++------------
include/scsi/scsi_transport_iscsi.h | 10 +-
2 files changed, 283 insertions(+), 198 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index bab6654d8ee9..b8a93e607891 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -86,15 +86,11 @@ struct iscsi_internal {
struct transport_container session_cont;
};
-/* Worker to perform connection failure on unresponsive connections
- * completely in kernel space.
- */
-static void stop_conn_work_fn(struct work_struct *work);
-static DECLARE_WORK(stop_conn_work, stop_conn_work_fn);
-
static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
static struct workqueue_struct *iscsi_eh_timer_workq;
+static struct workqueue_struct *iscsi_conn_cleanup_workq;
+
static DEFINE_IDA(iscsi_sess_ida);
/*
* list of registered transports and lock that must
@@ -1623,12 +1619,6 @@ static DECLARE_TRANSPORT_CLASS(iscsi_connection_class,
static struct sock *nls;
static DEFINE_MUTEX(rx_queue_mutex);
-/*
- * conn_mutex protects the {start,bind,stop,destroy}_conn from racing
- * against the kernel stop_connection recovery mechanism
- */
-static DEFINE_MUTEX(conn_mutex);
-
static LIST_HEAD(sesslist);
static DEFINE_SPINLOCK(sesslock);
static LIST_HEAD(connlist);
@@ -2245,6 +2235,123 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
}
EXPORT_SYMBOL_GPL(iscsi_remove_session);
+static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
+{
+ ISCSI_DBG_TRANS_CONN(conn, "Stopping conn.\n");
+
+ switch (flag) {
+ case STOP_CONN_RECOVER:
+ conn->state = ISCSI_CONN_FAILED;
+ break;
+ case STOP_CONN_TERM:
+ conn->state = ISCSI_CONN_DOWN;
+ break;
+ default:
+ iscsi_cls_conn_printk(KERN_ERR, conn, "invalid stop flag %d\n",
+ flag);
+ return;
+ }
+
+ conn->transport->stop_conn(conn, flag);
+ ISCSI_DBG_TRANS_CONN(conn, "Stopping conn done.\n");
+}
+
+static int iscsi_if_stop_conn(struct iscsi_transport *transport,
+ struct iscsi_uevent *ev)
+{
+ int flag = ev->u.stop_conn.flag;
+ struct iscsi_cls_conn *conn;
+
+ conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
+ if (!conn)
+ return -EINVAL;
+
+ ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
+ /*
+ * If this is a termination we have to call stop_conn with that flag
+ * so the correct states get set. If we haven't run the work yet try to
+ * avoid the extra run.
+ */
+ if (flag == STOP_CONN_TERM) {
+ cancel_work_sync(&conn->cleanup_work);
+ iscsi_stop_conn(conn, flag);
+ } else {
+ /*
+ * Figure out if it was the kernel or userspace initiating this.
+ */
+ if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
+ iscsi_stop_conn(conn, flag);
+ } else {
+ ISCSI_DBG_TRANS_CONN(conn,
+ "flush kernel conn cleanup.\n");
+ flush_work(&conn->cleanup_work);
+ }
+ /*
+ * Only clear for recovery to avoid extra cleanup runs during
+ * termination.
+ */
+ clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
+ }
+ ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
+ return 0;
+}
+
+static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
+{
+ struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
+ struct iscsi_endpoint *ep;
+
+ ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
+ conn->state = ISCSI_CONN_FAILED;
+
+ if (!conn->ep || !session->transport->ep_disconnect)
+ return;
+
+ ep = conn->ep;
+ conn->ep = NULL;
+
+ session->transport->unbind_conn(conn, is_active);
+ session->transport->ep_disconnect(ep);
+ ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
+}
+
+static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
+{
+ struct iscsi_cls_conn *conn = container_of(work, struct iscsi_cls_conn,
+ cleanup_work);
+ struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
+
+ mutex_lock(&conn->ep_mutex);
+ /*
+ * If we are not at least bound there is nothing for us to do. Userspace
+ * will do a ep_disconnect call if offload is used, but will not be
+ * doing a stop since there is nothing to clean up, so we have to clear
+ * the cleanup bit here.
+ */
+ if (conn->state != ISCSI_CONN_BOUND && conn->state != ISCSI_CONN_UP) {
+ ISCSI_DBG_TRANS_CONN(conn, "Got error while conn is already failed. Ignoring.\n");
+ clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
+ mutex_unlock(&conn->ep_mutex);
+ return;
+ }
+
+ iscsi_ep_disconnect(conn, false);
+
+ if (system_state != SYSTEM_RUNNING) {
+ /*
+ * If the user has set up for the session to never timeout
+ * then hang like they wanted. For all other cases fail right
+ * away since userspace is not going to relogin.
+ */
+ if (session->recovery_tmo > 0)
+ session->recovery_tmo = 0;
+ }
+
+ iscsi_stop_conn(conn, STOP_CONN_RECOVER);
+ mutex_unlock(&conn->ep_mutex);
+ ISCSI_DBG_TRANS_CONN(conn, "cleanup done.\n");
+}
+
void iscsi_free_session(struct iscsi_cls_session *session)
{
ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
@@ -2284,7 +2391,7 @@ iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
mutex_init(&conn->ep_mutex);
INIT_LIST_HEAD(&conn->conn_list);
- INIT_LIST_HEAD(&conn->conn_list_err);
+ INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
conn->transport = transport;
conn->cid = cid;
conn->state = ISCSI_CONN_DOWN;
@@ -2341,7 +2448,6 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
spin_lock_irqsave(&connlock, flags);
list_del(&conn->conn_list);
- list_del(&conn->conn_list_err);
spin_unlock_irqrestore(&connlock, flags);
transport_unregister_device(&conn->dev);
@@ -2456,83 +2562,6 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
}
EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
-/*
- * This can be called without the rx_queue_mutex, if invoked by the kernel
- * stop work. But, in that case, it is guaranteed not to race with
- * iscsi_destroy by conn_mutex.
- */
-static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
-{
- /*
- * It is important that this path doesn't rely on
- * rx_queue_mutex, otherwise, a thread doing allocation on a
- * start_session/start_connection could sleep waiting on a
- * writeback to a failed iscsi device, that cannot be recovered
- * because the lock is held. If we don't hold it here, the
- * kernel stop_conn_work_fn has a chance to stop the broken
- * session and resolve the allocation.
- *
- * Still, the user invoked .stop_conn() needs to be serialized
- * with stop_conn_work_fn by a private mutex. Not pretty, but
- * it works.
- */
- mutex_lock(&conn_mutex);
- switch (flag) {
- case STOP_CONN_RECOVER:
- conn->state = ISCSI_CONN_FAILED;
- break;
- case STOP_CONN_TERM:
- conn->state = ISCSI_CONN_DOWN;
- break;
- default:
- iscsi_cls_conn_printk(KERN_ERR, conn,
- "invalid stop flag %d\n", flag);
- goto unlock;
- }
-
- conn->transport->stop_conn(conn, flag);
-unlock:
- mutex_unlock(&conn_mutex);
-}
-
-static void stop_conn_work_fn(struct work_struct *work)
-{
- struct iscsi_cls_conn *conn, *tmp;
- unsigned long flags;
- LIST_HEAD(recovery_list);
-
- spin_lock_irqsave(&connlock, flags);
- if (list_empty(&connlist_err)) {
- spin_unlock_irqrestore(&connlock, flags);
- return;
- }
- list_splice_init(&connlist_err, &recovery_list);
- spin_unlock_irqrestore(&connlock, flags);
-
- list_for_each_entry_safe(conn, tmp, &recovery_list, conn_list_err) {
- uint32_t sid = iscsi_conn_get_sid(conn);
- struct iscsi_cls_session *session;
-
- session = iscsi_session_lookup(sid);
- if (session) {
- if (system_state != SYSTEM_RUNNING) {
- /*
- * If the user has set up for the session to
- * never timeout then hang like they wanted.
- * For all other cases fail right away since
- * userspace is not going to relogin.
- */
- if (session->recovery_tmo > 0)
- session->recovery_tmo = 0;
- }
-
- iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
- }
-
- list_del_init(&conn->conn_list_err);
- }
-}
-
void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
{
struct nlmsghdr *nlh;
@@ -2540,12 +2569,9 @@ void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
struct iscsi_uevent *ev;
struct iscsi_internal *priv;
int len = nlmsg_total_size(sizeof(*ev));
- unsigned long flags;
- spin_lock_irqsave(&connlock, flags);
- list_add(&conn->conn_list_err, &connlist_err);
- spin_unlock_irqrestore(&connlock, flags);
- queue_work(system_unbound_wq, &stop_conn_work);
+ if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags))
+ queue_work(iscsi_conn_cleanup_workq, &conn->cleanup_work);
priv = iscsi_if_transport_lookup(conn->transport);
if (!priv)
@@ -2875,26 +2901,17 @@ static int
iscsi_if_destroy_conn(struct iscsi_transport *transport, struct iscsi_uevent *ev)
{
struct iscsi_cls_conn *conn;
- unsigned long flags;
conn = iscsi_conn_lookup(ev->u.d_conn.sid, ev->u.d_conn.cid);
if (!conn)
return -EINVAL;
- spin_lock_irqsave(&connlock, flags);
- if (!list_empty(&conn->conn_list_err)) {
- spin_unlock_irqrestore(&connlock, flags);
- return -EAGAIN;
- }
- spin_unlock_irqrestore(&connlock, flags);
-
+ ISCSI_DBG_TRANS_CONN(conn, "Flushing cleanup during destruction\n");
+ flush_work(&conn->cleanup_work);
ISCSI_DBG_TRANS_CONN(conn, "Destroying transport conn\n");
- mutex_lock(&conn_mutex);
if (transport->destroy_conn)
transport->destroy_conn(conn);
- mutex_unlock(&conn_mutex);
-
return 0;
}
@@ -2973,7 +2990,7 @@ static int iscsi_if_ep_connect(struct iscsi_transport *transport,
}
static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
- u64 ep_handle, bool is_active)
+ u64 ep_handle)
{
struct iscsi_cls_conn *conn;
struct iscsi_endpoint *ep;
@@ -2984,17 +3001,30 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
ep = iscsi_lookup_endpoint(ep_handle);
if (!ep)
return -EINVAL;
+
conn = ep->conn;
- if (conn) {
- mutex_lock(&conn->ep_mutex);
- conn->ep = NULL;
+ if (!conn) {
+ /*
+ * conn was not even bound yet, so we can't get iscsi conn
+ * failures yet.
+ */
+ transport->ep_disconnect(ep);
+ goto put_ep;
+ }
+
+ mutex_lock(&conn->ep_mutex);
+ /* Check if this was a conn error and the kernel took ownership */
+ if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
+ ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
mutex_unlock(&conn->ep_mutex);
- conn->state = ISCSI_CONN_FAILED;
- transport->unbind_conn(conn, is_active);
+ flush_work(&conn->cleanup_work);
+ goto put_ep;
}
- transport->ep_disconnect(ep);
+ iscsi_ep_disconnect(conn, false);
+ mutex_unlock(&conn->ep_mutex);
+put_ep:
iscsi_put_endpoint(ep);
return 0;
}
@@ -3025,8 +3055,7 @@ iscsi_if_transport_ep(struct iscsi_transport *transport,
break;
case ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT:
rc = iscsi_if_ep_disconnect(transport,
- ev->u.ep_disconnect.ep_handle,
- false);
+ ev->u.ep_disconnect.ep_handle);
break;
}
return rc;
@@ -3653,18 +3682,129 @@ iscsi_get_host_stats(struct iscsi_transport *transport, struct nlmsghdr *nlh)
return err;
}
+static int iscsi_if_transport_conn(struct iscsi_transport *transport,
+ struct nlmsghdr *nlh)
+{
+ struct iscsi_uevent *ev = nlmsg_data(nlh);
+ struct iscsi_cls_session *session;
+ struct iscsi_cls_conn *conn = NULL;
+ struct iscsi_endpoint *ep;
+ uint32_t pdu_len;
+ int err = 0;
+
+ switch (nlh->nlmsg_type) {
+ case ISCSI_UEVENT_CREATE_CONN:
+ return iscsi_if_create_conn(transport, ev);
+ case ISCSI_UEVENT_DESTROY_CONN:
+ return iscsi_if_destroy_conn(transport, ev);
+ case ISCSI_UEVENT_STOP_CONN:
+ return iscsi_if_stop_conn(transport, ev);
+ }
+
+ /*
+ * The following cmds need to be run under the ep_mutex so in kernel
+ * conn cleanup (ep_disconnect + unbind and conn) is not done while
+ * these are running. They also must not run if we have just run a conn
+ * cleanup because they would set the state in a way that might allow
+ * IO or send IO themselves.
+ */
+ switch (nlh->nlmsg_type) {
+ case ISCSI_UEVENT_START_CONN:
+ conn = iscsi_conn_lookup(ev->u.start_conn.sid,
+ ev->u.start_conn.cid);
+ break;
+ case ISCSI_UEVENT_BIND_CONN:
+ conn = iscsi_conn_lookup(ev->u.b_conn.sid, ev->u.b_conn.cid);
+ break;
+ case ISCSI_UEVENT_SEND_PDU:
+ conn = iscsi_conn_lookup(ev->u.send_pdu.sid, ev->u.send_pdu.cid);
+ break;
+ }
+
+ if (!conn)
+ return -EINVAL;
+
+ mutex_lock(&conn->ep_mutex);
+ if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
+ mutex_unlock(&conn->ep_mutex);
+ ev->r.retcode = -ENOTCONN;
+ return 0;
+ }
+
+ switch (nlh->nlmsg_type) {
+ case ISCSI_UEVENT_BIND_CONN:
+ if (conn->ep) {
+ /*
+ * For offload boot support where iscsid is restarted
+ * during the pivot root stage, the ep will be intact
+ * here when the new iscsid instance starts up and
+ * reconnects.
+ */
+ iscsi_ep_disconnect(conn, true);
+ }
+
+ session = iscsi_session_lookup(ev->u.b_conn.sid);
+ if (!session) {
+ err = -EINVAL;
+ break;
+ }
+
+ ev->r.retcode = transport->bind_conn(session, conn,
+ ev->u.b_conn.transport_eph,
+ ev->u.b_conn.is_leading);
+ if (!ev->r.retcode)
+ conn->state = ISCSI_CONN_BOUND;
+
+ if (ev->r.retcode || !transport->ep_connect)
+ break;
+
+ ep = iscsi_lookup_endpoint(ev->u.b_conn.transport_eph);
+ if (ep) {
+ ep->conn = conn;
+ conn->ep = ep;
+ iscsi_put_endpoint(ep);
+ } else {
+ err = -ENOTCONN;
+ iscsi_cls_conn_printk(KERN_ERR, conn,
+ "Could not set ep conn binding\n");
+ }
+ break;
+ case ISCSI_UEVENT_START_CONN:
+ ev->r.retcode = transport->start_conn(conn);
+ if (!ev->r.retcode)
+ conn->state = ISCSI_CONN_UP;
+ break;
+ case ISCSI_UEVENT_SEND_PDU:
+ pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
+
+ if ((ev->u.send_pdu.hdr_size > pdu_len) ||
+ (ev->u.send_pdu.data_size > (pdu_len - ev->u.send_pdu.hdr_size))) {
+ err = -EINVAL;
+ break;
+ }
+
+ ev->r.retcode = transport->send_pdu(conn,
+ (struct iscsi_hdr *)((char *)ev + sizeof(*ev)),
+ (char *)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
+ ev->u.send_pdu.data_size);
+ break;
+ default:
+ err = -ENOSYS;
+ }
+
+ mutex_unlock(&conn->ep_mutex);
+ return err;
+}
static int
iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
{
int err = 0;
u32 portid;
- u32 pdu_len;
struct iscsi_uevent *ev = nlmsg_data(nlh);
struct iscsi_transport *transport = NULL;
struct iscsi_internal *priv;
struct iscsi_cls_session *session;
- struct iscsi_cls_conn *conn;
struct iscsi_endpoint *ep = NULL;
if (!netlink_capable(skb, CAP_SYS_ADMIN))
@@ -3741,90 +3881,16 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
else
err = -EINVAL;
break;
- case ISCSI_UEVENT_CREATE_CONN:
- err = iscsi_if_create_conn(transport, ev);
- break;
- case ISCSI_UEVENT_DESTROY_CONN:
- err = iscsi_if_destroy_conn(transport, ev);
- break;
- case ISCSI_UEVENT_BIND_CONN:
- session = iscsi_session_lookup(ev->u.b_conn.sid);
- conn = iscsi_conn_lookup(ev->u.b_conn.sid, ev->u.b_conn.cid);
-
- if (conn && conn->ep)
- iscsi_if_ep_disconnect(transport, conn->ep->id, true);
-
- if (!session || !conn) {
- err = -EINVAL;
- break;
- }
-
- mutex_lock(&conn_mutex);
- ev->r.retcode = transport->bind_conn(session, conn,
- ev->u.b_conn.transport_eph,
- ev->u.b_conn.is_leading);
- if (!ev->r.retcode)
- conn->state = ISCSI_CONN_BOUND;
- mutex_unlock(&conn_mutex);
-
- if (ev->r.retcode || !transport->ep_connect)
- break;
-
- ep = iscsi_lookup_endpoint(ev->u.b_conn.transport_eph);
- if (ep) {
- ep->conn = conn;
-
- mutex_lock(&conn->ep_mutex);
- conn->ep = ep;
- mutex_unlock(&conn->ep_mutex);
- iscsi_put_endpoint(ep);
- } else
- iscsi_cls_conn_printk(KERN_ERR, conn,
- "Could not set ep conn "
- "binding\n");
- break;
case ISCSI_UEVENT_SET_PARAM:
err = iscsi_set_param(transport, ev);
break;
- case ISCSI_UEVENT_START_CONN:
- conn = iscsi_conn_lookup(ev->u.start_conn.sid, ev->u.start_conn.cid);
- if (conn) {
- mutex_lock(&conn_mutex);
- ev->r.retcode = transport->start_conn(conn);
- if (!ev->r.retcode)
- conn->state = ISCSI_CONN_UP;
- mutex_unlock(&conn_mutex);
- }
- else
- err = -EINVAL;
- break;
+ case ISCSI_UEVENT_CREATE_CONN:
+ case ISCSI_UEVENT_DESTROY_CONN:
case ISCSI_UEVENT_STOP_CONN:
- conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
- if (conn)
- iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
- else
- err = -EINVAL;
- break;
+ case ISCSI_UEVENT_START_CONN:
+ case ISCSI_UEVENT_BIND_CONN:
case ISCSI_UEVENT_SEND_PDU:
- pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
-
- if ((ev->u.send_pdu.hdr_size > pdu_len) ||
- (ev->u.send_pdu.data_size > (pdu_len - ev->u.send_pdu.hdr_size))) {
- err = -EINVAL;
- break;
- }
-
- conn = iscsi_conn_lookup(ev->u.send_pdu.sid, ev->u.send_pdu.cid);
- if (conn) {
- mutex_lock(&conn_mutex);
- ev->r.retcode = transport->send_pdu(conn,
- (struct iscsi_hdr*)((char*)ev + sizeof(*ev)),
- (char*)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
- ev->u.send_pdu.data_size);
- mutex_unlock(&conn_mutex);
- }
- else
- err = -EINVAL;
+ err = iscsi_if_transport_conn(transport, nlh);
break;
case ISCSI_UEVENT_GET_STATS:
err = iscsi_if_get_stats(transport, nlh);
@@ -4827,8 +4893,18 @@ static __init int iscsi_transport_init(void)
goto release_nls;
}
+ iscsi_conn_cleanup_workq = alloc_workqueue("%s",
+ WQ_SYSFS | WQ_MEM_RECLAIM | WQ_UNBOUND, 0,
+ "iscsi_conn_cleanup");
+ if (!iscsi_conn_cleanup_workq) {
+ err = -ENOMEM;
+ goto destroy_wq;
+ }
+
return 0;
+destroy_wq:
+ destroy_workqueue(iscsi_eh_timer_workq);
release_nls:
netlink_kernel_release(nls);
unregister_flashnode_bus:
@@ -4850,6 +4926,7 @@ static __init int iscsi_transport_init(void)
static void __exit iscsi_transport_exit(void)
{
+ destroy_workqueue(iscsi_conn_cleanup_workq);
destroy_workqueue(iscsi_eh_timer_workq);
netlink_kernel_release(nls);
bus_unregister(&iscsi_flashnode_bus);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index d36a72cf049f..3974329d4d02 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -197,15 +197,23 @@ enum iscsi_connection_state {
ISCSI_CONN_BOUND,
};
+#define ISCSI_CLS_CONN_BIT_CLEANUP 1
+
struct iscsi_cls_conn {
struct list_head conn_list; /* item in connlist */
- struct list_head conn_list_err; /* item in connlist_err */
void *dd_data; /* LLD private data */
struct iscsi_transport *transport;
uint32_t cid; /* connection id */
+ /*
+ * This protects the conn startup and binding/unbinding of the ep to
+ * the conn. Unbinding includes ep_disconnect and stop_conn.
+ */
struct mutex ep_mutex;
struct iscsi_endpoint *ep;
+ unsigned long flags;
+ struct work_struct cleanup_work;
+
struct device dev; /* sysfs transport/container device */
enum iscsi_connection_state state;
};
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 07/28] scsi: iscsi: Fix in-kernel conn failure handling
2021-05-23 17:57 ` [PATCH 07/28] scsi: iscsi: Fix in-kernel conn failure handling Mike Christie
@ 2021-05-24 18:22 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:22 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> The commit 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely
> in kernel space") has the following regressions/bugs that this patch fixes:
>
> 1. It can return cmds to upper layers like dm-multipath where that can
> retry them. After they are successful the fs/app can send new IO to the
> same sectors, but we've left the cmds running in FW or in the net layer.
> We need to be calling ep_disconnect if userspace is not up.
>
> This patch only fixes the issue for offload drivers. iscsi_tcp will be
> fixed in separate patch because it doesn't have a ep_disconnect call.
>
> 2. The drivers that implement ep_disconnect expect that it's called before
> conn_stop. Besides crashes, if the cleanup_task callout is called before
> ep_disconnect it might free up driver/card resources for session1 then
> they could be allocated for session2. But because the driver's
> ep_disconnect is not called it has not cleaned up the firmware so the card
> is still using the resources for the original cmd.
>
> 3. The stop_conn_work_fn can run after userspace has done it's recovery
> and we are happily using the session. We will then end up with various
> bugs depending on what is going on at the time.
>
> We may also run stop_conn_work_fn late after userspace has called
> stop_conn and ep_disconnect and is now going to call start/bind conn. If
> stop_conn_work_fn runs after bind but before start, we would leave the
> conn in a unbound but sort of started state where IO might be allowed
> even though the drivers have been set in a state where they no longer
> expect IO.
>
> 4. returning -EAGAIN in iscsi_if_destroy_conn if we haven't yet run the in
> kernel stop_conn function is breaking userspace. We should have been doing
> this for the caller.
>
> Fixes: 0ab710458da1 ("scsi: iscsi: Perform connection failure entirely in
> kernel space")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 471 ++++++++++++++++------------
> include/scsi/scsi_transport_iscsi.h | 10 +-
> 2 files changed, 283 insertions(+), 198 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index bab6654d8ee9..b8a93e607891 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -86,15 +86,11 @@ struct iscsi_internal {
> struct transport_container session_cont;
> };
>
> -/* Worker to perform connection failure on unresponsive connections
> - * completely in kernel space.
> - */
> -static void stop_conn_work_fn(struct work_struct *work);
> -static DECLARE_WORK(stop_conn_work, stop_conn_work_fn);
> -
> static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
> static struct workqueue_struct *iscsi_eh_timer_workq;
>
> +static struct workqueue_struct *iscsi_conn_cleanup_workq;
> +
> static DEFINE_IDA(iscsi_sess_ida);
> /*
> * list of registered transports and lock that must
> @@ -1623,12 +1619,6 @@ static DECLARE_TRANSPORT_CLASS(iscsi_connection_class,
> static struct sock *nls;
> static DEFINE_MUTEX(rx_queue_mutex);
>
> -/*
> - * conn_mutex protects the {start,bind,stop,destroy}_conn from racing
> - * against the kernel stop_connection recovery mechanism
> - */
> -static DEFINE_MUTEX(conn_mutex);
> -
> static LIST_HEAD(sesslist);
> static DEFINE_SPINLOCK(sesslock);
> static LIST_HEAD(connlist);
> @@ -2245,6 +2235,123 @@ void iscsi_remove_session(struct iscsi_cls_session *session)
> }
> EXPORT_SYMBOL_GPL(iscsi_remove_session);
>
> +static void iscsi_stop_conn(struct iscsi_cls_conn *conn, int flag)
> +{
> + ISCSI_DBG_TRANS_CONN(conn, "Stopping conn.\n");
> +
> + switch (flag) {
> + case STOP_CONN_RECOVER:
> + conn->state = ISCSI_CONN_FAILED;
> + break;
> + case STOP_CONN_TERM:
> + conn->state = ISCSI_CONN_DOWN;
> + break;
> + default:
> + iscsi_cls_conn_printk(KERN_ERR, conn, "invalid stop flag %d\n",
> + flag);
> + return;
> + }
> +
> + conn->transport->stop_conn(conn, flag);
> + ISCSI_DBG_TRANS_CONN(conn, "Stopping conn done.\n");
> +}
> +
> +static int iscsi_if_stop_conn(struct iscsi_transport *transport,
> + struct iscsi_uevent *ev)
> +{
> + int flag = ev->u.stop_conn.flag;
> + struct iscsi_cls_conn *conn;
> +
> + conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
> + if (!conn)
> + return -EINVAL;
> +
> + ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop.\n");
> + /*
> + * If this is a termination we have to call stop_conn with that flag
> + * so the correct states get set. If we haven't run the work yet try to
> + * avoid the extra run.
> + */
> + if (flag == STOP_CONN_TERM) {
> + cancel_work_sync(&conn->cleanup_work);
> + iscsi_stop_conn(conn, flag);
> + } else {
> + /*
> + * Figure out if it was the kernel or userspace initiating this.
> + */
> + if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> + iscsi_stop_conn(conn, flag);
> + } else {
> + ISCSI_DBG_TRANS_CONN(conn,
> + "flush kernel conn cleanup.\n");
> + flush_work(&conn->cleanup_work);
> + }
> + /*
> + * Only clear for recovery to avoid extra cleanup runs during
> + * termination.
> + */
> + clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> + }
> + ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
> + return 0;
> +}
> +
> +static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
> +{
> + struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
> + struct iscsi_endpoint *ep;
> +
> + ISCSI_DBG_TRANS_CONN(conn, "disconnect ep.\n");
> + conn->state = ISCSI_CONN_FAILED;
> +
> + if (!conn->ep || !session->transport->ep_disconnect)
> + return;
> +
> + ep = conn->ep;
> + conn->ep = NULL;
> +
> + session->transport->unbind_conn(conn, is_active);
> + session->transport->ep_disconnect(ep);
> + ISCSI_DBG_TRANS_CONN(conn, "disconnect ep done.\n");
> +}
> +
> +static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
> +{
> + struct iscsi_cls_conn *conn = container_of(work, struct iscsi_cls_conn,
> + cleanup_work);
> + struct iscsi_cls_session *session = iscsi_conn_to_session(conn);
> +
> + mutex_lock(&conn->ep_mutex);
> + /*
> + * If we are not at least bound there is nothing for us to do. Userspace
> + * will do a ep_disconnect call if offload is used, but will not be
> + * doing a stop since there is nothing to clean up, so we have to clear
> + * the cleanup bit here.
> + */
> + if (conn->state != ISCSI_CONN_BOUND && conn->state != ISCSI_CONN_UP) {
> + ISCSI_DBG_TRANS_CONN(conn, "Got error while conn is already failed. Ignoring.\n");
> + clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> + mutex_unlock(&conn->ep_mutex);
> + return;
> + }
> +
> + iscsi_ep_disconnect(conn, false);
> +
> + if (system_state != SYSTEM_RUNNING) {
> + /*
> + * If the user has set up for the session to never timeout
> + * then hang like they wanted. For all other cases fail right
> + * away since userspace is not going to relogin.
> + */
> + if (session->recovery_tmo > 0)
> + session->recovery_tmo = 0;
> + }
> +
> + iscsi_stop_conn(conn, STOP_CONN_RECOVER);
> + mutex_unlock(&conn->ep_mutex);
> + ISCSI_DBG_TRANS_CONN(conn, "cleanup done.\n");
> +}
> +
> void iscsi_free_session(struct iscsi_cls_session *session)
> {
> ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
> @@ -2284,7 +2391,7 @@ iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
>
> mutex_init(&conn->ep_mutex);
> INIT_LIST_HEAD(&conn->conn_list);
> - INIT_LIST_HEAD(&conn->conn_list_err);
> + INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
> conn->transport = transport;
> conn->cid = cid;
> conn->state = ISCSI_CONN_DOWN;
> @@ -2341,7 +2448,6 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
>
> spin_lock_irqsave(&connlock, flags);
> list_del(&conn->conn_list);
> - list_del(&conn->conn_list_err);
> spin_unlock_irqrestore(&connlock, flags);
>
> transport_unregister_device(&conn->dev);
> @@ -2456,83 +2562,6 @@ int iscsi_offload_mesg(struct Scsi_Host *shost,
> }
> EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
>
> -/*
> - * This can be called without the rx_queue_mutex, if invoked by the kernel
> - * stop work. But, in that case, it is guaranteed not to race with
> - * iscsi_destroy by conn_mutex.
> - */
> -static void iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
> -{
> - /*
> - * It is important that this path doesn't rely on
> - * rx_queue_mutex, otherwise, a thread doing allocation on a
> - * start_session/start_connection could sleep waiting on a
> - * writeback to a failed iscsi device, that cannot be recovered
> - * because the lock is held. If we don't hold it here, the
> - * kernel stop_conn_work_fn has a chance to stop the broken
> - * session and resolve the allocation.
> - *
> - * Still, the user invoked .stop_conn() needs to be serialized
> - * with stop_conn_work_fn by a private mutex. Not pretty, but
> - * it works.
> - */
> - mutex_lock(&conn_mutex);
> - switch (flag) {
> - case STOP_CONN_RECOVER:
> - conn->state = ISCSI_CONN_FAILED;
> - break;
> - case STOP_CONN_TERM:
> - conn->state = ISCSI_CONN_DOWN;
> - break;
> - default:
> - iscsi_cls_conn_printk(KERN_ERR, conn,
> - "invalid stop flag %d\n", flag);
> - goto unlock;
> - }
> -
> - conn->transport->stop_conn(conn, flag);
> -unlock:
> - mutex_unlock(&conn_mutex);
> -}
> -
> -static void stop_conn_work_fn(struct work_struct *work)
> -{
> - struct iscsi_cls_conn *conn, *tmp;
> - unsigned long flags;
> - LIST_HEAD(recovery_list);
> -
> - spin_lock_irqsave(&connlock, flags);
> - if (list_empty(&connlist_err)) {
> - spin_unlock_irqrestore(&connlock, flags);
> - return;
> - }
> - list_splice_init(&connlist_err, &recovery_list);
> - spin_unlock_irqrestore(&connlock, flags);
> -
> - list_for_each_entry_safe(conn, tmp, &recovery_list, conn_list_err) {
> - uint32_t sid = iscsi_conn_get_sid(conn);
> - struct iscsi_cls_session *session;
> -
> - session = iscsi_session_lookup(sid);
> - if (session) {
> - if (system_state != SYSTEM_RUNNING) {
> - /*
> - * If the user has set up for the session to
> - * never timeout then hang like they wanted.
> - * For all other cases fail right away since
> - * userspace is not going to relogin.
> - */
> - if (session->recovery_tmo > 0)
> - session->recovery_tmo = 0;
> - }
> -
> - iscsi_if_stop_conn(conn, STOP_CONN_RECOVER);
> - }
> -
> - list_del_init(&conn->conn_list_err);
> - }
> -}
> -
> void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
> {
> struct nlmsghdr *nlh;
> @@ -2540,12 +2569,9 @@ void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
> struct iscsi_uevent *ev;
> struct iscsi_internal *priv;
> int len = nlmsg_total_size(sizeof(*ev));
> - unsigned long flags;
>
> - spin_lock_irqsave(&connlock, flags);
> - list_add(&conn->conn_list_err, &connlist_err);
> - spin_unlock_irqrestore(&connlock, flags);
> - queue_work(system_unbound_wq, &stop_conn_work);
> + if (!test_and_set_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags))
> + queue_work(iscsi_conn_cleanup_workq, &conn->cleanup_work);
>
> priv = iscsi_if_transport_lookup(conn->transport);
> if (!priv)
> @@ -2875,26 +2901,17 @@ static int
> iscsi_if_destroy_conn(struct iscsi_transport *transport, struct iscsi_uevent *ev)
> {
> struct iscsi_cls_conn *conn;
> - unsigned long flags;
>
> conn = iscsi_conn_lookup(ev->u.d_conn.sid, ev->u.d_conn.cid);
> if (!conn)
> return -EINVAL;
>
> - spin_lock_irqsave(&connlock, flags);
> - if (!list_empty(&conn->conn_list_err)) {
> - spin_unlock_irqrestore(&connlock, flags);
> - return -EAGAIN;
> - }
> - spin_unlock_irqrestore(&connlock, flags);
> -
> + ISCSI_DBG_TRANS_CONN(conn, "Flushing cleanup during destruction\n");
> + flush_work(&conn->cleanup_work);
> ISCSI_DBG_TRANS_CONN(conn, "Destroying transport conn\n");
>
> - mutex_lock(&conn_mutex);
> if (transport->destroy_conn)
> transport->destroy_conn(conn);
> - mutex_unlock(&conn_mutex);
> -
> return 0;
> }
>
> @@ -2973,7 +2990,7 @@ static int iscsi_if_ep_connect(struct iscsi_transport *transport,
> }
>
> static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
> - u64 ep_handle, bool is_active)
> + u64 ep_handle)
> {
> struct iscsi_cls_conn *conn;
> struct iscsi_endpoint *ep;
> @@ -2984,17 +3001,30 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
> ep = iscsi_lookup_endpoint(ep_handle);
> if (!ep)
> return -EINVAL;
> +
> conn = ep->conn;
> - if (conn) {
> - mutex_lock(&conn->ep_mutex);
> - conn->ep = NULL;
> + if (!conn) {
> + /*
> + * conn was not even bound yet, so we can't get iscsi conn
> + * failures yet.
> + */
> + transport->ep_disconnect(ep);
> + goto put_ep;
> + }
> +
> + mutex_lock(&conn->ep_mutex);
> + /* Check if this was a conn error and the kernel took ownership */
> + if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> + ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
> mutex_unlock(&conn->ep_mutex);
> - conn->state = ISCSI_CONN_FAILED;
>
> - transport->unbind_conn(conn, is_active);
> + flush_work(&conn->cleanup_work);
> + goto put_ep;
> }
>
> - transport->ep_disconnect(ep);
> + iscsi_ep_disconnect(conn, false);
> + mutex_unlock(&conn->ep_mutex);
> +put_ep:
> iscsi_put_endpoint(ep);
> return 0;
> }
> @@ -3025,8 +3055,7 @@ iscsi_if_transport_ep(struct iscsi_transport *transport,
> break;
> case ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT:
> rc = iscsi_if_ep_disconnect(transport,
> - ev->u.ep_disconnect.ep_handle,
> - false);
> + ev->u.ep_disconnect.ep_handle);
> break;
> }
> return rc;
> @@ -3653,18 +3682,129 @@ iscsi_get_host_stats(struct iscsi_transport *transport, struct nlmsghdr *nlh)
> return err;
> }
>
> +static int iscsi_if_transport_conn(struct iscsi_transport *transport,
> + struct nlmsghdr *nlh)
> +{
> + struct iscsi_uevent *ev = nlmsg_data(nlh);
> + struct iscsi_cls_session *session;
> + struct iscsi_cls_conn *conn = NULL;
> + struct iscsi_endpoint *ep;
> + uint32_t pdu_len;
> + int err = 0;
> +
> + switch (nlh->nlmsg_type) {
> + case ISCSI_UEVENT_CREATE_CONN:
> + return iscsi_if_create_conn(transport, ev);
> + case ISCSI_UEVENT_DESTROY_CONN:
> + return iscsi_if_destroy_conn(transport, ev);
> + case ISCSI_UEVENT_STOP_CONN:
> + return iscsi_if_stop_conn(transport, ev);
> + }
> +
> + /*
> + * The following cmds need to be run under the ep_mutex so in kernel
> + * conn cleanup (ep_disconnect + unbind and conn) is not done while
> + * these are running. They also must not run if we have just run a conn
> + * cleanup because they would set the state in a way that might allow
> + * IO or send IO themselves.
> + */
> + switch (nlh->nlmsg_type) {
> + case ISCSI_UEVENT_START_CONN:
> + conn = iscsi_conn_lookup(ev->u.start_conn.sid,
> + ev->u.start_conn.cid);
> + break;
> + case ISCSI_UEVENT_BIND_CONN:
> + conn = iscsi_conn_lookup(ev->u.b_conn.sid, ev->u.b_conn.cid);
> + break;
> + case ISCSI_UEVENT_SEND_PDU:
> + conn = iscsi_conn_lookup(ev->u.send_pdu.sid, ev->u.send_pdu.cid);
> + break;
> + }
> +
> + if (!conn)
> + return -EINVAL;
> +
> + mutex_lock(&conn->ep_mutex);
> + if (test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
> + mutex_unlock(&conn->ep_mutex);
> + ev->r.retcode = -ENOTCONN;
> + return 0;
> + }
> +
> + switch (nlh->nlmsg_type) {
> + case ISCSI_UEVENT_BIND_CONN:
> + if (conn->ep) {
> + /*
> + * For offload boot support where iscsid is restarted
> + * during the pivot root stage, the ep will be intact
> + * here when the new iscsid instance starts up and
> + * reconnects.
> + */
> + iscsi_ep_disconnect(conn, true);
> + }
> +
> + session = iscsi_session_lookup(ev->u.b_conn.sid);
> + if (!session) {
> + err = -EINVAL;
> + break;
> + }
> +
> + ev->r.retcode = transport->bind_conn(session, conn,
> + ev->u.b_conn.transport_eph,
> + ev->u.b_conn.is_leading);
> + if (!ev->r.retcode)
> + conn->state = ISCSI_CONN_BOUND;
> +
> + if (ev->r.retcode || !transport->ep_connect)
> + break;
> +
> + ep = iscsi_lookup_endpoint(ev->u.b_conn.transport_eph);
> + if (ep) {
> + ep->conn = conn;
> + conn->ep = ep;
> + iscsi_put_endpoint(ep);
> + } else {
> + err = -ENOTCONN;
> + iscsi_cls_conn_printk(KERN_ERR, conn,
> + "Could not set ep conn binding\n");
> + }
> + break;
> + case ISCSI_UEVENT_START_CONN:
> + ev->r.retcode = transport->start_conn(conn);
> + if (!ev->r.retcode)
> + conn->state = ISCSI_CONN_UP;
> + break;
> + case ISCSI_UEVENT_SEND_PDU:
> + pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
> +
> + if ((ev->u.send_pdu.hdr_size > pdu_len) ||
> + (ev->u.send_pdu.data_size > (pdu_len - ev->u.send_pdu.hdr_size))) {
> + err = -EINVAL;
> + break;
> + }
> +
> + ev->r.retcode = transport->send_pdu(conn,
> + (struct iscsi_hdr *)((char *)ev + sizeof(*ev)),
> + (char *)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
> + ev->u.send_pdu.data_size);
> + break;
> + default:
> + err = -ENOSYS;
> + }
> +
> + mutex_unlock(&conn->ep_mutex);
> + return err;
> +}
>
> static int
> iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
> {
> int err = 0;
> u32 portid;
> - u32 pdu_len;
> struct iscsi_uevent *ev = nlmsg_data(nlh);
> struct iscsi_transport *transport = NULL;
> struct iscsi_internal *priv;
> struct iscsi_cls_session *session;
> - struct iscsi_cls_conn *conn;
> struct iscsi_endpoint *ep = NULL;
>
> if (!netlink_capable(skb, CAP_SYS_ADMIN))
> @@ -3741,90 +3881,16 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
> else
> err = -EINVAL;
> break;
> - case ISCSI_UEVENT_CREATE_CONN:
> - err = iscsi_if_create_conn(transport, ev);
> - break;
> - case ISCSI_UEVENT_DESTROY_CONN:
> - err = iscsi_if_destroy_conn(transport, ev);
> - break;
> - case ISCSI_UEVENT_BIND_CONN:
> - session = iscsi_session_lookup(ev->u.b_conn.sid);
> - conn = iscsi_conn_lookup(ev->u.b_conn.sid, ev->u.b_conn.cid);
> -
> - if (conn && conn->ep)
> - iscsi_if_ep_disconnect(transport, conn->ep->id, true);
> -
> - if (!session || !conn) {
> - err = -EINVAL;
> - break;
> - }
> -
> - mutex_lock(&conn_mutex);
> - ev->r.retcode = transport->bind_conn(session, conn,
> - ev->u.b_conn.transport_eph,
> - ev->u.b_conn.is_leading);
> - if (!ev->r.retcode)
> - conn->state = ISCSI_CONN_BOUND;
> - mutex_unlock(&conn_mutex);
> -
> - if (ev->r.retcode || !transport->ep_connect)
> - break;
> -
> - ep = iscsi_lookup_endpoint(ev->u.b_conn.transport_eph);
> - if (ep) {
> - ep->conn = conn;
> -
> - mutex_lock(&conn->ep_mutex);
> - conn->ep = ep;
> - mutex_unlock(&conn->ep_mutex);
> - iscsi_put_endpoint(ep);
> - } else
> - iscsi_cls_conn_printk(KERN_ERR, conn,
> - "Could not set ep conn "
> - "binding\n");
> - break;
> case ISCSI_UEVENT_SET_PARAM:
> err = iscsi_set_param(transport, ev);
> break;
> - case ISCSI_UEVENT_START_CONN:
> - conn = iscsi_conn_lookup(ev->u.start_conn.sid, ev->u.start_conn.cid);
> - if (conn) {
> - mutex_lock(&conn_mutex);
> - ev->r.retcode = transport->start_conn(conn);
> - if (!ev->r.retcode)
> - conn->state = ISCSI_CONN_UP;
> - mutex_unlock(&conn_mutex);
> - }
> - else
> - err = -EINVAL;
> - break;
> + case ISCSI_UEVENT_CREATE_CONN:
> + case ISCSI_UEVENT_DESTROY_CONN:
> case ISCSI_UEVENT_STOP_CONN:
> - conn = iscsi_conn_lookup(ev->u.stop_conn.sid, ev->u.stop_conn.cid);
> - if (conn)
> - iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
> - else
> - err = -EINVAL;
> - break;
> + case ISCSI_UEVENT_START_CONN:
> + case ISCSI_UEVENT_BIND_CONN:
> case ISCSI_UEVENT_SEND_PDU:
> - pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
> -
> - if ((ev->u.send_pdu.hdr_size > pdu_len) ||
> - (ev->u.send_pdu.data_size > (pdu_len - ev->u.send_pdu.hdr_size))) {
> - err = -EINVAL;
> - break;
> - }
> -
> - conn = iscsi_conn_lookup(ev->u.send_pdu.sid, ev->u.send_pdu.cid);
> - if (conn) {
> - mutex_lock(&conn_mutex);
> - ev->r.retcode = transport->send_pdu(conn,
> - (struct iscsi_hdr*)((char*)ev + sizeof(*ev)),
> - (char*)ev + sizeof(*ev) + ev->u.send_pdu.hdr_size,
> - ev->u.send_pdu.data_size);
> - mutex_unlock(&conn_mutex);
> - }
> - else
> - err = -EINVAL;
> + err = iscsi_if_transport_conn(transport, nlh);
> break;
> case ISCSI_UEVENT_GET_STATS:
> err = iscsi_if_get_stats(transport, nlh);
> @@ -4827,8 +4893,18 @@ static __init int iscsi_transport_init(void)
> goto release_nls;
> }
>
> + iscsi_conn_cleanup_workq = alloc_workqueue("%s",
> + WQ_SYSFS | WQ_MEM_RECLAIM | WQ_UNBOUND, 0,
> + "iscsi_conn_cleanup");
> + if (!iscsi_conn_cleanup_workq) {
> + err = -ENOMEM;
> + goto destroy_wq;
> + }
> +
> return 0;
>
> +destroy_wq:
> + destroy_workqueue(iscsi_eh_timer_workq);
> release_nls:
> netlink_kernel_release(nls);
> unregister_flashnode_bus:
> @@ -4850,6 +4926,7 @@ static __init int iscsi_transport_init(void)
>
> static void __exit iscsi_transport_exit(void)
> {
> + destroy_workqueue(iscsi_conn_cleanup_workq);
> destroy_workqueue(iscsi_eh_timer_workq);
> netlink_kernel_release(nls);
> bus_unregister(&iscsi_flashnode_bus);
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index d36a72cf049f..3974329d4d02 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -197,15 +197,23 @@ enum iscsi_connection_state {
> ISCSI_CONN_BOUND,
> };
>
> +#define ISCSI_CLS_CONN_BIT_CLEANUP 1
> +
> struct iscsi_cls_conn {
> struct list_head conn_list; /* item in connlist */
> - struct list_head conn_list_err; /* item in connlist_err */
> void *dd_data; /* LLD private data */
> struct iscsi_transport *transport;
> uint32_t cid; /* connection id */
> + /*
> + * This protects the conn startup and binding/unbinding of the ep to
> + * the conn. Unbinding includes ep_disconnect and stop_conn.
> + */
> struct mutex ep_mutex;
> struct iscsi_endpoint *ep;
>
> + unsigned long flags;
> + struct work_struct cleanup_work;
> +
> struct device dev; /* sysfs transport/container device */
> enum iscsi_connection_state state;
> };
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 08/28] scsi: iscsi_tcp: Set no linger
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (6 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 07/28] scsi: iscsi: Fix in-kernel conn failure handling Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 09/28] scsi: iscsi_tcp: Start socket shutdown during conn stop Mike Christie
` (19 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Userspace (open-iscsi based tools at least) sets no linger on the socket
to prevent stale data from being sent. However, with the in kernel cleanup
if userspace is not up the sockfd_put will release the socket without
having set that sockopt.
iscsid sets that opt at socket close time, but it seems ok to set this at
setup time in the kernel for all tools.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/iscsi_tcp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index dd33ce0e3737..553e95ad6197 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -689,6 +689,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
sk->sk_sndtimeo = 15 * HZ; /* FIXME: make it configurable */
sk->sk_allocation = GFP_ATOMIC;
sk_set_memalloc(sk);
+ sock_no_linger(sk);
iscsi_sw_tcp_conn_set_callbacks(conn);
tcp_sw_conn->sendpage = tcp_sw_conn->sock->ops->sendpage;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 09/28] scsi: iscsi_tcp: Start socket shutdown during conn stop
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (7 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 08/28] scsi: iscsi_tcp: Set no linger Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 10/28] scsi: iscsi: Add iscsi_cls_conn refcount helpers Mike Christie
` (18 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Make sure the conn socket shutdown starts before we start the timer to fail
commands to upper layers.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/iscsi_tcp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 553e95ad6197..1bc37593c88f 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -600,6 +600,12 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
if (!sock)
return;
+ /*
+ * Make sure we start socket shutdown now in case userspace is up
+ * but delayed in releasing the socket.
+ */
+ kernel_sock_shutdown(sock, SHUT_RDWR);
+
sock_hold(sock->sk);
iscsi_sw_tcp_conn_restore_callbacks(conn);
sock_put(sock->sk);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 10/28] scsi: iscsi: Add iscsi_cls_conn refcount helpers
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (8 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 09/28] scsi: iscsi_tcp: Start socket shutdown during conn stop Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:34 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 11/28] scsi: iscsi: Have abort handler get ref to conn Mike Christie
` (17 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
There are a couple places where we could free the iscsi_cls_conn while
it's still in use. This adds some helpers to get/put a refcount on the
struct and converts an exiting user. The next patches will then use the
helpers to fix 2 bugs in the eh code.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 7 ++-----
drivers/scsi/scsi_transport_iscsi.c | 12 ++++++++++++
include/scsi/scsi_transport_iscsi.h | 2 ++
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 2aaf83678654..ab39d7f65bbb 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1361,7 +1361,6 @@ void iscsi_session_failure(struct iscsi_session *session,
enum iscsi_err err)
{
struct iscsi_conn *conn;
- struct device *dev;
spin_lock_bh(&session->frwd_lock);
conn = session->leadconn;
@@ -1370,10 +1369,8 @@ void iscsi_session_failure(struct iscsi_session *session,
return;
}
- dev = get_device(&conn->cls_conn->dev);
+ iscsi_get_conn(conn->cls_conn);
spin_unlock_bh(&session->frwd_lock);
- if (!dev)
- return;
/*
* if the host is being removed bypass the connection
* recovery initialization because we are going to kill
@@ -1383,7 +1380,7 @@ void iscsi_session_failure(struct iscsi_session *session,
iscsi_conn_error_event(conn->cls_conn, err);
else
iscsi_conn_failure(conn, err);
- put_device(dev);
+ iscsi_put_conn(conn->cls_conn);
}
EXPORT_SYMBOL_GPL(iscsi_session_failure);
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index b8a93e607891..909134b9c313 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2457,6 +2457,18 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
}
EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
+void iscsi_put_conn(struct iscsi_cls_conn *conn)
+{
+ put_device(&conn->dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_put_conn);
+
+void iscsi_get_conn(struct iscsi_cls_conn *conn)
+{
+ get_device(&conn->dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_get_conn);
+
/*
* iscsi interface functions
*/
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 3974329d4d02..c5d7810fd792 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -443,6 +443,8 @@ extern void iscsi_remove_session(struct iscsi_cls_session *session);
extern void iscsi_free_session(struct iscsi_cls_session *session);
extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
int dd_size, uint32_t cid);
+extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
+extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
extern void iscsi_unblock_session(struct iscsi_cls_session *session);
extern void iscsi_block_session(struct iscsi_cls_session *session);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 10/28] scsi: iscsi: Add iscsi_cls_conn refcount helpers
2021-05-23 17:57 ` [PATCH 10/28] scsi: iscsi: Add iscsi_cls_conn refcount helpers Mike Christie
@ 2021-05-24 18:34 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:34 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> There are a couple places where we could free the iscsi_cls_conn while
> it's still in use. This adds some helpers to get/put a refcount on the
> struct and converts an exiting user. The next patches will then use the
> helpers to fix 2 bugs in the eh code.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/libiscsi.c | 7 ++-----
> drivers/scsi/scsi_transport_iscsi.c | 12 ++++++++++++
> include/scsi/scsi_transport_iscsi.h | 2 ++
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 2aaf83678654..ab39d7f65bbb 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1361,7 +1361,6 @@ void iscsi_session_failure(struct iscsi_session *session,
> enum iscsi_err err)
> {
> struct iscsi_conn *conn;
> - struct device *dev;
>
> spin_lock_bh(&session->frwd_lock);
> conn = session->leadconn;
> @@ -1370,10 +1369,8 @@ void iscsi_session_failure(struct iscsi_session *session,
> return;
> }
>
> - dev = get_device(&conn->cls_conn->dev);
> + iscsi_get_conn(conn->cls_conn);
> spin_unlock_bh(&session->frwd_lock);
> - if (!dev)
> - return;
> /*
> * if the host is being removed bypass the connection
> * recovery initialization because we are going to kill
> @@ -1383,7 +1380,7 @@ void iscsi_session_failure(struct iscsi_session *session,
> iscsi_conn_error_event(conn->cls_conn, err);
> else
> iscsi_conn_failure(conn, err);
> - put_device(dev);
> + iscsi_put_conn(conn->cls_conn);
> }
> EXPORT_SYMBOL_GPL(iscsi_session_failure);
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index b8a93e607891..909134b9c313 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2457,6 +2457,18 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
> }
> EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
>
> +void iscsi_put_conn(struct iscsi_cls_conn *conn)
> +{
> + put_device(&conn->dev);
> +}
> +EXPORT_SYMBOL_GPL(iscsi_put_conn);
> +
> +void iscsi_get_conn(struct iscsi_cls_conn *conn)
> +{
> + get_device(&conn->dev);
> +}
> +EXPORT_SYMBOL_GPL(iscsi_get_conn);
> +
> /*
> * iscsi interface functions
> */
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 3974329d4d02..c5d7810fd792 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -443,6 +443,8 @@ extern void iscsi_remove_session(struct iscsi_cls_session *session);
> extern void iscsi_free_session(struct iscsi_cls_session *session);
> extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
> int dd_size, uint32_t cid);
> +extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
> +extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
> extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
> extern void iscsi_unblock_session(struct iscsi_cls_session *session);
> extern void iscsi_block_session(struct iscsi_cls_session *session);
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 11/28] scsi: iscsi: Have abort handler get ref to conn
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (9 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 10/28] scsi: iscsi: Add iscsi_cls_conn refcount helpers Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:36 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 12/28] scsi: iscsi: Get ref to conn during reset handling Mike Christie
` (16 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If scsi-ml is aborting a task when we are tearing down the conn we could
free the conn while the abort thread is accessing the conn. This has the
abort handler get a ref to the conn so it won't be freed from under it.
Note: this is not needed for device/target reset because we are holding
the eh_mutex when accessing the conn.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index ab39d7f65bbb..6ca3d35a3d11 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2285,6 +2285,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
}
conn = session->leadconn;
+ iscsi_get_conn(conn->cls_conn);
conn->eh_abort_cnt++;
age = session->age;
@@ -2295,9 +2296,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
ISCSI_DBG_EH(session, "sc completed while abort in progress\n");
spin_unlock(&session->back_lock);
- spin_unlock_bh(&session->frwd_lock);
- mutex_unlock(&session->eh_mutex);
- return SUCCESS;
+ goto success;
}
ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
__iscsi_get_task(task);
@@ -2364,6 +2363,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
ISCSI_DBG_EH(session, "abort success [sc %p itt 0x%x]\n",
sc, task->itt);
iscsi_put_task(task);
+ iscsi_put_conn(conn->cls_conn);
mutex_unlock(&session->eh_mutex);
return SUCCESS;
@@ -2373,6 +2373,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
task ? task->itt : 0);
iscsi_put_task(task);
+ iscsi_put_conn(conn->cls_conn);
mutex_unlock(&session->eh_mutex);
return FAILED;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 11/28] scsi: iscsi: Have abort handler get ref to conn
2021-05-23 17:57 ` [PATCH 11/28] scsi: iscsi: Have abort handler get ref to conn Mike Christie
@ 2021-05-24 18:36 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:36 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> If scsi-ml is aborting a task when we are tearing down the conn we could
> free the conn while the abort thread is accessing the conn. This has the
> abort handler get a ref to the conn so it won't be freed from under it.
>
> Note: this is not needed for device/target reset because we are holding
> the eh_mutex when accessing the conn.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/libiscsi.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ab39d7f65bbb..6ca3d35a3d11 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2285,6 +2285,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> }
>
> conn = session->leadconn;
> + iscsi_get_conn(conn->cls_conn);
> conn->eh_abort_cnt++;
> age = session->age;
>
> @@ -2295,9 +2296,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> ISCSI_DBG_EH(session, "sc completed while abort in progress\n");
>
> spin_unlock(&session->back_lock);
> - spin_unlock_bh(&session->frwd_lock);
> - mutex_unlock(&session->eh_mutex);
> - return SUCCESS;
> + goto success;
> }
> ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
> __iscsi_get_task(task);
> @@ -2364,6 +2363,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> ISCSI_DBG_EH(session, "abort success [sc %p itt 0x%x]\n",
> sc, task->itt);
> iscsi_put_task(task);
> + iscsi_put_conn(conn->cls_conn);
> mutex_unlock(&session->eh_mutex);
> return SUCCESS;
>
> @@ -2373,6 +2373,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
> task ? task->itt : 0);
> iscsi_put_task(task);
> + iscsi_put_conn(conn->cls_conn);
> mutex_unlock(&session->eh_mutex);
> return FAILED;
> }
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 12/28] scsi: iscsi: Get ref to conn during reset handling
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (10 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 11/28] scsi: iscsi: Have abort handler get ref to conn Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:37 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 13/28] scsi: iscsi: Fix conn use after free during resets Mike Christie
` (15 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
The comment in iscsi_eh_session_reset is wrong and we don't wait for the
EH to complete before tearing down the conn. This has us get a ref to the
conn when we are not holding the eh_mutex/frwd_lock so it does not get
freed from under us.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6ca3d35a3d11..b7445d9e99d6 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2492,7 +2492,6 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
cls_session = starget_to_session(scsi_target(sc->device));
session = cls_session->dd_data;
- conn = session->leadconn;
mutex_lock(&session->eh_mutex);
spin_lock_bh(&session->frwd_lock);
@@ -2507,13 +2506,14 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
return FAILED;
}
+ conn = session->leadconn;
+ iscsi_get_conn(conn->cls_conn);
+
spin_unlock_bh(&session->frwd_lock);
mutex_unlock(&session->eh_mutex);
- /*
- * we drop the lock here but the leadconn cannot be destoyed while
- * we are in the scsi eh
- */
+
iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
+ iscsi_put_conn(conn->cls_conn);
ISCSI_DBG_EH(session, "wait for relogin\n");
wait_event_interruptible(conn->ehwait,
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 12/28] scsi: iscsi: Get ref to conn during reset handling
2021-05-23 17:57 ` [PATCH 12/28] scsi: iscsi: Get ref to conn during reset handling Mike Christie
@ 2021-05-24 18:37 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:37 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> The comment in iscsi_eh_session_reset is wrong and we don't wait for the
> EH to complete before tearing down the conn. This has us get a ref to the
> conn when we are not holding the eh_mutex/frwd_lock so it does not get
> freed from under us.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/libiscsi.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 6ca3d35a3d11..b7445d9e99d6 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2492,7 +2492,6 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
>
> cls_session = starget_to_session(scsi_target(sc->device));
> session = cls_session->dd_data;
> - conn = session->leadconn;
>
> mutex_lock(&session->eh_mutex);
> spin_lock_bh(&session->frwd_lock);
> @@ -2507,13 +2506,14 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
> return FAILED;
> }
>
> + conn = session->leadconn;
> + iscsi_get_conn(conn->cls_conn);
> +
> spin_unlock_bh(&session->frwd_lock);
> mutex_unlock(&session->eh_mutex);
> - /*
> - * we drop the lock here but the leadconn cannot be destoyed while
> - * we are in the scsi eh
> - */
> +
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
> + iscsi_put_conn(conn->cls_conn);
>
> ISCSI_DBG_EH(session, "wait for relogin\n");
> wait_event_interruptible(conn->ehwait,
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 13/28] scsi: iscsi: Fix conn use after free during resets
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (11 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 12/28] scsi: iscsi: Get ref to conn during reset handling Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 14/28] scsi: iscsi: Fix shost->max_id use Mike Christie
` (14 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If we haven't done a unbind target call we can race where
iscsi_conn_teardown wakes up the EH thread and then frees the conn while
those threads are still accessing the conn ehwait.
We can only do one TMF per session so this just moves the TMF fields from
the conn to the session. We can then rely on the
iscsi_session_teardown->iscsi_remove_session->__iscsi_unbind_session call
to remove the target and it's devices, and know after that point there is
no device or scsi-ml callout trying to access the session.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 115 +++++++++++++++++++---------------------
include/scsi/libiscsi.h | 11 ++--
2 files changed, 60 insertions(+), 66 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b7445d9e99d6..94abb093098d 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -230,11 +230,11 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_task *task)
*/
static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
{
- struct iscsi_conn *conn = task->conn;
- struct iscsi_tm *tmf = &conn->tmhdr;
+ struct iscsi_session *session = task->conn->session;
+ struct iscsi_tm *tmf = &session->tmhdr;
u64 hdr_lun;
- if (conn->tmf_state == TMF_INITIAL)
+ if (session->tmf_state == TMF_INITIAL)
return 0;
if ((tmf->opcode & ISCSI_OPCODE_MASK) != ISCSI_OP_SCSI_TMFUNC)
@@ -254,24 +254,19 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
* Fail all SCSI cmd PDUs
*/
if (opcode != ISCSI_OP_SCSI_DATA_OUT) {
- iscsi_conn_printk(KERN_INFO, conn,
- "task [op %x itt "
- "0x%x/0x%x] "
- "rejected.\n",
- opcode, task->itt,
- task->hdr_itt);
+ iscsi_session_printk(KERN_INFO, session,
+ "task [op %x itt 0x%x/0x%x] rejected.\n",
+ opcode, task->itt, task->hdr_itt);
return -EACCES;
}
/*
* And also all data-out PDUs in response to R2T
* if fast_abort is set.
*/
- if (conn->session->fast_abort) {
- iscsi_conn_printk(KERN_INFO, conn,
- "task [op %x itt "
- "0x%x/0x%x] fast abort.\n",
- opcode, task->itt,
- task->hdr_itt);
+ if (session->fast_abort) {
+ iscsi_session_printk(KERN_INFO, session,
+ "task [op %x itt 0x%x/0x%x] fast abort.\n",
+ opcode, task->itt, task->hdr_itt);
return -EACCES;
}
break;
@@ -284,7 +279,7 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
*/
if (opcode == ISCSI_OP_SCSI_DATA_OUT &&
task->hdr_itt == tmf->rtt) {
- ISCSI_DBG_SESSION(conn->session,
+ ISCSI_DBG_SESSION(session,
"Preventing task %x/%x from sending "
"data-out due to abort task in "
"progress\n", task->itt,
@@ -936,20 +931,21 @@ iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
static void iscsi_tmf_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
{
struct iscsi_tm_rsp *tmf = (struct iscsi_tm_rsp *)hdr;
+ struct iscsi_session *session = conn->session;
conn->exp_statsn = be32_to_cpu(hdr->statsn) + 1;
conn->tmfrsp_pdus_cnt++;
- if (conn->tmf_state != TMF_QUEUED)
+ if (session->tmf_state != TMF_QUEUED)
return;
if (tmf->response == ISCSI_TMF_RSP_COMPLETE)
- conn->tmf_state = TMF_SUCCESS;
+ session->tmf_state = TMF_SUCCESS;
else if (tmf->response == ISCSI_TMF_RSP_NO_TASK)
- conn->tmf_state = TMF_NOT_FOUND;
+ session->tmf_state = TMF_NOT_FOUND;
else
- conn->tmf_state = TMF_FAILED;
- wake_up(&conn->ehwait);
+ session->tmf_state = TMF_FAILED;
+ wake_up(&session->ehwait);
}
static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
@@ -1826,15 +1822,14 @@ EXPORT_SYMBOL_GPL(iscsi_target_alloc);
static void iscsi_tmf_timedout(struct timer_list *t)
{
- struct iscsi_conn *conn = from_timer(conn, t, tmf_timer);
- struct iscsi_session *session = conn->session;
+ struct iscsi_session *session = from_timer(session, t, tmf_timer);
spin_lock(&session->frwd_lock);
- if (conn->tmf_state == TMF_QUEUED) {
- conn->tmf_state = TMF_TIMEDOUT;
+ if (session->tmf_state == TMF_QUEUED) {
+ session->tmf_state = TMF_TIMEDOUT;
ISCSI_DBG_EH(session, "tmf timedout\n");
/* unblock eh_abort() */
- wake_up(&conn->ehwait);
+ wake_up(&session->ehwait);
}
spin_unlock(&session->frwd_lock);
}
@@ -1857,8 +1852,8 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
return -EPERM;
}
conn->tmfcmd_pdus_cnt++;
- conn->tmf_timer.expires = timeout * HZ + jiffies;
- add_timer(&conn->tmf_timer);
+ session->tmf_timer.expires = timeout * HZ + jiffies;
+ add_timer(&session->tmf_timer);
ISCSI_DBG_EH(session, "tmf set timeout\n");
spin_unlock_bh(&session->frwd_lock);
@@ -1872,12 +1867,12 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
* 3) session is terminated or restarted or userspace has
* given up on recovery
*/
- wait_event_interruptible(conn->ehwait, age != session->age ||
+ wait_event_interruptible(session->ehwait, age != session->age ||
session->state != ISCSI_STATE_LOGGED_IN ||
- conn->tmf_state != TMF_QUEUED);
+ session->tmf_state != TMF_QUEUED);
if (signal_pending(current))
flush_signals(current);
- del_timer_sync(&conn->tmf_timer);
+ del_timer_sync(&session->tmf_timer);
mutex_lock(&session->eh_mutex);
spin_lock_bh(&session->frwd_lock);
@@ -2308,17 +2303,17 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
}
/* only have one tmf outstanding at a time */
- if (conn->tmf_state != TMF_INITIAL)
+ if (session->tmf_state != TMF_INITIAL)
goto failed;
- conn->tmf_state = TMF_QUEUED;
+ session->tmf_state = TMF_QUEUED;
- hdr = &conn->tmhdr;
+ hdr = &session->tmhdr;
iscsi_prep_abort_task_pdu(task, hdr);
if (iscsi_exec_task_mgmt_fn(conn, hdr, age, session->abort_timeout))
goto failed;
- switch (conn->tmf_state) {
+ switch (session->tmf_state) {
case TMF_SUCCESS:
spin_unlock_bh(&session->frwd_lock);
/*
@@ -2333,7 +2328,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
*/
spin_lock_bh(&session->frwd_lock);
fail_scsi_task(task, DID_ABORT);
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
memset(hdr, 0, sizeof(*hdr));
spin_unlock_bh(&session->frwd_lock);
iscsi_start_tx(conn);
@@ -2344,7 +2339,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
goto failed_unlocked;
case TMF_NOT_FOUND:
if (!sc->SCp.ptr) {
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
memset(hdr, 0, sizeof(*hdr));
/* task completed before tmf abort response */
ISCSI_DBG_EH(session, "sc completed while abort in "
@@ -2353,7 +2348,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
}
fallthrough;
default:
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
goto failed;
}
@@ -2414,11 +2409,11 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
conn = session->leadconn;
/* only have one tmf outstanding at a time */
- if (conn->tmf_state != TMF_INITIAL)
+ if (session->tmf_state != TMF_INITIAL)
goto unlock;
- conn->tmf_state = TMF_QUEUED;
+ session->tmf_state = TMF_QUEUED;
- hdr = &conn->tmhdr;
+ hdr = &session->tmhdr;
iscsi_prep_lun_reset_pdu(sc, hdr);
if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age,
@@ -2427,7 +2422,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
goto unlock;
}
- switch (conn->tmf_state) {
+ switch (session->tmf_state) {
case TMF_SUCCESS:
break;
case TMF_TIMEDOUT:
@@ -2435,7 +2430,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
goto done;
default:
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
goto unlock;
}
@@ -2447,7 +2442,7 @@ int iscsi_eh_device_reset(struct scsi_cmnd *sc)
spin_lock_bh(&session->frwd_lock);
memset(hdr, 0, sizeof(*hdr));
fail_scsi_tasks(conn, sc->device->lun, DID_ERROR);
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
spin_unlock_bh(&session->frwd_lock);
iscsi_start_tx(conn);
@@ -2470,8 +2465,7 @@ void iscsi_session_recovery_timedout(struct iscsi_cls_session *cls_session)
spin_lock_bh(&session->frwd_lock);
if (session->state != ISCSI_STATE_LOGGED_IN) {
session->state = ISCSI_STATE_RECOVERY_FAILED;
- if (session->leadconn)
- wake_up(&session->leadconn->ehwait);
+ wake_up(&session->ehwait);
}
spin_unlock_bh(&session->frwd_lock);
}
@@ -2516,7 +2510,7 @@ int iscsi_eh_session_reset(struct scsi_cmnd *sc)
iscsi_put_conn(conn->cls_conn);
ISCSI_DBG_EH(session, "wait for relogin\n");
- wait_event_interruptible(conn->ehwait,
+ wait_event_interruptible(session->ehwait,
session->state == ISCSI_STATE_TERMINATE ||
session->state == ISCSI_STATE_LOGGED_IN ||
session->state == ISCSI_STATE_RECOVERY_FAILED);
@@ -2577,11 +2571,11 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
conn = session->leadconn;
/* only have one tmf outstanding at a time */
- if (conn->tmf_state != TMF_INITIAL)
+ if (session->tmf_state != TMF_INITIAL)
goto unlock;
- conn->tmf_state = TMF_QUEUED;
+ session->tmf_state = TMF_QUEUED;
- hdr = &conn->tmhdr;
+ hdr = &session->tmhdr;
iscsi_prep_tgt_reset_pdu(sc, hdr);
if (iscsi_exec_task_mgmt_fn(conn, hdr, session->age,
@@ -2590,7 +2584,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
goto unlock;
}
- switch (conn->tmf_state) {
+ switch (session->tmf_state) {
case TMF_SUCCESS:
break;
case TMF_TIMEDOUT:
@@ -2598,7 +2592,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
goto done;
default:
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
goto unlock;
}
@@ -2610,7 +2604,7 @@ static int iscsi_eh_target_reset(struct scsi_cmnd *sc)
spin_lock_bh(&session->frwd_lock);
memset(hdr, 0, sizeof(*hdr));
fail_scsi_tasks(conn, -1, DID_ERROR);
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
spin_unlock_bh(&session->frwd_lock);
iscsi_start_tx(conn);
@@ -2940,7 +2934,10 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
session->tt = iscsit;
session->dd_data = cls_session->dd_data + sizeof(*session);
+ session->tmf_state = TMF_INITIAL;
+ timer_setup(&session->tmf_timer, iscsi_tmf_timedout, 0);
mutex_init(&session->eh_mutex);
+
spin_lock_init(&session->frwd_lock);
spin_lock_init(&session->back_lock);
@@ -3044,7 +3041,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
conn->c_stage = ISCSI_CONN_INITIAL_STAGE;
conn->id = conn_idx;
conn->exp_statsn = 0;
- conn->tmf_state = TMF_INITIAL;
timer_setup(&conn->transport_timer, iscsi_check_transport_timeouts, 0);
@@ -3069,8 +3065,7 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
goto login_task_data_alloc_fail;
conn->login_task->data = conn->data = data;
- timer_setup(&conn->tmf_timer, iscsi_tmf_timedout, 0);
- init_waitqueue_head(&conn->ehwait);
+ init_waitqueue_head(&session->ehwait);
return cls_conn;
@@ -3105,7 +3100,7 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
* leading connection? then give up on recovery.
*/
session->state = ISCSI_STATE_TERMINATE;
- wake_up(&conn->ehwait);
+ wake_up(&session->ehwait);
}
spin_unlock_bh(&session->frwd_lock);
@@ -3180,7 +3175,7 @@ int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
* commands after successful recovery
*/
conn->stop_stage = 0;
- conn->tmf_state = TMF_INITIAL;
+ session->tmf_state = TMF_INITIAL;
session->age++;
if (session->age == 16)
session->age = 0;
@@ -3194,7 +3189,7 @@ int iscsi_conn_start(struct iscsi_cls_conn *cls_conn)
spin_unlock_bh(&session->frwd_lock);
iscsi_unblock_session(session->cls_session);
- wake_up(&conn->ehwait);
+ wake_up(&session->ehwait);
return 0;
}
EXPORT_SYMBOL_GPL(iscsi_conn_start);
@@ -3288,7 +3283,7 @@ void iscsi_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
spin_lock_bh(&session->frwd_lock);
fail_scsi_tasks(conn, -1, DID_TRANSPORT_DISRUPTED);
fail_mgmt_tasks(session, conn);
- memset(&conn->tmhdr, 0, sizeof(conn->tmhdr));
+ memset(&session->tmhdr, 0, sizeof(session->tmhdr));
spin_unlock_bh(&session->frwd_lock);
mutex_unlock(&session->eh_mutex);
}
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 13d413a0b8b6..9d7908265afe 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -202,12 +202,6 @@ struct iscsi_conn {
unsigned long suspend_tx; /* suspend Tx */
unsigned long suspend_rx; /* suspend Rx */
- /* abort */
- wait_queue_head_t ehwait; /* used in eh_abort() */
- struct iscsi_tm tmhdr;
- struct timer_list tmf_timer;
- int tmf_state; /* see TMF_INITIAL, etc.*/
-
/* negotiated params */
unsigned max_recv_dlength; /* initiator_max_recv_dsl*/
unsigned max_xmit_dlength; /* target_max_recv_dsl */
@@ -277,6 +271,11 @@ struct iscsi_session {
* and recv lock.
*/
struct mutex eh_mutex;
+ /* abort */
+ wait_queue_head_t ehwait; /* used in eh_abort() */
+ struct iscsi_tm tmhdr;
+ struct timer_list tmf_timer;
+ int tmf_state; /* see TMF_INITIAL, etc.*/
/* iSCSI session-wide sequencing */
uint32_t cmdsn;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 14/28] scsi: iscsi: Fix shost->max_id use
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (12 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 13/28] scsi: iscsi: Fix conn use after free during resets Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 15/28] scsi: iscsi: Fix completion check during abort races Mike Christie
` (13 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
The iscsi offload drivers are setting the shost->max_id to the max number
of sessions they support. The problem is that max_id is not the max number
of targets but the highest identifier the targets can have. To use it to
limit the number of targets we need to set it to max sessions - 1, or we
can end up with a session we might not have preallocated resources for.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/be2iscsi/be_main.c | 4 ++--
drivers/scsi/bnx2i/bnx2i_iscsi.c | 2 +-
drivers/scsi/cxgbi/libcxgbi.c | 4 ++--
drivers/scsi/qedi/qedi_main.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 27c4f1598f76..d941e1561527 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -416,7 +416,7 @@ static struct beiscsi_hba *beiscsi_hba_alloc(struct pci_dev *pcidev)
"beiscsi_hba_alloc - iscsi_host_alloc failed\n");
return NULL;
}
- shost->max_id = BE2_MAX_SESSIONS;
+ shost->max_id = BE2_MAX_SESSIONS - 1;
shost->max_channel = 0;
shost->max_cmd_len = BEISCSI_MAX_CMD_LEN;
shost->max_lun = BEISCSI_NUM_MAX_LUN;
@@ -5318,7 +5318,7 @@ static int beiscsi_enable_port(struct beiscsi_hba *phba)
/* Re-enable UER. If different TPE occurs then it is recoverable. */
beiscsi_set_uer_feature(phba);
- phba->shost->max_id = phba->params.cxns_per_ctrl;
+ phba->shost->max_id = phba->params.cxns_per_ctrl - 1;
phba->shost->can_queue = phba->params.ios_per_ctrl;
ret = beiscsi_init_port(phba);
if (ret < 0) {
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 26cb1c6536ce..1b5f3e143f07 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -791,7 +791,7 @@ struct bnx2i_hba *bnx2i_alloc_hba(struct cnic_dev *cnic)
return NULL;
shost->dma_boundary = cnic->pcidev->dma_mask;
shost->transportt = bnx2i_scsi_xport_template;
- shost->max_id = ISCSI_MAX_CONNS_PER_HBA;
+ shost->max_id = ISCSI_MAX_CONNS_PER_HBA - 1;
shost->max_channel = 0;
shost->max_lun = 512;
shost->max_cmd_len = 16;
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index dbe22a7136f3..8c7d4dda4cf2 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -337,7 +337,7 @@ void cxgbi_hbas_remove(struct cxgbi_device *cdev)
EXPORT_SYMBOL_GPL(cxgbi_hbas_remove);
int cxgbi_hbas_add(struct cxgbi_device *cdev, u64 max_lun,
- unsigned int max_id, struct scsi_host_template *sht,
+ unsigned int max_conns, struct scsi_host_template *sht,
struct scsi_transport_template *stt)
{
struct cxgbi_hba *chba;
@@ -357,7 +357,7 @@ int cxgbi_hbas_add(struct cxgbi_device *cdev, u64 max_lun,
shost->transportt = stt;
shost->max_lun = max_lun;
- shost->max_id = max_id;
+ shost->max_id = max_conns - 1;
shost->max_channel = 0;
shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 2455d1448a7e..edf915432704 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -640,7 +640,7 @@ static struct qedi_ctx *qedi_host_alloc(struct pci_dev *pdev)
goto exit_setup_shost;
}
- shost->max_id = QEDI_MAX_ISCSI_CONNS_PER_HBA;
+ shost->max_id = QEDI_MAX_ISCSI_CONNS_PER_HBA - 1;
shost->max_channel = 0;
shost->max_lun = ~0;
shost->max_cmd_len = 16;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 15/28] scsi: iscsi: Fix completion check during abort races
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (13 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 14/28] scsi: iscsi: Fix shost->max_id use Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:38 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 16/28] scsi: iscsi: Flush block work before unblock Mike Christie
` (12 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We have a ref to the task being aborted, so SCp.ptr will never be NULL. We
need to use iscsi_task_is_completed to check for the completed state.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 94abb093098d..8222db4f8fef 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2338,7 +2338,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
goto failed_unlocked;
case TMF_NOT_FOUND:
- if (!sc->SCp.ptr) {
+ if (iscsi_task_is_completed(task)) {
session->tmf_state = TMF_INITIAL;
memset(hdr, 0, sizeof(*hdr));
/* task completed before tmf abort response */
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 15/28] scsi: iscsi: Fix completion check during abort races
2021-05-23 17:57 ` [PATCH 15/28] scsi: iscsi: Fix completion check during abort races Mike Christie
@ 2021-05-24 18:38 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:38 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> We have a ref to the task being aborted, so SCp.ptr will never be NULL. We
> need to use iscsi_task_is_completed to check for the completed state.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/libiscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 94abb093098d..8222db4f8fef 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2338,7 +2338,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
> goto failed_unlocked;
> case TMF_NOT_FOUND:
> - if (!sc->SCp.ptr) {
> + if (iscsi_task_is_completed(task)) {
> session->tmf_state = TMF_INITIAL;
> memset(hdr, 0, sizeof(*hdr));
> /* task completed before tmf abort response */
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 16/28] scsi: iscsi: Flush block work before unblock
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (14 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 15/28] scsi: iscsi: Fix completion check during abort races Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:43 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 17/28] scsi: iscsi: Hold task ref during TMF timeout handling Mike Christie
` (11 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We set the max_active iSCSI EH works to 1, so all work is going to execute
in order by default. However, userspace can now override this in sysfs. If
max_active > 1, we can end up with the block_work on CPU1 and
iscsi_unblock_session running the unblock_work on CPU2 and the session
and target/device state will end up out of sync with each other.
This adds a flush of the block_work in iscsi_unblock_session.
Fixes: 1d726aa6ef57 ("scsi: iscsi: Optimize work queue flush use")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 909134b9c313..b07105ae7c91 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1969,6 +1969,8 @@ static void __iscsi_unblock_session(struct work_struct *work)
*/
void iscsi_unblock_session(struct iscsi_cls_session *session)
{
+ flush_work(&session->block_work);
+
queue_work(iscsi_eh_timer_workq, &session->unblock_work);
/*
* Blocking the session can be done from any context so we only
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 16/28] scsi: iscsi: Flush block work before unblock
2021-05-23 17:57 ` [PATCH 16/28] scsi: iscsi: Flush block work before unblock Mike Christie
@ 2021-05-24 18:43 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:43 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> We set the max_active iSCSI EH works to 1, so all work is going to execute
> in order by default. However, userspace can now override this in sysfs. If
> max_active > 1, we can end up with the block_work on CPU1 and
> iscsi_unblock_session running the unblock_work on CPU2 and the session
> and target/device state will end up out of sync with each other.
>
> This adds a flush of the block_work in iscsi_unblock_session.
>
> Fixes: 1d726aa6ef57 ("scsi: iscsi: Optimize work queue flush use")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 909134b9c313..b07105ae7c91 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1969,6 +1969,8 @@ static void __iscsi_unblock_session(struct work_struct *work)
> */
> void iscsi_unblock_session(struct iscsi_cls_session *session)
> {
> + flush_work(&session->block_work);
> +
> queue_work(iscsi_eh_timer_workq, &session->unblock_work);
> /*
> * Blocking the session can be done from any context so we only
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 17/28] scsi: iscsi: Hold task ref during TMF timeout handling
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (15 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 16/28] scsi: iscsi: Flush block work before unblock Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-24 18:45 ` Lee Duncan
2021-05-23 17:57 ` [PATCH 18/28] scsi: iscsi: Move pool freeing Mike Christie
` (10 subsequent siblings)
27 siblings, 1 reply; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
For aborts, qedi needs to cleanup the FW then send the TMF from a worker
thread. While it's doing these the cmd could complete normally and the TMF
could time out. libiscsi would then complete the iscsi_task which will
call into the driver to cleanup the driver level resources while it still
might be accessig them for the cleanup/abort.
This has iscsi_eh_abort keep the iscsi_task ref if the TMF times out, so
qedi does not have to worry about if the task been freed while in use
and does not need to get its own ref.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 15 ++++++++++++++-
include/scsi/libiscsi.h | 1 +
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 8222db4f8fef..9247f70d2daa 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -573,6 +573,11 @@ static bool cleanup_queued_task(struct iscsi_task *task)
__iscsi_put_task(task);
}
+ if (conn->session->running_aborted_task == task) {
+ conn->session->running_aborted_task = NULL;
+ __iscsi_put_task(task);
+ }
+
if (conn->task == task) {
conn->task = NULL;
__iscsi_put_task(task);
@@ -2334,6 +2339,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
iscsi_start_tx(conn);
goto success_unlocked;
case TMF_TIMEDOUT:
+ session->running_aborted_task = task;
spin_unlock_bh(&session->frwd_lock);
iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
goto failed_unlocked;
@@ -2367,7 +2373,14 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
failed_unlocked:
ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
task ? task->itt : 0);
- iscsi_put_task(task);
+ /*
+ * The driver might be accessing the task so hold the ref. The conn
+ * stop cleanup will drop the ref after ep_disconnect so we know the
+ * driver's no longer touching the task.
+ */
+ if (!session->running_aborted_task)
+ iscsi_put_task(task);
+
iscsi_put_conn(conn->cls_conn);
mutex_unlock(&session->eh_mutex);
return FAILED;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 9d7908265afe..4ee233e5a6ff 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -276,6 +276,7 @@ struct iscsi_session {
struct iscsi_tm tmhdr;
struct timer_list tmf_timer;
int tmf_state; /* see TMF_INITIAL, etc.*/
+ struct iscsi_task *running_aborted_task;
/* iSCSI session-wide sequencing */
uint32_t cmdsn;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 17/28] scsi: iscsi: Hold task ref during TMF timeout handling
2021-05-23 17:57 ` [PATCH 17/28] scsi: iscsi: Hold task ref during TMF timeout handling Mike Christie
@ 2021-05-24 18:45 ` Lee Duncan
0 siblings, 0 replies; 37+ messages in thread
From: Lee Duncan @ 2021-05-24 18:45 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/23/21 10:57 AM, Mike Christie wrote:
> For aborts, qedi needs to cleanup the FW then send the TMF from a worker
> thread. While it's doing these the cmd could complete normally and the TMF
> could time out. libiscsi would then complete the iscsi_task which will
> call into the driver to cleanup the driver level resources while it still
> might be accessig them for the cleanup/abort.
>
> This has iscsi_eh_abort keep the iscsi_task ref if the TMF times out, so
> qedi does not have to worry about if the task been freed while in use
typo: "been" -> "being"
> and does not need to get its own ref.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---iscsi_stop_conn
> drivers/scsi/libiscsi.c | 15 ++++++++++++++-
> include/scsi/libiscsi.h | 1 +
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 8222db4f8fef..9247f70d2daa 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -573,6 +573,11 @@ static bool cleanup_queued_task(struct iscsi_task *task)
> __iscsi_put_task(task);
> }
>
> + if (conn->session->running_aborted_task == task) {
> + conn->session->running_aborted_task = NULL;
> + __iscsi_put_task(task);
> + }
> +
> if (conn->task == task) {iscsi_stop_conn
> conn->task = NULL;
> __iscsi_put_task(task);
> @@ -2334,6 +2339,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> iscsi_start_tx(conn);
> goto success_unlocked;
> case TMF_TIMEDOUT:
> + session->running_aborted_task = task;
> spin_unlock_bh(&session->frwd_lock);
> iscsi_conn_failure(conn, ISCSI_ERR_SCSI_EH_SESSION_RST);
> goto failed_unlocked;
> @@ -2367,7 +2373,14 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
> failed_unlocked:
> ISCSI_DBG_EH(session, "abort failed [sc %p itt 0x%x]\n", sc,
> task ? task->itt : 0);
> - iscsi_put_task(task);
> + /*
> + * The driver might be accessing the task so hold the ref. The conn
> + * stop cleanup will drop the ref after ep_disconnect so we know the
> + * driver's no longer touching the task.
> + */
> + if (!session->running_aborted_task)
> + iscsi_put_task(task);
> +
> iscsi_put_conn(conn->cls_conn);
> mutex_unlock(&session->eh_mutex);
> return FAILED;
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index 9d7908265afe..4ee233e5a6ff 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -276,6 +276,7 @@ struct iscsi_session {
> struct iscsi_tm tmhdr;
> struct timer_list tmf_timer;
> int tmf_state; /* see TMF_INITIAL, etc.*/
> + struct iscsi_task *running_aborted_task;
>
> /* iSCSI session-wide sequencing */
> uint32_t cmdsn;
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 18/28] scsi: iscsi: Move pool freeing
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (16 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 17/28] scsi: iscsi: Hold task ref during TMF timeout handling Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 19/28] scsi: qedi: Fix null ref during abort handling Mike Christie
` (9 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
This doesn't fix any bugs, but it makes more sense to free the pool after
we have removed the session. At that time we know nothing is touching any
of the session fields, because all devices have been removed and scans are
stopped.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 9247f70d2daa..baf2a6838c90 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3001,10 +3001,9 @@ void iscsi_session_teardown(struct iscsi_cls_session *cls_session)
struct module *owner = cls_session->transport->owner;
struct Scsi_Host *shost = session->host;
- iscsi_pool_free(&session->cmdpool);
-
iscsi_remove_session(cls_session);
+ iscsi_pool_free(&session->cmdpool);
kfree(session->password);
kfree(session->password_in);
kfree(session->username);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 19/28] scsi: qedi: Fix null ref during abort handling
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (17 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 18/28] scsi: iscsi: Move pool freeing Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 20/28] scsi: qedi: Fix race during abort timeouts Mike Christie
` (8 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If qedi_process_cmd_cleanup_resp finds the cmd it frees the work and sets
list_tmf_work to NULL, so qedi_tmf_work should check if list_tmf_work is
non-NULL when it wants to force cleanup.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index 440ddd2309f1..cf57b4e49700 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -1453,7 +1453,7 @@ static void qedi_tmf_work(struct work_struct *work)
ldel_exit:
spin_lock_bh(&qedi_conn->tmf_work_lock);
- if (!qedi_cmd->list_tmf_work) {
+ if (qedi_cmd->list_tmf_work) {
list_del_init(&list_work->list);
qedi_cmd->list_tmf_work = NULL;
kfree(list_work);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 20/28] scsi: qedi: Fix race during abort timeouts
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (18 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 19/28] scsi: qedi: Fix null ref during abort handling Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 21/28] scsi: qedi: Fix use after free during abort cleanup Mike Christie
` (7 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If the SCSI cmd completes after qedi_tmf_work calls iscsi_itt_to_task then
the qedi qedi_cmd->task_id could be freed and used for another cmd. If we
then call qedi_iscsi_cleanup_task with that task_id we will be cleaning up
the wrong cmd.
This patch has us wait to release the task_id until the last put has been
done on the iscsi_task. Because libiscsi grabs a ref to the task when
sending the abort, we know that for the non abort timeout case that the
task_id we are referencing is for the cmd that was supposed to be aborted.
The next patch will fix the case where the abort timesout while we are
running qedi_tmf_work.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 15 ---------------
drivers/scsi/qedi/qedi_iscsi.c | 20 +++++++++++++++++---
2 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index cf57b4e49700..c12bb2dd5ff9 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -73,7 +73,6 @@ static void qedi_process_logout_resp(struct qedi_ctx *qedi,
spin_unlock(&qedi_conn->list_lock);
cmd->state = RESPONSE_RECEIVED;
- qedi_clear_task_idx(qedi, cmd->task_id);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr, NULL, 0);
spin_unlock(&session->back_lock);
@@ -138,7 +137,6 @@ static void qedi_process_text_resp(struct qedi_ctx *qedi,
spin_unlock(&qedi_conn->list_lock);
cmd->state = RESPONSE_RECEIVED;
- qedi_clear_task_idx(qedi, cmd->task_id);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr,
qedi_conn->gen_pdu.resp_buf,
@@ -164,13 +162,11 @@ static void qedi_tmf_resp_work(struct work_struct *work)
iscsi_block_session(session->cls_session);
rval = qedi_cleanup_all_io(qedi, qedi_conn, qedi_cmd->task, true);
if (rval) {
- qedi_clear_task_idx(qedi, qedi_cmd->task_id);
iscsi_unblock_session(session->cls_session);
goto exit_tmf_resp;
}
iscsi_unblock_session(session->cls_session);
- qedi_clear_task_idx(qedi, qedi_cmd->task_id);
spin_lock(&session->back_lock);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);
@@ -245,8 +241,6 @@ static void qedi_process_tmf_resp(struct qedi_ctx *qedi,
goto unblock_sess;
}
- qedi_clear_task_idx(qedi, qedi_cmd->task_id);
-
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);
kfree(resp_hdr_ptr);
@@ -314,7 +308,6 @@ static void qedi_process_login_resp(struct qedi_ctx *qedi,
"Freeing tid=0x%x for cid=0x%x\n",
cmd->task_id, qedi_conn->iscsi_conn_id);
cmd->state = RESPONSE_RECEIVED;
- qedi_clear_task_idx(qedi, cmd->task_id);
}
static void qedi_get_rq_bdq_buf(struct qedi_ctx *qedi,
@@ -468,7 +461,6 @@ static int qedi_process_nopin_mesg(struct qedi_ctx *qedi,
}
spin_unlock(&qedi_conn->list_lock);
- qedi_clear_task_idx(qedi, cmd->task_id);
}
done:
@@ -673,7 +665,6 @@ static void qedi_scsi_completion(struct qedi_ctx *qedi,
if (qedi_io_tracing)
qedi_trace_io(qedi, task, cmd->task_id, QEDI_IO_TRACE_RSP);
- qedi_clear_task_idx(qedi, cmd->task_id);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr,
conn->data, datalen);
error:
@@ -730,7 +721,6 @@ static void qedi_process_nopin_local_cmpl(struct qedi_ctx *qedi,
cqe->itid, cmd->task_id);
cmd->state = RESPONSE_RECEIVED;
- qedi_clear_task_idx(qedi, cmd->task_id);
spin_lock_bh(&session->back_lock);
__iscsi_put_task(task);
@@ -748,7 +738,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
itt_t protoitt = 0;
int found = 0;
struct qedi_cmd *qedi_cmd = NULL;
- u32 rtid = 0;
u32 iscsi_cid;
struct qedi_conn *qedi_conn;
struct qedi_cmd *dbg_cmd;
@@ -779,7 +768,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
found = 1;
mtask = qedi_cmd->task;
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
- rtid = work->rtid;
list_del_init(&work->list);
kfree(work);
@@ -821,8 +809,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
if (qedi_cmd->state == CLEANUP_WAIT_FAILED)
qedi_cmd->state = CLEANUP_RECV;
- qedi_clear_task_idx(qedi_conn->qedi, rtid);
-
spin_lock(&qedi_conn->list_lock);
if (likely(dbg_cmd->io_cmd_in_list)) {
dbg_cmd->io_cmd_in_list = false;
@@ -856,7 +842,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_TID,
"Freeing tid=0x%x for cid=0x%x\n",
cqe->itid, qedi_conn->iscsi_conn_id);
- qedi_clear_task_idx(qedi_conn->qedi, cqe->itid);
} else {
qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 80f8d35b5900..5304a028db0a 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -783,7 +783,6 @@ static int qedi_mtask_xmit(struct iscsi_conn *conn, struct iscsi_task *task)
}
cmd->conn = conn->dd_data;
- cmd->scsi_cmd = NULL;
return qedi_iscsi_send_generic_request(task);
}
@@ -794,6 +793,10 @@ static int qedi_task_xmit(struct iscsi_task *task)
struct qedi_cmd *cmd = task->dd_data;
struct scsi_cmnd *sc = task->sc;
+ /* Clear now so in cleanup_task we know it didn't make it */
+ cmd->scsi_cmd = NULL;
+ cmd->task_id = U16_MAX;
+
if (test_bit(QEDI_IN_SHUTDOWN, &qedi_conn->qedi->flags))
return -ENODEV;
@@ -1391,13 +1394,24 @@ static umode_t qedi_attr_is_visible(int param_type, int param)
static void qedi_cleanup_task(struct iscsi_task *task)
{
- if (!task->sc || task->state == ISCSI_TASK_PENDING) {
+ struct qedi_cmd *cmd;
+
+ if (task->state == ISCSI_TASK_PENDING) {
QEDI_INFO(NULL, QEDI_LOG_IO, "Returning ref_cnt=%d\n",
refcount_read(&task->refcount));
return;
}
- qedi_iscsi_unmap_sg_list(task->dd_data);
+ if (task->sc)
+ qedi_iscsi_unmap_sg_list(task->dd_data);
+
+ cmd = task->dd_data;
+ if (cmd->task_id != U16_MAX)
+ qedi_clear_task_idx(iscsi_host_priv(task->conn->session->host),
+ cmd->task_id);
+
+ cmd->task_id = U16_MAX;
+ cmd->scsi_cmd = NULL;
}
struct iscsi_transport qedi_iscsi_transport = {
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 21/28] scsi: qedi: Fix use after free during abort cleanup
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (19 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 20/28] scsi: qedi: Fix race during abort timeouts Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 22/28] scsi: qedi: Fix TMF tid allocation Mike Christie
` (6 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If qedi_tmf_work's qedi_wait_for_cleanup_request call times out we will
also force the clean up of the qedi_work_map but
qedi_process_cmd_cleanup_resp could still be accessing the qedi_cmd.
To fix this issue we extend where we hold the tmf_work_lock and back_lock
so the qedi_process_cmd_cleanup_resp access is serialized with the cleanup
done in qedi_tmf_work and any completion handling for the iscsi_task.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 113 ++++++++++++++++++---------------
drivers/scsi/qedi/qedi_iscsi.h | 1 +
2 files changed, 63 insertions(+), 51 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index c12bb2dd5ff9..61c1ebb3004c 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -729,7 +729,6 @@ static void qedi_process_nopin_local_cmpl(struct qedi_ctx *qedi,
static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
struct iscsi_cqe_solicited *cqe,
- struct iscsi_task *task,
struct iscsi_conn *conn)
{
struct qedi_work_map *work, *work_tmp;
@@ -741,7 +740,7 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
u32 iscsi_cid;
struct qedi_conn *qedi_conn;
struct qedi_cmd *dbg_cmd;
- struct iscsi_task *mtask;
+ struct iscsi_task *mtask, *task;
struct iscsi_tm *tmf_hdr = NULL;
iscsi_cid = cqe->conn_id;
@@ -767,6 +766,7 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
}
found = 1;
mtask = qedi_cmd->task;
+ task = work->ctask;
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
list_del_init(&work->list);
@@ -774,52 +774,47 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
qedi_cmd->list_tmf_work = NULL;
}
}
- spin_unlock_bh(&qedi_conn->tmf_work_lock);
-
- if (found) {
- QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
- "TMF work, cqe->tid=0x%x, tmf flags=0x%x, cid=0x%x\n",
- proto_itt, tmf_hdr->flags, qedi_conn->iscsi_conn_id);
-
- if ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_ABORT_TASK) {
- spin_lock_bh(&conn->session->back_lock);
-
- protoitt = build_itt(get_itt(tmf_hdr->rtt),
- conn->session->age);
- task = iscsi_itt_to_task(conn, protoitt);
- spin_unlock_bh(&conn->session->back_lock);
+ if (!found) {
+ spin_unlock_bh(&qedi_conn->tmf_work_lock);
+ goto check_cleanup_reqs;
+ }
- if (!task) {
- QEDI_NOTICE(&qedi->dbg_ctx,
- "IO task completed, tmf rtt=0x%x, cid=0x%x\n",
- get_itt(tmf_hdr->rtt),
- qedi_conn->iscsi_conn_id);
- return;
- }
+ QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
+ "TMF work, cqe->tid=0x%x, tmf flags=0x%x, cid=0x%x\n",
+ proto_itt, tmf_hdr->flags, qedi_conn->iscsi_conn_id);
+
+ spin_lock_bh(&conn->session->back_lock);
+ if (iscsi_task_is_completed(task)) {
+ QEDI_NOTICE(&qedi->dbg_ctx,
+ "IO task completed, tmf rtt=0x%x, cid=0x%x\n",
+ get_itt(tmf_hdr->rtt), qedi_conn->iscsi_conn_id);
+ goto unlock;
+ }
- dbg_cmd = task->dd_data;
+ dbg_cmd = task->dd_data;
- QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
- "Abort tmf rtt=0x%x, i/o itt=0x%x, i/o tid=0x%x, cid=0x%x\n",
- get_itt(tmf_hdr->rtt), get_itt(task->itt),
- dbg_cmd->task_id, qedi_conn->iscsi_conn_id);
+ QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
+ "Abort tmf rtt=0x%x, i/o itt=0x%x, i/o tid=0x%x, cid=0x%x\n",
+ get_itt(tmf_hdr->rtt), get_itt(task->itt), dbg_cmd->task_id,
+ qedi_conn->iscsi_conn_id);
- if (qedi_cmd->state == CLEANUP_WAIT_FAILED)
- qedi_cmd->state = CLEANUP_RECV;
+ spin_lock(&qedi_conn->list_lock);
+ if (likely(dbg_cmd->io_cmd_in_list)) {
+ dbg_cmd->io_cmd_in_list = false;
+ list_del_init(&dbg_cmd->io_cmd);
+ qedi_conn->active_cmd_count--;
+ }
+ spin_unlock(&qedi_conn->list_lock);
+ qedi_cmd->state = CLEANUP_RECV;
+unlock:
+ spin_unlock_bh(&conn->session->back_lock);
+ spin_unlock_bh(&qedi_conn->tmf_work_lock);
+ wake_up_interruptible(&qedi_conn->wait_queue);
+ return;
- spin_lock(&qedi_conn->list_lock);
- if (likely(dbg_cmd->io_cmd_in_list)) {
- dbg_cmd->io_cmd_in_list = false;
- list_del_init(&dbg_cmd->io_cmd);
- qedi_conn->active_cmd_count--;
- }
- spin_unlock(&qedi_conn->list_lock);
- qedi_cmd->state = CLEANUP_RECV;
- wake_up_interruptible(&qedi_conn->wait_queue);
- }
- } else if (qedi_conn->cmd_cleanup_req > 0) {
+check_cleanup_reqs:
+ if (qedi_conn->cmd_cleanup_req > 0) {
spin_lock_bh(&conn->session->back_lock);
qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);
protoitt = build_itt(ptmp_itt, conn->session->age);
@@ -842,6 +837,7 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_TID,
"Freeing tid=0x%x for cid=0x%x\n",
cqe->itid, qedi_conn->iscsi_conn_id);
+ qedi_clear_task_idx(qedi_conn->qedi, cqe->itid);
} else {
qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);
@@ -944,8 +940,7 @@ void qedi_fp_process_cqes(struct qedi_work *work)
goto exit_fp_process;
case ISCSI_CQE_TYPE_TASK_CLEANUP:
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM, "CleanUp CqE\n");
- qedi_process_cmd_cleanup_resp(qedi, &cqe->cqe_solicited, task,
- conn);
+ qedi_process_cmd_cleanup_resp(qedi, &cqe->cqe_solicited, conn);
goto exit_fp_process;
default:
QEDI_ERR(&qedi->dbg_ctx, "Error cqe.\n");
@@ -1372,11 +1367,25 @@ static void qedi_tmf_work(struct work_struct *work)
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
set_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
- ctask = iscsi_itt_to_task(conn, tmf_hdr->rtt);
- if (!ctask || !ctask->sc) {
- QEDI_ERR(&qedi->dbg_ctx, "Task already completed\n");
- goto abort_ret;
+ spin_lock_bh(&conn->session->back_lock);
+ ctask = iscsi_itt_to_ctask(conn, tmf_hdr->rtt);
+ if (!ctask) {
+ spin_unlock_bh(&conn->session->back_lock);
+ QEDI_ERR(&qedi->dbg_ctx, "Invalid RTT. Letting abort timeout.\n");
+ goto clear_cleanup;
+ }
+
+ if (iscsi_task_is_completed(ctask)) {
+ spin_unlock_bh(&conn->session->back_lock);
+ QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
+ "Task already completed\n");
+ /*
+ * We have to still send the TMF because libiscsi needs the
+ * response to avoid a timeout.
+ */
+ goto send_tmf;
}
+ spin_unlock_bh(&conn->session->back_lock);
cmd = (struct qedi_cmd *)ctask->dd_data;
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
@@ -1387,19 +1396,20 @@ static void qedi_tmf_work(struct work_struct *work)
if (qedi_do_not_recover) {
QEDI_ERR(&qedi->dbg_ctx, "DONT SEND CLEANUP/ABORT %d\n",
qedi_do_not_recover);
- goto abort_ret;
+ goto clear_cleanup;
}
list_work = kzalloc(sizeof(*list_work), GFP_ATOMIC);
if (!list_work) {
QEDI_ERR(&qedi->dbg_ctx, "Memory allocation failed\n");
- goto abort_ret;
+ goto clear_cleanup;
}
qedi_cmd->type = TYPEIO;
list_work->qedi_cmd = qedi_cmd;
list_work->rtid = cmd->task_id;
list_work->state = QEDI_WORK_SCHEDULED;
+ list_work->ctask = ctask;
qedi_cmd->list_tmf_work = list_work;
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
@@ -1422,6 +1432,7 @@ static void qedi_tmf_work(struct work_struct *work)
goto ldel_exit;
}
+send_tmf:
tid = qedi_get_task_idx(qedi);
if (tid == -1) {
QEDI_ERR(&qedi->dbg_ctx, "Invalid tid, cid=0x%x\n",
@@ -1432,7 +1443,7 @@ static void qedi_tmf_work(struct work_struct *work)
qedi_cmd->task_id = tid;
qedi_send_iscsi_tmf(qedi_conn, qedi_cmd->task);
-abort_ret:
+clear_cleanup:
clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
return;
diff --git a/drivers/scsi/qedi/qedi_iscsi.h b/drivers/scsi/qedi/qedi_iscsi.h
index 39dc27c85e3c..68ef519f5480 100644
--- a/drivers/scsi/qedi/qedi_iscsi.h
+++ b/drivers/scsi/qedi/qedi_iscsi.h
@@ -212,6 +212,7 @@ struct qedi_cmd {
struct qedi_work_map {
struct list_head list;
struct qedi_cmd *qedi_cmd;
+ struct iscsi_task *ctask;
int rtid;
int state;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 22/28] scsi: qedi: Fix TMF tid allocation
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (20 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 21/28] scsi: qedi: Fix use after free during abort cleanup Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 23/28] scsi: qedi: Use GFP_NOIO for TMF allocation Mike Christie
` (5 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
qedi_iscsi_abort_work and qedi_tmf_work allocates a tid then calls
qedi_send_iscsi_tmf which also allcoates a tid. This removes the tid
allocation from the callers.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 76 ++++++++++------------------------
drivers/scsi/qedi/qedi_gbl.h | 3 +-
drivers/scsi/qedi/qedi_iscsi.c | 2 +-
3 files changed, 25 insertions(+), 56 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index 61c1ebb3004c..6812dc023def 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -14,8 +14,8 @@
#include "qedi_fw_iscsi.h"
#include "qedi_fw_scsi.h"
-static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
- struct iscsi_task *mtask);
+static int send_iscsi_tmf(struct qedi_conn *qedi_conn,
+ struct iscsi_task *mtask);
void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
{
@@ -1348,7 +1348,7 @@ static int qedi_wait_for_cleanup_request(struct qedi_ctx *qedi,
return 0;
}
-static void qedi_tmf_work(struct work_struct *work)
+static void qedi_abort_work(struct work_struct *work)
{
struct qedi_cmd *qedi_cmd =
container_of(work, struct qedi_cmd, tmf_work);
@@ -1361,7 +1361,6 @@ static void qedi_tmf_work(struct work_struct *work)
struct iscsi_task *ctask;
struct iscsi_tm *tmf_hdr;
s16 rval = 0;
- s16 tid = 0;
mtask = qedi_cmd->task;
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
@@ -1406,6 +1405,7 @@ static void qedi_tmf_work(struct work_struct *work)
}
qedi_cmd->type = TYPEIO;
+ qedi_cmd->state = CLEANUP_WAIT;
list_work->qedi_cmd = qedi_cmd;
list_work->rtid = cmd->task_id;
list_work->state = QEDI_WORK_SCHEDULED;
@@ -1433,15 +1433,7 @@ static void qedi_tmf_work(struct work_struct *work)
}
send_tmf:
- tid = qedi_get_task_idx(qedi);
- if (tid == -1) {
- QEDI_ERR(&qedi->dbg_ctx, "Invalid tid, cid=0x%x\n",
- qedi_conn->iscsi_conn_id);
- goto ldel_exit;
- }
-
- qedi_cmd->task_id = tid;
- qedi_send_iscsi_tmf(qedi_conn, qedi_cmd->task);
+ send_iscsi_tmf(qedi_conn, qedi_cmd->task);
clear_cleanup:
clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
@@ -1467,8 +1459,7 @@ static void qedi_tmf_work(struct work_struct *work)
clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
}
-static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
- struct iscsi_task *mtask)
+static int send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
{
struct iscsi_tmf_request_hdr tmf_pdu_header;
struct iscsi_task_params task_params;
@@ -1483,7 +1474,6 @@ static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
u32 scsi_lun[2];
s16 tid = 0;
u16 sq_idx = 0;
- int rval = 0;
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
qedi_cmd = (struct qedi_cmd *)mtask->dd_data;
@@ -1547,10 +1537,7 @@ static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
task_params.sqe = &ep->sq[sq_idx];
memset(task_params.sqe, 0, sizeof(struct iscsi_wqe));
- rval = init_initiator_tmf_request_task(&task_params,
- &tmf_pdu_header);
- if (rval)
- return -1;
+ init_initiator_tmf_request_task(&task_params, &tmf_pdu_header);
spin_lock(&qedi_conn->list_lock);
list_add_tail(&qedi_cmd->io_cmd, &qedi_conn->active_cmd_list);
@@ -1562,47 +1549,30 @@ static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
return 0;
}
-int qedi_iscsi_abort_work(struct qedi_conn *qedi_conn,
- struct iscsi_task *mtask)
+int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
{
+ struct iscsi_tm *tmf_hdr = (struct iscsi_tm *)mtask->hdr;
+ struct qedi_cmd *qedi_cmd = mtask->dd_data;
struct qedi_ctx *qedi = qedi_conn->qedi;
- struct iscsi_tm *tmf_hdr;
- struct qedi_cmd *qedi_cmd = (struct qedi_cmd *)mtask->dd_data;
- s16 tid = 0;
+ int rc = 0;
- tmf_hdr = (struct iscsi_tm *)mtask->hdr;
- qedi_cmd->task = mtask;
-
- /* If abort task then schedule the work and return */
- if ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_ABORT_TASK) {
- qedi_cmd->state = CLEANUP_WAIT;
- INIT_WORK(&qedi_cmd->tmf_work, qedi_tmf_work);
+ switch (tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) {
+ case ISCSI_TM_FUNC_ABORT_TASK:
+ INIT_WORK(&qedi_cmd->tmf_work, qedi_abort_work);
queue_work(qedi->tmf_thread, &qedi_cmd->tmf_work);
-
- } else if (((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_LOGICAL_UNIT_RESET) ||
- ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_TARGET_WARM_RESET) ||
- ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_TARGET_COLD_RESET)) {
- tid = qedi_get_task_idx(qedi);
- if (tid == -1) {
- QEDI_ERR(&qedi->dbg_ctx, "Invalid tid, cid=0x%x\n",
- qedi_conn->iscsi_conn_id);
- return -1;
- }
- qedi_cmd->task_id = tid;
-
- qedi_send_iscsi_tmf(qedi_conn, qedi_cmd->task);
-
- } else {
+ break;
+ case ISCSI_TM_FUNC_LOGICAL_UNIT_RESET:
+ case ISCSI_TM_FUNC_TARGET_WARM_RESET:
+ case ISCSI_TM_FUNC_TARGET_COLD_RESET:
+ rc = send_iscsi_tmf(qedi_conn, mtask);
+ break;
+ default:
QEDI_ERR(&qedi->dbg_ctx, "Invalid tmf, cid=0x%x\n",
qedi_conn->iscsi_conn_id);
- return -1;
+ return -EINVAL;
}
- return 0;
+ return rc;
}
int qedi_send_iscsi_text(struct qedi_conn *qedi_conn,
diff --git a/drivers/scsi/qedi/qedi_gbl.h b/drivers/scsi/qedi/qedi_gbl.h
index 116645c08c71..fb44a282613e 100644
--- a/drivers/scsi/qedi/qedi_gbl.h
+++ b/drivers/scsi/qedi/qedi_gbl.h
@@ -31,8 +31,7 @@ int qedi_send_iscsi_login(struct qedi_conn *qedi_conn,
struct iscsi_task *task);
int qedi_send_iscsi_logout(struct qedi_conn *qedi_conn,
struct iscsi_task *task);
-int qedi_iscsi_abort_work(struct qedi_conn *qedi_conn,
- struct iscsi_task *mtask);
+int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask);
int qedi_send_iscsi_text(struct qedi_conn *qedi_conn,
struct iscsi_task *task);
int qedi_send_iscsi_nopout(struct qedi_conn *qedi_conn,
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 5304a028db0a..0ece2c3b105b 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -753,7 +753,7 @@ static int qedi_iscsi_send_generic_request(struct iscsi_task *task)
rc = qedi_send_iscsi_logout(qedi_conn, task);
break;
case ISCSI_OP_SCSI_TMFUNC:
- rc = qedi_iscsi_abort_work(qedi_conn, task);
+ rc = qedi_send_iscsi_tmf(qedi_conn, task);
break;
case ISCSI_OP_TEXT:
rc = qedi_send_iscsi_text(qedi_conn, task);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 23/28] scsi: qedi: Use GFP_NOIO for TMF allocation
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (21 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 22/28] scsi: qedi: Fix TMF tid allocation Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 24/28] scsi: qedi: Fix TMF session block/unblock use Mike Christie
` (4 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We run from a workqueue with no locks held so use GFP_NOIO.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index 6812dc023def..e82c68f660f8 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -1398,7 +1398,7 @@ static void qedi_abort_work(struct work_struct *work)
goto clear_cleanup;
}
- list_work = kzalloc(sizeof(*list_work), GFP_ATOMIC);
+ list_work = kzalloc(sizeof(*list_work), GFP_NOIO);
if (!list_work) {
QEDI_ERR(&qedi->dbg_ctx, "Memory allocation failed\n");
goto clear_cleanup;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 24/28] scsi: qedi: Fix TMF session block/unblock use
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (22 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 23/28] scsi: qedi: Use GFP_NOIO for TMF allocation Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 25/28] scsi: qedi: Fix cleanup " Mike Christie
` (3 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Drivers shouldn't be calling block/unblock session for tmf handling
because the functions can change the session state from under libiscsi.
iscsi_queuecommand's call to iscsi_prep_scsi_cmd_pdu->
iscsi_check_tmf_restrictions will prevent new cmds from being sent to qedi
after we've started handling a TMF. So we don't need to try and block it
in the driver, and we can remove these block calls.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index e82c68f660f8..f08fe967bcfe 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -159,14 +159,9 @@ static void qedi_tmf_resp_work(struct work_struct *work)
set_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
resp_hdr_ptr = (struct iscsi_tm_rsp *)qedi_cmd->tmf_resp_buf;
- iscsi_block_session(session->cls_session);
rval = qedi_cleanup_all_io(qedi, qedi_conn, qedi_cmd->task, true);
- if (rval) {
- iscsi_unblock_session(session->cls_session);
+ if (rval)
goto exit_tmf_resp;
- }
-
- iscsi_unblock_session(session->cls_session);
spin_lock(&session->back_lock);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 25/28] scsi: qedi: Fix cleanup session block/unblock use
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (23 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 24/28] scsi: qedi: Fix TMF session block/unblock use Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 26/28] scsi: qedi: Pass send_iscsi_tmf task to abort Mike Christie
` (2 subsequent siblings)
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Drivers shouldn't be calling block/unblock session for cmd cleanup
because the functions can change the session state from under libiscsi.
This adds a new a driver level bit so it can block all IO the host while
it drains the card.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi.h | 1 +
drivers/scsi/qedi/qedi_iscsi.c | 17 +++++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qedi/qedi.h b/drivers/scsi/qedi/qedi.h
index c342defc3f52..ce199a7a16b8 100644
--- a/drivers/scsi/qedi/qedi.h
+++ b/drivers/scsi/qedi/qedi.h
@@ -284,6 +284,7 @@ struct qedi_ctx {
#define QEDI_IN_RECOVERY 5
#define QEDI_IN_OFFLINE 6
#define QEDI_IN_SHUTDOWN 7
+#define QEDI_BLOCK_IO 8
u8 mac[ETH_ALEN];
u32 src_ip[4];
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 0ece2c3b105b..ddb47784eb4a 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -330,12 +330,22 @@ qedi_conn_create(struct iscsi_cls_session *cls_session, uint32_t cid)
void qedi_mark_device_missing(struct iscsi_cls_session *cls_session)
{
- iscsi_block_session(cls_session);
+ struct iscsi_session *session = cls_session->dd_data;
+ struct qedi_conn *qedi_conn = session->leadconn->dd_data;
+
+ spin_lock_bh(&session->frwd_lock);
+ set_bit(QEDI_BLOCK_IO, &qedi_conn->qedi->flags);
+ spin_unlock_bh(&session->frwd_lock);
}
void qedi_mark_device_available(struct iscsi_cls_session *cls_session)
{
- iscsi_unblock_session(cls_session);
+ struct iscsi_session *session = cls_session->dd_data;
+ struct qedi_conn *qedi_conn = session->leadconn->dd_data;
+
+ spin_lock_bh(&session->frwd_lock);
+ clear_bit(QEDI_BLOCK_IO, &qedi_conn->qedi->flags);
+ spin_unlock_bh(&session->frwd_lock);
}
static int qedi_bind_conn_to_iscsi_cid(struct qedi_ctx *qedi,
@@ -800,6 +810,9 @@ static int qedi_task_xmit(struct iscsi_task *task)
if (test_bit(QEDI_IN_SHUTDOWN, &qedi_conn->qedi->flags))
return -ENODEV;
+ if (test_bit(QEDI_BLOCK_IO, &qedi_conn->qedi->flags))
+ return -EACCES;
+
cmd->state = 0;
cmd->task = NULL;
cmd->use_slowpath = false;
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 26/28] scsi: qedi: Pass send_iscsi_tmf task to abort
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (24 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 25/28] scsi: qedi: Fix cleanup " Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 27/28] scsi: qedi: Complete TMF works before disconnect Mike Christie
2021-05-23 17:57 ` [PATCH 28/28] scsi: qedi: Wake up if cmd_cleanup_req is set Mike Christie
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
qedi_abort_work knows what task to abort so just pass it to send_iscsi_tmf.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index f08fe967bcfe..4ee1ce1dcdef 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -15,7 +15,7 @@
#include "qedi_fw_scsi.h"
static int send_iscsi_tmf(struct qedi_conn *qedi_conn,
- struct iscsi_task *mtask);
+ struct iscsi_task *mtask, struct iscsi_task *ctask);
void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
{
@@ -1428,7 +1428,7 @@ static void qedi_abort_work(struct work_struct *work)
}
send_tmf:
- send_iscsi_tmf(qedi_conn, qedi_cmd->task);
+ send_iscsi_tmf(qedi_conn, qedi_cmd->task, ctask);
clear_cleanup:
clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
@@ -1454,14 +1454,13 @@ static void qedi_abort_work(struct work_struct *work)
clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
}
-static int send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
+static int send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask,
+ struct iscsi_task *ctask)
{
struct iscsi_tmf_request_hdr tmf_pdu_header;
struct iscsi_task_params task_params;
struct qedi_ctx *qedi = qedi_conn->qedi;
struct e4_iscsi_task_context *fw_task_ctx;
- struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
- struct iscsi_task *ctask;
struct iscsi_tm *tmf_hdr;
struct qedi_cmd *qedi_cmd;
struct qedi_cmd *cmd;
@@ -1501,12 +1500,6 @@ static int send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
if ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
ISCSI_TM_FUNC_ABORT_TASK) {
- ctask = iscsi_itt_to_task(conn, tmf_hdr->rtt);
- if (!ctask || !ctask->sc) {
- QEDI_ERR(&qedi->dbg_ctx,
- "Could not get reference task\n");
- return 0;
- }
cmd = (struct qedi_cmd *)ctask->dd_data;
tmf_pdu_header.rtt =
qedi_set_itt(cmd->task_id,
@@ -1559,7 +1552,7 @@ int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
case ISCSI_TM_FUNC_LOGICAL_UNIT_RESET:
case ISCSI_TM_FUNC_TARGET_WARM_RESET:
case ISCSI_TM_FUNC_TARGET_COLD_RESET:
- rc = send_iscsi_tmf(qedi_conn, mtask);
+ rc = send_iscsi_tmf(qedi_conn, mtask, NULL);
break;
default:
QEDI_ERR(&qedi->dbg_ctx, "Invalid tmf, cid=0x%x\n",
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 27/28] scsi: qedi: Complete TMF works before disconnect
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (25 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 26/28] scsi: qedi: Pass send_iscsi_tmf task to abort Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
2021-05-23 17:57 ` [PATCH 28/28] scsi: qedi: Wake up if cmd_cleanup_req is set Mike Christie
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We need to make sure that abort and reset completion work has completed
before ep_disconnect returns. After ep_disconnect we can't manipulate
cmds because libiscsi will call conn_stop and take onwership.
We are trying to make sure abort work and reset completion work has
completed before we do the cmd clean up in ep_disconnect. The problem is
that:
1. the work function sets the QEDI_CONN_FW_CLEANUP bit, so if the work was
still pending we would not see the bit set. We need to do this before the
work is queued.
2. If we had multiple works queued then we could break from the loop in
qedi_ep_disconnect early because when abort work 1 completes it could
clear QEDI_CONN_FW_CLEANUP. qedi_ep_disconnect could then see that before
work 2 has run.
3. A TMF reset completion work could run after ep_disconnect starts
cleaning up cmds via qedi_clearsq. ep_disconnect's call to qedi_clearsq
-> qedi_cleanup_all_io would might think it's done cleaning up cmds, but
the reset completion work could still be running. We then return from
ep_disconnect while still doing cleanup.
This replaces the bit with a counter to track the number of queued TMF
works, and adds a bool to prevent new works from starting from the
completion path once a ep_disconnect starts.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 42 ++++++++++++++++++++++------------
drivers/scsi/qedi/qedi_iscsi.c | 23 +++++++++++++------
drivers/scsi/qedi/qedi_iscsi.h | 4 ++--
3 files changed, 46 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index 4ee1ce1dcdef..3de1422ce80b 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -156,7 +156,6 @@ static void qedi_tmf_resp_work(struct work_struct *work)
struct iscsi_tm_rsp *resp_hdr_ptr;
int rval = 0;
- set_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
resp_hdr_ptr = (struct iscsi_tm_rsp *)qedi_cmd->tmf_resp_buf;
rval = qedi_cleanup_all_io(qedi, qedi_conn, qedi_cmd->task, true);
@@ -169,7 +168,10 @@ static void qedi_tmf_resp_work(struct work_struct *work)
exit_tmf_resp:
kfree(resp_hdr_ptr);
- clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
+
+ spin_lock(&qedi_conn->tmf_work_lock);
+ qedi_conn->fw_cleanup_works--;
+ spin_unlock(&qedi_conn->tmf_work_lock);
}
static void qedi_process_tmf_resp(struct qedi_ctx *qedi,
@@ -225,16 +227,25 @@ static void qedi_process_tmf_resp(struct qedi_ctx *qedi,
}
spin_unlock(&qedi_conn->list_lock);
- if (((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_LOGICAL_UNIT_RESET) ||
- ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_TARGET_WARM_RESET) ||
- ((tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) ==
- ISCSI_TM_FUNC_TARGET_COLD_RESET)) {
+ spin_lock(&qedi_conn->tmf_work_lock);
+ switch (tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) {
+ case ISCSI_TM_FUNC_LOGICAL_UNIT_RESET:
+ case ISCSI_TM_FUNC_TARGET_WARM_RESET:
+ case ISCSI_TM_FUNC_TARGET_COLD_RESET:
+ if (qedi_conn->ep_disconnect_starting) {
+ /* Session is down so ep_disconnect will clean up */
+ spin_unlock(&qedi_conn->tmf_work_lock);
+ goto unblock_sess;
+ }
+
+ qedi_conn->fw_cleanup_works++;
+ spin_unlock(&qedi_conn->tmf_work_lock);
+
INIT_WORK(&qedi_cmd->tmf_work, qedi_tmf_resp_work);
queue_work(qedi->tmf_thread, &qedi_cmd->tmf_work);
goto unblock_sess;
}
+ spin_unlock(&qedi_conn->tmf_work_lock);
__iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr_ptr, NULL, 0);
kfree(resp_hdr_ptr);
@@ -1359,7 +1370,6 @@ static void qedi_abort_work(struct work_struct *work)
mtask = qedi_cmd->task;
tmf_hdr = (struct iscsi_tm *)mtask->hdr;
- set_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
spin_lock_bh(&conn->session->back_lock);
ctask = iscsi_itt_to_ctask(conn, tmf_hdr->rtt);
@@ -1429,10 +1439,7 @@ static void qedi_abort_work(struct work_struct *work)
send_tmf:
send_iscsi_tmf(qedi_conn, qedi_cmd->task, ctask);
-
-clear_cleanup:
- clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
- return;
+ goto clear_cleanup;
ldel_exit:
spin_lock_bh(&qedi_conn->tmf_work_lock);
@@ -1451,7 +1458,10 @@ static void qedi_abort_work(struct work_struct *work)
}
spin_unlock(&qedi_conn->list_lock);
- clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
+clear_cleanup:
+ spin_lock(&qedi_conn->tmf_work_lock);
+ qedi_conn->fw_cleanup_works--;
+ spin_unlock(&qedi_conn->tmf_work_lock);
}
static int send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask,
@@ -1546,6 +1556,10 @@ int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn, struct iscsi_task *mtask)
switch (tmf_hdr->flags & ISCSI_FLAG_TM_FUNC_MASK) {
case ISCSI_TM_FUNC_ABORT_TASK:
+ spin_lock(&qedi_conn->tmf_work_lock);
+ qedi_conn->fw_cleanup_works++;
+ spin_unlock(&qedi_conn->tmf_work_lock);
+
INIT_WORK(&qedi_cmd->tmf_work, qedi_abort_work);
queue_work(qedi->tmf_thread, &qedi_cmd->tmf_work);
break;
diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index ddb47784eb4a..bf581ecea897 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -603,7 +603,11 @@ static int qedi_conn_start(struct iscsi_cls_conn *cls_conn)
goto start_err;
}
- clear_bit(QEDI_CONN_FW_CLEANUP, &qedi_conn->flags);
+ spin_lock(&qedi_conn->tmf_work_lock);
+ qedi_conn->fw_cleanup_works = 0;
+ qedi_conn->ep_disconnect_starting = false;
+ spin_unlock(&qedi_conn->tmf_work_lock);
+
qedi_conn->abrt_conn = 0;
rval = iscsi_conn_start(cls_conn);
@@ -1019,7 +1023,6 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
int ret = 0;
int wait_delay;
int abrt_conn = 0;
- int count = 10;
wait_delay = 60 * HZ + DEF_MAX_RT_TIME;
qedi_ep = ep->dd_data;
@@ -1035,13 +1038,19 @@ static void qedi_ep_disconnect(struct iscsi_endpoint *ep)
qedi_conn = qedi_ep->conn;
abrt_conn = qedi_conn->abrt_conn;
- while (count--) {
- if (!test_bit(QEDI_CONN_FW_CLEANUP,
- &qedi_conn->flags)) {
- break;
- }
+ QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
+ "cid=0x%x qedi_ep=%p waiting for %d tmfs\n",
+ qedi_ep->iscsi_cid, qedi_ep,
+ qedi_conn->fw_cleanup_works);
+
+ spin_lock(&qedi_conn->tmf_work_lock);
+ qedi_conn->ep_disconnect_starting = true;
+ while (qedi_conn->fw_cleanup_works > 0) {
+ spin_unlock(&qedi_conn->tmf_work_lock);
msleep(1000);
+ spin_lock(&qedi_conn->tmf_work_lock);
}
+ spin_unlock(&qedi_conn->tmf_work_lock);
if (test_bit(QEDI_IN_RECOVERY, &qedi->flags)) {
if (qedi_do_not_recover) {
diff --git a/drivers/scsi/qedi/qedi_iscsi.h b/drivers/scsi/qedi/qedi_iscsi.h
index 68ef519f5480..758735209e15 100644
--- a/drivers/scsi/qedi/qedi_iscsi.h
+++ b/drivers/scsi/qedi/qedi_iscsi.h
@@ -169,8 +169,8 @@ struct qedi_conn {
struct list_head tmf_work_list;
wait_queue_head_t wait_queue;
spinlock_t tmf_work_lock; /* tmf work lock */
- unsigned long flags;
-#define QEDI_CONN_FW_CLEANUP 1
+ bool ep_disconnect_starting;
+ int fw_cleanup_works;
};
struct qedi_cmd {
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 28/28] scsi: qedi: Wake up if cmd_cleanup_req is set
2021-05-23 17:57 iscsi error handler fixes Mike Christie
` (26 preceding siblings ...)
2021-05-23 17:57 ` [PATCH 27/28] scsi: qedi: Complete TMF works before disconnect Mike Christie
@ 2021-05-23 17:57 ` Mike Christie
27 siblings, 0 replies; 37+ messages in thread
From: Mike Christie @ 2021-05-23 17:57 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If we got a response then we should always wake up the conn. For both
the cmd_cleanup_req == 0 or cmd_cleanup_req > 0, we shouldn't dig into
iscsi_itt_to_task because we don't know what the upper layers are doing.
We can also remove the qedi_clear_task_idx call here because once we
signal success libiscsi will loop over the affected commands and end
up calling the cleanup_task callout which will release it.
Reviewed-by: Manish Rangankar <mrangankar@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_fw.c | 31 ++++---------------------------
1 file changed, 4 insertions(+), 27 deletions(-)
diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
index 3de1422ce80b..71333d3c5c86 100644
--- a/drivers/scsi/qedi/qedi_fw.c
+++ b/drivers/scsi/qedi/qedi_fw.c
@@ -739,7 +739,6 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
{
struct qedi_work_map *work, *work_tmp;
u32 proto_itt = cqe->itid;
- u32 ptmp_itt = 0;
itt_t protoitt = 0;
int found = 0;
struct qedi_cmd *qedi_cmd = NULL;
@@ -821,37 +820,15 @@ static void qedi_process_cmd_cleanup_resp(struct qedi_ctx *qedi,
check_cleanup_reqs:
if (qedi_conn->cmd_cleanup_req > 0) {
- spin_lock_bh(&conn->session->back_lock);
- qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);
- protoitt = build_itt(ptmp_itt, conn->session->age);
- task = iscsi_itt_to_task(conn, protoitt);
- QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_SCSI_TM,
- "cleanup io itid=0x%x, protoitt=0x%x, cmd_cleanup_cmpl=%d, cid=0x%x\n",
- cqe->itid, protoitt, qedi_conn->cmd_cleanup_cmpl,
- qedi_conn->iscsi_conn_id);
-
- spin_unlock_bh(&conn->session->back_lock);
- if (!task) {
- QEDI_NOTICE(&qedi->dbg_ctx,
- "task is null, itid=0x%x, cid=0x%x\n",
- cqe->itid, qedi_conn->iscsi_conn_id);
- return;
- }
- qedi_conn->cmd_cleanup_cmpl++;
- wake_up(&qedi_conn->wait_queue);
-
QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_TID,
"Freeing tid=0x%x for cid=0x%x\n",
cqe->itid, qedi_conn->iscsi_conn_id);
- qedi_clear_task_idx(qedi_conn->qedi, cqe->itid);
-
+ qedi_conn->cmd_cleanup_cmpl++;
+ wake_up(&qedi_conn->wait_queue);
} else {
- qedi_get_proto_itt(qedi, cqe->itid, &ptmp_itt);
- protoitt = build_itt(ptmp_itt, conn->session->age);
- task = iscsi_itt_to_task(conn, protoitt);
QEDI_ERR(&qedi->dbg_ctx,
- "Delayed or untracked cleanup response, itt=0x%x, tid=0x%x, cid=0x%x, task=%p\n",
- protoitt, cqe->itid, qedi_conn->iscsi_conn_id, task);
+ "Delayed or untracked cleanup response, itt=0x%x, tid=0x%x, cid=0x%x\n",
+ protoitt, cqe->itid, qedi_conn->iscsi_conn_id);
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread