* [PATCH 01/13] scsi: iscsi: Fix HW conn removal use after free
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-18 15:44 ` Lee Duncan
2022-05-17 22:24 ` [PATCH 02/13] scsi: iscsi: Add helper to remove a session from the kernel Mike Christie
` (11 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If qla4xxx doesn't remove the connection before the session the iscsi
class tries to remove the connection for it. We were doing a
iscsi_put_conn in the iter function which is not needed and will result in
a use after free because iscsi_remove_conn will free the connection.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 2c0dd64159b0..e6084e158cc0 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
return 0;
iscsi_remove_conn(iscsi_dev_to_conn(dev));
- iscsi_put_conn(iscsi_dev_to_conn(dev));
-
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 01/13] scsi: iscsi: Fix HW conn removal use after free
2022-05-17 22:24 ` [PATCH 01/13] scsi: iscsi: Fix HW conn removal use after free Mike Christie
@ 2022-05-18 15:44 ` Lee Duncan
0 siblings, 0 replies; 18+ messages in thread
From: Lee Duncan @ 2022-05-18 15:44 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/17/22 15:24, Mike Christie wrote:
> If qla4xxx doesn't remove the connection before the session the iscsi
> class tries to remove the connection for it. We were doing a
> iscsi_put_conn in the iter function which is not needed and will result in
> a use after free because iscsi_remove_conn will free the connection.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 2c0dd64159b0..e6084e158cc0 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2138,8 +2138,6 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
> return 0;
>
> iscsi_remove_conn(iscsi_dev_to_conn(dev));
> - iscsi_put_conn(iscsi_dev_to_conn(dev));
> -
> return 0;
> }
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 02/13] scsi: iscsi: Add helper to remove a session from the kernel
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
2022-05-17 22:24 ` [PATCH 01/13] scsi: iscsi: Fix HW conn removal use after free Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-18 16:01 ` Lee Duncan
2022-05-17 22:24 ` [PATCH 03/13] scsi: qedi: Use QEDI_MODE_NORMAL for error handling Mike Christie
` (10 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
qedi requires that we at least tell the FW to disconnect and cleanup
connections during shutdown, and patch;
Commit: d1f2ce77638d ("scsi: qedi: Fix host removal with running
sessions")
converted the driver to use the libicsi helper to drive session removal.
The problem is that during shutdown iscsid will not be running so when we
try to remove the root session we will hang wait for userspace to reply.
This patch adds a helper that will drive the destruction of sessions like
these during system shutdown.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/scsi_transport_iscsi.c | 54 +++++++++++++++++++++++------
include/scsi/scsi_transport_iscsi.h | 1 +
2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index e6084e158cc0..3b5e07544324 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2257,16 +2257,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
}
}
-static int iscsi_if_stop_conn(struct iscsi_transport *transport,
- struct iscsi_uevent *ev)
+static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
{
- 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
@@ -2342,6 +2334,43 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
ISCSI_DBG_TRANS_CONN(conn, "cleanup done.\n");
}
+static int iscsi_iter_force_destroy_conn_fn(struct device *dev, void *data)
+{
+ struct iscsi_transport *transport;
+ struct iscsi_cls_conn *conn;
+
+ if (!iscsi_is_conn_dev(dev))
+ return 0;
+
+ conn = iscsi_dev_to_conn(dev);
+ transport = conn->transport;
+
+ if (READ_ONCE(conn->state) != ISCSI_CONN_DOWN)
+ iscsi_if_stop_conn(conn, STOP_CONN_TERM);
+
+ transport->destroy_conn(conn);
+ return 0;
+}
+
+/**
+ * iscsi_force_destroy_session - destroy a session from the kernel
+ * @session: session to destroy
+ *
+ * Force the destruction of a session from the kernel. This should only be
+ * used when userspace is no longer running during system shutdown.
+ */
+void iscsi_force_destroy_session(struct iscsi_cls_session *session)
+{
+ struct iscsi_transport *transport = session->transport;
+
+ WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
+
+ device_for_each_child(&session->dev, NULL,
+ iscsi_iter_force_destroy_conn_fn);
+ transport->destroy_session(session);
+}
+EXPORT_SYMBOL_GPL(iscsi_force_destroy_session);
+
void iscsi_free_session(struct iscsi_cls_session *session)
{
ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
@@ -3713,7 +3742,12 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
case ISCSI_UEVENT_DESTROY_CONN:
return iscsi_if_destroy_conn(transport, ev);
case ISCSI_UEVENT_STOP_CONN:
- return iscsi_if_stop_conn(transport, ev);
+ conn = iscsi_conn_lookup(ev->u.stop_conn.sid,
+ ev->u.stop_conn.cid);
+ if (!conn)
+ return -EINVAL;
+
+ return iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
}
/*
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 9acb8422f680..d6eab7cb221a 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -442,6 +442,7 @@ extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
struct iscsi_transport *t,
int dd_size,
unsigned int target_id);
+extern void iscsi_force_destroy_session(struct iscsi_cls_session *session);
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_alloc_conn(struct iscsi_cls_session *sess,
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 02/13] scsi: iscsi: Add helper to remove a session from the kernel
2022-05-17 22:24 ` [PATCH 02/13] scsi: iscsi: Add helper to remove a session from the kernel Mike Christie
@ 2022-05-18 16:01 ` Lee Duncan
0 siblings, 0 replies; 18+ messages in thread
From: Lee Duncan @ 2022-05-18 16:01 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/17/22 15:24, Mike Christie wrote:
> qedi requires that we at least tell the FW to disconnect and cleanup
> connections during shutdown, and patch;
>
> Commit: d1f2ce77638d ("scsi: qedi: Fix host removal with running
> sessions")
>
> converted the driver to use the libicsi helper to drive session removal.
> The problem is that during shutdown iscsid will not be running so when we
> try to remove the root session we will hang wait for userspace to reply.
>
> This patch adds a helper that will drive the destruction of sessions like
> these during system shutdown.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/scsi_transport_iscsi.c | 54 +++++++++++++++++++++++------
> include/scsi/scsi_transport_iscsi.h | 1 +
> 2 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index e6084e158cc0..3b5e07544324 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2257,16 +2257,8 @@ static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
> }
> }
>
> -static int iscsi_if_stop_conn(struct iscsi_transport *transport,
> - struct iscsi_uevent *ev)
> +static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)
> {
> - 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
> @@ -2342,6 +2334,43 @@ static void iscsi_cleanup_conn_work_fn(struct work_struct *work)
> ISCSI_DBG_TRANS_CONN(conn, "cleanup done.\n");
> }
>
> +static int iscsi_iter_force_destroy_conn_fn(struct device *dev, void *data)
> +{
> + struct iscsi_transport *transport;
> + struct iscsi_cls_conn *conn;
> +
> + if (!iscsi_is_conn_dev(dev))
> + return 0;
> +
> + conn = iscsi_dev_to_conn(dev);
> + transport = conn->transport;
> +
> + if (READ_ONCE(conn->state) != ISCSI_CONN_DOWN)
> + iscsi_if_stop_conn(conn, STOP_CONN_TERM);
> +
> + transport->destroy_conn(conn);
> + return 0;
> +}
> +
> +/**
> + * iscsi_force_destroy_session - destroy a session from the kernel
> + * @session: session to destroy
> + *
> + * Force the destruction of a session from the kernel. This should only be
> + * used when userspace is no longer running during system shutdown.
> + */
> +void iscsi_force_destroy_session(struct iscsi_cls_session *session)
> +{
> + struct iscsi_transport *transport = session->transport;
> +
> + WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
> +
> + device_for_each_child(&session->dev, NULL,
> + iscsi_iter_force_destroy_conn_fn);
> + transport->destroy_session(session);
> +}
> +EXPORT_SYMBOL_GPL(iscsi_force_destroy_session);
> +
> void iscsi_free_session(struct iscsi_cls_session *session)
> {
> ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
> @@ -3713,7 +3742,12 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
> case ISCSI_UEVENT_DESTROY_CONN:
> return iscsi_if_destroy_conn(transport, ev);
> case ISCSI_UEVENT_STOP_CONN:
> - return iscsi_if_stop_conn(transport, ev);
> + conn = iscsi_conn_lookup(ev->u.stop_conn.sid,
> + ev->u.stop_conn.cid);
> + if (!conn)
> + return -EINVAL;
> +
> + return iscsi_if_stop_conn(conn, ev->u.stop_conn.flag);
> }
>
> /*
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 9acb8422f680..d6eab7cb221a 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -442,6 +442,7 @@ extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
> struct iscsi_transport *t,
> int dd_size,
> unsigned int target_id);
> +extern void iscsi_force_destroy_session(struct iscsi_cls_session *session);
> 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_alloc_conn(struct iscsi_cls_session *sess,
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 03/13] scsi: qedi: Use QEDI_MODE_NORMAL for error handling
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
2022-05-17 22:24 ` [PATCH 01/13] scsi: iscsi: Fix HW conn removal use after free Mike Christie
2022-05-17 22:24 ` [PATCH 02/13] scsi: iscsi: Add helper to remove a session from the kernel Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-18 16:04 ` Lee Duncan
2022-05-17 22:24 ` [PATCH 04/13] scsi: iscsi: Fix session removal on shutdown Mike Christie
` (9 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
When handling errors that lead to host removal use QEDI_MODE_NORMAL. There
is currently no difference in behavior between NORMAL and SHUTDOWN, but in
the next patch we will want to know when we are called from the pci_driver
shutdown callout vs remove/err_handler so we know when userspace is up.
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/qedi/qedi_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 83ffba7f51da..deebe62e2b41 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2491,7 +2491,7 @@ static void qedi_board_disable_work(struct work_struct *work)
if (test_and_set_bit(QEDI_IN_SHUTDOWN, &qedi->flags))
return;
- __qedi_remove(qedi->pdev, QEDI_MODE_SHUTDOWN);
+ __qedi_remove(qedi->pdev, QEDI_MODE_NORMAL);
}
static void qedi_shutdown(struct pci_dev *pdev)
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 03/13] scsi: qedi: Use QEDI_MODE_NORMAL for error handling
2022-05-17 22:24 ` [PATCH 03/13] scsi: qedi: Use QEDI_MODE_NORMAL for error handling Mike Christie
@ 2022-05-18 16:04 ` Lee Duncan
0 siblings, 0 replies; 18+ messages in thread
From: Lee Duncan @ 2022-05-18 16:04 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/17/22 15:24, Mike Christie wrote:
> When handling errors that lead to host removal use QEDI_MODE_NORMAL. There
> is currently no difference in behavior between NORMAL and SHUTDOWN, but in
> the next patch we will want to know when we are called from the pci_driver
> shutdown callout vs remove/err_handler so we know when userspace is up.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 83ffba7f51da..deebe62e2b41 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -2491,7 +2491,7 @@ static void qedi_board_disable_work(struct work_struct *work)
> if (test_and_set_bit(QEDI_IN_SHUTDOWN, &qedi->flags))
> return;
>
> - __qedi_remove(qedi->pdev, QEDI_MODE_SHUTDOWN);
> + __qedi_remove(qedi->pdev, QEDI_MODE_NORMAL);
> }
>
> static void qedi_shutdown(struct pci_dev *pdev)
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 04/13] scsi: iscsi: Fix session removal on shutdown
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (2 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 03/13] scsi: qedi: Use QEDI_MODE_NORMAL for error handling Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-18 21:42 ` Lee Duncan
2022-05-17 22:24 ` [PATCH 05/13] scsi: iscsi: Rename iscsi_conn_queue_work Mike Christie
` (8 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
When the system is shutting down iscsid is not running, so we will not get
a response to the ISCSI_ERR_INVALID_HOST error event. The system shutdown
will then hang waiting on userspace to remove the session. This has
libiscsi force the destruction of the session from the kernel when
iscsi_host_remove is called from a driver's shutdown callout.
This fixes a regression added in qedi boot with patch:
Commit: d1f2ce77638d ("scsi: qedi: Fix host removal with running
sessions")
where in that patch I had qedi use the common session removal function
that waits on userspace instead of rolling it's own kernel based removal.
Fixes: d1f2ce77638d ("scsi: qedi: Fix host removal with running sessions")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/infiniband/ulp/iser/iscsi_iser.c | 4 ++--
drivers/scsi/be2iscsi/be_main.c | 2 +-
drivers/scsi/bnx2i/bnx2i_iscsi.c | 2 +-
drivers/scsi/cxgbi/libcxgbi.c | 2 +-
drivers/scsi/iscsi_tcp.c | 4 ++--
drivers/scsi/libiscsi.c | 9 +++++++--
drivers/scsi/qedi/qedi_main.c | 9 ++++++---
include/scsi/libiscsi.h | 2 +-
8 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index f8d0bab4424c..e36036b8f386 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -568,7 +568,7 @@ static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
iscsi_session_teardown(cls_session);
- iscsi_host_remove(shost);
+ iscsi_host_remove(shost, false);
iscsi_host_free(shost);
}
@@ -685,7 +685,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
return cls_session;
remove_host:
- iscsi_host_remove(shost);
+ iscsi_host_remove(shost, false);
free_host:
iscsi_host_free(shost);
return NULL;
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 3bb0adefbe06..02026476c39c 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -5745,7 +5745,7 @@ static void beiscsi_remove(struct pci_dev *pcidev)
cancel_work_sync(&phba->sess_work);
beiscsi_iface_destroy_default(phba);
- iscsi_host_remove(phba->shost);
+ iscsi_host_remove(phba->shost, false);
beiscsi_disable_port(phba, 1);
/* after cancelling boot_work */
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index 15fbd09baa94..a3c800e04a2e 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -909,7 +909,7 @@ void bnx2i_free_hba(struct bnx2i_hba *hba)
{
struct Scsi_Host *shost = hba->shost;
- iscsi_host_remove(shost);
+ iscsi_host_remove(shost, false);
INIT_LIST_HEAD(&hba->ep_ofld_list);
INIT_LIST_HEAD(&hba->ep_active_list);
INIT_LIST_HEAD(&hba->ep_destroy_list);
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 4365d52c6430..32abdf0fa9aa 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -328,7 +328,7 @@ void cxgbi_hbas_remove(struct cxgbi_device *cdev)
chba = cdev->hbas[i];
if (chba) {
cdev->hbas[i] = NULL;
- iscsi_host_remove(chba->shost);
+ iscsi_host_remove(chba->shost, false);
pci_dev_put(cdev->pdev);
iscsi_host_free(chba->shost);
}
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9fee70d6434a..52c6f70d60ec 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -898,7 +898,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
remove_session:
iscsi_session_teardown(cls_session);
remove_host:
- iscsi_host_remove(shost);
+ iscsi_host_remove(shost, false);
free_host:
iscsi_host_free(shost);
return NULL;
@@ -915,7 +915,7 @@ static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session)
iscsi_tcp_r2tpool_free(cls_session->dd_data);
iscsi_session_teardown(cls_session);
- iscsi_host_remove(shost);
+ iscsi_host_remove(shost, false);
iscsi_host_free(shost);
}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 797abf4f5399..3ddb701cd29c 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -2828,11 +2828,12 @@ static void iscsi_notify_host_removed(struct iscsi_cls_session *cls_session)
/**
* iscsi_host_remove - remove host and sessions
* @shost: scsi host
+ * @is_shutdown: true if called from a driver shutdown callout
*
* If there are any sessions left, this will initiate the removal and wait
* for the completion.
*/
-void iscsi_host_remove(struct Scsi_Host *shost)
+void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown)
{
struct iscsi_host *ihost = shost_priv(shost);
unsigned long flags;
@@ -2841,7 +2842,11 @@ void iscsi_host_remove(struct Scsi_Host *shost)
ihost->state = ISCSI_HOST_REMOVED;
spin_unlock_irqrestore(&ihost->lock, flags);
- iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
+ if (!is_shutdown)
+ iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
+ else
+ iscsi_host_for_each_session(shost, iscsi_force_destroy_session);
+
wait_event_interruptible(ihost->session_removal_wq,
ihost->num_sessions == 0);
if (signal_pending(current))
diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index deebe62e2b41..cecfb2cb4c7b 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2414,9 +2414,12 @@ static void __qedi_remove(struct pci_dev *pdev, int mode)
int rval;
u16 retry = 10;
- if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
- iscsi_host_remove(qedi->shost);
+ if (mode == QEDI_MODE_NORMAL)
+ iscsi_host_remove(qedi->shost, false);
+ else if (mode == QEDI_MODE_SHUTDOWN)
+ iscsi_host_remove(qedi->shost, true);
+ if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
if (qedi->tmf_thread) {
destroy_workqueue(qedi->tmf_thread);
qedi->tmf_thread = NULL;
@@ -2791,7 +2794,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
#ifdef CONFIG_DEBUG_FS
qedi_dbg_host_exit(&qedi->dbg_ctx);
#endif
- iscsi_host_remove(qedi->shost);
+ iscsi_host_remove(qedi->shost, false);
stop_iscsi_func:
qedi_ops->stop(qedi->cdev);
stop_slowpath:
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index d0a24779c52d..471422641ab3 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -411,7 +411,7 @@ extern int iscsi_host_add(struct Scsi_Host *shost, struct device *pdev);
extern struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
int dd_data_size,
bool xmit_can_sleep);
-extern void iscsi_host_remove(struct Scsi_Host *shost);
+extern void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown);
extern void iscsi_host_free(struct Scsi_Host *shost);
extern int iscsi_target_alloc(struct scsi_target *starget);
extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 04/13] scsi: iscsi: Fix session removal on shutdown
2022-05-17 22:24 ` [PATCH 04/13] scsi: iscsi: Fix session removal on shutdown Mike Christie
@ 2022-05-18 21:42 ` Lee Duncan
0 siblings, 0 replies; 18+ messages in thread
From: Lee Duncan @ 2022-05-18 21:42 UTC (permalink / raw)
To: Mike Christie, cleech, njavali, mrangankar,
GR-QLogic-Storage-Upstream, martin.petersen, linux-scsi, jejb
On 5/17/22 15:24, Mike Christie wrote:
> When the system is shutting down iscsid is not running, so we will not get
> a response to the ISCSI_ERR_INVALID_HOST error event. The system shutdown
> will then hang waiting on userspace to remove the session. This has
> libiscsi force the destruction of the session from the kernel when
> iscsi_host_remove is called from a driver's shutdown callout.
>
> This fixes a regression added in qedi boot with patch:
>
> Commit: d1f2ce77638d ("scsi: qedi: Fix host removal with running
> sessions")
>
> where in that patch I had qedi use the common session removal function
> that waits on userspace instead of rolling it's own kernel based removal.
>
> Fixes: d1f2ce77638d ("scsi: qedi: Fix host removal with running sessions")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
> drivers/infiniband/ulp/iser/iscsi_iser.c | 4 ++--
> drivers/scsi/be2iscsi/be_main.c | 2 +-
> drivers/scsi/bnx2i/bnx2i_iscsi.c | 2 +-
> drivers/scsi/cxgbi/libcxgbi.c | 2 +-
> drivers/scsi/iscsi_tcp.c | 4 ++--
> drivers/scsi/libiscsi.c | 9 +++++++--
> drivers/scsi/qedi/qedi_main.c | 9 ++++++---
> include/scsi/libiscsi.h | 2 +-
> 8 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index f8d0bab4424c..e36036b8f386 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -568,7 +568,7 @@ static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
> struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
>
> iscsi_session_teardown(cls_session);
> - iscsi_host_remove(shost);
> + iscsi_host_remove(shost, false);
> iscsi_host_free(shost);
> }
>
> @@ -685,7 +685,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
> return cls_session;
>
> remove_host:
> - iscsi_host_remove(shost);
> + iscsi_host_remove(shost, false);
> free_host:
> iscsi_host_free(shost);
> return NULL;
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 3bb0adefbe06..02026476c39c 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -5745,7 +5745,7 @@ static void beiscsi_remove(struct pci_dev *pcidev)
> cancel_work_sync(&phba->sess_work);
>
> beiscsi_iface_destroy_default(phba);
> - iscsi_host_remove(phba->shost);
> + iscsi_host_remove(phba->shost, false);
> beiscsi_disable_port(phba, 1);
>
> /* after cancelling boot_work */
> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> index 15fbd09baa94..a3c800e04a2e 100644
> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> @@ -909,7 +909,7 @@ void bnx2i_free_hba(struct bnx2i_hba *hba)
> {
> struct Scsi_Host *shost = hba->shost;
>
> - iscsi_host_remove(shost);
> + iscsi_host_remove(shost, false);
> INIT_LIST_HEAD(&hba->ep_ofld_list);
> INIT_LIST_HEAD(&hba->ep_active_list);
> INIT_LIST_HEAD(&hba->ep_destroy_list);
> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index 4365d52c6430..32abdf0fa9aa 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
> @@ -328,7 +328,7 @@ void cxgbi_hbas_remove(struct cxgbi_device *cdev)
> chba = cdev->hbas[i];
> if (chba) {
> cdev->hbas[i] = NULL;
> - iscsi_host_remove(chba->shost);
> + iscsi_host_remove(chba->shost, false);
> pci_dev_put(cdev->pdev);
> iscsi_host_free(chba->shost);
> }
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 9fee70d6434a..52c6f70d60ec 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -898,7 +898,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
> remove_session:
> iscsi_session_teardown(cls_session);
> remove_host:
> - iscsi_host_remove(shost);
> + iscsi_host_remove(shost, false);
> free_host:
> iscsi_host_free(shost);
> return NULL;
> @@ -915,7 +915,7 @@ static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session)
> iscsi_tcp_r2tpool_free(cls_session->dd_data);
> iscsi_session_teardown(cls_session);
>
> - iscsi_host_remove(shost);
> + iscsi_host_remove(shost, false);
> iscsi_host_free(shost);
> }
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 797abf4f5399..3ddb701cd29c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2828,11 +2828,12 @@ static void iscsi_notify_host_removed(struct iscsi_cls_session *cls_session)
> /**
> * iscsi_host_remove - remove host and sessions
> * @shost: scsi host
> + * @is_shutdown: true if called from a driver shutdown callout
> *
> * If there are any sessions left, this will initiate the removal and wait
> * for the completion.
> */
> -void iscsi_host_remove(struct Scsi_Host *shost)
> +void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown)
> {
> struct iscsi_host *ihost = shost_priv(shost);
> unsigned long flags;
> @@ -2841,7 +2842,11 @@ void iscsi_host_remove(struct Scsi_Host *shost)
> ihost->state = ISCSI_HOST_REMOVED;
> spin_unlock_irqrestore(&ihost->lock, flags);
>
> - iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
> + if (!is_shutdown)
> + iscsi_host_for_each_session(shost, iscsi_notify_host_removed);
> + else
> + iscsi_host_for_each_session(shost, iscsi_force_destroy_session);
> +
> wait_event_interruptible(ihost->session_removal_wq,
> ihost->num_sessions == 0);
> if (signal_pending(current))
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index deebe62e2b41..cecfb2cb4c7b 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -2414,9 +2414,12 @@ static void __qedi_remove(struct pci_dev *pdev, int mode)
> int rval;
> u16 retry = 10;
>
> - if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
> - iscsi_host_remove(qedi->shost);
> + if (mode == QEDI_MODE_NORMAL)
> + iscsi_host_remove(qedi->shost, false);
> + else if (mode == QEDI_MODE_SHUTDOWN)
> + iscsi_host_remove(qedi->shost, true);
>
> + if (mode == QEDI_MODE_NORMAL || mode == QEDI_MODE_SHUTDOWN) {
> if (qedi->tmf_thread) {
> destroy_workqueue(qedi->tmf_thread);
> qedi->tmf_thread = NULL;
> @@ -2791,7 +2794,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
> #ifdef CONFIG_DEBUG_FS
> qedi_dbg_host_exit(&qedi->dbg_ctx);
> #endif
> - iscsi_host_remove(qedi->shost);
> + iscsi_host_remove(qedi->shost, false);
> stop_iscsi_func:
> qedi_ops->stop(qedi->cdev);
> stop_slowpath:
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index d0a24779c52d..471422641ab3 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -411,7 +411,7 @@ extern int iscsi_host_add(struct Scsi_Host *shost, struct device *pdev);
> extern struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
> int dd_data_size,
> bool xmit_can_sleep);
> -extern void iscsi_host_remove(struct Scsi_Host *shost);
> +extern void iscsi_host_remove(struct Scsi_Host *shost, bool is_shutdown);
> extern void iscsi_host_free(struct Scsi_Host *shost);
> extern int iscsi_target_alloc(struct scsi_target *starget);
> extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 05/13] scsi: iscsi: Rename iscsi_conn_queue_work
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (3 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 04/13] scsi: iscsi: Fix session removal on shutdown Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 06/13] scsi: iscsi: Add recv workqueue helpers Mike Christie
` (7 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie, Wu Bo
Rename iscsi_conn_queue_work to iscsi_conn_queue_xmit to reflect it
handles queueing of xmits only.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/cxgbi/libcxgbi.c | 2 +-
drivers/scsi/iscsi_tcp.c | 2 +-
drivers/scsi/libiscsi.c | 12 ++++++------
include/scsi/libiscsi.h | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 32abdf0fa9aa..af281e271f88 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -1455,7 +1455,7 @@ void cxgbi_conn_tx_open(struct cxgbi_sock *csk)
if (conn) {
log_debug(1 << CXGBI_DBG_SOCK,
"csk 0x%p, cid %d.\n", csk, conn->id);
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
}
}
EXPORT_SYMBOL_GPL(cxgbi_conn_tx_open);
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 52c6f70d60ec..da1dc345b873 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -205,7 +205,7 @@ static void iscsi_sw_tcp_write_space(struct sock *sk)
old_write_space(sk);
ISCSI_SW_TCP_DBG(conn, "iscsi_write_space\n");
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
}
static void iscsi_sw_tcp_conn_set_callbacks(struct iscsi_conn *conn)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3ddb701cd29c..1bd772d9b804 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -83,7 +83,7 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
"%s " dbg_fmt, __func__, ##arg); \
} while (0);
-inline void iscsi_conn_queue_work(struct iscsi_conn *conn)
+inline void iscsi_conn_queue_xmit(struct iscsi_conn *conn)
{
struct Scsi_Host *shost = conn->session->host;
struct iscsi_host *ihost = shost_priv(shost);
@@ -91,7 +91,7 @@ inline void iscsi_conn_queue_work(struct iscsi_conn *conn)
if (ihost->workq)
queue_work(ihost->workq, &conn->xmitwork);
}
-EXPORT_SYMBOL_GPL(iscsi_conn_queue_work);
+EXPORT_SYMBOL_GPL(iscsi_conn_queue_xmit);
static void __iscsi_update_cmdsn(struct iscsi_session *session,
uint32_t exp_cmdsn, uint32_t max_cmdsn)
@@ -765,7 +765,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
goto free_task;
} else {
list_add_tail(&task->running, &conn->mgmtqueue);
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
}
return task;
@@ -1513,7 +1513,7 @@ void iscsi_requeue_task(struct iscsi_task *task)
*/
iscsi_put_task(task);
}
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
spin_unlock_bh(&conn->session->frwd_lock);
}
EXPORT_SYMBOL_GPL(iscsi_requeue_task);
@@ -1782,7 +1782,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *sc)
}
} else {
list_add_tail(&task->running, &conn->cmdqueue);
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
}
session->queued_cmdsn++;
@@ -1963,7 +1963,7 @@ EXPORT_SYMBOL_GPL(iscsi_suspend_tx);
static void iscsi_start_tx(struct iscsi_conn *conn)
{
clear_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
- iscsi_conn_queue_work(conn);
+ iscsi_conn_queue_xmit(conn);
}
/*
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 471422641ab3..cd1049393733 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -453,7 +453,7 @@ extern int iscsi_conn_get_addr_param(struct sockaddr_storage *addr,
enum iscsi_param param, char *buf);
extern void iscsi_suspend_tx(struct iscsi_conn *conn);
extern void iscsi_suspend_queue(struct iscsi_conn *conn);
-extern void iscsi_conn_queue_work(struct iscsi_conn *conn);
+extern void iscsi_conn_queue_xmit(struct iscsi_conn *conn);
#define iscsi_conn_printk(prefix, _c, fmt, a...) \
iscsi_cls_conn_printk(prefix, ((struct iscsi_conn *)_c)->cls_conn, \
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 06/13] scsi: iscsi: Add recv workqueue helpers
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (4 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 05/13] scsi: iscsi: Rename iscsi_conn_queue_work Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 07/13] scsi: iscsi: Run recv path from workqueue Mike Christie
` (6 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
Add helpers to allow the drivers to run their recv paths from libiscsi's
workqueue.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 29 +++++++++++++++++++++++++++--
include/scsi/libiscsi.h | 4 ++++
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 1bd772d9b804..8f73c8d6ef22 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -93,6 +93,16 @@ inline void iscsi_conn_queue_xmit(struct iscsi_conn *conn)
}
EXPORT_SYMBOL_GPL(iscsi_conn_queue_xmit);
+inline void iscsi_conn_queue_recv(struct iscsi_conn *conn)
+{
+ struct Scsi_Host *shost = conn->session->host;
+ struct iscsi_host *ihost = shost_priv(shost);
+
+ if (ihost->workq && !test_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags))
+ queue_work(ihost->workq, &conn->recvwork);
+}
+EXPORT_SYMBOL_GPL(iscsi_conn_queue_recv);
+
static void __iscsi_update_cmdsn(struct iscsi_session *session,
uint32_t exp_cmdsn, uint32_t max_cmdsn)
{
@@ -1943,7 +1953,7 @@ EXPORT_SYMBOL_GPL(iscsi_suspend_queue);
/**
* iscsi_suspend_tx - suspend iscsi_data_xmit
- * @conn: iscsi conn tp stop processing IO on.
+ * @conn: iscsi conn to stop processing IO on.
*
* This function sets the suspend bit to prevent iscsi_data_xmit
* from sending new IO, and if work is queued on the xmit thread
@@ -1956,7 +1966,7 @@ void iscsi_suspend_tx(struct iscsi_conn *conn)
set_bit(ISCSI_CONN_FLAG_SUSPEND_TX, &conn->flags);
if (ihost->workq)
- flush_workqueue(ihost->workq);
+ flush_work(&conn->xmitwork);
}
EXPORT_SYMBOL_GPL(iscsi_suspend_tx);
@@ -1966,6 +1976,21 @@ static void iscsi_start_tx(struct iscsi_conn *conn)
iscsi_conn_queue_xmit(conn);
}
+/**
+ * iscsi_suspend_rx - Prevent recvwork from running again.
+ * @conn: iscsi conn to stop.
+ */
+void iscsi_suspend_rx(struct iscsi_conn *conn)
+{
+ struct Scsi_Host *shost = conn->session->host;
+ struct iscsi_host *ihost = shost_priv(shost);
+
+ set_bit(ISCSI_CONN_FLAG_SUSPEND_RX, &conn->flags);
+ if (ihost->workq)
+ flush_work(&conn->recvwork);
+}
+EXPORT_SYMBOL_GPL(iscsi_suspend_rx);
+
/*
* We want to make sure a ping is in flight. It has timed out.
* And we are not busy processing a pdu that is making
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index cd1049393733..1e7c5c7f19ac 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -213,6 +213,8 @@ struct iscsi_conn {
struct list_head cmdqueue; /* data-path cmd queue */
struct list_head requeue; /* tasks needing another run */
struct work_struct xmitwork; /* per-conn. xmit workqueue */
+ /* recv */
+ struct work_struct recvwork;
unsigned long flags; /* ISCSI_CONN_FLAGs */
/* negotiated params */
@@ -452,8 +454,10 @@ extern int iscsi_conn_get_param(struct iscsi_cls_conn *cls_conn,
extern int iscsi_conn_get_addr_param(struct sockaddr_storage *addr,
enum iscsi_param param, char *buf);
extern void iscsi_suspend_tx(struct iscsi_conn *conn);
+extern void iscsi_suspend_rx(struct iscsi_conn *conn);
extern void iscsi_suspend_queue(struct iscsi_conn *conn);
extern void iscsi_conn_queue_xmit(struct iscsi_conn *conn);
+extern void iscsi_conn_queue_recv(struct iscsi_conn *conn);
#define iscsi_conn_printk(prefix, _c, fmt, a...) \
iscsi_cls_conn_printk(prefix, ((struct iscsi_conn *)_c)->cls_conn, \
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 07/13] scsi: iscsi: Run recv path from workqueue
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (5 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 06/13] scsi: iscsi: Add recv workqueue helpers Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 08/13] scsi: iscsi_tcp: Tell net when there's more data Mike Christie
` (5 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We don't always want to run the recv path from the network softirq
because when we have to have multiple sessions sharing the same CPUs some
sessions can eat up the napi softirq budget and affect other sessions or
users. This patch allows us to queue the recv handling to the iscsi
workqueue so we can have the scheduler/wq code try to balance the work and
CPU use across all sessions's worker threads.
Note: It wasn't the original intent of the patch but a nice side effect is
that for some workloads/configs we get a nice perf boost. For a simple
read heavy test:
fio --direct=1 --filename=/dev/dm-0 --rw=randread --bs=256K
--ioengine=libaio --iodepth=128 --numjobs=4
where the iscsi threads, fio jobs and rps_cpus share CPUs we see a 32%
throughput boost. We also see increases for small IO IOPs tests but it's
not as high.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/iscsi_tcp.c | 65 ++++++++++++++++++++++++++++++++--------
drivers/scsi/iscsi_tcp.h | 2 ++
2 files changed, 54 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index da1dc345b873..10d7f2b7dd0e 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -52,6 +52,10 @@ static struct iscsi_transport iscsi_sw_tcp_transport;
static unsigned int iscsi_max_lun = ~0;
module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO);
+static bool iscsi_recv_from_iscsi_q;
+module_param_named(recv_from_iscsi_q, iscsi_recv_from_iscsi_q, bool, 0644);
+MODULE_PARM_DESC(recv_from_iscsi_q, "Set to true to read iSCSI data/headers from the iscsi_q workqueue. The default is false which will perform reads from the network softirq context.");
+
static int iscsi_sw_tcp_dbg;
module_param_named(debug_iscsi_tcp, iscsi_sw_tcp_dbg, int,
S_IRUGO | S_IWUSR);
@@ -122,20 +126,13 @@ static inline int iscsi_sw_sk_state_check(struct sock *sk)
return 0;
}
-static void iscsi_sw_tcp_data_ready(struct sock *sk)
+static void iscsi_sw_tcp_recv_data(struct iscsi_conn *conn)
{
- struct iscsi_conn *conn;
- struct iscsi_tcp_conn *tcp_conn;
+ struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
+ struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+ struct sock *sk = tcp_sw_conn->sock->sk;
read_descriptor_t rd_desc;
- read_lock_bh(&sk->sk_callback_lock);
- conn = sk->sk_user_data;
- if (!conn) {
- read_unlock_bh(&sk->sk_callback_lock);
- return;
- }
- tcp_conn = conn->dd_data;
-
/*
* Use rd_desc to pass 'conn' to iscsi_tcp_recv.
* We set count to 1 because we want the network layer to
@@ -144,13 +141,48 @@ static void iscsi_sw_tcp_data_ready(struct sock *sk)
*/
rd_desc.arg.data = conn;
rd_desc.count = 1;
- tcp_read_sock(sk, &rd_desc, iscsi_sw_tcp_recv);
- iscsi_sw_sk_state_check(sk);
+ tcp_read_sock(sk, &rd_desc, iscsi_sw_tcp_recv);
/* If we had to (atomically) map a highmem page,
* unmap it now. */
iscsi_tcp_segment_unmap(&tcp_conn->in.segment);
+
+ iscsi_sw_sk_state_check(sk);
+}
+
+static void iscsi_sw_tcp_recv_data_work(struct work_struct *work)
+{
+ struct iscsi_conn *conn = container_of(work, struct iscsi_conn,
+ recvwork);
+ struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
+ struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+ struct sock *sk = tcp_sw_conn->sock->sk;
+
+ lock_sock(sk);
+ iscsi_sw_tcp_recv_data(conn);
+ release_sock(sk);
+}
+
+static void iscsi_sw_tcp_data_ready(struct sock *sk)
+{
+ struct iscsi_sw_tcp_conn *tcp_sw_conn;
+ struct iscsi_tcp_conn *tcp_conn;
+ struct iscsi_conn *conn;
+
+ read_lock_bh(&sk->sk_callback_lock);
+ conn = sk->sk_user_data;
+ if (!conn) {
+ read_unlock_bh(&sk->sk_callback_lock);
+ return;
+ }
+ tcp_conn = conn->dd_data;
+ tcp_sw_conn = tcp_conn->dd_data;
+
+ if (tcp_sw_conn->queue_recv)
+ iscsi_conn_queue_recv(conn);
+ else
+ iscsi_sw_tcp_recv_data(conn);
read_unlock_bh(&sk->sk_callback_lock);
}
@@ -276,6 +308,9 @@ static int iscsi_sw_tcp_xmit_segment(struct iscsi_tcp_conn *tcp_conn,
if (segment->total_copied + segment->size < segment->total_size)
flags |= MSG_MORE;
+ if (tcp_sw_conn->queue_recv)
+ flags |= MSG_DONTWAIT;
+
/* Use sendpage if we can; else fall back to sendmsg */
if (!segment->data) {
sg = segment->sg;
@@ -557,6 +592,8 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session *cls_session,
conn = cls_conn->dd_data;
tcp_conn = conn->dd_data;
tcp_sw_conn = tcp_conn->dd_data;
+ INIT_WORK(&conn->recvwork, iscsi_sw_tcp_recv_data_work);
+ tcp_sw_conn->queue_recv = iscsi_recv_from_iscsi_q;
tfm = crypto_alloc_ahash("crc32c", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
@@ -610,6 +647,8 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
iscsi_sw_tcp_conn_restore_callbacks(conn);
sock_put(sock->sk);
+ iscsi_suspend_rx(conn);
+
spin_lock_bh(&session->frwd_lock);
tcp_sw_conn->sock = NULL;
spin_unlock_bh(&session->frwd_lock);
diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h
index 791453195099..850a018aefb9 100644
--- a/drivers/scsi/iscsi_tcp.h
+++ b/drivers/scsi/iscsi_tcp.h
@@ -28,6 +28,8 @@ struct iscsi_sw_tcp_send {
struct iscsi_sw_tcp_conn {
struct socket *sock;
+ struct work_struct recvwork;
+ bool queue_recv;
struct iscsi_sw_tcp_send out;
/* old values for socket callbacks */
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 08/13] scsi: iscsi_tcp: Tell net when there's more data
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (6 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 07/13] scsi: iscsi: Run recv path from workqueue Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 09/13] scsi: iscsi_tcp: Drop target_alloc use Mike Christie
` (4 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
If we have more data set the MSG_SENDPAGE_NOTLAST in case we go down the
sendpage path.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/iscsi_tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 10d7f2b7dd0e..5e3b59ecf5b0 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -306,7 +306,7 @@ static int iscsi_sw_tcp_xmit_segment(struct iscsi_tcp_conn *tcp_conn,
copy = segment->size - offset;
if (segment->total_copied + segment->size < segment->total_size)
- flags |= MSG_MORE;
+ flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST;
if (tcp_sw_conn->queue_recv)
flags |= MSG_DONTWAIT;
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 09/13] scsi: iscsi_tcp: Drop target_alloc use
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (7 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 08/13] scsi: iscsi_tcp: Tell net when there's more data Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 10/13] scsi: iscsi: remove unneeded task state check Mike Christie
` (3 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie, Wu Bo
For software iscsi, we do a session per host so there is no need to set
the target's can_queue since its the same as the host one. It just results
in extra atomic checks in the main IO path.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Reviewed-by: Wu Bo <wubo40@huawei.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/iscsi_tcp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 5e3b59ecf5b0..29b1bd755afe 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -1042,7 +1042,6 @@ static struct scsi_host_template iscsi_sw_tcp_sht = {
.eh_target_reset_handler = iscsi_eh_recover_target,
.dma_boundary = PAGE_SIZE - 1,
.slave_configure = iscsi_sw_tcp_slave_configure,
- .target_alloc = iscsi_target_alloc,
.proc_name = "iscsi_tcp",
.this_id = -1,
.track_queue_depth = 1,
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 10/13] scsi: iscsi: remove unneeded task state check
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (8 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 09/13] scsi: iscsi_tcp: Drop target_alloc use Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 11/13] scsi: iscsi: Remove iscsi_get_task back_lock requirement Mike Christie
` (2 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie, Wu Bo
The patch:
commit 5923d64b7ab6 ("scsi: libiscsi: Drop taskqueuelock")
added an extra task->state because for
commit 6f8830f5bbab ("scsi: libiscsi: add lock around task lists to fix
list corruption regression")
we didn't know why we ended up with cmds on the list and thought it
might have been a bad target sending a response while we were still
sending the cmd. We were never able to get a target to send us a response
early, because it turns out the bug was just a race in libiscsi/
libiscsi_tcp where
1. iscsi_tcp_r2t_rsp queues a r2t to tcp_task->r2tqueue.
2. iscsi_tcp_task_xmit runs iscsi_tcp_get_curr_r2t and sees we have a r2t.
It dequeues it and iscsi_tcp_task_xmit starts to process it.
3. iscsi_tcp_r2t_rsp runs iscsi_requeue_task and puts the task on the
requeue list.
4. iscsi_tcp_task_xmit sends the data for r2t. This is the final chunk of
data, so the cmd is done.
5. target sends the response.
6. On a different CPU from #3, iscsi_complete_task processes the response.
Since there was no common lock for the list, the lists/tasks pointers are
not fully in sync, so could end up with list corruption.
Since it was just a race on our side, this patch removes the extra check
and fixes up the comments.
Reviewed-by: Wu Bo <wubo40@huawei.com>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 8f73c8d6ef22..72ed303184cc 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -567,16 +567,19 @@ static bool cleanup_queued_task(struct iscsi_task *task)
struct iscsi_conn *conn = task->conn;
bool early_complete = false;
- /* Bad target might have completed task while it was still running */
+ /*
+ * We might have raced where we handled a R2T early and got a response
+ * but have not yet taken the task off the requeue list, then a TMF or
+ * recovery happened and so we can still see it here.
+ */
if (task->state == ISCSI_TASK_COMPLETED)
early_complete = true;
if (!list_empty(&task->running)) {
list_del_init(&task->running);
/*
- * If it's on a list but still running, this could be from
- * a bad target sending a rsp early, cleanup from a TMF, or
- * session recovery.
+ * If it's on a list but still running this could be cleanup
+ * from a TMF or session recovery.
*/
if (task->state == ISCSI_TASK_RUNNING ||
task->state == ISCSI_TASK_COMPLETED)
@@ -1485,7 +1488,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
}
/* regular RX path uses back_lock */
spin_lock(&conn->session->back_lock);
- if (rc && task->state == ISCSI_TASK_RUNNING) {
+ if (rc) {
/*
* get an extra ref that is released next time we access it
* as conn->task above.
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 11/13] scsi: iscsi: Remove iscsi_get_task back_lock requirement
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (9 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 10/13] scsi: iscsi: remove unneeded task state check Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 12/13] scsi: iscsi: Try to avoid taking back_lock in xmit path Mike Christie
2022-05-17 22:24 ` [PATCH 13/13] scsi: libiscsi: improve conn_send_pdu API Mike Christie
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We currently require that the back_lock is held when calling the functions
that manipulate the iscsi_task refcount. The only reason for this is to
handle races where we are handling SCSI-ml eh callbacks and the cmd is
completing at the same time the normal completion path is running, and we
can't return from the EH callback until the driver has stopped accessing
the cmd. Holding the back_lock while also accessing the task->state made
it simple to check that a cmd is completing and also get/put a refcount at
the same time, and at the time we were not as concerned about performance.
The problem is that we don't want to take the back_lock from the xmit path
for normal IO since it causes contention with the completion path if the
user has chosen to try and split those paths on different CPUs (in this
case abusing the CPUs and igoring caching improves perf for some uses).
This patch begins to remove the back_lock requirement for
iscsi_get/put_task by removing the requirement for the get path. Instead
of always holding the back_lock we detect if something has done the last
put and is about to call iscsi_free_task. The next patch will then allow
iscsi code to do the last put on a task and only grab the back_lock if
the refcount is now zero and it's going to call iscsi_free_task.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/be2iscsi/be_main.c | 19 ++++++-
drivers/scsi/libiscsi.c | 95 +++++++++++++++++++++++----------
drivers/scsi/libiscsi_tcp.c | 5 +-
include/scsi/libiscsi.h | 2 +-
4 files changed, 88 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 02026476c39c..50a577ac3bb4 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -231,6 +231,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
cls_session = starget_to_session(scsi_target(sc->device));
session = cls_session->dd_data;
+completion_check:
/* check if we raced, task just got cleaned up under us */
spin_lock_bh(&session->back_lock);
if (!abrt_task || !abrt_task->sc) {
@@ -238,7 +239,13 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
return SUCCESS;
}
/* get a task ref till FW processes the req for the ICD used */
- __iscsi_get_task(abrt_task);
+ if (!iscsi_get_task(abrt_task)) {
+ spin_unlock(&session->back_lock);
+ /* We are just about to call iscsi_free_task so wait for it. */
+ udelay(5);
+ goto completion_check;
+ }
+
abrt_io_task = abrt_task->dd_data;
conn = abrt_task->conn;
beiscsi_conn = conn->dd_data;
@@ -323,7 +330,15 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
}
/* get a task ref till FW processes the req for the ICD used */
- __iscsi_get_task(task);
+ if (!iscsi_get_task(task)) {
+ /*
+ * The task has completed in the driver and is
+ * completing in libiscsi. Just ignore it here. When we
+ * call iscsi_eh_device_reset, it will wait for us.
+ */
+ continue;
+ }
+
io_task = task->dd_data;
/* mark WRB invalid which have been not processed by FW yet */
if (is_chip_be2_be3r(phba)) {
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 72ed303184cc..dee6e2d5c86e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -83,6 +83,8 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
"%s " dbg_fmt, __func__, ##arg); \
} while (0);
+#define ISCSI_CMD_COMPL_WAIT 5
+
inline void iscsi_conn_queue_xmit(struct iscsi_conn *conn)
{
struct Scsi_Host *shost = conn->session->host;
@@ -482,11 +484,11 @@ static void iscsi_free_task(struct iscsi_task *task)
}
}
-void __iscsi_get_task(struct iscsi_task *task)
+bool iscsi_get_task(struct iscsi_task *task)
{
- refcount_inc(&task->refcount);
+ return refcount_inc_not_zero(&task->refcount);
}
-EXPORT_SYMBOL_GPL(__iscsi_get_task);
+EXPORT_SYMBOL_GPL(iscsi_get_task);
void __iscsi_put_task(struct iscsi_task *task)
{
@@ -600,20 +602,17 @@ static bool cleanup_queued_task(struct iscsi_task *task)
}
/*
- * session frwd lock must be held and if not called for a task that is still
- * pending or from the xmit thread, then xmit thread must be suspended
+ * session back and frwd lock must be held and if not called for a task that
+ * is still pending or from the xmit thread, then xmit thread must be suspended
*/
-static void fail_scsi_task(struct iscsi_task *task, int err)
+static void __fail_scsi_task(struct iscsi_task *task, int err)
{
struct iscsi_conn *conn = task->conn;
struct scsi_cmnd *sc;
int state;
- spin_lock_bh(&conn->session->back_lock);
- if (cleanup_queued_task(task)) {
- spin_unlock_bh(&conn->session->back_lock);
+ if (cleanup_queued_task(task))
return;
- }
if (task->state == ISCSI_TASK_PENDING) {
/*
@@ -632,7 +631,15 @@ static void fail_scsi_task(struct iscsi_task *task, int err)
sc->result = err << 16;
scsi_set_resid(sc, scsi_bufflen(sc));
iscsi_complete_task(task, state);
- spin_unlock_bh(&conn->session->back_lock);
+}
+
+static void fail_scsi_task(struct iscsi_task *task, int err)
+{
+ struct iscsi_session *session = task->conn->session;
+
+ spin_lock_bh(&session->back_lock);
+ __fail_scsi_task(task, err);
+ spin_unlock_bh(&session->back_lock);
}
static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
@@ -1450,8 +1457,17 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
spin_lock_bh(&conn->session->back_lock);
if (!conn->task) {
- /* Take a ref so we can access it after xmit_task() */
- __iscsi_get_task(task);
+ /*
+ * Take a ref so we can access it after xmit_task().
+ *
+ * This should never fail because the failure paths will have
+ * stopped the xmit thread. WARN on move on.
+ */
+ if (!iscsi_get_task(task)) {
+ spin_unlock_bh(&conn->session->back_lock);
+ WARN_ON_ONCE(1);
+ return 0;
+ }
} else {
/* Already have a ref from when we failed to send it last call */
conn->task = NULL;
@@ -1493,7 +1509,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
* get an extra ref that is released next time we access it
* as conn->task above.
*/
- __iscsi_get_task(task);
+ iscsi_get_task(task);
conn->task = task;
}
@@ -1908,6 +1924,7 @@ static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
struct iscsi_task *task;
int i;
+restart_cmd_loop:
spin_lock_bh(&session->back_lock);
for (i = 0; i < session->cmds_max; i++) {
task = session->cmds[i];
@@ -1916,22 +1933,25 @@ static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
if (lun != -1 && lun != task->sc->device->lun)
continue;
-
- __iscsi_get_task(task);
- spin_unlock_bh(&session->back_lock);
+ /*
+ * The cmd is completing but if this is called from an eh
+ * callout path then when we return scsi-ml owns the cmd. Wait
+ * for the completion path to finish freeing the cmd.
+ */
+ if (!iscsi_get_task(task)) {
+ spin_unlock_bh(&session->back_lock);
+ spin_unlock_bh(&session->frwd_lock);
+ udelay(ISCSI_CMD_COMPL_WAIT);
+ spin_lock_bh(&session->frwd_lock);
+ goto restart_cmd_loop;
+ }
ISCSI_DBG_SESSION(session,
"failing sc %p itt 0x%x state %d\n",
task->sc, task->itt, task->state);
- fail_scsi_task(task, error);
-
- spin_unlock_bh(&session->frwd_lock);
- iscsi_put_task(task);
- spin_lock_bh(&session->frwd_lock);
-
- spin_lock_bh(&session->back_lock);
+ __fail_scsi_task(task, error);
+ __iscsi_put_task(task);
}
-
spin_unlock_bh(&session->back_lock);
}
@@ -2036,7 +2056,16 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
spin_unlock(&session->back_lock);
goto done;
}
- __iscsi_get_task(task);
+ if (!iscsi_get_task(task)) {
+ /*
+ * Racing with the completion path right now, so give it more
+ * time so that path can complete it like normal.
+ */
+ rc = BLK_EH_RESET_TIMER;
+ task = NULL;
+ spin_unlock(&session->back_lock);
+ goto done;
+ }
spin_unlock(&session->back_lock);
if (session->state != ISCSI_STATE_LOGGED_IN) {
@@ -2285,6 +2314,7 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
ISCSI_DBG_EH(session, "aborting sc %p\n", sc);
+completion_check:
mutex_lock(&session->eh_mutex);
spin_lock_bh(&session->frwd_lock);
/*
@@ -2324,13 +2354,20 @@ int iscsi_eh_abort(struct scsi_cmnd *sc)
return SUCCESS;
}
+ if (!iscsi_get_task(task)) {
+ spin_unlock(&session->back_lock);
+ spin_unlock_bh(&session->frwd_lock);
+ mutex_unlock(&session->eh_mutex);
+ /* We are just about to call iscsi_free_task so wait for it. */
+ udelay(ISCSI_CMD_COMPL_WAIT);
+ goto completion_check;
+ }
+
+ ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
conn = session->leadconn;
iscsi_get_conn(conn->cls_conn);
conn->eh_abort_cnt++;
age = session->age;
-
- ISCSI_DBG_EH(session, "aborting [sc %p itt 0x%x]\n", sc, task->itt);
- __iscsi_get_task(task);
spin_unlock(&session->back_lock);
if (task->state == ISCSI_TASK_PENDING) {
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 883005757ddb..defe08142b75 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -558,7 +558,10 @@ static int iscsi_tcp_r2t_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
return 0;
}
task->last_xfer = jiffies;
- __iscsi_get_task(task);
+ if (!iscsi_get_task(task)) {
+ /* Let the path that got the early rsp complete it */
+ return 0;
+ }
tcp_conn = conn->dd_data;
rhdr = (struct iscsi_r2t_rsp *)tcp_conn->in.hdr;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 1e7c5c7f19ac..7baffeac279f 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -482,7 +482,7 @@ extern struct iscsi_task *iscsi_itt_to_task(struct iscsi_conn *, itt_t);
extern void iscsi_requeue_task(struct iscsi_task *task);
extern void iscsi_put_task(struct iscsi_task *task);
extern void __iscsi_put_task(struct iscsi_task *task);
-extern void __iscsi_get_task(struct iscsi_task *task);
+extern bool iscsi_get_task(struct iscsi_task *task);
extern void iscsi_complete_scsi_task(struct iscsi_task *task,
uint32_t exp_cmdsn, uint32_t max_cmdsn);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 12/13] scsi: iscsi: Try to avoid taking back_lock in xmit path
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (10 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 11/13] scsi: iscsi: Remove iscsi_get_task back_lock requirement Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
2022-05-17 22:24 ` [PATCH 13/13] scsi: libiscsi: improve conn_send_pdu API Mike Christie
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
We need the back lock when freeing a task, so we hold it when calling
__iscsi_put_task from the completion path to make it easier and to avoid
having to retake it in that path. For iscsi_put_task we just grabbed it
while also doing the decrement on the refcount but it's only really needed
if the refcount is zero and we free the task. This modifies iscsi_put_task
to just take the lock when needed then has the xmit path use it. Normally
we will then not take the back lock from the xmit path. It will only be
rare cases where the network is so fast that we get a response right after
we send the header/data.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index dee6e2d5c86e..2bba10464cfa 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -490,6 +490,12 @@ bool iscsi_get_task(struct iscsi_task *task)
}
EXPORT_SYMBOL_GPL(iscsi_get_task);
+/**
+ * __iscsi_put_task - drop the refcount on a task
+ * @task: iscsi_task to drop the refcount on
+ *
+ * The back_lock must be held when calling in case it frees the task.
+ */
void __iscsi_put_task(struct iscsi_task *task)
{
if (refcount_dec_and_test(&task->refcount))
@@ -501,10 +507,11 @@ void iscsi_put_task(struct iscsi_task *task)
{
struct iscsi_session *session = task->conn->session;
- /* regular RX path uses back_lock */
- spin_lock_bh(&session->back_lock);
- __iscsi_put_task(task);
- spin_unlock_bh(&session->back_lock);
+ if (refcount_dec_and_test(&task->refcount)) {
+ spin_lock_bh(&session->back_lock);
+ iscsi_free_task(task);
+ spin_unlock_bh(&session->back_lock);
+ }
}
EXPORT_SYMBOL_GPL(iscsi_put_task);
@@ -1454,8 +1461,6 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
{
int rc;
- spin_lock_bh(&conn->session->back_lock);
-
if (!conn->task) {
/*
* Take a ref so we can access it after xmit_task().
@@ -1464,7 +1469,6 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
* stopped the xmit thread. WARN on move on.
*/
if (!iscsi_get_task(task)) {
- spin_unlock_bh(&conn->session->back_lock);
WARN_ON_ONCE(1);
return 0;
}
@@ -1478,7 +1482,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
* case a bad target sends a cmd rsp before we have handled the task.
*/
if (was_requeue)
- __iscsi_put_task(task);
+ iscsi_put_task(task);
/*
* Do this after dropping the extra ref because if this was a requeue
@@ -1490,10 +1494,8 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
* task and get woken up again.
*/
conn->task = task;
- spin_unlock_bh(&conn->session->back_lock);
return -ENODATA;
}
- spin_unlock_bh(&conn->session->back_lock);
spin_unlock_bh(&conn->session->frwd_lock);
rc = conn->session->tt->xmit_task(task);
@@ -1501,10 +1503,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
if (!rc) {
/* done with this task */
task->last_xfer = jiffies;
- }
- /* regular RX path uses back_lock */
- spin_lock(&conn->session->back_lock);
- if (rc) {
+ } else {
/*
* get an extra ref that is released next time we access it
* as conn->task above.
@@ -1513,8 +1512,7 @@ static int iscsi_xmit_task(struct iscsi_conn *conn, struct iscsi_task *task,
conn->task = task;
}
- __iscsi_put_task(task);
- spin_unlock(&conn->session->back_lock);
+ iscsi_put_task(task);
return rc;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 13/13] scsi: libiscsi: improve conn_send_pdu API
2022-05-17 22:24 [PATCH 00/13] iscsi fixes, perf improvements and cleanups Mike Christie
` (11 preceding siblings ...)
2022-05-17 22:24 ` [PATCH 12/13] scsi: iscsi: Try to avoid taking back_lock in xmit path Mike Christie
@ 2022-05-17 22:24 ` Mike Christie
12 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2022-05-17 22:24 UTC (permalink / raw)
To: lduncan, cleech, njavali, mrangankar, GR-QLogic-Storage-Upstream,
martin.petersen, linux-scsi, jejb
Cc: Mike Christie
The conn_send_pdu API is evil in that it returns a pointer to a
iscsi_task, but that task might have been freed already so you can't
touch it. This patch splits the task allocation and transmission, so
functions like iscsi_send_nopout can access the task before its sent and
do whatever book keeping is needed before it's sent.
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
drivers/scsi/libiscsi.c | 85 ++++++++++++++++++++++++++++++-----------
include/scsi/libiscsi.h | 3 --
2 files changed, 62 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 2bba10464cfa..ea0c67bc54bd 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -695,12 +695,18 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
return 0;
}
+/**
+ * iscsi_alloc_mgmt_task - allocate and setup a mgmt task.
+ * @conn: iscsi conn that the task will be sent on.
+ * @hdr: iscsi pdu that will be sent.
+ * @data: buffer for data segment if needed.
+ * @data_size: length of data in bytes.
+ */
static struct iscsi_task *
-__iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
+iscsi_alloc_mgmt_task(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
char *data, uint32_t data_size)
{
struct iscsi_session *session = conn->session;
- struct iscsi_host *ihost = shost_priv(session->host);
uint8_t opcode = hdr->opcode & ISCSI_OPCODE_MASK;
struct iscsi_task *task;
itt_t itt;
@@ -781,28 +787,57 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
task->conn->session->age);
}
- if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
- WRITE_ONCE(conn->ping_task, task);
+ return task;
+
+free_task:
+ iscsi_put_task(task);
+ return NULL;
+}
+
+/**
+ * iscsi_send_mgmt_task - Send task created with iscsi_alloc_mgmt_task.
+ * @task: iscsi task to send.
+ *
+ * On failure this returns a non-zero error code, and the driver must free
+ * the task with iscsi_put_task;
+ */
+static int iscsi_send_mgmt_task(struct iscsi_task *task)
+{
+ struct iscsi_conn *conn = task->conn;
+ struct iscsi_session *session = conn->session;
+ struct iscsi_host *ihost = shost_priv(conn->session->host);
+ int rc = 0;
if (!ihost->workq) {
- if (iscsi_prep_mgmt_task(conn, task))
- goto free_task;
+ rc = iscsi_prep_mgmt_task(conn, task);
+ if (rc)
+ return rc;
- if (session->tt->xmit_task(task))
- goto free_task;
+ rc = session->tt->xmit_task(task);
+ if (rc)
+ return rc;
} else {
list_add_tail(&task->running, &conn->mgmtqueue);
iscsi_conn_queue_xmit(conn);
}
- return task;
+ return 0;
+}
-free_task:
- /* regular RX path uses back_lock */
- spin_lock(&session->back_lock);
- __iscsi_put_task(task);
- spin_unlock(&session->back_lock);
- return NULL;
+static int __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
+ char *data, uint32_t data_size)
+{
+ struct iscsi_task *task;
+ int rc;
+
+ task = iscsi_alloc_mgmt_task(conn, hdr, data, data_size);
+ if (!task)
+ return -ENOMEM;
+
+ rc = iscsi_send_mgmt_task(task);
+ if (rc)
+ iscsi_put_task(task);
+ return rc;
}
int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr,
@@ -813,7 +848,7 @@ int iscsi_conn_send_pdu(struct iscsi_cls_conn *cls_conn, struct iscsi_hdr *hdr,
int err = 0;
spin_lock_bh(&session->frwd_lock);
- if (!__iscsi_conn_send_pdu(conn, hdr, data, data_size))
+ if (__iscsi_conn_send_pdu(conn, hdr, data, data_size))
err = -EPERM;
spin_unlock_bh(&session->frwd_lock);
return err;
@@ -986,7 +1021,6 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
if (!rhdr) {
if (READ_ONCE(conn->ping_task))
return -EINVAL;
- WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK);
}
memset(&hdr, 0, sizeof(struct iscsi_nopout));
@@ -1000,10 +1034,18 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, struct iscsi_nopin *rhdr)
} else
hdr.ttt = RESERVED_ITT;
- task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)&hdr, NULL, 0);
- if (!task) {
+ task = iscsi_alloc_mgmt_task(conn, (struct iscsi_hdr *)&hdr, NULL, 0);
+ if (!task)
+ return -ENOMEM;
+
+ if (!rhdr)
+ WRITE_ONCE(conn->ping_task, task);
+
+ if (iscsi_send_mgmt_task(task)) {
if (!rhdr)
WRITE_ONCE(conn->ping_task, NULL);
+ iscsi_put_task(task);
+
iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n");
return -EIO;
} else if (!rhdr) {
@@ -1870,11 +1912,8 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
__must_hold(&session->frwd_lock)
{
struct iscsi_session *session = conn->session;
- struct iscsi_task *task;
- task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr,
- NULL, 0);
- if (!task) {
+ if (__iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0)) {
spin_unlock_bh(&session->frwd_lock);
iscsi_conn_printk(KERN_ERR, conn, "Could not send TMF.\n");
iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 7baffeac279f..b3efcd318f47 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -135,9 +135,6 @@ struct iscsi_task {
void *dd_data; /* driver/transport data */
};
-/* invalid scsi_task pointer */
-#define INVALID_SCSI_TASK (struct iscsi_task *)-1l
-
static inline int iscsi_task_has_unsol_data(struct iscsi_task *task)
{
return task->unsol_r2t.data_length > task->unsol_r2t.sent;
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread