[PATCHv2 0/3] nvme-fc: track state change failures

[PATCHv2 0/3] nvme-fc: track state change failures

[Hannes Reinecke]

Hi all,

this patchset improves logging in the nvme-fc driver if expcected
state changes fail, and fixes up one case where a failed state change
might lead to a stuck controller.

Changes to v1:
- simplified accessor function as suggested by Minwoo Im
- Included suggestions from James Smart

Hannes Reinecke (3):
  nvme: separate out nvme_ctrl_state_name()
  nvme-fc: track state change failures during reconnect
  nvme-fc: fail reconnect if state change fails

 drivers/nvme/host/core.c | 37 ++++++++++++++++++++++---------------
 drivers/nvme/host/fc.c   | 24 ++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 3 files changed, 45 insertions(+), 17 deletions(-)

-- 
2.16.4

[PATCH 1/3] nvme: separate out nvme_ctrl_state_name()

[Hannes Reinecke]

Separate out nvme_ctrl_state_name() to return the controller state
as a string.

Signed-off-by: Hannes Reinecke <hare at suse.com>
Reviewed-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/core.c | 37 ++++++++++++++++++++++---------------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2e4fa694f79..bd1bc7fcbcde 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -380,6 +380,25 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
 
+static const char *const nvme_ctrl_state_names[] = {
+	[NVME_CTRL_NEW]		= "new",
+	[NVME_CTRL_LIVE]	= "live",
+	[NVME_CTRL_ADMIN_ONLY]	= "only-admin",
+	[NVME_CTRL_RESETTING]	= "resetting",
+	[NVME_CTRL_CONNECTING]	= "connecting",
+	[NVME_CTRL_DELETING]	= "deleting",
+	[NVME_CTRL_DEAD]	= "dead",
+};
+
+const char *nvme_ctrl_state_name(struct nvme_ctrl *ctrl)
+{
+	if ((unsigned)ctrl->state < ARRAY_SIZE(nvme_ctrl_state_names) &&
+	    nvme_ctrl_state_names[ctrl->state])
+		return nvme_ctrl_state_names[ctrl->state];
+	return "unknown state";
+}
+EXPORT_SYMBOL_GPL(nvme_ctrl_state_name);
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =
@@ -2989,21 +3008,9 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
 				     char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	static const char *const state_name[] = {
-		[NVME_CTRL_NEW]		= "new",
-		[NVME_CTRL_LIVE]	= "live",
-		[NVME_CTRL_ADMIN_ONLY]	= "only-admin",
-		[NVME_CTRL_RESETTING]	= "resetting",
-		[NVME_CTRL_CONNECTING]	= "connecting",
-		[NVME_CTRL_DELETING]	= "deleting",
-		[NVME_CTRL_DEAD]	= "dead",
-	};
-
-	if ((unsigned)ctrl->state < ARRAY_SIZE(state_name) &&
-	    state_name[ctrl->state])
-		return sprintf(buf, "%s\n", state_name[ctrl->state]);
-
-	return sprintf(buf, "unknown state\n");
+	const char *state_name = nvme_ctrl_state_name(ctrl);
+
+	return sprintf(buf, "%s\n", state_name);
 }
 
 static DEVICE_ATTR(state, S_IRUGO, nvme_sysfs_show_state, NULL);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5ee75b5ff83f..b3b13e465dc6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -419,6 +419,7 @@ void nvme_complete_rq(struct request *req);
 bool nvme_cancel_request(struct request *req, void *data, bool reserved);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
+const char *nvme_ctrl_state_name(struct nvme_ctrl *ctrl);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
-- 
2.16.4

[PATCH 1/3] nvme: separate out nvme_ctrl_state_name()

[Minwoo Im]

Hi Hannes,

Thanks for applying my review point on this.
This looks good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>

[PATCH 2/3] nvme-fc: track state change failures during reconnect

[Hannes Reinecke]

The nvme-fc driver has several situation under which an expected
state transition fails, but doesn't print out any messages if
this happens.
The patch adds logging for these situations.

Signed-off-by: Hannes Reinecke <hare at suse.com>
Reviewed-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0c9e036afd09..e5c81ba2b7a1 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2867,8 +2867,12 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 	unsigned long recon_delay = ctrl->ctrl.opts->reconnect_delay * HZ;
 	bool recon = true;
 
-	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING)
+	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
+		dev_info(ctrl->ctrl.device,
+			 "NVME-FC{%d}: couldn't reconnect in state %s\n",
+			 ctrl->cnum, nvme_ctrl_state_name(&ctrl->ctrl));
 		return;
+	}
 
 	if (portptr->port_state == FC_OBJSTATE_ONLINE)
 		dev_info(ctrl->ctrl.device,
@@ -2914,7 +2918,8 @@ __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
 	    !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);
+			"from %s to CONNECTING\n", ctrl->cnum,
+			nvme_ctrl_state_name(&ctrl->ctrl));
 }
 
 static void
-- 
2.16.4

[PATCH 3/3] nvme-fc: fail reconnect if state change fails

[Hannes Reinecke]

If the final state change to LIVE in nvme_fc_create_association()
fails the controller is not operational as no I/O is possible.
So we should be returning an error here to reschedule reconnect.
Additionally it should only be called while in CONNECTING state, so
add a check for this, too.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/fc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index e5c81ba2b7a1..a8ef62512d9a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2624,6 +2624,14 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 
 	++ctrl->ctrl.nr_reconnects;
 
+	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
+		dev_info(ctrl->ctrl.device,
+			 "NVME-FC{%d}: state %s cancelled new "
+			 "association attempt\n",
+			 ctrl->cnum, nvme_ctrl_state_name(&ctrl->ctrl));
+		return -ENODEV;
+	}
+
 	if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE)
 		return -ENODEV;
 
@@ -2726,6 +2734,13 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
+	if (!changed && ctrl->ctrl.state != NVME_CTRL_DELETING) {
+		dev_err(ctrl->ctrl.device,
+			"NVME-FC{%d}: error_recovery: Couldn't change "
+			"state from %s to LIVE\n", ctrl->cnum,
+			nvme_ctrl_state_name(&ctrl->ctrl));
+		return -EAGAIN;
+	}
 
 	ctrl->ctrl.nr_reconnects = 0;
 
-- 
2.16.4

[PATCH 3/3] nvme-fc: fail reconnect if state change fails

[James Smart]

On 5/16/2019 11:42 PM, Hannes Reinecke wrote:
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index e5c81ba2b7a1..a8ef62512d9a 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2726,6 +2734,13 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>   	}
>   
>   	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> +	if (!changed && ctrl->ctrl.state != NVME_CTRL_DELETING) {
> +		dev_err(ctrl->ctrl.device,
> +			"NVME-FC{%d}: error_recovery: Couldn't change "
> +			"state from %s to LIVE\n", ctrl->cnum,
> +			nvme_ctrl_state_name(&ctrl->ctrl));
> +		return -EAGAIN;
> +	}
>   
>   	ctrl->ctrl.nr_reconnects = 0;
>   

Sorry, I should have spotted this last time.?? We shouldn't be exiting 
in the bad case - we should be setting ret=-EAGAIN then have a goto 
out_term_aen_ops so that whatever was built up for the association is 
torn down.

-- james

[PATCH 3/3] nvme-fc: fail reconnect if state change fails

[Hannes Reinecke]

On 5/17/19 6:10 PM, James Smart wrote:
> On 5/16/2019 11:42 PM, Hannes Reinecke wrote:
>> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
>> index e5c81ba2b7a1..a8ef62512d9a 100644
>> --- a/drivers/nvme/host/fc.c
>> +++ b/drivers/nvme/host/fc.c
>> @@ -2726,6 +2734,13 @@ nvme_fc_create_association(struct nvme_fc_ctrl 
>> *ctrl)
>> ????? }
>> ????? changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>> +??? if (!changed && ctrl->ctrl.state != NVME_CTRL_DELETING) {
>> +??????? dev_err(ctrl->ctrl.device,
>> +??????????? "NVME-FC{%d}: error_recovery: Couldn't change "
>> +??????????? "state from %s to LIVE\n", ctrl->cnum,
>> +??????????? nvme_ctrl_state_name(&ctrl->ctrl));
>> +??????? return -EAGAIN;
>> +??? }
>> ????? ctrl->ctrl.nr_reconnects = 0;
> 
> Sorry, I should have spotted this last time.?? We shouldn't be exiting 
> in the bad case - we should be setting ret=-EAGAIN then have a goto 
> out_term_aen_ops so that whatever was built up for the association is 
> torn down.
> 
I _actually_ did do that on purpose; I did ask me the very same question.
Reasoning was that this is the final step, and reverting this would 
amount to a full reset. Hence we could just return an error, and let the 
reset handle things. Especially as we do not clear the reconnect counter 
here.
Am I wrong?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH 3/3] nvme-fc: fail reconnect if state change fails

[James Smart]

On 5/17/2019 11:34 AM, Hannes Reinecke wrote:
> On 5/17/19 6:10 PM, James Smart wrote:
>> On 5/16/2019 11:42 PM, Hannes Reinecke wrote:
>>> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
>>> index e5c81ba2b7a1..a8ef62512d9a 100644
>>> --- a/drivers/nvme/host/fc.c
>>> +++ b/drivers/nvme/host/fc.c
>>> @@ -2726,6 +2734,13 @@ nvme_fc_create_association(struct 
>>> nvme_fc_ctrl *ctrl)
>>> ????? }
>>> ????? changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
>>> +??? if (!changed && ctrl->ctrl.state != NVME_CTRL_DELETING) {
>>> +??????? dev_err(ctrl->ctrl.device,
>>> +??????????? "NVME-FC{%d}: error_recovery: Couldn't change "
>>> +??????????? "state from %s to LIVE\n", ctrl->cnum,
>>> +??????????? nvme_ctrl_state_name(&ctrl->ctrl));
>>> +??????? return -EAGAIN;
>>> +??? }
>>> ????? ctrl->ctrl.nr_reconnects = 0;
>>
>> Sorry, I should have spotted this last time.?? We shouldn't be 
>> exiting in the bad case - we should be setting ret=-EAGAIN then have 
>> a goto out_term_aen_ops so that whatever was built up for the 
>> association is torn down.
>>
> I _actually_ did do that on purpose; I did ask me the very same question.
> Reasoning was that this is the final step, and reverting this would 
> amount to a full reset. Hence we could just return an error, and let 
> the reset handle things. Especially as we do not clear the reconnect 
> counter here.
> Am I wrong?
>

Yes, partly no.??? No, in that tearing down what you built up for the 
association will be similar to the action of a reset but the problem is 
- the association create expected that "teardown" to have happened 
before it runs. In this new case, data structures are left built up, 
queues live, flags live, yet the controller by state is in limbo.? After 
the failure return, all that happens in the transport schedules a 
connect timeout. When the timeout occurs it will attempt to rebuild 
everything that is already in place. There will be no attempt to 
terminate the prior association you left live and I have no idea what 
some of the reallocation paths will do when they start building up on 
things already live. At best they just overlay and we have leaks - I 
don't know. You've created a very new and unique path by returning.? 
Don't do that.

-- james