[PATCH 0/4] nvme-fc: clean up error recovery implementation

[PATCH 0/4] nvme-fc: clean up error recovery implementation

[James Smart]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

After reviewing the error_recovery paths to address the abort on the
io to cover timeouts on connecting ios, it became clear that some of the
thing in the way it was coded were unnecessary.

This patch set cleans up those items and refactors. It then addresses the
failure case of io errors during reconnect which wants to take the create
failure cases, but there is no status back from the core routines to
trigger that path.

James Smart (4):
  nvme-fc: remove err_work work item
  nvme-fc: eliminate terminate_io use by nvme_fc_error_recovery
  nvme-fc: remove nvme_fc_terminate_io()
  nvme_fc: track error_recovery while connecting

 drivers/nvme/host/fc.c | 270 +++++++++++++++++------------------------
 1 file changed, 109 insertions(+), 161 deletions(-)

-- 
2.26.2


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/4] nvme-fc: remove err_work work item

[James Smart]

[-- Attachment #1.1: Type: text/plain, Size: 3838 bytes --]

err_work was created to handle errors (mainly io timeout) while in
CONNECTING state. The flag for err_work_active is also unneeded.

Remove err_work_active and err_work.  The actions to abort ios are moved
inline to nvme_error_recovery().

Signed-off-by: James Smart <james.smart@broadcom.com>
---
 drivers/nvme/host/fc.c | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 7067aaf50bf7..06fb208ab350 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -153,7 +153,6 @@ struct nvme_fc_ctrl {
 	u32			cnum;
 
 	bool			ioq_live;
-	atomic_t		err_work_active;
 	u64			association_id;
 	struct nvmefc_ls_rcv_op	*rcv_disconn;
 
@@ -163,7 +162,6 @@ struct nvme_fc_ctrl {
 	struct blk_mq_tag_set	tag_set;
 
 	struct delayed_work	connect_work;
-	struct work_struct	err_work;
 
 	struct kref		ref;
 	unsigned long		flags;
@@ -2410,11 +2408,11 @@ nvme_fc_nvme_ctrl_freed(struct nvme_ctrl *nctrl)
 	nvme_fc_ctrl_put(ctrl);
 }
 
+static void __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl);
+
 static void
 nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 {
-	int active;
-
 	/*
 	 * if an error (io timeout, etc) while (re)connecting,
 	 * it's an error on creating the new association.
@@ -2423,11 +2421,14 @@ nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 	 * ios hitting this path before things are cleaned up.
 	 */
 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
-		active = atomic_xchg(&ctrl->err_work_active, 1);
-		if (!active && !queue_work(nvme_fc_wq, &ctrl->err_work)) {
-			atomic_set(&ctrl->err_work_active, 0);
-			WARN_ON(1);
-		}
+		__nvme_fc_terminate_io(ctrl);
+
+		/*
+		 * Rescheduling the connection after recovering
+		 * from the io error is left to the reconnect work
+		 * item, which is what should have stalled waiting on
+		 * the io that had the error that scheduled this work.
+		 */
 		return;
 	}
 
@@ -3233,7 +3234,6 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
 
-	cancel_work_sync(&ctrl->err_work);
 	cancel_delayed_work_sync(&ctrl->connect_work);
 	/*
 	 * kill the association on the link side.  this will block
@@ -3343,23 +3343,6 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
 			ctrl->cnum);
 }
 
-static void
-nvme_fc_connect_err_work(struct work_struct *work)
-{
-	struct nvme_fc_ctrl *ctrl =
-			container_of(work, struct nvme_fc_ctrl, err_work);
-
-	__nvme_fc_terminate_io(ctrl);
-
-	atomic_set(&ctrl->err_work_active, 0);
-
-	/*
-	 * Rescheduling the connection after recovering
-	 * from the io error is left to the reconnect work
-	 * item, which is what should have stalled waiting on
-	 * the io that had the error that scheduled this work.
-	 */
-}
 
 static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = {
 	.name			= "fc",
@@ -3474,7 +3457,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	ctrl->dev = lport->dev;
 	ctrl->cnum = idx;
 	ctrl->ioq_live = false;
-	atomic_set(&ctrl->err_work_active, 0);
 	init_waitqueue_head(&ctrl->ioabort_wait);
 
 	get_device(ctrl->dev);
@@ -3482,7 +3464,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work);
 	INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work);
-	INIT_WORK(&ctrl->err_work, nvme_fc_connect_err_work);
 	spin_lock_init(&ctrl->lock);
 
 	/* io queue count */
@@ -3575,7 +3556,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 fail_ctrl:
 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
 	cancel_work_sync(&ctrl->ctrl.reset_work);
-	cancel_work_sync(&ctrl->err_work);
 	cancel_delayed_work_sync(&ctrl->connect_work);
 
 	ctrl->ctrl.opts = NULL;
-- 
2.26.2


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvme-fc: remove err_work work item

