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

From: David Noveck <[email-masked]>
Date: Sun, 28 Sep 2014 08:08:50 -0400

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
.

*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.

*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".

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.

*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.

*Section 1.4.4:*

The "And" starting the last sentence is wrong. It needs to be deleted or
replaced by "Also,"
* 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".

*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.

*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.

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.

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

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

*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.

*Section 7.1:*

In the first sentence of the last paragraph, suggest replacing 'this
problem" by "these attributes".

*Section 9.7:*

Suggest replacing the section title by "Security Considerations for Labeled
NFS".

*Section 10:*

This is another chapter in which the only section is an introduction.

*Section 10.1*

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

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

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.

*Section 11:*

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

*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.

*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.

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.

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.

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.

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.

*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.