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

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

On 5/16/24 11:55, Nilay Shroff wrote:
> 
> 
> On 5/7/24 17:14, Nilay Shroff wrote:
>>
>>
>> On 5/6/24 20:35, Daniel Wagner wrote:
>>> On Mon, May 06, 2024 at 07:45:20PM GMT, Nilay Shroff wrote:
>>>> There could be multiple ways to address this issue. However my proposal would be to address
>>>> it in nvme-cli. As nvme-cli builds the nvme topology it shall be easy
>>>> for nvme-cli to detect
>>>
>>> The topology scan in libnvme is only using the information available in
>>> sysfs. libnvme doesn't issue any commands anymore and I'd like to keep
>>> it this in this way. So if the kernel doesn't exposes this information
>>> to userspace via sysfs, I don't think it's simple to 'fix' this in
>>> nvme-cli/libnvme.
>>>
>> 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.
>>
>> 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) {
>> +		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) {
>> +			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);
>>
> Gentle ping... 
> 
> If there's no objection to the above proposed patch then may I send a formal patch?
> 
> 
Hi Daniel,

A gentle ping, if the proposed patch looks good then may I send a formal patch?

Thanks,
--Nilay