[PATCH] tree: Add NVM subsystem controller identifier

[PATCH] tree: Add NVM subsystem controller identifier

[Nilay Shroff]

This commit introduces a field "cntlid" for controller,
that contains the NVM subsystem unique identifier assigned
to each controller device in an NVM subsystem.

While attaching a namespace, typically user needs to specify the
controller identifier (cntlid). The cntlid could be referenced from
sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
a direct option.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
Hi all,

While attaching an NVMe namespace using nvme-cli command 
"nvme attach-ns", we need to specifiy controller identifier 
(cntlid) against which the namespace is being attached. 

Not specifying cntlid while attaching namespace would result in 
no actual change in namespace attachment. For instance,

# nvme create-ns /dev/nvme0 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096 
create-ns: Success, created nsid:1

# nvme attach-ns /dev/nvme0 -n 1
warning: empty controller-id list will result in no actual change in namespace attachment
attach-ns: Success, nsid:1

# nvme list -o json
{
  "Devices":[
  ]
}

Though it's possible to find the cntlid looking at the sysfs file, 
it'd be convenient to have it readily avaliable under nvme list 
verbose output. 

This patch adds support for retrieving the cntlid in libnvme, 
the subsequent nvme-cli patch shall use this information to 
print the cntlid field in nvme list verbose output.

Thanks,
--Nilay

---
 src/libnvme.map    | 1 +
 src/nvme/private.h | 1 +
 src/nvme/tree.c    | 7 +++++++
 src/nvme/tree.h    | 7 +++++++
 4 files changed, 16 insertions(+)

