[PATCH 2/3] fine tune the nvme-discover manpage
hch at lst.de
Thu Oct 13 03:26:32 PDT 2016
On Wed, Oct 12, 2016 at 10:18:05AM -0700, J Freyensee wrote:
> > -nvme-discover - Send Get Log Page request to Discovery Controller.
> > +nvme-discover - Send Discovery requests to Fabrics Discovery
> > Controllers.
> I preferred mentioning "Get Log Page" because that is really what is
> happening and reflects the NVMe specification, which I was aiming to
> push the envelope a bit and help educate and accelerate the learning
> curve for people new to these features (map more accurately what is
> being described here to the NVMe specification(s).
While Get Log Page is technicaly correct I don't really think it
serves the user much - it's the way the discovery is implemented,
the important part is that we perform discovery.
> > SYNOPSIS
> > --------
> > [verse]
> > -'nvme discover' [device]
> > +'nvme discover'
> It was there because nvme-cli works regardless if a device parameter is
> there or not (such as /dev/nvme-fabrics/). Granted it seems a bit
> pointless to include it if the same correct behavior works without it,
> but it made it common with the rest of the nvme-cli command man pages.
Well, the point is that we hardcode the /dev/nvme-fabrics path in
fabrics.c. So while nvme-cli accept as a device argument it is ignored.
We'll need to fix the option parse to properly reject it, I think.
> This is unnecessary to be mentioning the flags here when they have
> already been mentioned and shown in "SYNOPSIS" as well as described in
> detail below with examples provided after that. This isn't adding much
> additional value and making this slightly harder to maintain if flags
> are added/adjusted.
> (necessary is misspelled btw).
The important addition I wanted is something to balance the description
of what happens if no arguments are specified.
That being said I really, really hate the having the read config file
behavior the default without option. I already complained about it
when it went in, and having to read the manpage it's even more confusing.
I'd much prefer making it a --config option that points to a config
file, as the prime user of it is the system startup script anyway.
> > |=================
> > -a <traddr>::
> > --traddr=<traddr>::
> > This field specifies the network address of the Discovery
> > Controller.
> > + For transports using IP addressing (e.g. rdma) this should
> > be an IPv4
> > + address.
> We shouldn't be mentioning that this can be specifically a IPv4 address
> when there has already been work to support an IPv6 address (for
> example) and then this will have to be tweaked again. I think the
> explanation was fine the way it is and should be explained generically
> that leaves out specific mentions of IPv4 (plus the examples provided
> afterword show using an IPv4 address).
I was tempted to add IPv6, but given that it's not supported I skipped it.
We'll need some guidance on what the valid address format are. I'm not
sure what the best way is, but I don't think example are sufficient.
Maybe a table in a separate section or even man page.
> Good start, but I can see an interpretation that this can make it sound
> like the kernel will autogenerate hostnqn if this parameter is not
> supplied, which would be incorrect because it's already generated upon
> NVMe Host module load to be used as a 'catch-all default'. I think a
> further tweak to this could be:
> "If this option is not specified, the default is read from
> /etc/nvme/hostnqn first and if that does not exist, the autogenerated
> NQN value from the NVMe Host kernel module is used next."
> > + The Host NQN uniquely identifies the NVMe Host, and may be
> > used by the
> > + the Discovery Controller to control what NVMe Target
> > resources are
> > + allocated to the NVMe Host for a connection.
> This sounds like it could go in the BACKGROUND section I wrote, which I
> admit I didn't not state this as clearly as here, but then again it's
> probably ok to keep this blurb here to re-emphasize this important
> feature of NVMe devices.
I'm fine with moving this around.
More information about the Linux-nvme