From: Lee Duncan <lduncan@suse.com>
To: Mike Christie <michael.christie@oracle.com>,
martin.petersen@oracle.com, mrangankar@marvell.com,
svernekar@marvell.com, linux-scsi@vger.kernel.org,
jejb@linux.ibm.com
Subject: Re: [PATCH v3 03/17] scsi: iscsi: stop queueing during ep_disconnect
Date: Tue, 20 Apr 2021 07:28:11 -0700 [thread overview]
Message-ID: <a68ffcb4-73c7-cc2a-d90e-90993c44d043@suse.com> (raw)
In-Reply-To: <20210416020440.259271-4-michael.christie@oracle.com>
On 4/15/21 7:04 PM, Mike Christie wrote:
> 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 timedout 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.
>
> 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 | 61 +++++++++++++++++++++---
> drivers/scsi/qedi/qedi_iscsi.c | 1 +
> drivers/scsi/qla4xxx/ql4_os.c | 1 +
> drivers/scsi/scsi_transport_iscsi.c | 3 ++
> include/scsi/libiscsi.h | 1 +
> include/scsi/scsi_transport_iscsi.h | 1 +
> 11 files changed, 67 insertions(+), 6 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 90fcddb76f46..e9658a67d9da 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 37d99357120f..edcd3fab6973 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 aa5ceaffc697..ce3898fdb10f 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1387,22 +1387,28 @@ 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 void 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);
> + if (session->state == ISCSI_STATE_FAILED)
> return;
> - }
>
> 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);
> +}
> +
> +void iscsi_conn_failure(struct iscsi_conn *conn, enum iscsi_err err)
> +{
> + struct iscsi_session *session = conn->session;
> +
> + spin_lock_bh(&session->frwd_lock);
> + iscsi_set_conn_failed(conn);
> + spin_unlock_bh(&session->frwd_lock);
> +
> iscsi_conn_error_event(conn->cls_conn, err);
> }
> EXPORT_SYMBOL_GPL(iscsi_conn_failure);
> @@ -2220,6 +2226,49 @@ static void iscsi_check_transport_timeouts(struct timer_list *t)
> spin_unlock(&session->frwd_lock);
> }
>
> +/*
> + * iscsi_conn_unbind - prevent queueing to conn.
> + * @conn: iscsi conn ep is bound to.
> + *
> + * 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)
> +{
> + 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 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 7bd9a4a04ad5..ff663cb330c2 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..833114c8e197 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -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);
> }
>
> transport->ep_disconnect(ep);
> @@ -4656,6 +4658,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..ec6d508e7a4a 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);
> 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..afc61a23628d 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);
> int (*bind_conn) (struct iscsi_cls_session *session,
> struct iscsi_cls_conn *cls_conn,
> uint64_t transport_eph, int is_leading);
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
next prev parent reply other threads:[~2021-04-20 14:28 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 2:04 [PATCH v3 00/17] libicsi and qedi TMF fixes Mike Christie
2021-04-16 2:04 ` [PATCH v3 01/17] scsi: iscsi: add task completion helper Mike Christie
2021-04-16 21:56 ` Lee Duncan
2021-04-16 2:04 ` [PATCH v3 02/17] scsi: iscsi: sync libiscsi and driver reset cleanup Mike Christie
2021-04-17 17:22 ` Lee Duncan
2021-04-17 17:26 ` Mike Christie
2021-04-16 2:04 ` [PATCH v3 03/17] scsi: iscsi: stop queueing during ep_disconnect Mike Christie
2021-04-20 14:28 ` Lee Duncan [this message]
2021-04-16 2:04 ` [PATCH v3 04/17] scsi: iscsi: drop suspend calls from ep_disconnect Mike Christie
2021-04-20 14:29 ` Lee Duncan
2021-04-16 2:04 ` [PATCH v3 05/17] scsi: iscsi: wait on cmds before freeing conn Mike Christie
2021-04-22 15:02 ` Lee Duncan
2021-04-22 20:09 ` Mike Christie
2021-04-16 2:04 ` [PATCH v3 06/17] scsi: iscsi: fix use conn use after free Mike Christie
2021-04-24 21:11 ` Lee Duncan
2021-04-16 2:04 ` [PATCH v3 07/17] scsi: iscsi: move pool freeing Mike Christie
2021-04-24 21:12 ` Lee Duncan
2021-04-16 2:04 ` [PATCH v3 08/17] scsi: qedi: fix null ref during abort handling Mike Christie
2021-04-16 2:04 ` [PATCH v3 09/17] scsi: qedi: fix race during abort timeouts Mike Christie
2021-04-16 11:39 ` kernel test robot
2021-04-16 15:23 ` michael.christie
2021-04-16 2:04 ` [PATCH v3 10/17] scsi: qedi: fix use after free during abort cleanup Mike Christie
2021-04-16 2:04 ` [PATCH v3 11/17] scsi: qedi: fix TMF tid allocation Mike Christie
2021-04-16 2:04 ` [PATCH v3 12/17] scsi: qedi: use GFP_NOIO for tmf allocation Mike Christie
2021-04-16 2:04 ` [PATCH v3 13/17] scsi: qedi: fix TMF session block/unblock use Mike Christie
2021-04-16 2:04 ` [PATCH v3 14/17] scsi: qedi: fix cleanup " Mike Christie
2021-04-16 2:04 ` [PATCH v3 15/17] scsi: qedi: pass send_iscsi_tmf task to abort Mike Christie
2021-04-16 2:04 ` [PATCH v3 16/17] scsi: qedi: complete TMF works before disconnect Mike Christie
2021-04-16 2:04 ` [PATCH v3 17/17] scsi: qedi: always wake up if cmd_cleanup_req is set Mike Christie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a68ffcb4-73c7-cc2a-d90e-90993c44d043@suse.com \
--to=lduncan@suse.com \
--cc=jejb@linux.ibm.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=michael.christie@oracle.com \
--cc=mrangankar@marvell.com \
--cc=svernekar@marvell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox