[PATCH 0/2] nvme: improve logging

[PATCH 0/2] nvme: improve logging

[Chaitanya Kulkarni]

Hi,

These code changes are originally posted by Hannes Reinecke <hare at suse.com> :-
http://lists.infradead.org/pipermail/linux-nvme/2019-April/023336.html

I've added all the comments posted on this patch-series along with my signoff
to the patches. Please keep my sign off if Hannes is okay with it.

Regards,
-Chaitanya

Chaitanya Kulkarni (2):
  nvme: improve logging
  nvme-multipath: improve logging

 drivers/nvme/host/core.c      | 42 ++++++++++++++++++++++-------------
 drivers/nvme/host/multipath.c |  9 +++++++-
 2 files changed, 34 insertions(+), 17 deletions(-)

-- 
2.17.0

[PATCH 1/2] nvme: improve logging

[Chaitanya Kulkarni]

Currently nvme is very reluctant if it comes to logging anything,
_unless_ it's an ANA AEN. So this patch tries to remedy this by
decreasing the priority for the ANA AEN logging message, and improve
the logging when calling nvme_reset_ctrl().

Signed-off-by: Hannes Reinecke <hare at suse.com>
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c | 42 +++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ddb943395118..eac2b4441a42 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -94,6 +94,23 @@ static void nvme_put_subsystem(struct nvme_subsystem *subsys);
 static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
 					   unsigned nsid);
 
+static const char *nvme_ctrl_state_name(struct nvme_ctrl *ctrl)
+{
+	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 int)ctrl->state < ARRAY_SIZE(state_name))
+		return state_name[ctrl->state];
+	return "unknown";
+}
+
 static void nvme_set_queue_dying(struct nvme_ns *ns)
 {
 	/*
@@ -119,10 +136,15 @@ static void nvme_queue_scan(struct nvme_ctrl *ctrl)
 
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
 {
-	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) {
+		dev_warn(ctrl->device, "cannot reset ctrl in state %s\n",
+			 nvme_ctrl_state_name(ctrl));
 		return -EBUSY;
-	if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
+	}
+	if (!queue_work(nvme_reset_wq, &ctrl->reset_work)) {
+		dev_dbg(ctrl->device, "ctrl reset already queued\n");
 		return -EBUSY;
+	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nvme_reset_ctrl);
@@ -2955,21 +2977,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]);
+	const char *state_name = nvme_ctrl_state_name(ctrl);
 
-	return sprintf(buf, "unknown state\n");
+	return sprintf(buf, "%s\n", state_name);
 }
 
 static DEVICE_ATTR(state, S_IRUGO, nvme_sysfs_show_state, NULL);
-- 
2.17.0

[PATCH 2/2] nvme-multipath: improve logging

[Chaitanya Kulkarni]

This patch improves the logging for nvme-multipath code.

Signed-off-by: Hannes Reinecke <hare at suse.com>
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/multipath.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f0716f6ce41f..721aa2ea4363 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -81,6 +81,9 @@ void nvme_failover_req(struct request *req)
 		 * Reset the controller for any non-ANA error as we don't know
 		 * what caused the error.
 		 */
+		dev_info(ns->ctrl->device,
+			"nvme status 0x%04x, resetting controller\n",
+			status);
 		nvme_reset_ctrl(ns->ctrl);
 		break;
 	}
@@ -421,9 +424,13 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
 
