Re: [nfsv4] Review of draft-ietf-nfsv4-scsi-layout-nvme-01

[Christoph Hellwig <[e-mail]>]

Hi Dave,

sorry for the delay.  I think I've addressed most comments in the just
uploaded -02, but more comments below.

On Sat, Jan 21, 2023 at 01:24:51PM -0500, David Noveck wrote:
> Although the details will be more completely discussed under *Per-section
> Comments* there seem to  be  cases in which the wording suggests an
> informational approach that is no longer appropriate  For example, the word
> "explain" is used in many contexts where it is the job of a standards-track
> document  to "define" or  "specify" something.

Agreed.

> *Issues with bcp14 Terms*
> 
> These terms will need to be wrapped in "<bcp14>...</bcp14>.

As far as I can tell these tags do not exist in the xml2rfc vocabulary.
Can you provide a reference for this wrapping?

> *Authors*
> 
> I think you need to designate an editor.

Done.

> *Title*
> 
> Given the decision to make this a standards-track document, feel something
> like "Use of NVMe-based Transports to Access Block Devices using the SCSI
> Layout Type" would be more appropriate.

I agree that the name isn't a little misleading now.  But your
suggestion seems overly verbose and a little misleading as NVMe
is not a transport for SCSI, but rather a different protocol
family.

We'll look into a different title going forward but haven't agreed on
one yet.

> 
> *Abstract *
> 
> I feel "explains" is now inappropriate and suggest a replacement like the
> following:
> 
> Defines the use of block devices accessed by NVMe-based Transports in
> connection with the pNFS SCSI layout type.

Fixed.

> *1. Introduction *
> 
> I feel that the first paragraph presumes a lot of knowledge that the
> authors and most working group members have but that many readers might not
> have. An example is the use of the term MDS which is never defined in this
> document.

Thanks, I've taken up all your suggestions for the introduction.

> *1.2 General Definitions*
> 
> With regard to the definition of "client", I have problems with the use of
> the word "traditional" in connection with clients embedded within OS's that
> support multiple applications.  I don't think this belongs here.

Agreed.

> With regard to "server", I note that "server" is defined here but never
> really used in this document, except in the definition of "client".
> Elsewhere there are references to the metadata server and to the data
> server. I  think these need to be defined rather than the orphaned term
> "server".

Agreed.

> 
> *2. SCSI Layout Mapping to NVM*
> 
> In the first sentence,
> 
> Suggest replacing "only references few" by "references only a few".
> 
> Believe "SCSI specific" should be hyphenated.

Fixed.

> 
> With regard to the use of the word  "SHOULD", It is unclear to whom this
> recommendation is directed.
> 
> Recommendations are generally as to what an implementation might do or not
> do and mapping of concepts doesn't fit that model.
> 
> It is hard to conceive of what might be "valid reasons" to do other than
> what is recommended.

We've done a scrube of potentially problematic SHOULDs.  Please take
a look if you still find some that looks problematic to you in -O2.

> Regarding the banning of the use serial numbers, I agree that they not be
> allowed but I'm guessing that they were allowed in RFC8154, which calls
> into question the idea that this spec does not amend RFC8154.  It seems
> that it does, although for a good reason.

There is no ammendment here, as there is no equivalent to the SCSI
Name string in NVMe.  The NVMe serial number is something very different.
SCSI name string.  I hope the new version makes this a bit more clear.

> Along these lines, there might need to be some consideration of the case in
> which the SCSI layout is used to support both SCSI and NVMe-connected
> devices.  Is it possible that SCSI serial number might conflict with EUI64
> or NGUID device ids.

Both of them are defined based on IEEE identifiers that shall be
globally uniqueue, independent of how they are used.

> The third paragraph does not make it sufficiently clear what exactly  are
> the "unique addressing needs" that need to be satisfied.

What additional information would you like to see here?

> 
> *2.2 Client Fencing*
> 
> In the second sentence of the first paragraph, suggest replacing "For this"
> by "For this to be achieved".
> 
> In the second paragraph suggest replacing "The following is" by "The
> following sub-sections (Section 2.2.1 through 2.2.4) provides".

I've updated this, but without explicitly listing the sections.

> *2.3 Write Caches*
> 
> In this case, it appears that we are not, as with other subsections of *2.0
> SCSI Layout Mapping to NVMe*, mapping a SCSI operation to an NVMe one. If
> so,
> this should be moved to its own top-level section and further tinkering
> with the statement about not amending RFC8154 might be required.

This is the NVMe version of section 2.8 in RFC8154, and it is very
much intendent to translate the SCSI concepts there.  I've added a blurb
to make that more clear.

> 
> *4. Security Considerations *
> 
> Think you need to add a new paragraph regarding the obligation of the
> clients regarding the enforcement of MDS decisions.   I am proposing
> something like the following after the current first paragraph:
> 
> Decisions regarding authorization of particular users to access file
> objects are made by the MDS and communicated to the clients by the ranting,
> recall, and revocation of layouts, as described in the definition of
> the pNFS feature (see [RFC8881}, [RFC8534]).  As a result, the security of
> the data made available to clients depends on clients not making
> 
> IO requests without the appropriate layout.   When clients cannot be
> trusted to conform to this requirement, direct access to block devices MUST
> NOT be provided.

This is all very much in the domain of 8154.  We've added reference
to the relevant sections of 8154 instead of rehashing this here.

> 
> 
> *5.Normative References *
> 
> Feel RFC8534 should be added
>
>

RFC8534 is "Explicit Tracking with Wildcard Routes in Multicast VPN".
Do you mean  RFC8434?  This is not a new layout type, so it's not
really applicable.

Thanks a lot again for your review!