Subject: Re: [nfsv4] My review of draft-ietf-nfsv4-minorversion2-27

From: Tom Haynes <[email-masked]>
Date: Mon, 24 Nov 2014 10:25:20 -0800

Hi Dave,

Thanks for the due diligence.

Tom

> On Sep 28, 2014, at 5:08 AM, David Noveck <[email-masked]> wrote:
> 
> Overall, this document is good in shape.
> 
> Abstract:
> 
> Saying that the document will "focus mainly" on the 4.2 extensions, seems to imply there is something else in the document.  As there isn't, I suggest replacing the first sentence by:
> 
> This Internet-Draft describes NFS version 4 minor version two, presenting the protocol extensions and other changes made from NFS version 4 minor one .

The intent is to say we are not going to rehash everything again.

        This Internet-Draft describes NFS version 4 minor version two,
        describing the protocol extensions made from NFS
        version 4 minor version 1.

> 
> Section 1.[12]:
> 
> Not sure if this goes in 1.1 or 1.2, but in one of those places we need to state the basic rule that all operations, attributes , etc. not mentioned in this document are to be as described in the definition of NFSv4.1.

I thought 1.2 did that.

1st bullet item:

describe the NFSv4.0 or NFSv4.1 protocols, except where needed to contrast with NFSv4.2

> 
> Section 1.3:
> 
> Not clear that all of the 4.2 features can be subsumed under this goal.  Suggest replacing "The goal" by "A major goal".
> 

Ack

> Also this section consists two sentences.  The second of these has three internal bullet points followed by a clause beginning "but".
> 
> Suggest replacing the material after the bullets by the following:
> 
> NFSv4.2 provides means for clients to leverage these  features on the server in cases in which that had previously not been possible within the confines of the NFS protocol.

Ack

> 
> Section 1..1:
> 
> The intraserver case of server side copy is not mentioned but should be.  Suggest replacing "from one server to another" by "from a location on one server to another location on the same server or a different one".  Also replace "the two servers'" by "source and destination servers".
> 
> Section 1.4.2:
> 
> Suggest replacing the last sentence by 
> 
> This operation can be used to support the posix_fadvise function.  In addition, it may be helpful to applications such as databases and video editors.

Ack

> 
> Section 1.4.4:
> 
> The "And" starting the last sentence is wrong.  It needs to be deleted or replaced by "Also,"
> 

Ack

> Section 1.5:
> 
> The existing section title, "Differences from NFSv4.1" is troublesome since that is basically the contents of the entire document.  Suggest "Enhancements to Minor Versioning Model”.

Ack, but we need to visit this again given the whack job taken to the minot versioning section.

> 
> Section 2:
> 
> As a number of people have noted, it is kind of weird for each minor version to define it's own minor versioning rules.  I've been thinking that we need to move forward with a wg standards-track document defining minor versioning rules for NFSv4 as a whole.  
> 
> Nevertheless, we are where we are and should not let this issue hold up the v4.2 RFC.  When a minor-versioning RFC Is published, it will have to be marked as updating rfc3530bis, RFC5661, and the v4.2 RFC.
> 
> Suggest replacing "future accepted minor version will be documented in one or more Standards Track RFCs" by "future accepted minor versions and their constituent features will be documented in one or more Standards Track RFCs"
> 
> Suggest replacing the third paragraph by:
> 
> The following items represent the existing basic rules for the development of subsequent minor versions.  It is possible that a future minor version will modify or add to the following rules as part of the minor version definition.  However, the base assumption noted above is not subject to such change.
> 

Taken care of in the context of the new document.

> Section 4.4.2:
> 
> I have problems with the sentence: "The client SHOULD write back the data to the source or be prepared for the destination to get a corrupt copy of the file".  Being prepared for data corruption is not an implementation requirement.  How can one verify if an implementation meets it?  Also, given that it is a "SHOULD" and not a "MUST", what considerations might allow an implementation to not be prepared for data corruption?  Suggest the following replacement:
> 
> The client SHOULD write back the data to the source.  If it does not do so, it is possible that the destination will receive a corrupt copy of file.
> 

Ack

> Section 6.3.1:
> 
> Suggest replacing the first sentence of the last paragraph by the following:
> 
> When a client sends a READ operation, it is not prepared to accept a READ_PLUS-style response providing a compact encoding of the scope of holes.

Ack

> 
> In the second sentence suggest deleting "So" at start.

Ack

> 
> In the last sentence suggest replacing "In contrast" by "By contrast”.

Ack

> 
> 
> Section 7:
> 
> This currently consists of a single sub-section named "Introduction" which deals with space reservation and some space-related attributes.  This chapter needs re-organization.

No sub-section now

> 
> Section 7.1:
> 
> In the first sentence of the last paragraph, suggest replacing 'this problem" by "these attributes”.

Thanks, good suggestion.

> 
> Section 9.7:
> 
> Suggest replacing the section title by "Security Considerations for Labeled NFS”.

Ack

> 
> Section 10:
> 
> This is another chapter in which the only section is an introduction.

Ack

> 
> Section 10.1
> 
> In the first sentence of the first paragraph, suggest replacing "guidance" by "guidance as to its construction”.

Ack

> 
> In the second sentence of the first paragraph, suggest replacing "feature" by "constraint".
> 

Ack

> Suggest replacing the first sentence of the second paragraph by the following:
> 
> While this allows for a wide range of implementations, it also leaves the client with no way to determine which is the most recent value for the change attribute in a case where several RPC calls have been issued in parallel.
> 

Ack

> Section 11:
> 
> I think it makes more sense for this section to appear near the end of the document.

Agreed
:w

> 
> Section 12.2:
> 
> This contains error lists for operations which are not new: e.g. SEQUENCE and GETDEVICELIST.   Not sure exactly how to best fix this.

The GETDEVICELIST changes are an update from Christoph’s errata to 5661.

SEQUENCE does not need to be there.

> 
> Section 14:
> 
> This is a great addition to minor version documents.  Thanks for including it.
> 
> I suggest that the status values listed after the fifth paragraph be indented or bulleted.

These are done with the hangText attribute for the list. I.e., the first word is a label and the rest is the associated text. It would show up better, see Section 4.2.1, if there were multiple lines.

I’ll add “:” and see if it makes it better.

> 
> I suggest replacing the first sentence of the sixth paragraph by the following:
> 
> For the NFSv4.2 features that are OPTIONAL, the operations that support those features are OPTIONAL, and the server MUST return NFS4ERR_NOTSUPP in response to the client's use of those operations, when those operations are not implemented by the server.
> 

ACK

> I don't think it is right to list GETDEVICELIST as pNFS(MNI) as it is MNI independent of whether the pNFS feature is supported or not.

But you would never implement it if you did not do pNFS.

> 
> Listing WRITE_SAME as ADB(REQ) really conveys no information.  Like LINK, it is an optional operation not tied to support for any other operation.

It isn’t stating that WRITE_SAME is tied to another operation. What is supposed to convey is that if you claim to do ABD, then you MUST do WRITE_SAME. So it ties operation to feature.

> 
> I note that listing COPY_NOTIFY as COPY(REQ) seems to imply that a server may not support intra-server copy withuout supporting inter-server copy.  I'm not sure this is right.

Agreed - even though 4.2.2 spells this out in better detail.

Let me think about how to call this out in the table itself.

> 
> Section 15.1.4:
> 
> The way the last bulleted item is written it seems to suggest that all servers should be clustered.  I can't believe that's the intention.

Good catch, took me several reads to spot what you meant.