nvme: ANA implementation fixes

Hi Christoph,

here's a set of patches relative to your nvme-ana branch. Most of these issues have been encountered when running my blktest test case (to be submitted separately). With these patches I haven't found any other issues with the ANA implementation, so we're good to go in principle.

Note, I've submitted another patch ("nvme: register ns_id attributes ...") which will probably clash with the ANA series and these patches.

But we can fix it up once we've agreed on when and how to submit the ANA series proper.

As usual, comments and reviews are welcome.

Hannes Reinecke (5):
nvme: count all ANA groups for ANA Log page
nvmet: fixup crash on NULL device path
nvme-multipath: fixup crash during disconnect
nvme-multipath: disable ANA support if parsing fails
nvme-multipath: parse entire ANA log page in nvme_set_ns_ana_state()

 drivers/nvme/host/core.c        |  2 +-
 drivers/nvme/host/multipath.c   | 77 ++++++++++++++++++++++++++++-------------
 drivers/nvme/target/admin-cmd.c |  5 +++
 drivers/nvme/target/configfs.c  |  6 ++++
 drivers/nvme/target/core.c      |  6 +++-
 5 files changed, 69 insertions(+), 27 deletions(-)

2.12.3

When issuing a short read on the ANA log page the number of groups should not change, even though the final returned data might contain less groups than that number.

---
 drivers/nvme/target/admin-cmd.c | 5 +++++
 1 file changed, 5 insertions(+)

index 8de264ece826..92682ed11bc4 100644
@@ -200,6 +200,11 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 		offset += len;
 		ngrps++;
 	}
+	while (grpid <= NVMET_MAX_ANAGRPS) {
+		if (nvmet_ana_group_enabled[grpid])
+			ngrps++;
+		grpid++;
+	}
 
 	hdr.chgcnt = cpu_to_le64(nvmet_ana_chgcnt);
 	hdr.ngrps = cpu_to_le16(ngrps);

2.12.3

This is based on a "clarification" to the spec that Fred posted a few days ago, which honestly looks entirely bogus. I'll wait until that is sorted out in the WG.

Applied to nvme-4.19 now that 4004a is making progress..

When writing an empty string into the device_path attribute the kernel will crash with

nvmet: failed to open block device (null): (-22)

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000

This patch sanitizes the error handling for invalid device path settings.

---
 drivers/nvme/target/configfs.c | 6 ++++++
 drivers/nvme/target/core.c     | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 094d216ae62a..76efc8120c00 100644
@@ -295,6 +295,12 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,
 	ns->device_path = kstrndup(page, strcspn(page, "\n"), GFP_KERNEL);
 	if (!ns->device_path)
 		goto out_unlock;
+	if (!strlen(ns->device_path)) {
+		kfree(ns->device_path);
+		ns->device_path = NULL;
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 
 	mutex_unlock(&subsys->lock);
 	return count;

index 0aeccbddd716..956d293d293d 100644
@@ -373,8 +373,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 	if (ns->enabled)
 		goto out_unlock;
 
+	if (!ns->device_path) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
 	ret = nvmet_bdev_ns_enable(ns);
-	if (ret)
+	if (ret == -ENOTBLK)
 		ret = nvmet_file_ns_enable(ns);
 	if (ret)
 		goto out_unlock;

2.12.3

Okay.

Right. And should be sent independently of the ANA stuff, too.

Will be updating both.

Cheers,

Hannes

Dr. Hannes Reinecke		               zSeries & Storage
hare at suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

Actually, based on a comment in mm/util.c we could even use kmemdup_nul instead of kstrndup, might be worth to give it a spin.

Yes, this looks like 4.18 material.

Can you resend them?

Done.

Cheers,

Hannes

Dr. Hannes Reinecke		               zSeries & Storage
hare at suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

The kernel will crash during disconnect, if the first path is 'optimized' and the second 'non-optimized'. Then upon removing the first path any I/O coming in _before_ removing the second path will find the first path to be unusable, and assign 'fallback' to the second. But after exiting the loop it will assign the _first_ path to 'current_path', leading to a crash later on.

---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8980b0137fa5..5205f41c0f6c 100644
@@ -129,7 +129,7 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head)
 	}
 
 	if (fallback)
