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

[Nilay Shroff]

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);

Thanks,
--Nilay