show-regs has excess dependencies

[NateThornton]

The show-regs command has some additional dependencies that the drive is operational and responding to an Identify admin command. For a drive that is truly unresponsive, say in a Controller Fatal Status = True state, this means one cannot inspect the controller registers.

Call stack:

show_registers
  nvme_scan
    nvme_scan_topology
      nvme_scan_subsystem
        nvme_subsystem_scan_namespace
          nvme_subsystem_scan_namespace
            __nvme_scan_namespace
              nvme_ns_open
                nvme_ns_init
                  nvme_ns_identify

Doing a namespace scan seems unnecessary for the show-regs command, and I'm wondering if this can somehow be removed so the show-regs is still functional in fatal situations. That would likely include an enhancement to libnvme.

If someone can provide guidance on the necessary code changes, I can either work on the PR or help with validation.

Thanks

[keithbusch]

That does seem excessive just to get some registers. I think part of this might be trying to establish what transport the device is since there are different ways to access the registers depending on the interface. For pci, though, we don't even really need a device handle, which I suspect is the transport you're interested in.

Since it's possible a device handle doesn't even exist for a failed controller, we could add a toggle parameter to the show-regs command to take a /sys/bus/pci/devices/.../resource0 argument instead of a /dev/nvme0 and skip all the identification steps.

[NateThornton]

Thanks Keith, the change ended up being pretty trivial and I just needed a dummy root object.

	if (!cfg.no_scan)
		r = nvme_scan(NULL);
	else
		r = nvme_create_root(NULL, DEFAULT_LOGLEVEL);

I can get a PR prepared.

Is using --no-scan with help text "skip NVMe scan and use the provided device directly" acceptable naming/description? We can visit that on the PR too.

[keithbusch]

That will still scan the controller and/or namespace in mmap_registers() though, right? This change will just avoid scanning every nvme device in the system, which honestly doesn't appear to provide any use in this path, so your suggestion should just be the default. But even that looks like it's doing to much; we just want to the sysfs directory of the requested device/namespace. We don't need all the extra device properties that are being queried for show-registers.

[NateThornton]

I checked via strace and no ioctls were observed with the added --no-scan behavior when a controller is found. Otherwise you are correct, we'd still do a nvme_scan_namespace if the controller is not found.

nvme-cli/nvme.c

Lines 5469 to 5483 in 9e532bb

	c = nvme_scan_ctrl(r, dev->name);
	if (c) {
	snprintf(path, sizeof(path), "%s/device/resource0",
	nvme_ctrl_get_sysfs_dir(c));
	nvme_free_ctrl(c);
	} else {
	n = nvme_scan_namespace(dev->name);
	if (!n) {
	nvme_show_error("Unable to find %s", dev->name);
	return NULL;
	}
	snprintf(path, sizeof(path), "%s/device/device/resource0",
	nvme_ns_get_sysfs_dir(n));
	nvme_free_ns(n);
	}

[igaw]

The nvme_ns_identify call will be avoided with linux-nvme/libnvme#754

This means command still scans the hierarchy scanned. Let's see if we can cut down some more of the overhead.

[igaw]

This should be fixed with #2251.