[Himanshu Madhani]

> On Oct 16, 2020, at 4:27 PM, James Smart <james.smart@broadcom.com> wrote:
> 
> err_work was created to handle errors (mainly io timeout) while in
> CONNECTING state. The flag for err_work_active is also unneeded.
> 
> Remove err_work_active and err_work.  The actions to abort ios are moved
> inline to nvme_error_recovery().
> 
> Signed-off-by: James Smart <james.smart@broadcom.com>
> ---
> drivers/nvme/host/fc.c | 40 ++++++++++------------------------------
> 1 file changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 7067aaf50bf7..06fb208ab350 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -153,7 +153,6 @@ struct nvme_fc_ctrl {
> 	u32			cnum;
> 
> 	bool			ioq_live;
> -	atomic_t		err_work_active;
> 	u64			association_id;
> 	struct nvmefc_ls_rcv_op	*rcv_disconn;
> 
> @@ -163,7 +162,6 @@ struct nvme_fc_ctrl {
> 	struct blk_mq_tag_set	tag_set;
> 
> 	struct delayed_work	connect_work;
> -	struct work_struct	err_work;
> 
> 	struct kref		ref;
> 	unsigned long		flags;
> @@ -2410,11 +2408,11 @@ nvme_fc_nvme_ctrl_freed(struct nvme_ctrl *nctrl)
> 	nvme_fc_ctrl_put(ctrl);
> }
> 
> +static void __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl);
> +
> static void
> nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> {
> -	int active;
> -
> 	/*
> 	 * if an error (io timeout, etc) while (re)connecting,
> 	 * it's an error on creating the new association.
> @@ -2423,11 +2421,14 @@ nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> 	 * ios hitting this path before things are cleaned up.
> 	 */
> 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
> -		active = atomic_xchg(&ctrl->err_work_active, 1);
> -		if (!active && !queue_work(nvme_fc_wq, &ctrl->err_work)) {
> -			atomic_set(&ctrl->err_work_active, 0);
> -			WARN_ON(1);
> -		}
> +		__nvme_fc_terminate_io(ctrl);
> +
> +		/*
> +		 * Rescheduling the connection after recovering
> +		 * from the io error is left to the reconnect work
> +		 * item, which is what should have stalled waiting on
> +		 * the io that had the error that scheduled this work.
> +		 */
> 		return;
> 	}
> 
> @@ -3233,7 +3234,6 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
> {
> 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
> 
> -	cancel_work_sync(&ctrl->err_work);
> 	cancel_delayed_work_sync(&ctrl->connect_work);
> 	/*
> 	 * kill the association on the link side.  this will block
> @@ -3343,23 +3343,6 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
> 			ctrl->cnum);
> }
> 
> -static void
> -nvme_fc_connect_err_work(struct work_struct *work)
> -{
> -	struct nvme_fc_ctrl *ctrl =
> -			container_of(work, struct nvme_fc_ctrl, err_work);
> -
> -	__nvme_fc_terminate_io(ctrl);
> -
> -	atomic_set(&ctrl->err_work_active, 0);
> -
> -	/*
> -	 * Rescheduling the connection after recovering
> -	 * from the io error is left to the reconnect work
> -	 * item, which is what should have stalled waiting on
> -	 * the io that had the error that scheduled this work.
> -	 */
> -}
> 
> static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = {
> 	.name			= "fc",
> @@ -3474,7 +3457,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> 	ctrl->dev = lport->dev;
> 	ctrl->cnum = idx;
> 	ctrl->ioq_live = false;
> -	atomic_set(&ctrl->err_work_active, 0);
> 	init_waitqueue_head(&ctrl->ioabort_wait);
> 
> 	get_device(ctrl->dev);
> @@ -3482,7 +3464,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> 
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work);
> 	INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work);
> -	INIT_WORK(&ctrl->err_work, nvme_fc_connect_err_work);
> 	spin_lock_init(&ctrl->lock);
> 
> 	/* io queue count */
> @@ -3575,7 +3556,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> fail_ctrl:
> 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
> 	cancel_work_sync(&ctrl->ctrl.reset_work);
> -	cancel_work_sync(&ctrl->err_work);
> 	cancel_delayed_work_sync(&ctrl->connect_work);
> 
> 	ctrl->ctrl.opts = NULL;
> -- 
> 2.26.2
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Looks Good. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/4] nvme-fc: eliminate terminate_io use by nvme_fc_error_recovery

[James Smart]