-	dev_info(ctrl->device, "ANA group %d: %s.\n",
+	if (desc->state < ARRAY_SIZE(nvme_ana_state_names))
+		dev_dbg(ctrl->device, "ANA group %d: %s.\n",
 			le32_to_cpu(desc->grpid),
 			nvme_ana_state_names[desc->state]);
+	else
+		dev_dbg(ctrl->device, "ANA group %d: unknown.\n",
+			le32_to_cpu(desc->grpid));
 
 	if (desc->state == NVME_ANA_CHANGE)
 		(*nr_change_groups)++;
-- 
2.17.0

[PATCH 1/2] nvme: improve logging

[Christoph Hellwig]

On Wed, Apr 24, 2019@07:40:40PM -0700, Chaitanya Kulkarni wrote:
> Currently nvme is very reluctant if it comes to logging anything,
> _unless_ it's an ANA AEN. So this patch tries to remedy this by
> decreasing the priority for the ANA AEN logging message, and improve
> the logging when calling nvme_reset_ctrl().
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

If this is originally from Hannes it also needs a From: tag in the
body so that git attribute it to him.

Also please add a changelog vs Hannes v1 in the cover letter.

> +static const char *nvme_ctrl_state_name(struct nvme_ctrl *ctrl)
> +{
> +	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",
> +	};

Can we move this array outside the function?  Yes, with the static
it doesn't really make a difference, but it just feels nicer to me.

> +	const char *state_name = nvme_ctrl_state_name(ctrl);
>  
> +	return sprintf(buf, "%s\n", state_name);

I don't think we even need the state_name local variable here.

[PATCH 2/2] nvme-multipath: improve logging

[Christoph Hellwig]

On Wed, Apr 24, 2019@07:40:41PM -0700, Chaitanya Kulkarni wrote:
> This patch improves the logging for nvme-multipath code.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> ---
>  drivers/nvme/host/multipath.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index f0716f6ce41f..721aa2ea4363 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -81,6 +81,9 @@ void nvme_failover_req(struct request *req)
>  		 * Reset the controller for any non-ANA error as we don't know
>  		 * what caused the error.
>  		 */
> +		dev_info(ns->ctrl->device,
> +			"nvme status 0x%04x, resetting controller\n",
> +			status);

Do we need something ind?cating this is the multipath driver failing
over?

> -	dev_info(ctrl->device, "ANA group %d: %s.\n",
> +	if (desc->state < ARRAY_SIZE(nvme_ana_state_names))
> +		dev_dbg(ctrl->device, "ANA group %d: %s.\n",
>  			le32_to_cpu(desc->grpid),
>  			nvme_ana_state_names[desc->state]);
> +	else
> +		dev_dbg(ctrl->device, "ANA group %d: unknown.\n",
> +			le32_to_cpu(desc->grpid));

I don't think we need the unknown case here as nvme_parse_ana_log
rejects invalid states already.

[PATCH 1/2] nvme: improve logging

[Chaitanya Kulkarni]

On 4/28/19 5:05 AM, Christoph Hellwig wrote:
> On Wed, Apr 24, 2019@07:40:40PM -0700, Chaitanya Kulkarni wrote:
>> Currently nvme is very reluctant if it comes to logging anything,
>> _unless_ it's an ANA AEN. So this patch tries to remedy this by
>> decreasing the priority for the ANA AEN logging message, and improve
>> the logging when calling nvme_reset_ctrl().
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> 
> If this is originally from Hannes it also needs a From: tag in the
> body so that git attribute it to him.
> 
> Also please add a changelog vs Hannes v1 in the cover letter.
Okay.
> 
>> +static const char *nvme_ctrl_state_name(struct nvme_ctrl *ctrl)
>> +{
>> +	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",
>> +	};
> 
> Can we move this array outside the function?  Yes, with the static
> it doesn't really make a difference, but it just feels nicer to me.
I've actually suggested that in my review but then it is static 
afterall. I'll move it to header near NVME_CTRL_XXX definitions.
Let me know if you prefer any other place.
> 
>> +	const char *state_name = nvme_ctrl_state_name(ctrl);
>>   
>> +	return sprintf(buf, "%s\n", state_name);
> 
> I don't think we even need the state_name local variable here.
> 
Okay, will send out next version.

[PATCH 1/2] nvme: improve logging

[Christoph Hellwig]

On Sun, Apr 28, 2019@07:32:32PM +0000, Chaitanya Kulkarni wrote:
> I've actually suggested that in my review but then it is static 
> afterall. I'll move it to header near NVME_CTRL_XXX definitions.
> Let me know if you prefer any other place.

Just keep it near the top of multipath.c..  But I can fix that up
if it is the last remanining issue in the new version.

[PATCH 1/2] nvme: improve logging

[Chaitanya Kulkarni]

Okay, that works too.

?On 4/29/19, 4:50 AM, "Christoph Hellwig" <hch@infradead.org> wrote:

    On Sun, Apr 28, 2019@07:32:32PM +0000, Chaitanya Kulkarni wrote:
    > I've actually suggested that in my review but then it is static 
    > afterall. I'll move it to header near NVME_CTRL_XXX definitions.
    > Let me know if you prefer any other place.
    
    Just keep it near the top of multipath.c..  But I can fix that up
    if it is the last remanining issue in the new version.