[PATCH] nvme-cli: attach ns to local controller if none specified

[PATCH] nvme-cli: attach ns to local controller if none specified

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Assume the user meant to attach the namespace to the controller the
command was sent to if the user didn't provide a controller id list.

Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 nvme.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/nvme.c b/nvme.c
index bb412843..7a2c2e9f 100644
--- a/nvme.c
+++ b/nvme.c
@@ -2953,9 +2953,6 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
 
 	num = argconfig_parse_comma_sep_array_u16(cfg.cntlist,
 						  list, ARRAY_SIZE(list));
-	if (!num)
-		fprintf(stderr, "warning: empty controller-id list will result in no actual change in namespace attachment\n");
-
 	if (num == -1) {
 		nvme_show_error("%s: controller id list is malformed", cmd->name);
 		return -EINVAL;
@@ -2965,7 +2962,18 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
 	if (!cntlist)
 		return -ENOMEM;
 
-	nvme_init_ctrl_list(cntlist, num, list);
+	if (argconfig_parse_seen(opts, "controllers")) {
+		nvme_init_ctrl_list(cntlist, num, list);
+	} else {
+		struct nvme_id_ctrl ctrl = { 0 };
+
+		if (nvme_cli_identify_ctrl(dev, &ctrl)) {
+			perror("identify-ctrl");
+			return -errno;
+		}
+		cntlist->identifier[0] = ctrl.cntlid;
+	}
+
 
 	if (attach)
 		err = nvme_cli_ns_attach_ctrls(dev, cfg.namespace_id,
-- 
2.47.1

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Chaitanya Kulkarni]

On 3/31/25 12:08, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
>
> Assume the user meant to attach the namespace to the controller the
> command was sent to if the user didn't provide a controller id list.
>
> Suggested-by: Nilay Shroff<nilay@linux.ibm.com>
> Signed-off-by: Keith Busch<kbusch@kernel.org>

this will not be backward compatible hence all the existing deployments
that are resulting in error [1] will now attach the default controller,
I believe that is what is expected ?

if not then do we need add a version check so it will stay as backward
compatible ?

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1]
-		fprintf(stderr, "warning: empty controller-id list will result in no actual change in namespace attachment\n");

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Keith Busch]

On Mon, Mar 31, 2025 at 08:37:31PM +0000, Chaitanya Kulkarni wrote:
> On 3/31/25 12:08, Keith Busch wrote:
> > From: Keith Busch<kbusch@kernel.org>
> >
> > Assume the user meant to attach the namespace to the controller the
> > command was sent to if the user didn't provide a controller id list.
> >
> > Suggested-by: Nilay Shroff<nilay@linux.ibm.com>
> > Signed-off-by: Keith Busch<kbusch@kernel.org>
> 
> this will not be backward compatible hence all the existing deployments
> that are resulting in error [1] will now attach the default controller,
> I believe that is what is expected ?

Yes, this changes behavior. I don't think a user would purposefully
request to attach a namespace to nothing. If they explicitly request
that via '-c ""', then that request will still be honored. But if they
omit the list entirely, I think it's reasonable to assume you want the
controller servicing the command to attach the namespace to itself.

It's really a nice workflow optimization since you can skip finding the
controller id when you just want local attachment.
 
> if not then do we need add a version check so it will stay as backward
> compatible ?

I don't follow what you mean here. What version of what component could
be checked to guide this behavior?

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Chaitanya Kulkarni]

On 3/31/25 13:57, Keith Busch wrote:
> On Mon, Mar 31, 2025 at 08:37:31PM +0000, Chaitanya Kulkarni wrote:
>> On 3/31/25 12:08, Keith Busch wrote:
>>> From: Keith Busch<kbusch@kernel.org>
>>>
>>> Assume the user meant to attach the namespace to the controller the
>>> command was sent to if the user didn't provide a controller id list.
>>>
>>> Suggested-by: Nilay Shroff<nilay@linux.ibm.com>
>>> Signed-off-by: Keith Busch<kbusch@kernel.org>
>> this will not be backward compatible hence all the existing deployments
>> that are resulting in error [1] will now attach the default controller,
>> I believe that is what is expected ?
> Yes, this changes behavior. I don't think a user would purposefully
> request to attach a namespace to nothing. If they explicitly request
> that via '-c ""', then that request will still be honored. But if they
> omit the list entirely, I think it's reasonable to assume you want the
> controller servicing the command to attach the namespace to itself.
>
> It's really a nice workflow optimization since you can skip finding the
> controller id when you just want local attachment.
>   

Yes, someone who's using nvme-cli from very start I totally agree :).

>> if not then do we need add a version check so it will stay as backward
>> compatible ?
> I don't follow what you mean here. What version of what component could
> be checked to guide this behavior?

if we can only enable this functionality for nvme-cli/libnvme for the 
current
version and any other versions after current version.

In case nvme-cli or libnvme has a version below the current version, then
it can follow the original behavior, will this work or not ?

-ck

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Daniel Wagner]

On Mon, Mar 31, 2025 at 09:16:49PM +0000, Chaitanya Kulkarni wrote:
> On 3/31/25 13:57, Keith Busch wrote:
> > On Mon, Mar 31, 2025 at 08:37:31PM +0000, Chaitanya Kulkarni wrote:
> > > if not then do we need add a version check so it will stay as backward
> > > compatible ?
> > I don't follow what you mean here. What version of what component could
> > be checked to guide this behavior?
> 
> if we can only enable this functionality for nvme-cli/libnvme for the 
> current version and any other versions after current version.

This is a new feature which is backwards compatible as I understand it.
If a user provides the -c option it still does exactly what it supposed
to do.

> In case nvme-cli or libnvme has a version below the current version, then
> it can follow the original behavior, will this work or not ?

Older version will complain, that you need to provide the controller id,
newer ones will just assume you wanted the first controller.

For extra points, this could be documented in the docs :)

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Keith Busch]

On Mon, Mar 31, 2025 at 12:08:43PM -0700, Keith Busch wrote:
> +	} else {
> +		struct nvme_id_ctrl ctrl = { 0 };
> +
> +		if (nvme_cli_identify_ctrl(dev, &ctrl)) {
> +			perror("identify-ctrl");
> +			return -errno;
> +		}
> +		cntlist->identifier[0] = ctrl.cntlid;

This is not a zero-based-value, so this will need a

		cntlist->num = cpu_to_le16(1);

in this section.

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Nilay Shroff]

On 4/1/25 12:38 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Assume the user meant to attach the namespace to the controller the
> command was sent to if the user didn't provide a controller id list.
> 
> Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  nvme.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/nvme.c b/nvme.c
> index bb412843..7a2c2e9f 100644
> --- a/nvme.c
> +++ b/nvme.c
> @@ -2953,9 +2953,6 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
>  
>  	num = argconfig_parse_comma_sep_array_u16(cfg.cntlist,
>  						  list, ARRAY_SIZE(list));
> -	if (!num)
> -		fprintf(stderr, "warning: empty controller-id list will result in no actual change in namespace attachment\n");
> -
>  	if (num == -1) {
>  		nvme_show_error("%s: controller id list is malformed", cmd->name);
>  		return -EINVAL;
> @@ -2965,7 +2962,18 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
>  	if (!cntlist)
>  		return -ENOMEM;
>  
> -	nvme_init_ctrl_list(cntlist, num, list);
> +	if (argconfig_parse_seen(opts, "controllers")) {
> +		nvme_init_ctrl_list(cntlist, num, list);
> +	} else {
> +		struct nvme_id_ctrl ctrl = { 0 };
> +
> +		if (nvme_cli_identify_ctrl(dev, &ctrl)) {
> +			perror("identify-ctrl");
> +			return -errno;
> +		}
> +		cntlist->identifier[0] = ctrl.cntlid;

Thanks Keith for working on this!

I think we also need to set the num of cntrls in the list here. In this particular case, 
we need to add the following line after we initialize the cntrl identifier:

cntlist->num = cpu_to_le16(1);

Otherwise changes looks good to me. 

Thanks,
--Nilay

Re: [PATCH] nvme-cli: attach ns to local controller if none specified

[Christoph Hellwig]

On Mon, Mar 31, 2025 at 12:08:43PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Assume the user meant to attach the namespace to the controller the
> command was sent to if the user didn't provide a controller id list.

This looks good, but I think it also needs a documentation update.