[-- Attachment #1.1: Type: text/plain, Size: 9602 bytes --]

nvme_fc_error_recovery() special cases handling when in CONNECTING state
and calls __nvme_fc_terminate_io(). __nvme_fc_terminate_io() itself
special cases CONNECTING state and calls the routine to abort outstanding
ios.

Simplify the sequence by putting the call to abort outstanding ios directly
in nvme_fc_error_recovery.

Move the location of __nvme_fc_abort_outstanding_ios(), and
nvme_fc_terminate_exchange() which is called by it, to avoid adding
function prototypes for nvme_fc_error_recovery().

Signed-off-by: James Smart <james.smart@broadcom.com>
---
 drivers/nvme/host/fc.c | 185 ++++++++++++++++++-----------------------
 1 file changed, 83 insertions(+), 102 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 06fb208ab350..d65a4a9f4808 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2408,27 +2408,96 @@ nvme_fc_nvme_ctrl_freed(struct nvme_ctrl *nctrl)
 	nvme_fc_ctrl_put(ctrl);
 }
 
-static void __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl);
+/*
+ * This routine is used by the transport when it needs to find active
+ * io on a queue that is to be terminated. The transport uses
+ * blk_mq_tagset_busy_itr() to find the busy requests, which then invoke
+ * this routine to kill them on a 1 by 1 basis.
+ *
+ * As FC allocates FC exchange for each io, the transport must contact
+ * the LLDD to terminate the exchange, thus releasing the FC exchange.
+ * After terminating the exchange the LLDD will call the transport's
+ * normal io done path for the request, but it will have an aborted
+ * status. The done path will return the io request back to the block
+ * layer with an error status.
+ */
+static bool
+nvme_fc_terminate_exchange(struct request *req, void *data, bool reserved)
+{
+	struct nvme_ctrl *nctrl = data;
+	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
+	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
+
+	__nvme_fc_abort_op(ctrl, op);
+	return true;
+}
+
+/*
+ * This routine runs through all outstanding commands on the association
+ * and aborts them.  This routine is typically be called by the
+ * delete_association routine. It is also called due to an error during
+ * reconnect. In that scenario, it is most likely a command that initializes
+ * the controller, including fabric Connect commands on io queues, that
+ * may have timed out or failed thus the io must be killed for the connect
+ * thread to see the error.
+ */
+static void
+__nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
+{
+	/*
+	 * If io queues are present, stop them and terminate all outstanding
+	 * ios on them. As FC allocates FC exchange for each io, the
+	 * transport must contact the LLDD to terminate the exchange,
+	 * thus releasing the FC exchange. We use blk_mq_tagset_busy_itr()
+	 * to tell us what io's are busy and invoke a transport routine
+	 * to kill them with the LLDD.  After terminating the exchange
+	 * the LLDD will call the transport's normal io done path, but it
+	 * will have an aborted status. The done path will return the
+	 * io requests back to the block layer as part of normal completions
+	 * (but with error status).
+	 */
+	if (ctrl->ctrl.queue_count > 1) {
+		nvme_stop_queues(&ctrl->ctrl);
+		blk_mq_tagset_busy_iter(&ctrl->tag_set,
+				nvme_fc_terminate_exchange, &ctrl->ctrl);
+		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
+		if (start_queues)
+			nvme_start_queues(&ctrl->ctrl);
+	}
+
+	/*
+	 * Other transports, which don't have link-level contexts bound
+	 * to sqe's, would try to gracefully shutdown the controller by
+	 * writing the registers for shutdown and polling (call
+	 * nvme_shutdown_ctrl()). Given a bunch of i/o was potentially
+	 * just aborted and we will wait on those contexts, and given
+	 * there was no indication of how live the controlelr is on the
+	 * link, don't send more io to create more contexts for the
+	 * shutdown. Let the controller fail via keepalive failure if
+	 * its still present.
+	 */
+
+	/*
+	 * clean up the admin queue. Same thing as above.
+	 */
+	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
+	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
+				nvme_fc_terminate_exchange, &ctrl->ctrl);
+	blk_mq_tagset_wait_completed_request(&ctrl->admin_tag_set);
+}
 
 static void
 nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 {
 	/*
-	 * if an error (io timeout, etc) while (re)connecting,
-	 * it's an error on creating the new association.
-	 * Start the error recovery thread if it hasn't already
-	 * been started. It is expected there could be multiple
-	 * ios hitting this path before things are cleaned up.
+	 * if an error (io timeout, etc) while (re)connecting, the remote
+	 * port requested terminating of the association (disconnect_ls)
+	 * or an error (timeout or abort) occurred on an io while creating
+	 * the controller.  Abort any ios on the association and let the
+	 * create_association error path resolve things.
 	 */
 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
-		__nvme_fc_terminate_io(ctrl);
-
-		/*
-		 * Rescheduling the connection after recovering
-		 * from the io error is left to the reconnect work
-		 * item, which is what should have stalled waiting on
-		 * the io that had the error that scheduled this work.
-		 */
+		__nvme_fc_abort_outstanding_ios(ctrl, true);
 		return;
 	}
 
@@ -2742,30 +2811,6 @@ nvme_fc_complete_rq(struct request *rq)
 	nvme_fc_ctrl_put(ctrl);
 }
 
