nvmecontrol: Use active nslist to enumerate namespaces

[jhb]

Summary

Rather than probing all namespace IDs up to cdata.nn for the devlist
command, fetch the active namespace list and iterate over that. This
can be much quicker on Fabrics controllers which often advertise a
large cdata.nn value to support adding additional namespaces at
runtime.

Reported by: Neven Z <[e-mail]>
Sponsored by: Chelsio Communications

[imp]

sbin/nvmecontrol/devlist.c

158

what's magic about these numbers? Don't we have #defines for it?

[jhb]

sbin/nvmecontrol/devlist.c

158

This one is less obvious and we do not have a constant for it. The spec says this about the active namespace list identify query:

5.15.2.3 Active Namespace ID list (CNS 02h)
A list of 1,024 namespace IDs is returned to the host containing active NSIDs in increasing order that are
greater than the value specified in the Namespace Identifier (NSID) field of the command. The controller
should abort the command with status code Invalid Namespace or Format if the NSID field is set to
FFFFFFFEh or FFFFFFFFh. The NSID field may be cleared to 0h to retrieve a Namespace List including
the namespace starting with NSID of 1h. The data structure returned is a Namespace List (refer to section
4.10).

The only constant we have is for the all F's NSID:

#define NVME_GLOBAL_NAMESPACE_TAG	((uint32_t)0xFFFFFFFF)

I guess I should be allowing d actually and only breaking for 'e'. I could rewrite this as:

if (nsid >= NVME_GLOBAL_NAMESPACE_TAG - 1)
    break;

(I have this same 'd' vs 'e' bug in nvmf actually)

[jhb]

Use constant and fix off by one

[olce]

sbin/nvmecontrol/devlist.c

158

Reading the extract you quoted, it seems that the FFFFFFFEh or FFFFFFFFh invalid values are with respect to the nsid value passed to read_active_namespaces(), and not values returned by the controller.

If this is correct, then the if (nsid >= NVME_GLOBAL_NAMESPACE_TAG - 1) should be moved after nsid++, or conversely, the previous version of the test was the right one (this one is off by one).

[jhb]

sbin/nvmecontrol/devlist.c

158

Actually, I should probably remove the 'nsid++' (and also fix that in the nvmf(4) driver as well). I had missed the fact that the first returned nsid is > not >= the initial nsid.

[jhb]

Remove spurious nsid++

[imp]

sbin/nvmecontrol/devlist.c

158

There is an ambiguity around fffffffe in the standard. But on the whole, i don't think it's a valid nsid and is special...

158

Any reason we don't have a define for the broadcast namespace? Isn't that what 0xfffffffe is.?

[jhb]

sbin/nvmecontrol/devlist.c

158

I do think fffffffe is a valid NS ID. Only 0 and the broadcast ID are invalid from my reading of the spec. The reason though you can't pass fffffffe to this IDENTIFY request is that it uses the input nsid + 1 as the starting ID to return, and that starting ID would be the broadcast ID.

158

No, all 1's is the broadcast and is the constant now used in the patch.

[chuck]

sbin/nvmecontrol/nvmecontrol.c

153

At some point, it'd be good to add macros for the collection of Identify commands we use

[jhb]

sbin/nvmecontrol/nvmecontrol.c

153

Yeah, we use magic numbers here, in nvme(4), and in nvmf(4). Probably in the target side of nvmf(4) also and in bhyve. It would be nice to add constants to nvme.h.