* [PATCH 0/2] nvme: improve logging @ 2019-04-25 2:40 Chaitanya Kulkarni 2019-04-25 2:40 ` [PATCH 1/2] " Chaitanya Kulkarni 2019-04-25 2:40 ` [PATCH 2/2] nvme-multipath: " Chaitanya Kulkarni 0 siblings, 2 replies; 8+ messages in thread From: Chaitanya Kulkarni @ 2019-04-25 2:40 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme: improve logging 2019-04-25 2:40 [PATCH 0/2] nvme: improve logging Chaitanya Kulkarni @ 2019-04-25 2:40 ` Chaitanya Kulkarni 2019-04-28 12:05 ` Christoph Hellwig 2019-04-25 2:40 ` [PATCH 2/2] nvme-multipath: " Chaitanya Kulkarni 1 sibling, 1 reply; 8+ messages in thread From: Chaitanya Kulkarni @ 2019-04-25 2:40 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme: improve logging 2019-04-25 2:40 ` [PATCH 1/2] " Chaitanya Kulkarni @ 2019-04-28 12:05 ` Christoph Hellwig 2019-04-28 19:32 ` Chaitanya Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2019-04-28 12:05 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme: improve logging 2019-04-28 12:05 ` Christoph Hellwig @ 2019-04-28 19:32 ` Chaitanya Kulkarni 2019-04-29 11:50 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Chaitanya Kulkarni @ 2019-04-28 19:32 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme: improve logging 2019-04-28 19:32 ` Chaitanya Kulkarni @ 2019-04-29 11:50 ` Christoph Hellwig 2019-04-29 15:58 ` Chaitanya Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2019-04-29 11:50 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme: improve logging 2019-04-29 11:50 ` Christoph Hellwig @ 2019-04-29 15:58 ` Chaitanya Kulkarni 0 siblings, 0 replies; 8+ messages in thread From: Chaitanya Kulkarni @ 2019-04-29 15:58 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] nvme-multipath: improve logging 2019-04-25 2:40 [PATCH 0/2] nvme: improve logging Chaitanya Kulkarni 2019-04-25 2:40 ` [PATCH 1/2] " Chaitanya Kulkarni @ 2019-04-25 2:40 ` Chaitanya Kulkarni 2019-04-28 12:09 ` Christoph Hellwig 1 sibling, 1 reply; 8+ messages in thread From: Chaitanya Kulkarni @ 2019-04-25 2:40 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] nvme-multipath: improve logging 2019-04-25 2:40 ` [PATCH 2/2] nvme-multipath: " Chaitanya Kulkarni @ 2019-04-28 12:09 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2019-04-28 12:09 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-29 15:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-25 2:40 [PATCH 0/2] nvme: improve logging Chaitanya Kulkarni 2019-04-25 2:40 ` [PATCH 1/2] " Chaitanya Kulkarni 2019-04-28 12:05 ` Christoph Hellwig 2019-04-28 19:32 ` Chaitanya Kulkarni 2019-04-29 11:50 ` Christoph Hellwig 2019-04-29 15:58 ` Chaitanya Kulkarni 2019-04-25 2:40 ` [PATCH 2/2] nvme-multipath: " Chaitanya Kulkarni 2019-04-28 12:09 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.