-/*
- * This routine is used by the transport when it needs to find active
- * io on a queue that is to be terminated. The transport uses
- * blk_mq_tagset_busy_itr() to find the busy requests, which then invoke
- * this routine to kill them on a 1 by 1 basis.
- *
- * As FC allocates FC exchange for each io, the transport must contact
- * the LLDD to terminate the exchange, thus releasing the FC exchange.
- * After terminating the exchange the LLDD will call the transport's
- * normal io done path for the request, but it will have an aborted
- * status. The done path will return the io request back to the block
- * layer with an error status.
- */
-static bool
-nvme_fc_terminate_exchange(struct request *req, void *data, bool reserved)
-{
-	struct nvme_ctrl *nctrl = data;
-	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
-	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
-
-	__nvme_fc_abort_op(ctrl, op);
-	return true;
-}
-
 
 static const struct blk_mq_ops nvme_fc_mq_ops = {
 	.queue_rq	= nvme_fc_queue_rq,
@@ -3104,60 +3149,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 }
 
 
-/*
- * This routine runs through all outstanding commands on the association
- * and aborts them.  This routine is typically be called by the
- * delete_association routine. It is also called due to an error during
- * reconnect. In that scenario, it is most likely a command that initializes
- * the controller, including fabric Connect commands on io queues, that
- * may have timed out or failed thus the io must be killed for the connect
- * thread to see the error.
- */
-static void
-__nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
-{
-	/*
-	 * If io queues are present, stop them and terminate all outstanding
-	 * ios on them. As FC allocates FC exchange for each io, the
-	 * transport must contact the LLDD to terminate the exchange,
-	 * thus releasing the FC exchange. We use blk_mq_tagset_busy_itr()
-	 * to tell us what io's are busy and invoke a transport routine
-	 * to kill them with the LLDD.  After terminating the exchange
-	 * the LLDD will call the transport's normal io done path, but it
-	 * will have an aborted status. The done path will return the
-	 * io requests back to the block layer as part of normal completions
-	 * (but with error status).
-	 */
-	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-				nvme_fc_terminate_exchange, &ctrl->ctrl);
-		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
-		if (start_queues)
-			nvme_start_queues(&ctrl->ctrl);
-	}
-
-	/*
-	 * Other transports, which don't have link-level contexts bound
-	 * to sqe's, would try to gracefully shutdown the controller by
-	 * writing the registers for shutdown and polling (call
-	 * nvme_shutdown_ctrl()). Given a bunch of i/o was potentially
-	 * just aborted and we will wait on those contexts, and given
-	 * there was no indication of how live the controlelr is on the
-	 * link, don't send more io to create more contexts for the
-	 * shutdown. Let the controller fail via keepalive failure if
-	 * its still present.
-	 */
-
-	/*
-	 * clean up the admin queue. Same thing as above.
-	 */
-	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
-	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
-				nvme_fc_terminate_exchange, &ctrl->ctrl);
-	blk_mq_tagset_wait_completed_request(&ctrl->admin_tag_set);
-}
-
 /*
  * This routine stops operation of the controller on the host side.
  * On the host os stack side: Admin and IO queues are stopped,
@@ -3290,16 +3281,6 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 static void
 __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
 {
-	/*
-	 * if state is CONNECTING - the error occurred as part of a
-	 * reconnect attempt. Abort any ios on the association and
-	 * let the create_association error paths resolve things.
-	 */
-	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
-		__nvme_fc_abort_outstanding_ios(ctrl, true);
-		return;
-	}
-
 	/*
 	 * For any other state, kill the association. As this routine
 	 * is a common io abort routine for resetting and such, after
-- 
2.26.2


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 2/4] nvme-fc: eliminate terminate_io use by nvme_fc_error_recovery

[Himanshu Madhani]

> On Oct 16, 2020, at 4:27 PM, James Smart <james.smart@broadcom.com> wrote:
> 
> nvme_fc_error_recovery() special cases handling when in CONNECTING state
> and calls __nvme_fc_terminate_io(). __nvme_fc_terminate_io() itself
> special cases CONNECTING state and calls the routine to abort outstanding
> ios.
> 
> Simplify the sequence by putting the call to abort outstanding ios directly
> in nvme_fc_error_recovery.
> 
> Move the location of __nvme_fc_abort_outstanding_ios(), and
> nvme_fc_terminate_exchange() which is called by it, to avoid adding
> function prototypes for nvme_fc_error_recovery().
> 
> Signed-off-by: James Smart <james.smart@broadcom.com>
> ---
> drivers/nvme/host/fc.c | 185 ++++++++++++++++++-----------------------
> 1 file changed, 83 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 06fb208ab350..d65a4a9f4808 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2408,27 +2408,96 @@ nvme_fc_nvme_ctrl_freed(struct nvme_ctrl *nctrl)
> 	nvme_fc_ctrl_put(ctrl);
> }
> 
> -static void __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl);
> +/*
> + * This routine is used by the transport when it needs to find active
> + * io on a queue that is to be terminated. The transport uses
> + * blk_mq_tagset_busy_itr() to find the busy requests, which then invoke
> + * this routine to kill them on a 1 by 1 basis.
> + *
> + * As FC allocates FC exchange for each io, the transport must contact
> + * the LLDD to terminate the exchange, thus releasing the FC exchange.
> + * After terminating the exchange the LLDD will call the transport's
> + * normal io done path for the request, but it will have an aborted
> + * status. The done path will return the io request back to the block
> + * layer with an error status.
> + */
> +static bool
> +nvme_fc_terminate_exchange(struct request *req, void *data, bool reserved)
> +{
> +	struct nvme_ctrl *nctrl = data;
> +	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
> +	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
> +
> +	__nvme_fc_abort_op(ctrl, op);
> +	return true;
> +}
> +
> +/*
> + * This routine runs through all outstanding commands on the association
> + * and aborts them.  This routine is typically be called by the
> + * delete_association routine. It is also called due to an error during
> + * reconnect. In that scenario, it is most likely a command that initializes
> + * the controller, including fabric Connect commands on io queues, that
> + * may have timed out or failed thus the io must be killed for the connect
> + * thread to see the error.
> + */
> +static void
> +__nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
> +{
> +	/*
> +	 * If io queues are present, stop them and terminate all outstanding
> +	 * ios on them. As FC allocates FC exchange for each io, the
> +	 * transport must contact the LLDD to terminate the exchange,
> +	 * thus releasing the FC exchange. We use blk_mq_tagset_busy_itr()
> +	 * to tell us what io's are busy and invoke a transport routine
> +	 * to kill them with the LLDD.  After terminating the exchange
> +	 * the LLDD will call the transport's normal io done path, but it
> +	 * will have an aborted status. The done path will return the
> +	 * io requests back to the block layer as part of normal completions
> +	 * (but with error status).
> +	 */
> +	if (ctrl->ctrl.queue_count > 1) {
> +		nvme_stop_queues(&ctrl->ctrl);
> +		blk_mq_tagset_busy_iter(&ctrl->tag_set,
> +				nvme_fc_terminate_exchange, &ctrl->ctrl);
> +		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
> +		if (start_queues)
> +			nvme_start_queues(&ctrl->ctrl);
> +	}
> +
> +	/*
> +	 * Other transports, which don't have link-level contexts bound
> +	 * to sqe's, would try to gracefully shutdown the controller by
> +	 * writing the registers for shutdown and polling (call
> +	 * nvme_shutdown_ctrl()). Given a bunch of i/o was potentially
> +	 * just aborted and we will wait on those contexts, and given
> +	 * there was no indication of how live the controlelr is on the
> +	 * link, don't send more io to create more contexts for the
> +	 * shutdown. Let the controller fail via keepalive failure if
> +	 * its still present.
> +	 */
> +
> +	/*
> +	 * clean up the admin queue. Same thing as above.
> +	 */
> +	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
> +	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
> +				nvme_fc_terminate_exchange, &ctrl->ctrl);
> +	blk_mq_tagset_wait_completed_request(&ctrl->admin_tag_set);
> +}
> 
> static void
> nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> {
> 	/*
> -	 * if an error (io timeout, etc) while (re)connecting,
> -	 * it's an error on creating the new association.
> -	 * Start the error recovery thread if it hasn't already
> -	 * been started. It is expected there could be multiple
> -	 * ios hitting this path before things are cleaned up.
> +	 * if an error (io timeout, etc) while (re)connecting, the remote
> +	 * port requested terminating of the association (disconnect_ls)
> +	 * or an error (timeout or abort) occurred on an io while creating
> +	 * the controller.  Abort any ios on the association and let the
> +	 * create_association error path resolve things.
> 	 */
> 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
> -		__nvme_fc_terminate_io(ctrl);
> -
> -		/*
> -		 * Rescheduling the connection after recovering
> -		 * from the io error is left to the reconnect work
> -		 * item, which is what should have stalled waiting on
> -		 * the io that had the error that scheduled this work.
> -		 */
> +		__nvme_fc_abort_outstanding_ios(ctrl, true);
> 		return;
> 	}
> 
> @@ -2742,30 +2811,6 @@ nvme_fc_complete_rq(struct request *rq)
> 	nvme_fc_ctrl_put(ctrl);
> }
> 
> -/*
> - * This routine is used by the transport when it needs to find active
> - * io on a queue that is to be terminated. The transport uses
> - * blk_mq_tagset_busy_itr() to find the busy requests, which then invoke
> - * this routine to kill them on a 1 by 1 basis.
> - *
> - * As FC allocates FC exchange for each io, the transport must contact
> - * the LLDD to terminate the exchange, thus releasing the FC exchange.
> - * After terminating the exchange the LLDD will call the transport's
> - * normal io done path for the request, but it will have an aborted
> - * status. The done path will return the io request back to the block
> - * layer with an error status.
> - */
> -static bool
> -nvme_fc_terminate_exchange(struct request *req, void *data, bool reserved)
> -{
> -	struct nvme_ctrl *nctrl = data;
> -	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
> -	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
> -
> -	__nvme_fc_abort_op(ctrl, op);
> -	return true;
> -}
> -
> 
> static const struct blk_mq_ops nvme_fc_mq_ops = {
> 	.queue_rq	= nvme_fc_queue_rq,
> @@ -3104,60 +3149,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> }
> 
> 
> -/*
> - * This routine runs through all outstanding commands on the association
> - * and aborts them.  This routine is typically be called by the
> - * delete_association routine. It is also called due to an error during
> - * reconnect. In that scenario, it is most likely a command that initializes
> - * the controller, including fabric Connect commands on io queues, that
> - * may have timed out or failed thus the io must be killed for the connect
> - * thread to see the error.
> - */
> -static void
> -__nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
> -{
> -	/*
> -	 * If io queues are present, stop them and terminate all outstanding
> -	 * ios on them. As FC allocates FC exchange for each io, the
> -	 * transport must contact the LLDD to terminate the exchange,
> -	 * thus releasing the FC exchange. We use blk_mq_tagset_busy_itr()
> -	 * to tell us what io's are busy and invoke a transport routine
> -	 * to kill them with the LLDD.  After terminating the exchange
> -	 * the LLDD will call the transport's normal io done path, but it
> -	 * will have an aborted status. The done path will return the
> -	 * io requests back to the block layer as part of normal completions
> -	 * (but with error status).
> -	 */
> -	if (ctrl->ctrl.queue_count > 1) {
> -		nvme_stop_queues(&ctrl->ctrl);
> -		blk_mq_tagset_busy_iter(&ctrl->tag_set,
> -				nvme_fc_terminate_exchange, &ctrl->ctrl);
> -		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
> -		if (start_queues)
> -			nvme_start_queues(&ctrl->ctrl);
> -	}
> -
> -	/*
> -	 * Other transports, which don't have link-level contexts bound
> -	 * to sqe's, would try to gracefully shutdown the controller by
> -	 * writing the registers for shutdown and polling (call
> -	 * nvme_shutdown_ctrl()). Given a bunch of i/o was potentially
> -	 * just aborted and we will wait on those contexts, and given
> -	 * there was no indication of how live the controlelr is on the
> -	 * link, don't send more io to create more contexts for the
> -	 * shutdown. Let the controller fail via keepalive failure if
> -	 * its still present.
> -	 */
> -
> -	/*
> -	 * clean up the admin queue. Same thing as above.
> -	 */
> -	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
> -	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
> -				nvme_fc_terminate_exchange, &ctrl->ctrl);
> -	blk_mq_tagset_wait_completed_request(&ctrl->admin_tag_set);
> -}
> -
> /*
>  * This routine stops operation of the controller on the host side.
>  * On the host os stack side: Admin and IO queues are stopped,
> @@ -3290,16 +3281,6 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
> static void
> __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
> {
> -	/*
> -	 * if state is CONNECTING - the error occurred as part of a
> -	 * reconnect attempt. Abort any ios on the association and
> -	 * let the create_association error paths resolve things.
> -	 */
> -	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
> -		__nvme_fc_abort_outstanding_ios(ctrl, true);
> -		return;
> -	}
> -
> 	/*
> 	 * For any other state, kill the association. As this routine
> 	 * is a common io abort routine for resetting and such, after
> -- 
> 2.26.2
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Looks Okay. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/4] nvme-fc: remove nvme_fc_terminate_io()

[James Smart]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]

__nvme_fc_terminate_io() is now called by only 1 place, in reset_work.
Consoldate and move the functionality of terminate_io into reset_work.

In reset_work, rather than calling the create_association directly,
schedule the connect work element to do its thing. After scheduling, flush
the connect work element to continue with semantic of not returning until
connect has been attempted at least once.

Signed-off-by: James Smart <james.smart@broadcom.com>
---
 drivers/nvme/host/fc.c | 49 ++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d65a4a9f4808..b2f9b3752df7 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3279,49 +3279,32 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 }
 
 static void
-__nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
+nvme_fc_reset_ctrl_work(struct work_struct *work)
 {
-	/*
-	 * For any other state, kill the association. As this routine
-	 * is a common io abort routine for resetting and such, after
-	 * the association is terminated, ensure that the state is set
-	 * to CONNECTING.
-	 */
+	struct nvme_fc_ctrl *ctrl =
+		container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
 
