[nfsv4] Review of draft-rmacklem-nfsv4-new-attributes-00

[David Noveck <[e-mail]>]

*General Comments*

*Overall Evaluation*
Document is in pretty good shape for a -00.   I only see minor technical
issues which should be easy to address. Nevertheless, there are important
editorial and organizational issues that the author needs to address before
requesting working group adoption.

*Organizational Issues to Address*
A major issue that makes the document harder to read than it might be is
that there are three lists of new attributes so that the reader has to
correlate things said about an attribute in one list, with related text in
another list.
A related issue with the current choice of organization concerns the
table-of-contents in which none of the attributes has an entry.  There are
a number of ways to address this issue, discussed in *Section 5.1
(Definitions of New "RECOMMENDED" Attributes)*

*Editorial issues to Address*
It seems to me that the following issues need to be addressed:

   - The absence of a "Requirements Language" section

This will be needed for a standards-track document.  It should probably be
Section 2 and will probably have to be the new form that mentions rfc 8174.


   - The repeated use of the term "RECOMMENDED" in a way that is not in
   line with RFC2119.

I realize this is in line with previous practice but that practice is
changing in rfc5661bis and flat-out contradicts the (correct) description
of these  attributes as "OPTIONAL".

*Technical Issues to Address*
Most of these are discussed in *Comments by Section*.  The only issue that
needs to be mentioned here concerns the potential for non-uniform behavior
with an fs. It is not unreasonable to assume uniform behavior but the
existing specs are insufficiently clear on this point, making it necessary
to address this for each attribute.  Sigh!

*Comments by Section*

*Abstract*
Given that these attributes are only proposed so far, suggest replacing
"are" by "would be" in the second sentence.

*Section 1 (Introduction)*
The second sentence, as written, suggests that these are the only such
attributes. I think it makes sense to rewrite it as "This document
identifies an important set of additional recommended attributes."

*Section 3 (**List of new per-file system Attributes)*
Suggest nuking this section since it doesn't have any information in
addition to that provided by the table in Section 5 below.

*Section 4 (New Attributes)*
There is no point in this one-line section.   If necessary, this fact could
be in a new paragraph of Section 5.

*Section 5 ("RECOMMENDED" New Attrbutes - List and Definitions)*
This table is harder to read than it might be because the attribute names
are too long.  Suggest:

   - Replacing "directory_cookie_montonically_increasing" by
   "dir_cookie_rising"
   - Replacing "supported_operations" by "supported_ops"
   - Replacing "maximum_extended_attribute_length" by "max_xattr_length"

Also suggest replacing "mandatory_locking" by  "mandatory_br_locks" for
clarity.

*Section 5.1 (**Definitions of New "RECOMMENDED" Attributes)*
This one-line section provides no value and should be removed.  The
included subsections 5.1.x should either be:

   - Made into sections 5.x
   - Made into top-level sections.

*Section 5.1.1 (**Attribute 83: supported_operations)*
Suggest replacing the first paragraph by the following:

This bit vector indicates which operations are supported for objects
(of the appropriate type) with an fsid matching that the specified
object.

In the second sentence of the second paragraph, suggest replacing "bit n"
by "the bit corresponding to operation n".

*Section 5.1.2 (**Attribute 84: directory_cookie_monotonically_increasing)*
Suggest replacing this by the following:

TRUE, if performing the READDIR operation on directories with a
matching fsid always returns monotonically increasing directory offset
cookies.   The includes, if named attributes are supported,
directories returned by OPENATTR.

*Section 5.1.3 (Attribute 85:  seek_granularity)*
Suggest replacing the first paragraph by the following:

This attribute indicates the granularity of unallocated regions for
data objects with an fsid matching that of the specified object.
Data objects include regular files and, when named attributes are
supported, named attributes.

*Section 5.1.4 (Attribute 86: mandatory_locking)*
Suggest replacing this by the following:

TRUE, if byte range locks obtained on data objects with an fsid matching
that of the specified object have mandatory semantics potentially affecting
IO operations done overlapping areas. Data objects include regular files
and, when named attributes are supported, named attributes.

*Section 5.1.5 (Attribute 87: **maximum_extended_attribute_length)*
Suggest replacing this by the following:

The maximum length, in bytes, of the extended attribute that can be
set by the SETXATTR operation for file system objects with an fsid
matching that of the specified object. The SETXATTR operation is
described in the [RFC8276] extension to NFSv4.2.

*Section 5.2 (**Rational for New RECOMMENDED Attributes)*
I recommend the following regarding this section and included subsections:

   - That section 5.2 be deleted
   - That the included sections 5.2.x each be merged with the corresponding
   section 5.1.x.

*Section 5.2.1 (**Attribute 83: supported_operations)*
Suggest adding the following paragraph:

This attribute is likely to be particularly helpful in dealing with
OPTIONAL attributes whose support is likely to be different for different
file systems.

*Section 5.2.4 (Attribute 86: mandatory_locking*)
I disagree with the rationale currently there.  If a  client were to get
mandatory locks on cached areas on its own, as suggested, applications
rewriting those areas would get IO errors.   This approach would require
byte-range *delegations*.
I propose something like the following:

Applications that work with advisory byte- range locks will fail with
mandatory byte-range locks and vice versa.  Given that both forms are
allowed yet incompatible, it is necessary to provide a way, other than
trial-and-error, to determine which form is supported.


*Section 7.2 (I**nformational References)*
The reference to RFC2119 should be normative and put in Section 7.1 as
should the reference to rfc8174 when that is added.