-		rcu_assign_pointer(head->current_path, ns);
+		rcu_assign_pointer(head->current_path, fallback);
 	return fallback;
 }
 

2.12.3

Actually, error on my side - the tree on this laptop wasn't uptodate, sorry. Applied for the next version.

If we cannot parse the ANA log page we should just disable ANA support altogether, otherwise we fail to connect to the controller and we have no way of figuring out what went wrong.
And we should kill the WARN_ON() statements as we now have a valid recovery strategy.

---
 drivers/nvme/host/core.c      |  2 +-
 drivers/nvme/host/multipath.c | 71 ++++++++++++++++++++++++++++++-------------
 2 files changed, 51 insertions(+), 22 deletions(-)

index e84eb938b952..16128f03ae37 100644
@@ -3392,7 +3392,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 		break;
 #ifdef CONFIG_NVME_MULTIPATH
 	case NVME_AER_NOTICE_ANA:
-		if (WARN_ON_ONCE(!ctrl->ana_log_buf))
+		if (!ctrl->ana_log_buf)
 			break;
 		queue_work(nvme_wq, &ctrl->ana_work);
 		break;

2.12.3

I don't like this. There is a lot magic now where we need to check if we have a buffer independent of wheter the controller supports ANA to start with.

If we really have a that buggy controller we'll need to quirk it.

Well, I've came across this will testing ANA support on the target.

There are two reasons why I did this:

1. There's this statement in nvme_failover_req():

	if (!WARN_ON_ONCE(!ns->ctrl->ana_log_buf))
		queue_work(nvme_wq, &ns->ctrl->ana_work);

(or a similar statement in nvme_handle_aen_notice())
which really doesn't make sense if we require 'ana_log_buf' to always be filled out.
If we require ana_log_buf to always be set we would crash immediately afterwards in nvme_read_ana_log() anyway, rendering the WARN_ON quite pointless.
So, from my POV, either remove the WARN_ON() (or at least replace it with a BUG_ON() to clarify we've messed up) or handle the case where ana_log_buf is _NOT_ set. But that involves graceful failure handling while parsing ANA log pages.

2. If we come across a target with invalid or incorrect ANA implementations we _cannot_ connect at all (as nvme_mpath_init() will return an error, causing mpath_init_identify() to fail, too). Which means we don't have _any_ way of figuring out what went wrong. So I've opted to handle error gracefully and disabling ANA support for those targets (which is perfectly legit methinks), allowing us to inspect the log page via nvme-cli and figure out what went wrong.

And in general I thought it was good practice to handle errors gracefully...

Cheers,

Hannes

We could still have a controller return the ANA status code or send the AEN if we don't use ANA. So I guess we do need to remove the warnings at least.

Well, a conditional to skip code and warn when something is not supported is a lot better than crashing, don't you agree?

As said above we probably should remove the WARN_ON, although these cases mean we had an issue either in the driver initialization or the data returned by the controller.

Which seems pretty sensible. Without that people might just keep using the setup in this form (note that a driver bug is another possibility, it's not always the controllers that are broken :))

And I fail to see where we handle errors much more gracefully here, the only difference seems to be that we entirely ignore ANA errors.

We need to parse the entire ANA log page, otherwise later namespaces will not be updates with the new ANA state.

---
 drivers/nvme/host/multipath.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 3ba7a975fd2a..b92f0a6048c2 100644
@@ -486,10 +486,8 @@ static int nvme_set_ns_ana_state(struct nvme_ctrl *ctrl,
 {
 	struct nvme_ns *ns = data;
 
-	if (ns->ana_grpid == le32_to_cpu(desc->grpid)) {
+	if (ns->ana_grpid == le32_to_cpu(desc->grpid))
 		nvme_update_ns_ana_state(desc, ns);
-		return -ENXIO; /* just break out of the loop */
-	}
 
 	return 0;
 }

2.12.3

nvme_set_ns_ana_state is only used if we looks for exactly one namespace, there are no later namespaces to be updated.