-	nvme_stop_keep_alive(&ctrl->ctrl);
+	nvme_stop_ctrl(&ctrl->ctrl);
 
 	/* will block will waiting for io to terminate */
 	nvme_fc_delete_association(ctrl);
 
-	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING &&
-	    !nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: error_recovery: Couldn't change state "
 			"to CONNECTING\n", ctrl->cnum);
-}
-
-static void
-nvme_fc_reset_ctrl_work(struct work_struct *work)
-{
-	struct nvme_fc_ctrl *ctrl =
-		container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
-	int ret;
-
-	__nvme_fc_terminate_io(ctrl);
 
-	nvme_stop_ctrl(&ctrl->ctrl);
-
-	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE)
-		ret = nvme_fc_create_association(ctrl);
-	else
-		ret = -ENOTCONN;
-
-	if (ret)
-		nvme_fc_reconnect_or_delete(ctrl, ret);
-	else
-		dev_info(ctrl->ctrl.device,
-			"NVME-FC{%d}: controller reset complete\n",
-			ctrl->cnum);
+	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
+		if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
+			dev_err(ctrl->ctrl.device,
+				"NVME-FC{%d}: failed to schedule connect "
+				"after reset\n", ctrl->cnum);
+		} else {
+			flush_delayed_work(&ctrl->connect_work);
+		}
+	} else {
+		nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
+	}
 }
 
 
