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

Daniel Wagner
2024-06-20T06:53:13Z

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
>