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

[PATCH 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.

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 | 36 +++++++++++++++++++++++-------------
 drivers/nvme/host/fc.c   | 23 +++++++++++++++++------
 drivers/nvme/host/nvme.h |  1 +
 3 files changed, 41 insertions(+), 19 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>
---
 drivers/nvme/host/core.c | 36 +++++++++++++++++++++++-------------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2e4fa694f79..2632276458f5 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 NULL;
+}
+EXPORT_SYMBOL_GPL(nvme_ctrl_state_name);
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =
@@ -2989,19 +3008,10 @@ 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]);
+	const char *state_name = nvme_ctrl_state_name(ctrl);
+
+	if (state_name)
+		return sprintf(buf, "%s\n", state_name);
 
 	return sprintf(buf, "unknown state\n");
 }
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 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>
---
 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.

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

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index e5c81ba2b7a1..9f9300cbdb62 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2620,7 +2620,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 {
 	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
 	int ret;
-	bool changed;
 
 	++ctrl->ctrl.nr_reconnects;
 
@@ -2725,12 +2724,19 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 			goto out_term_aen_ops;
 	}
 
-	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
+	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
+		if (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;
 
-	if (changed)
-		nvme_start_ctrl(&ctrl->ctrl);
+	nvme_start_ctrl(&ctrl->ctrl);
 
 	return 0;	/* Success */
 
-- 
2.16.4

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

[Minwoo Im]

Hi Hannes,

> +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 NULL;

Perhaps we can return "unknown" here that can make callers do not
need to consider that if it gives NULL and not return a string there also.

> @@ -2989,19 +3008,10 @@ 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]);
> +	const char *state_name = nvme_ctrl_state_name(ctrl);
> +
> +	if (state_name)
> +		return sprintf(buf, "%s\n", state_name);
>  
>  	return sprintf(buf, "unknown state\n");

If so, we can make these three lines to a single one without considering
the NULL case.

What do you think?

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

[James Smart]

On 5/16/2019 1:37 AM, Hannes Reinecke wrote:
> Separate out nvme_ctrl_state_name() to return the controller state
> as a string.
>
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/host/core.c | 36 +++++++++++++++++++++++-------------
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 24 insertions(+), 13 deletions(-)
>
>

Reviewed-by:? James Smart? <james.smart at broadcom.com>

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

[James Smart]

On 5/16/2019 1:37 AM, Hannes Reinecke wrote:
> 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>
> ---
>   drivers/nvme/host/fc.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
>

Reviewed-by:? James Smart?? <james.smart at broadcom.com>

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

[James Smart]

On 5/16/2019 1:37 AM, Hannes Reinecke wrote:
> 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.
>
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/host/fc.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index e5c81ba2b7a1..9f9300cbdb62 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2620,7 +2620,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>   {
>   	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
>   	int ret;
> -	bool changed;
>   
>   	++ctrl->ctrl.nr_reconnects;
>   
> @@ -2725,12 +2724,19 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>   			goto out_term_aen_ops;
>   	}
>   
> -	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> +	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
> +		if (ctrl->ctrl.state != NVME_CTRL_DELETING) {
is this easier to arrange as:
 ??? if (nvme_change_ctrl_state(...) &&
 ???????? 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;
>   
> -	if (changed)
> -		nvme_start_ctrl(&ctrl->ctrl);
> +	nvme_start_ctrl(&ctrl->ctrl);

I don't think you want to eliminate this check - you don't want to call 
nvme_start_ctrl() if state did transition to DELETING. You want to 
continue out to release the create thread, but the delete work will be 
coming along shortly.

-- james

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

[Arun Easi]

On Thu, 16 May 2019, 1:37am, Hannes Reinecke wrote:

> 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.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>  drivers/nvme/host/fc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index e5c81ba2b7a1..9f9300cbdb62 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2620,7 +2620,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>  {
>  	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
>  	int ret;
> -	bool changed;
>  
>  	++ctrl->ctrl.nr_reconnects;
>  
> @@ -2725,12 +2724,19 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>  			goto out_term_aen_ops;
>  	}
>  
> -	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> +	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {

Should not this be !nvme_change_ctrl_state()?

Regards,
-Arun
> +		if (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;
>  
> -	if (changed)
> -		nvme_start_ctrl(&ctrl->ctrl);
> +	nvme_start_ctrl(&ctrl->ctrl);
>  
>  	return 0;	/* Success */
>  
> 

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

[Arun Easi]

Please ignore the comment. I see that v2 already has addressed this.

Regards,
-Arun

On Fri, 17 May 2019, 5:18pm, Arun Easi wrote:

> On Thu, 16 May 2019, 1:37am, Hannes Reinecke wrote:
> 
> > 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.
> > 
> > Signed-off-by: Hannes Reinecke <hare at suse.com>
> > ---
> >  drivers/nvme/host/fc.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> > index e5c81ba2b7a1..9f9300cbdb62 100644
> > --- a/drivers/nvme/host/fc.c
> > +++ b/drivers/nvme/host/fc.c
> > @@ -2620,7 +2620,6 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> >  {
> >  	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
> >  	int ret;
> > -	bool changed;
> >  
> >  	++ctrl->ctrl.nr_reconnects;
> >  
> > @@ -2725,12 +2724,19 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> >  			goto out_term_aen_ops;
> >  	}
> >  
> > -	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> > +	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
> 
> Should not this be !nvme_change_ctrl_state()?
> 
> Regards,
> -Arun
> > +		if (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;
> >  
> > -	if (changed)
> > -		nvme_start_ctrl(&ctrl->ctrl);
> > +	nvme_start_ctrl(&ctrl->ctrl);
> >  
> >  	return 0;	/* Success */
> >  
> > 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
> 

[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 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