-- 
2.26.2


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/4] nvme-fc: remove nvme_fc_terminate_io()

[Himanshu Madhani]

> On Oct 16, 2020, at 4:27 PM, James Smart <james.smart@broadcom.com> wrote:
> 
> __nvme_fc_terminate_io() is now called by only 1 place, in reset_work.
> Consoldate and move the functionality of terminate_io into reset_work.
> 
> In reset_work, rather than calling the create_association directly,
> schedule the connect work element to do its thing. After scheduling, flush
> the connect work element to continue with semantic of not returning until
> connect has been attempted at least once.
> 
> Signed-off-by: James Smart <james.smart@broadcom.com>
> ---
> drivers/nvme/host/fc.c | 49 ++++++++++++++----------------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index d65a4a9f4808..b2f9b3752df7 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3279,49 +3279,32 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
> }
> 
> static void
> -__nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
> +nvme_fc_reset_ctrl_work(struct work_struct *work)
> {
> -	/*
> -	 * For any other state, kill the association. As this routine
> -	 * is a common io abort routine for resetting and such, after
> -	 * the association is terminated, ensure that the state is set
> -	 * to CONNECTING.
> -	 */
> +	struct nvme_fc_ctrl *ctrl =
> +		container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
> 
> -	nvme_stop_keep_alive(&ctrl->ctrl);
> +	nvme_stop_ctrl(&ctrl->ctrl);
> 
> 	/* will block will waiting for io to terminate */
> 	nvme_fc_delete_association(ctrl);
> 
> -	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING &&
> -	    !nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
> +	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
> 		dev_err(ctrl->ctrl.device,
> 			"NVME-FC{%d}: error_recovery: Couldn't change state "
> 			"to CONNECTING\n", ctrl->cnum);
> -}
> -
> -static void
> -nvme_fc_reset_ctrl_work(struct work_struct *work)
> -{
> -	struct nvme_fc_ctrl *ctrl =
> -		container_of(work, struct nvme_fc_ctrl, ctrl.reset_work);
> -	int ret;
> -
> -	__nvme_fc_terminate_io(ctrl);
> 
> -	nvme_stop_ctrl(&ctrl->ctrl);
> -
> -	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE)
> -		ret = nvme_fc_create_association(ctrl);
> -	else
> -		ret = -ENOTCONN;
> -
> -	if (ret)
> -		nvme_fc_reconnect_or_delete(ctrl, ret);
> -	else
> -		dev_info(ctrl->ctrl.device,
> -			"NVME-FC{%d}: controller reset complete\n",
> -			ctrl->cnum);
> +	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
> +		if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
> +			dev_err(ctrl->ctrl.device,
> +				"NVME-FC{%d}: failed to schedule connect "
> +				"after reset\n", ctrl->cnum);
> +		} else {
> +			flush_delayed_work(&ctrl->connect_work);
> +		}
> +	} else {
> +		nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
> +	}
> }
> 
> 
> -- 
> 2.26.2
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Looks Okay. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/4] nvme_fc: track error_recovery while connecting

