Re: nvme-cli : attaching a namespace to an undiscovered nvme controller on multi-controller nvme disk

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/20/24 12:22, Daniel Wagner wrote:
> On Tue, May 07, 2024 at 05:14:40PM GMT, Nilay Shroff wrote:
>> I think the information which we need to contain this issue is already
>> available through sysfs. If we scan nvme topology then we could find 
>> the controller id assigned to each controller under each nvme subsystem.
>> We can then leverage this information to figure out whether each controller 
>> id specified in the attach-ns command is valid or not. So essentially we 
>> match controller id specified in attach-ns command against the controller id 
>> learnt through scanning the topology. If we find that any discrepancy then we
>> can show the WARNING to the user. I have worked out a patch using this method. 
>> I have attached the patch below for suggestion/feedback.
> 
> So this is just about issuing a warning. That's fine. I thought you want
> to do some more 'difficult' stuff.
> 
>> diff --git a/nvme.c b/nvme.c
>> index c1d4352a..533cc390 100644
>> --- a/nvme.c
>> +++ b/nvme.c
>> @@ -2784,6 +2784,23 @@ static int delete_ns(int argc, char **argv, struct command *cmd, struct plugin *
>>  	return err;
>>  }
>>  
>> +static bool nvme_match_subsys_device_filter(nvme_subsystem_t s, nvme_ctrl_t c,
>> +		   nvme_ns_t ns, void *f_arg)
>> +{
>> +	nvme_ctrl_t _c;
>> +	const char *devname = (const char *)f_arg;
>> +
>> +	if (s) {
> 
> use the early return pattern to avoid long indents, e.g.
> 
>     if (!s)
>         return false;
> 
ok will do.

>> +		nvme_subsystem_for_each_ctrl(s, _c) {
>> +			if (!strcmp(devname, nvme_ctrl_get_name(_c)))
>> +				return true;
>> +		}
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>  static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, struct command *cmd)
>>  {
>>  	_cleanup_free_ struct nvme_ctrl_list *cntlist = NULL;
>> @@ -2839,12 +2856,68 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
>>  
>>  	nvme_init_ctrl_list(cntlist, num, ctrlist);
>>  
>> -	if (attach)
>> +	if (attach) {
>> +		const char *cntlid;
>> +		int __cntlid;
>> +		char *p;
>> +		nvme_host_t h;
>> +		nvme_subsystem_t s;
>> +		nvme_ctrl_t c;
>> +		nvme_root_t r = NULL;
>> +		int matched = 0;
>> +		nvme_scan_filter_t filter = nvme_match_subsys_device_filter;
>> +
>> +		r = nvme_create_root(stderr, log_level);
>> +		if (!r) {
>> +			nvme_show_error("Failed to create topology root: %s",
>> +					nvme_strerror(errno));
>> +			return -errno;
>> +		}
>> +
>> +		err = nvme_scan_topology(r, filter, (void *)dev->name);
>> +		if (err < 0) {
>> +			if (errno != ENOENT)
>> +				nvme_show_error("Failed to scan topology: %s",
>> +						nvme_strerror(errno));
>> +			nvme_free_tree(r);
>> +			return err;
>> +		}
>> +		nvme_for_each_host(r, h) {
>> +			nvme_for_each_subsystem(h, s) {
>> +				nvme_subsystem_for_each_ctrl(s, c) {
>> +					cntlid = nvme_ctrl_get_cntlid(c);
>> +					errno = 0;
>> +					__cntlid = strtoul(cntlid, &p, 0);
>> +					if (errno || *p != 0)
>> +						continue;
>> +					for (i = 0; i < num; i++) {
>> +						if (__cntlid == list[i])
>> +							matched++;
>> +					}
>> +				}
>> +			}
>> +		}
>> +
>> +		nvme_free_tree(r);
>> +
>> +		if (matched != num) {
> 
> Wouldn't this check also trigger when the user wants to attach to the
> discovered controller?
> 
Yes it will be triggered however in that case the if check would evaluate 
to false and hence we would not enter into the exception case.

> I would also suggest to move this check into a helper function in order
> to keep this function a bit more readable.
Yeah I will create a helper and call it from nvme_attach_ns().

> 
>> +			fprintf(stderr,
>> +				"You are about to attach namespace 0x%x to an undiscovered nvme controller.\n",
>> +				cfg.namespace_id);
>> +			fprintf(stderr,
>> +				"WARNING: Attaching nampespace to undiscovered nvme controller may have undesired side effect!\n"
>> +				"You may not be able to perform any IO to such namespace.\n"
>> +				"You have 10 seconds to press Ctrl-C to cancel this operation.\n\n");
>> +			sleep(10);
>> +			fprintf(stderr, "Sending attach-ns operation ...\n");
>> +		}
>> +
>>  		err = nvme_cli_ns_attach_ctrls(dev, cfg.namespace_id,
>>  					       cntlist);
>> -	else
>> +	} else {
>>  		err = nvme_cli_ns_detach_ctrls(dev, cfg.namespace_id,
>>  					       cntlist);
>> +	}
>>  
>>  	if (!err)
>>  		printf("%s: Success, nsid:%d\n", cmd->name, cfg.namespace_id);

I think, I will send a formal patch now with above comments incorporated and we may 
discuss further if there's any additional comment.
 Thanks,
--Nilay