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

[Daniel Wagner]

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;

> +		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?

I would also suggest to move this check into a helper function in order
to keep this function a bit more readable.

> +			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);
> 
> Thanks,
> --Nilay
>