[James Smart]

[-- Attachment #1.1: Type: text/plain, Size: 2824 bytes --]

Whenever there are errors during CONNECTING, the driver recovers by
aborting all outstanding ios and counts on the io completion to fail them
and thus the connection/association they are on.  However, the connection
failure depends on a failure state from the core routines.  Not all
commands that are issued by the core routine are guaranteed to cause a
failure of the core routine. They may be treated as a failure status and
the status is then ignored.

As such, whenever the transport enters error_recovery while CONNECTING,
it will set a new flag indicating an association failed. The
create_association routine which creates and initializes the controller,
will monitor the state of the flag as well as the core routine error
status and ensure the association fails if there was an error.

Signed-off-by: James Smart <james.smart@broadcom.com>
---
 drivers/nvme/host/fc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index b2f9b3752df7..6352068c0c4a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -142,7 +142,8 @@ struct nvme_fc_rport {
 
 /* fc_ctrl flags values - specified as bit positions */
 #define ASSOC_ACTIVE		0
-#define FCCTRL_TERMIO		1
+#define ASSOC_FAILED		1
+#define FCCTRL_TERMIO		2
 
 struct nvme_fc_ctrl {
 	spinlock_t		lock;
@@ -2498,6 +2499,7 @@ nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 	 */
 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
 		__nvme_fc_abort_outstanding_ios(ctrl, true);
+		set_bit(ASSOC_FAILED, &ctrl->flags);
 		return;
 	}
 
@@ -3030,6 +3032,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 		ctrl->cnum, ctrl->lport->localport.port_name,
 		ctrl->rport->remoteport.port_name, ctrl->ctrl.opts->subsysnqn);
 
+	clear_bit(ASSOC_FAILED, &ctrl->flags);
+
 	/*
 	 * Create the admin queue
 	 */
@@ -3058,7 +3062,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	 */
 
 	ret = nvme_enable_ctrl(&ctrl->ctrl);
-	if (ret)
+	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
 		goto out_disconnect_admin_queue;
 
 	ctrl->ctrl.max_segments = ctrl->lport->ops->max_sgl_segments;
@@ -3068,7 +3072,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
 
 	ret = nvme_init_identify(&ctrl->ctrl);
-	if (ret)
+	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
 		goto out_disconnect_admin_queue;
 
 	/* sanity checks */
@@ -3113,9 +3117,9 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 			ret = nvme_fc_create_io_queues(ctrl);
 		else
 			ret = nvme_fc_recreate_io_queues(ctrl);
-		if (ret)
-			goto out_term_aen_ops;
 	}