diff --git a/src/libnvme.map b/src/libnvme.map
index c03a99d1..8710c41f 100644
--- a/src/libnvme.map
+++ b/src/libnvme.map
@@ -9,6 +9,7 @@ LIBNVME_1.9 {
 		nvme_submit_passthru;
 		nvme_submit_passthru64;
 		nvme_update_key;
+		nvme_ctrl_get_cntlid;
 };
 
 LIBNVME_1_8 {
diff --git a/src/nvme/private.h b/src/nvme/private.h
index 11744c25..723740be 100644
--- a/src/nvme/private.h
+++ b/src/nvme/private.h
@@ -86,6 +86,7 @@ struct nvme_ctrl {
 	char *dhchap_key;
 	char *dhchap_ctrl_key;
 	char *cntrltype;
+	char *cntlid;
 	char *dctype;
 	char *phy_slot;
 	bool discovery_ctrl;
diff --git a/src/nvme/tree.c b/src/nvme/tree.c
index 584e3f6a..6efdf992 100644
--- a/src/nvme/tree.c
+++ b/src/nvme/tree.c
@@ -1008,6 +1008,11 @@ const char *nvme_ctrl_get_dhchap_host_key(nvme_ctrl_t c)
 	return c->dhchap_key;
 }
 
+const char *nvme_ctrl_get_cntlid(nvme_ctrl_t c)
+{
+	return c->cntlid;
+}
+
 void nvme_ctrl_set_dhchap_host_key(nvme_ctrl_t c, const char *key)
 {
 	if (c->dhchap_key) {
@@ -1117,6 +1122,7 @@ void nvme_deconfigure_ctrl(nvme_ctrl_t c)
 	FREE_CTRL_ATTR(c->address);
 	FREE_CTRL_ATTR(c->dctype);
 	FREE_CTRL_ATTR(c->cntrltype);
+	FREE_CTRL_ATTR(c->cntlid);
 	FREE_CTRL_ATTR(c->phy_slot);
 }
 
@@ -1800,6 +1806,7 @@ static int nvme_configure_ctrl(nvme_root_t r, nvme_ctrl_t c, const char *path,
 		}
 	}
 	c->cntrltype = nvme_get_ctrl_attr(c, "cntrltype");
+	c->cntlid = nvme_get_ctrl_attr(c, "cntlid");
 	c->dctype = nvme_get_ctrl_attr(c, "dctype");
 	c->phy_slot = nvme_ctrl_lookup_phy_slot(r, c->address);
 
diff --git a/src/nvme/tree.h b/src/nvme/tree.h
index a30e8eb7..1d635baa 100644
--- a/src/nvme/tree.h
+++ b/src/nvme/tree.h
@@ -1025,6 +1025,13 @@ const char *nvme_ctrl_get_host_iface(nvme_ctrl_t c);
  */
 const char *nvme_ctrl_get_dhchap_host_key(nvme_ctrl_t c);
 
+/**
+ *  nvme_ctrl_get_cntlid() - Controller id
+ *  @c:	Controller to be checked
+ *
+ *  Return : Controller id of @c
+ */
+const char *nvme_ctrl_get_cntlid(nvme_ctrl_t c);
 /**
  * nvme_ctrl_set_dhchap_host_key() - Set host key
  * @c:		Host for which the key should be set
-- 
2.44.0

Re: [PATCH] tree: Add NVM subsystem controller identifier

[Nilay Shroff]

I forgot to add that this is libnvme patch in the subject. Sorry about that.

Thanks,
--Nilay

On 3/26/24 12:27, Nilay Shroff wrote:
> This commit introduces a field "cntlid" for controller,
> that contains the NVM subsystem unique identifier assigned
> to each controller device in an NVM subsystem.
> 
> While attaching a namespace, typically user needs to specify the
> controller identifier (cntlid). The cntlid could be referenced from
> sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
> a direct option.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
> Hi all,
> 
> While attaching an NVMe namespace using nvme-cli command 
> "nvme attach-ns", we need to specifiy controller identifier 
> (cntlid) against which the namespace is being attached. 
> 
> Not specifying cntlid while attaching namespace would result in 
> no actual change in namespace attachment. For instance,
> 
> # nvme create-ns /dev/nvme0 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096 
> create-ns: Success, created nsid:1
> 
> # nvme attach-ns /dev/nvme0 -n 1
> warning: empty controller-id list will result in no actual change in namespace attachment
> attach-ns: Success, nsid:1
> 
> # nvme list -o json
> {
>   "Devices":[
>   ]
> }
> 
> Though it's possible to find the cntlid looking at the sysfs file, 
> it'd be convenient to have it readily avaliable under nvme list 
> verbose output. 
> 
> This patch adds support for retrieving the cntlid in libnvme, 
> the subsequent nvme-cli patch shall use this information to 
> print the cntlid field in nvme list verbose output.
> 
> Thanks,
> --Nilay
> 
> ---
>  src/libnvme.map    | 1 +
>  src/nvme/private.h | 1 +
>  src/nvme/tree.c    | 7 +++++++
>  src/nvme/tree.h    | 7 +++++++
>  4 files changed, 16 insertions(+)
> 
> diff --git a/src/libnvme.map b/src/libnvme.map
> index c03a99d1..8710c41f 100644
> --- a/src/libnvme.map
> +++ b/src/libnvme.map
> @@ -9,6 +9,7 @@ LIBNVME_1.9 {
>  		nvme_submit_passthru;
>  		nvme_submit_passthru64;
>  		nvme_update_key;
> +		nvme_ctrl_get_cntlid;
>  };
>  
>  LIBNVME_1_8 {
> diff --git a/src/nvme/private.h b/src/nvme/private.h
> index 11744c25..723740be 100644
> --- a/src/nvme/private.h
> +++ b/src/nvme/private.h
> @@ -86,6 +86,7 @@ struct nvme_ctrl {
>  	char *dhchap_key;
>  	char *dhchap_ctrl_key;
>  	char *cntrltype;
> +	char *cntlid;
>  	char *dctype;
>  	char *phy_slot;
>  	bool discovery_ctrl;
> diff --git a/src/nvme/tree.c b/src/nvme/tree.c
> index 584e3f6a..6efdf992 100644
> --- a/src/nvme/tree.c
> +++ b/src/nvme/tree.c
> @@ -1008,6 +1008,11 @@ const char *nvme_ctrl_get_dhchap_host_key(nvme_ctrl_t c)
>  	return c->dhchap_key;
>  }
>  
> +const char *nvme_ctrl_get_cntlid(nvme_ctrl_t c)
> +{
> +	return c->cntlid;
> +}
> +
>  void nvme_ctrl_set_dhchap_host_key(nvme_ctrl_t c, const char *key)
>  {
>  	if (c->dhchap_key) {
> @@ -1117,6 +1122,7 @@ void nvme_deconfigure_ctrl(nvme_ctrl_t c)
>  	FREE_CTRL_ATTR(c->address);
>  	FREE_CTRL_ATTR(c->dctype);
>  	FREE_CTRL_ATTR(c->cntrltype);
> +	FREE_CTRL_ATTR(c->cntlid);
>  	FREE_CTRL_ATTR(c->phy_slot);
>  }
>  
> @@ -1800,6 +1806,7 @@ static int nvme_configure_ctrl(nvme_root_t r, nvme_ctrl_t c, const char *path,
>  		}
>  	}
>  	c->cntrltype = nvme_get_ctrl_attr(c, "cntrltype");
> +	c->cntlid = nvme_get_ctrl_attr(c, "cntlid");
>  	c->dctype = nvme_get_ctrl_attr(c, "dctype");
>  	c->phy_slot = nvme_ctrl_lookup_phy_slot(r, c->address);
>  
> diff --git a/src/nvme/tree.h b/src/nvme/tree.h
> index a30e8eb7..1d635baa 100644
> --- a/src/nvme/tree.h
> +++ b/src/nvme/tree.h
> @@ -1025,6 +1025,13 @@ const char *nvme_ctrl_get_host_iface(nvme_ctrl_t c);
>   */
>  const char *nvme_ctrl_get_dhchap_host_key(nvme_ctrl_t c);
>  
> +/**
> + *  nvme_ctrl_get_cntlid() - Controller id
> + *  @c:	Controller to be checked
> + *
> + *  Return : Controller id of @c
> + */
> +const char *nvme_ctrl_get_cntlid(nvme_ctrl_t c);
>  /**
>   * nvme_ctrl_set_dhchap_host_key() - Set host key
>   * @c:		Host for which the key should be set

Re: [PATCH] tree: Add NVM subsystem controller identifier

[Hannes Reinecke]

On 3/26/24 07:57, Nilay Shroff wrote:
> This commit introduces a field "cntlid" for controller,
> that contains the NVM subsystem unique identifier assigned
> to each controller device in an NVM subsystem.
> 
> While attaching a namespace, typically user needs to specify the
> controller identifier (cntlid). The cntlid could be referenced from
> sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
> a direct option.
>  > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
> Hi all,
> 
> While attaching an NVMe namespace using nvme-cli command
> "nvme attach-ns", we need to specifiy controller identifier
> (cntlid) against which the namespace is being attached.
> 
> Not specifying cntlid while attaching namespace would result in
> no actual change in namespace attachment. For instance,
> 
> # nvme create-ns /dev/nvme0 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096
> create-ns: Success, created nsid:1
> 
> # nvme attach-ns /dev/nvme0 -n 1
> warning: empty controller-id list will result in no actual change in namespace attachment
> attach-ns: Success, nsid:1
> 

Errm. How would that work? /dev/nvme0 _is_ a controller, and as such 
will have an assigned cntlid. And if one would issue an 'attach-ns'
command the only controller which it could address is that controller
to which the command is issued.
I'd be _very_ surprised if you could issue 'attach-ns' against _another_
controller. So why do we need this option?
Shouldn't we rather infer the cntlid from the existing controller?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] tree: Add NVM subsystem controller identifier

[Nilay Shroff]

On 3/26/24 12:40, Hannes Reinecke wrote:
> On 3/26/24 07:57, Nilay Shroff wrote:
>> This commit introduces a field "cntlid" for controller,
>> that contains the NVM subsystem unique identifier assigned
>> to each controller device in an NVM subsystem.
>>
>> While attaching a namespace, typically user needs to specify the
>> controller identifier (cntlid). The cntlid could be referenced from
>> sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
>> a direct option.
>>  > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>> ---
>> Hi all,
>>
>> While attaching an NVMe namespace using nvme-cli command
>> "nvme attach-ns", we need to specifiy controller identifier
>> (cntlid) against which the namespace is being attached.
>>
>> Not specifying cntlid while attaching namespace would result in
>> no actual change in namespace attachment. For instance,
>>
>> # nvme create-ns /dev/nvme0 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096
>> create-ns: Success, created nsid:1
>>
>> # nvme attach-ns /dev/nvme0 -n 1
>> warning: empty controller-id list will result in no actual change in namespace attachment
>> attach-ns: Success, nsid:1
>>
> 
> Errm. How would that work? /dev/nvme0 _is_ a controller, and as such will have an assigned cntlid. And if one would issue an 'attach-ns'
> command the only controller which it could address is that controller
> to which the command is issued.
> I'd be _very_ surprised if you could issue 'attach-ns' against _another_
> controller. So why do we need this option?
> Shouldn't we rather infer the cntlid from the existing controller?
> 
Yes it's possible to attach a namespace against the cntlid which could be different from 
the nvmeX device we use while attaching the namespace. Moreover, if we want to create a shared 
namespace then we need to specify all controller identifiers which would share that namespace.

For reference, I would show the example output below:

Example 1: Create a shared namespace and attach to multiple controllers:
------------------------------------------------------------------------

# nvme list -v 
Subsystem        Subsystem-NQN                                                                                    Controllers
---------------- ------------------------------------------------------------------------------------------------ ----------------
nvme-subsys1     nqn.1994-11.com.samsung:nvme:PM1735a:2.5-inch:S6RTNE0R900057                                     nvme1, nvme3
[...]

As we could see above I have two controllers (nvme1 and nvme3) under NVM subsys1. 
Lets find cntlid of each controller.

# cat /sys/class/nvme/nvme1/cntlid 
66 
# cat /sys/class/nvme/nvme3/cntlid 
65

Now create a shared namespace and attach it against the above controllers:

# nvme create-ns /dev/nvme1 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096 -m 1 
create-ns: Success, created nsid:1

# nvme attach-ns /dev/nvme1 -n 0x1 -c 65,66 
attach-ns: Success, nsid:1

# nvme list -v
[...]

Device       Generic      NSID       Usage                      Format           Controllers     
------------ ------------ ---------- -------------------------- ---------------- ----------------
/dev/nvme1n1 /dev/ng1n1   0x1          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme1, nvme3


As we could see above for creating shared namespace we need to know the cntlid of both 
nvme1 and nvme3 controllers.

Example 2: Create a private namespace and attach to a controller other than the one specified in nvmeX
------------------------------------------------------------------------------------------------------

# nvme create-ns /dev/nvme1 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096
create-ns: Success, created nsid:2

# nvme attach-ns /dev/nvme1 -n 2 -c 65
attach-ns: Success, nsid:2

Please note above that I attached the namespace against controller nvme3 (the cntlid 65 is assigned to nvme3)

# nvme list -v
[...]

Device       Generic      NSID       Usage                      Format           Controllers     
------------ ------------ ---------- -------------------------- ---------------- ----------------
/dev/nvme1n1 /dev/ng1n1   0x1          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme1, nvme3
/dev/nvme1n2 /dev/ng1n2   0x2          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme3

Thanks,
--Nilay

Re: [PATCH] tree: Add NVM subsystem controller identifier

[Nilay Shroff]

On 3/26/24 14:20, Nilay Shroff wrote:
> 
> 
> On 3/26/24 12:40, Hannes Reinecke wrote:
>> On 3/26/24 07:57, Nilay Shroff wrote:
>>> This commit introduces a field "cntlid" for controller,
>>> that contains the NVM subsystem unique identifier assigned
>>> to each controller device in an NVM subsystem.
>>>
>>> While attaching a namespace, typically user needs to specify the
>>> controller identifier (cntlid). The cntlid could be referenced from
>>> sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
>>> a direct option.
>>>  > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>

>>
>> Errm. How would that work? /dev/nvme0 _is_ a controller, and as such will have an assigned cntlid. And if one would issue an 'attach-ns'
>> command the only controller which it could address is that controller
>> to which the command is issued.
>> I'd be _very_ surprised if you could issue 'attach-ns' against _another_
>> controller. So why do we need this option?
>> Shouldn't we rather infer the cntlid from the existing controller?
>>
> Yes it's possible to attach a namespace against the cntlid which could be different from 
> the nvmeX device we use while attaching the namespace. Moreover, if we want to create a shared 
> namespace then we need to specify all controller identifiers which would share that namespace.
> 
> For reference, I would show the example output below:
> 
> Example 1: Create a shared namespace and attach to multiple controllers:
> ------------------------------------------------------------------------
> 
> # nvme list -v 
> Subsystem        Subsystem-NQN                                                                                    Controllers
> ---------------- ------------------------------------------------------------------------------------------------ ----------------
> nvme-subsys1     nqn.1994-11.com.samsung:nvme:PM1735a:2.5-inch:S6RTNE0R900057                                     nvme1, nvme3
> [...]
> 
> As we could see above I have two controllers (nvme1 and nvme3) under NVM subsys1. 
> Lets find cntlid of each controller.
> 
> # cat /sys/class/nvme/nvme1/cntlid 
> 66 
> # cat /sys/class/nvme/nvme3/cntlid 
> 65
> 
> Now create a shared namespace and attach it against the above controllers:
> 
> # nvme create-ns /dev/nvme1 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096 -m 1 
> create-ns: Success, created nsid:1
> 
> # nvme attach-ns /dev/nvme1 -n 0x1 -c 65,66 
> attach-ns: Success, nsid:1
> 
> # nvme list -v
> [...]
> 
> Device       Generic      NSID       Usage                      Format           Controllers     
> ------------ ------------ ---------- -------------------------- ---------------- ----------------
> /dev/nvme1n1 /dev/ng1n1   0x1          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme1, nvme3
> 
> 
> As we could see above for creating shared namespace we need to know the cntlid of both 
> nvme1 and nvme3 controllers.
> 
> Example 2: Create a private namespace and attach to a controller other than the one specified in nvmeX
> ------------------------------------------------------------------------------------------------------
> 
> # nvme create-ns /dev/nvme1 --nsze=0x156d56 --ncap=0x156d56  --block-size=4096
> create-ns: Success, created nsid:2
> 
> # nvme attach-ns /dev/nvme1 -n 2 -c 65
> attach-ns: Success, nsid:2
> 
> Please note above that I attached the namespace against controller nvme3 (the cntlid 65 is assigned to nvme3)
> 
> # nvme list -v
> [...]
> 
> Device       Generic      NSID       Usage                      Format           Controllers     
> ------------ ------------ ---------- -------------------------- ---------------- ----------------
> /dev/nvme1n1 /dev/ng1n1   0x1          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme1, nvme3
> /dev/nvme1n2 /dev/ng1n2   0x2          5.75  GB /   5.75  GB      4 KiB +  0 B   nvme3
> 

If there's no other objection to the above change then can we accept the patch?

Thanks,
--Nilay

 

Re: [PATCH] tree: Add NVM subsystem controller identifier

[Daniel Wagner]

On Tue, 26 Mar 2024 12:27:23 +0530, Nilay Shroff wrote:
> This commit introduces a field "cntlid" for controller,
> that contains the NVM subsystem unique identifier assigned
> to each controller device in an NVM subsystem.
> 
> While attaching a namespace, typically user needs to specify the
> controller identifier (cntlid). The cntlid could be referenced from
> sysfs (/sys/class/nvme/nvmeX/cntlid) but it would be nice to have
> a direct option.
> 
> [...]

Applied, thanks!

[1/1] tree: Add NVM subsystem controller identifier
      commit: d638f037623742003c5ef0ba6127b981e925b351

Best regards,
-- 
Daniel Wagner <dwagner@suse.de>