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

Nilay Shroff
2024-05-27T11:21:03Z

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