+	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
+		goto out_term_aen_ops;
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
 
-- 
2.26.2


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/4] nvme_fc: track error_recovery while connecting

[Himanshu Madhani]

> On Oct 16, 2020, at 4:27 PM, James Smart <james.smart@broadcom.com> wrote:
> 
> Whenever there are errors during CONNECTING, the driver recovers by
> aborting all outstanding ios and counts on the io completion to fail them
> and thus the connection/association they are on.  However, the connection
> failure depends on a failure state from the core routines.  Not all
> commands that are issued by the core routine are guaranteed to cause a
> failure of the core routine. They may be treated as a failure status and
> the status is then ignored.
> 
> As such, whenever the transport enters error_recovery while CONNECTING,
> it will set a new flag indicating an association failed. The
> create_association routine which creates and initializes the controller,
> will monitor the state of the flag as well as the core routine error
> status and ensure the association fails if there was an error.
> 
> Signed-off-by: James Smart <james.smart@broadcom.com>
> ---
> drivers/nvme/host/fc.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index b2f9b3752df7..6352068c0c4a 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -142,7 +142,8 @@ struct nvme_fc_rport {
> 
> /* fc_ctrl flags values - specified as bit positions */
> #define ASSOC_ACTIVE		0
> -#define FCCTRL_TERMIO		1
> +#define ASSOC_FAILED		1
> +#define FCCTRL_TERMIO		2
> 
> struct nvme_fc_ctrl {
> 	spinlock_t		lock;
> @@ -2498,6 +2499,7 @@ nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> 	 */
> 	if (ctrl->ctrl.state == NVME_CTRL_CONNECTING) {
> 		__nvme_fc_abort_outstanding_ios(ctrl, true);
> +		set_bit(ASSOC_FAILED, &ctrl->flags);
> 		return;
> 	}
> 
> @@ -3030,6 +3032,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> 		ctrl->cnum, ctrl->lport->localport.port_name,
> 		ctrl->rport->remoteport.port_name, ctrl->ctrl.opts->subsysnqn);
> 
> +	clear_bit(ASSOC_FAILED, &ctrl->flags);
> +
> 	/*
> 	 * Create the admin queue
> 	 */
> @@ -3058,7 +3062,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> 	 */
> 
> 	ret = nvme_enable_ctrl(&ctrl->ctrl);
> -	if (ret)
> +	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
> 		goto out_disconnect_admin_queue;
> 
> 	ctrl->ctrl.max_segments = ctrl->lport->ops->max_sgl_segments;
> @@ -3068,7 +3072,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
> 
> 	ret = nvme_init_identify(&ctrl->ctrl);
> -	if (ret)
> +	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
> 		goto out_disconnect_admin_queue;
> 
> 	/* sanity checks */
> @@ -3113,9 +3117,9 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> 			ret = nvme_fc_create_io_queues(ctrl);
> 		else
> 			ret = nvme_fc_recreate_io_queues(ctrl);
> -		if (ret)
> -			goto out_term_aen_ops;
> 	}
> +	if (ret || test_bit(ASSOC_FAILED, &ctrl->flags))
> +		goto out_term_aen_ops;
> 
> 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> 
> -- 
> 2.26.2
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Looks fine. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme