nvme list-subsys fails to error out if invalid device is passed to it

[martin-gpy]

nvme list-subsys fails to error out if invalid device is passed to it via the <device> option. For e.g. suppose one has a single subsystem with two namespaces in it:

# nvme list
Node                  Generic               SN                   Model                                    Namespace  Usage                      Format           FW Rev
--------------------- --------------------- -------------------- ---------------------------------------- ---------- -------------------------- ---------------- --------
/dev/nvme1n1          /dev/ng1n1            81CYrBSoKGRJAAAAAAAN NetApp ONTAP Controller                  0x2        219.47  MB /  10.74  GB    512   B +  0 B   9.18.1
/dev/nvme1n2          /dev/ng1n2            81CYrBSoKGRJAAAAAAAN NetApp ONTAP Controller                  0x1        330.56  MB /  10.74  GB      4 KiB +  0 B   9.18.1

# nvme list-subsys /dev/nvme1n1
nvme-subsys1 - NQN=nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0
               hostnqn=nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a
\
 +- nvme1 tcp traddr=192.168.2.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live optimized
 +- nvme2 tcp traddr=192.168.1.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live optimized
 +- nvme3 tcp traddr=192.168.2.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live non-optimized
 +- nvme4 tcp traddr=192.168.1.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live non-optimized

But I also see a similar list-subsys output even for an invalid device like say nvme1n3, when ideally it should have errored out with an invalid device output:

# nvme list-subsys /dev/nvme1n3
nvme-subsys1 - NQN=nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0
               hostnqn=nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a
\
 +- nvme1 tcp traddr=192.168.2.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme2 tcp traddr=192.168.1.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme3 tcp traddr=192.168.2.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme4 tcp traddr=192.168.1.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live

And one can also see a blank output if a different invalid device with a different subsys number is passed to it. For e.g.

# nvme list-subsys /dev/nvme2n1
# echo $?
0

So obviously list-subsys is broken while handling invalid or non-existent NVMe devices.

[martin-gpy]

An attempt to fix this is available at #2827.

But now there are multiple opinions on how to address this. @igaw prefers addressing this in the nvme_match_device_filter() whereas @hreinecke prefers checking whether the passed device is valid through a stat check before proceeding further. So we need to reach a consensus on how to proceed here.

[igaw]

I want to understand why the filter does not work. The commit message doesn't explain it.

[martin-gpy]

I want to understand why the filter does not work. The commit message doesn't explain it.

I don't know why the filter doesn't work - haven't checked it yet. But the question is do we even want to fix the filter for catching this issue, or go with the simpler stat check for the device without waiting for the filter to catch it? Isn't that what we discussed today with Hannes to get a consensus? Or have you already made up your mind that the filter fix is the only acceptable approach to address this? Please clarify.

[igaw]

Adding the stat might be the right fix but I don't understand what is broken. Why doesn't the filter work? Currently, this feels like a workaround as I don't know how it fixes your problem.

[martin-gpy]

Looks like the filter is working fine by correctly filtering the valid subsystems and underlying entities. For e.g.

# nvme list
Node                  Generic               SN                   Model                                    Namespace  Usage                      Format           FW Rev
--------------------- --------------------- -------------------- ---------------------------------------- ---------- -------------------------- ---------------- --------
/dev/nvme1n1          /dev/ng1n1            81CYrBSoKGRJAAAAAAAN NetApp ONTAP Controller                  0x1        330.56  MB /  10.74  GB      4 KiB +  0 B   9.18.1

Now running list-subsys on the above valid nvme1n1 device with debug logging:

# nvme list-subsys /dev/nvme1n1 -vv
scan controller nvme1
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme1
scan controller nvme1 path nvme1c1n1
scan controller nvme2
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme2
scan controller nvme2 path nvme1c2n1
scan controller nvme3
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme3
scan controller nvme3 path nvme1c3n1
scan controller nvme4
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme4
scan controller nvme4 path nvme1c4n1
scan subsystem nvme-subsys1
scan subsystem nvme-subsys1 namespace nvme1n1
opcode       : 06
flags        : 00
rsvd1        : 0000
nsid         : 00000001
cdw2         : 00000000
cdw3         : 00000000
data_len     : 00001000
metadata_len : 00000000
addr         : 2e620000
metadata     : 0
cdw10        : 00000000
cdw11        : 00000000
cdw12        : 00000000
cdw13        : 00000000
cdw14        : 00000000
cdw15        : 00000000
timeout_ms   : 00000000
result       : 00000000
err          : 0
latency      : 9311 us
nvme-subsys1 - NQN=nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0
               hostnqn=nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a
               model=NetApp ONTAP Controller
               serial=81CYrBSoKGRJAAAAAAAN
               firmware=9.18.1
               iopolicy=queue-depth
               type=nvm
