[PATCH v2] nvme: check at least one ns identification reported

[PATCH v2] nvme: check at least one ns identification reported

[Guixin Liu]

Per the NVMe spec after 1.3:
  At least one Namespace Identification Descriptor identifying the
  namespace (i.e., NIDT field set to 1h, 2h, or 3h).
Check there is at least one reported.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
Changes from v1 to v2:
- Check only when the nvme version is greater than or
equal to 1.3.

 drivers/nvme/host/core.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..cf767ac697dc 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1426,7 +1426,7 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 }
 
 static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
-		struct nvme_ns_id_desc *cur, bool *csi_seen)
+		struct nvme_ns_id_desc *cur, bool *csi_seen, bool *has_id)
 {
 	const char *warn_str = "ctrl returned bogus length:";
 	void *data = cur;
@@ -1438,6 +1438,7 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 				 warn_str, cur->nidl);
 			return -1;
 		}
+		*has_id = true;
 		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
 			return NVME_NIDT_EUI64_LEN;
 		memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
@@ -1448,6 +1449,7 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 				 warn_str, cur->nidl);
 			return -1;
 		}
+		*has_id = true;
 		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
 			return NVME_NIDT_NGUID_LEN;
 		memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
@@ -1458,6 +1460,7 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 				 warn_str, cur->nidl);
 			return -1;
 		}
+		*has_id = true;
 		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
 			return NVME_NIDT_UUID_LEN;
 		uuid_copy(&ids->uuid, data + sizeof(*cur));
@@ -1482,6 +1485,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
 {
 	struct nvme_command c = { };
 	bool csi_seen = false;
+	bool has_id = false;
 	int status, pos, len;
 	void *data;
 
@@ -1513,13 +1517,25 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
 		if (cur->nidl == 0)
 			break;
 
-		len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
+		len = nvme_process_ns_desc(ctrl, &info->ids, cur,
+					   &csi_seen, &has_id);
 		if (len < 0)
 			break;
 
 		len += sizeof(*cur);
 	}
 
+	/*
+	 * NVMe 1.3 spec introduced ns identification, a controller should
+	 * return at least one.
+	 */
+	if (!has_id && ctrl->vs >= NVME_VS(1, 3, 0)) {
+		dev_warn(ctrl->device,
+			 "No identification reported for nsid:%d\n",
+			 info->nsid);
+		status = -EINVAL;
+	}
+
 	if (nvme_multi_css(ctrl) && !csi_seen) {
 		dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
 			 info->nsid);
-- 
2.43.0

Re: [PATCH v2] nvme: check at least one ns identification reported

[Christoph Hellwig]

On Mon, Nov 04, 2024 at 11:14:18AM +0800, Guixin Liu wrote:
> Per the NVMe spec after 1.3:
>   At least one Namespace Identification Descriptor identifying the
>   namespace (i.e., NIDT field set to 1h, 2h, or 3h).
> Check there is at least one reported.

What problem does this try to solve?

Re: [PATCH v2] nvme: check at least one ns identification reported

[Guixin Liu]

在 2024/11/4 21:15, Christoph Hellwig 写道:
> On Mon, Nov 04, 2024 at 11:14:18AM +0800, Guixin Liu wrote:
>> Per the NVMe spec after 1.3:
>>    At least one Namespace Identification Descriptor identifying the
>>    namespace (i.e., NIDT field set to 1h, 2h, or 3h).
>> Check there is at least one reported.
> What problem does this try to solve?

I assume that the dm-multipath will not recognize nvme device if nothing 
to return,

I was wrong, ignore this path plz.

Best Regards,

Guixin Liu

Re: [PATCH v2] nvme: check at least one ns identification reported

[Christoph Hellwig]

On Wed, Nov 06, 2024 at 06:47:43PM +0800, Guixin Liu wrote:
> I assume that the dm-multipath

dm-multipath is not supported with nvme..

> will not recognize nvme device if nothing to 
> return,

But more importantly the user of the sysfs files is the right place
the just check for these values being non-present.  As said before
it's perfectly valid for 1.0 devices so they have to cope with it
anyway.  I don't think a message in the kernel log that is very to
process algorithmically is not very useful.

(I really with we'd have useful and required identifiers from the
start, but that's a separate discussion)

Re: [PATCH v2] nvme: check at least one ns identification reported

[Guixin Liu]

在 2024/11/7 13:28, Christoph Hellwig 写道:
> On Wed, Nov 06, 2024 at 06:47:43PM +0800, Guixin Liu wrote:
>> I assume that the dm-multipath
> dm-multipath is not supported with nvme..

Dm-multipath can be configured to support nvme,

/etc/multipath.conf:

     devices {
         device {
                 vendor "NVME"
                 product ".*"
                 path_grouping_policy group_by_prio
         }
     }

Here is the result of "multipath -l":

uuid.a2c24be7-ed8e-4033-9f26-5b9f230265fb dm-0NVME,Linux
size=1.0G features='0' hwhandler='0' wp=rw
`-+- policy='service-time 0' prio=0 status=active
   |- 0:1:2:1 nvme0n2 259:0 active undef running
   `- 1:2:2:1 nvme1n2 259:3 active undef running


If the target haven't report any indentification, the dm-multipath

will use wwid which generated by nvme host driver to recognize

which namespaces should be merged.

Here is the result of "multipath -l", if we dont report any identification:

nvme.0000-6135363837366563633830646133663261643034-4c696e7578-00000001 
dm-0 NVME,Linux
size=1.0G features='0' hwhandler='0' wp=rw
`-+- policy='service-time 0' prio=0 status=active
   |- 0:1:1:1 nvme0n1 259:0 active undef running
   `- 1:2:1:1 nvme1n1 259:3 active undef running

>> will not recognize nvme device if nothing to
>> return,
> But more importantly the user of the sysfs files is the right place
> the just check for these values being non-present.  As said before
> it's perfectly valid for 1.0 devices so they have to cope with it
> anyway.  I don't think a message in the kernel log that is very to
> process algorithmically is not very useful.
>
> (I really with we'd have useful and required identifiers from the
> start, but that's a separate discussion)

Agree, I just worry about that some components denpend on this, currently,

no problem.

Best Regards,

Guixin Liu