[PATCH] driver/nvme/host: Support duplicated nsid for the private

[PATCH] driver/nvme/host: Support duplicated nsid for the private

[Sungup Moon]

When the multi-controller, managed by a special admin command, has private
namespace with same nsid, current linux driver raise "Duplicate unshared
namespace" error. But, NVMe Specification defines the NSID usage like this:

If Namespace Management, ANA Reporting, or NVM Sets are supported, the
NSIDs shall be unique within the NVM subsystem. If the Namespace
Management, ANA Reporting, and NVM Sets are not supported, then NSIDs:
a) for shared namespace shall be unique; and
b) for private namespace are not required to be unique.
(reference: 6.1.6 NSID and Namespace Usage; NVM Express 1.4c spec)

So, if a multi-controller, which is not managed by Namespace Management
function, creates some private namespaces without ANA and NVM Sets, the
duplicated NSID should be allowed because that is not a NVMe specification
violation.

But, current nvme driver checks only namespace is shared or not, so I
propose following patch:
1. nvme_ctrl has unique_nsid field to identify that controller should
   assign unique nsid.
2. nvme_init_ns_head function creates new nvme_ns_head instance not only
   head is null but controller's unique_nsid is false (no flagged
   attribute) and namespace is not shared.
3. for creating bdev device file, nvme_mpath_set_disk_name will return
   false when unique_nsid is false and namespace is not shared.
4. also, nvme_mpath_alloc_disk alto return 0 with same manner.

Signed-off-by: Sungup Moon <sungup.moon@samsung.com>
---
 drivers/nvme/host/core.c      | 20 +++++++++++++++++++-
 drivers/nvme/host/multipath.c |  5 +++--
 drivers/nvme/host/nvme.h      |  1 +
 include/linux/nvme.h          |  1 +
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 51c08f206cbf..f1807b0ca361 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2942,6 +2942,18 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		if (ret)
 			goto out_free;
 
+		/*
+		 * NSID should be unique on the following condition
+		 * 1. Namespace Management support; or
+		 * 2. ANA Reporting support; or
+		 * 3. NVM Set support; or
+		 * 4. Namespace is shared
+		 * Other case, private namespaces are not required to be unique.
+		 */
+		ctrl->unique_nsid = (ctrl->oacs & NVME_CTRL_OACS_NS_MNGT_SUPP) ||
+			(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA) ||
+			(ctrl->ctratt & NVME_CTRL_CTRATT_NVM_SETS);
+
 		/*
 		 * Check for quirks.  Quirk can depend on firmware version,
 		 * so, in principle, the set of quirks present can change
@@ -3816,7 +3828,13 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 
 	mutex_lock(&ctrl->subsys->lock);
 	head = nvme_find_ns_head(ctrl->subsys, nsid);
-	if (!head) {
+	if (!head || !(ctrl->unique_nsid || is_shared || head->shared)) {
+		/*
+		 * If the found ns head is null or both of ns are not shared without
+		 * the unique namespace condition (this means both namespace are
+		 * private namespaces and those can share the same nsid), allocate the
+		 * new head. Private namespace can reuse nsid with the others.
+		 */
 		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, ids);
 		if (ret) {
 			dev_err(ctrl->device,
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ff775235534c..7fe91754fe69 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -88,7 +88,7 @@ void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
  */
 bool nvme_mpath_set_disk_name(struct nvme_ns *ns, char *disk_name, int *flags)
 {
-	if (!multipath)
+	if (!multipath || !(ns->ctrl->unique_nsid || ns->head->shared))
 		return false;
 	if (!ns->head->disk) {
 		sprintf(disk_name, "nvme%dn%d", ns->ctrl->subsys->instance,
@@ -507,7 +507,8 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	 * We also do this for private namespaces as the namespace sharing data could
 	 * change after a rescan.
 	 */
-	if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) || !multipath)
+	if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) || !multipath ||
+	    !(ctrl->unique_nsid || head->shared))
 		return 0;
 
 	head->disk = blk_alloc_disk(ctrl->numa_node);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e7ccdb119ede..8348c405c6d3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -297,6 +297,7 @@ struct nvme_ctrl {
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
+	bool unique_nsid;
 	unsigned long quirks;
 	struct nvme_id_power_state psd[32];
 	struct nvme_effects_log *effects;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 4f44f83817a9..f626a445d1a8 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -346,6 +346,7 @@ enum {
 	NVME_CTRL_ONCS_TIMESTAMP		= 1 << 6,
 	NVME_CTRL_VWC_PRESENT			= 1 << 0,
 	NVME_CTRL_OACS_SEC_SUPP                 = 1 << 0,
+	NVME_CTRL_OACS_NS_MNGT_SUPP		= 1 << 3,
 	NVME_CTRL_OACS_DIRECTIVES		= 1 << 5,
 	NVME_CTRL_OACS_DBBUF_SUPP		= 1 << 8,
 	NVME_CTRL_LPA_CMD_EFFECTS_LOG		= 1 << 1,
-- 
2.25.1

Re: [PATCH] driver/nvme/host: Support duplicated nsid for the private

[hch]

On Mon, Mar 14, 2022 at 04:05:05PM +0900, Sungup Moon wrote:
> When the multi-controller, managed by a special admin command, has private
> namespace with same nsid, current linux driver raise "Duplicate unshared
> namespace" error. But, NVMe Specification defines the NSID usage like this:
> 
> If Namespace Management, ANA Reporting, or NVM Sets are supported, the
> NSIDs shall be unique within the NVM subsystem. If the Namespace
> Management, ANA Reporting, and NVM Sets are not supported, then NSIDs:
> a) for shared namespace shall be unique; and
> b) for private namespace are not required to be unique.
> (reference: 6.1.6 NSID and Namespace Usage; NVM Express 1.4c spec)
> 
> So, if a multi-controller, which is not managed by Namespace Management
> function, creates some private namespaces without ANA and NVM Sets, the
> duplicated NSID should be allowed because that is not a NVMe specification
> violation.
> 
> But, current nvme driver checks only namespace is shared or not, so I
> propose following patch:
> 1. nvme_ctrl has unique_nsid field to identify that controller should
>    assign unique nsid.
> 2. nvme_init_ns_head function creates new nvme_ns_head instance not only
>    head is null but controller's unique_nsid is false (no flagged
>    attribute) and namespace is not shared.
> 3. for creating bdev device file, nvme_mpath_set_disk_name will return
>    false when unique_nsid is false and namespace is not shared.
> 4. also, nvme_mpath_alloc_disk alto return 0 with same manner.

From a very quick glance this looks good.  But please make sure you don't
spill over 80 charactes per line.  Also I think instead of adding the
unique_nsid field a little helper that checks the relevant flags might
be a lіttle nicer.  It is not checked in a fast path anywere and the
checks are pretty trivial.

RE:(2) [PATCH] driver/nvme/host: Support duplicated nsid for the private

[Sungup Moon]

Thank you for your reply,

>  
> On Mon, Mar 14, 2022 at 04:05:05PM +0900, Sungup Moon wrote:
> > When the multi-controller, managed by a special admin command, has private
> > namespace with same nsid, current linux driver raise "Duplicate unshared
> > namespace" error. But, NVMe Specification defines the NSID usage like this:
> > 
> > If Namespace Management, ANA Reporting, or NVM Sets are supported, the
> > NSIDs shall be unique within the NVM subsystem. If the Namespace
> > Management, ANA Reporting, and NVM Sets are not supported, then NSIDs:
> > a) for shared namespace shall be unique; and
> > b) for private namespace are not required to be unique.
> > (reference: 6.1.6 NSID and Namespace Usage; NVM Express 1.4c spec)
> > 
> > So, if a multi-controller, which is not managed by Namespace Management
> > function, creates some private namespaces without ANA and NVM Sets, the
> > duplicated NSID should be allowed because that is not a NVMe specification
> > violation.
> > 
> > But, current nvme driver checks only namespace is shared or not, so I
> > propose following patch:
> > 1. nvme_ctrl has unique_nsid field to identify that controller should
> >    assign unique nsid.
> > 2. nvme_init_ns_head function creates new nvme_ns_head instance not only
> >    head is null but controller's unique_nsid is false (no flagged
> >    attribute) and namespace is not shared.
> > 3. for creating bdev device file, nvme_mpath_set_disk_name will return
> >    false when unique_nsid is false and namespace is not shared.
> > 4. also, nvme_mpath_alloc_disk alto return 0 with same manner.
>  
> From a very quick glance this looks good.  But please make sure you don't
> spill over 80 charactes per line.

I checked changes using "scripts/checkpatch.pl --terse --file {changed file}",
but there is no warning on my changes. However I will recheck the spill-over
lines over 80 characters.

> Also I think instead of adding the
> unique_nsid field a little helper that checks the relevant flags might
> be a lіttle nicer.  It is not checked in a fast path anywere and the
> checks are pretty trivial.
> 

Thank you for your advise! I will remove flag and add checking function for
unique nsid.

Thank you,
Sungup Moon