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

Nilay Shroff
2024-06-20T09:02:49Z

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