\
 +- nvme1 tcp traddr=192.168.2.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live optimized
 +- nvme2 tcp traddr=192.168.1.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live optimized
 +- nvme3 tcp traddr=192.168.2.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live non-optimized
 +- nvme4 tcp traddr=192.168.1.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live non-optimized

And running list-subsys for an invalid ns device in the same subsystem:

# nvme list-subsys /dev/nvme1n3 -vv
scan controller nvme1
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme1
scan controller nvme1 path nvme1c1n1
scan controller nvme2
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme2
scan controller nvme2 path nvme1c2n1
scan controller nvme3
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme3
scan controller nvme3 path nvme1c3n1
scan controller nvme4
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme4
scan controller nvme4 path nvme1c4n1
scan subsystem nvme-subsys1
scan subsystem nvme-subsys1 namespace nvme1n1
opcode       : 06
flags        : 00
rsvd1        : 0000
nsid         : 00000001
cdw2         : 00000000
cdw3         : 00000000
data_len     : 00001000
metadata_len : 00000000
addr         : 243b4000
metadata     : 0
cdw10        : 00000000
cdw11        : 00000000
cdw12        : 00000000
cdw13        : 00000000
cdw14        : 00000000
cdw15        : 00000000
timeout_ms   : 00000000
result       : 00000000
err          : 0
latency      : 7071 us
filter out namespace nvme1n1
nvme-subsys1 - NQN=nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0
               hostnqn=nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a
               model=NetApp ONTAP Controller
               serial=81CYrBSoKGRJAAAAAAAN
               firmware=9.18.1
               iopolicy=queue-depth
               type=nvm
\
 +- nvme1 tcp traddr=192.168.2.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme2 tcp traddr=192.168.1.116,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme3 tcp traddr=192.168.2.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live
 +- nvme4 tcp traddr=192.168.1.117,trsvcid=4420,host_traddr=192.168.1.6,src_addr=192.168.1.6 live

And then running list-subsys on an invalid device from a different invalid subsystem altogether:

# nvme list-subsys /dev/nvme2n1 -vv
scan controller nvme1
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme1
scan controller nvme1 path nvme1c1n1
filter out controller nvme1
scan controller nvme2
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme2
scan controller nvme2 path nvme1c2n1
filter out controller nvme2
scan controller nvme3
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme3
scan controller nvme3 path nvme1c3n1
filter out controller nvme3
scan controller nvme4
lookup subsystem /sys/class/nvme-subsystem/nvme-subsys1/nvme4
scan controller nvme4 path nvme1c4n1
filter out controller nvme4
scan subsystem nvme-subsys1
filter out subsystem nvme-subsys1
failed to scan subsystem nvme-subsys1: Invalid argument

[martin-gpy]

I've attempted fixing this using the stat approach for checking ns device validity with a new commit message explaining the rationale at #2827. That seems the cleaner & simpler approach instead of trying to fix this in the filter which only seems complicated and probably not worth the effort.

[martin-gpy]

@igaw ,

This is the snippet from the list-subsys code:

        if (devname) {
                int subsys_num;

                if (sscanf(devname, "nvme%dn%d", &subsys_num, &nsid) != 2) {
                        nvme_show_error("Invalid device name %s", devname);
                        return -EINVAL;
                }
                filter = nvme_match_device_filter;
        }

        err = nvme_scan_topology(r, filter, (void *)devname);
        if (err) {
                nvme_show_error("Failed to scan topology: %s", nvme_strerror(errno));
                return -errno;
        }

Are you expecting nvme_scan_topology() to fail here for an invalid device - either from a valid subsystem (like nvme1n3 in the example above) or invalid subsystem itself (like nvme2n1 above) and print the above "Failed to scan topology" error?

[igaw]

As I tried to explain, the filter needs to filter out. Adding a stat and then exit if it fails, is a possible way to achieve it. BUT this means the existing filter code should also be removed. Since libnvme already has this filter API, we should just fix it and then this will work as well.