[nfsv4] WGLC Review of draft-Ietf-nfsv4-xattrs-03 (part two of two)

[David Noveck <[e-mail]>]

*Review Structure*

This review is presented in multiple parts to get around size limits
for emails on the working group list.

This is the second part and covers from Section 5 to the end of the
document. General comments and comments on the first part of the
document appear in the first review section.

*Comments by Section (from section 5)*

*5.  Relationship with Named Attributes*

Suggest replacing the last sentence of the first paragraph by the following:

Although this makes xattrs similar in concept and use to named
attributes, there are important semantic differences.

 Suggest replacing the third paragraph by the following:

Individual named attributes are analogous to files and are opened and
closed just as files are.  Caching of the data for these needs to be
handled just as data caching is for ordinary files following
close-to-open semantics.  Xattrs, on the other hand, have caching
requirements similar to those for other file attributes.

Suggest replacing the first sentence of the fourth paragraph by the
following:

Named attributes and xattrs have different semantics and are treated
by applications as belonging to disjoint namespaces.

In the second sentence of the fourth paragraph, suggest replacing "one to
another is" by "from one to the other would be".

In the first sentence of the fifth paragraph, suggest:

   - Replacing "should" by "might".
   - Replacing "carving" by "by carving"
   - Replacing "this is problematic" by "such an approach is sufficiently
   problematic that it will not be attempted here".

*7.  Protocol Extensions*

There is a problem with  the second bulleted item as it stands.  The
issue is that a server would send previously ACEs with previously
undefined acemask4 bits to clients who may not have given any
indication that they understand them.  Extensions like that fit within
the XDR extension paradigm and are allowable in a new minor version,
there are problems putting them in a standalone extension.

There has recently been some discussion of issues related to extension
in reply messages on the list in connection with the review of
versioning-07.  There will e a more complete discussion of this
subject in versioning-08 which will be out in early December.

Some very simple ways of addressing this issue would be:


   - Dropping the acemask4 extensions in the near-term and derive
permissions for xattrs from those relating to ordinary attributes.
   - Make this part of a very small minor version, including just this
feature  (plus later extensions).

Some other possible approaches are discussed in *Acemask4 Extensions*
(in the previous review section).

If you choose the first approach suggest the following replacement for
the last paragraph:

These changes follow applicable guidelines for valid NFSv4 XDR
protocol extensions, as specified in [NFSv4-vers], and obey the rules
for extensions capable of being made without a change in minor version
number.

On the other hand, if you decide on a new minor version, suggest the
following replacement:

These changes follow applicable guidelines for valid NFSv4 XDR
protocol extension, as specified in [NFSv4-vers], and also obey the
rules for valid minor version creation as described in [RFC5661].

Most of the alternatives described in *Acemask4 Extensions* would fit
better with the first alternative. YMMV.

*7.3.  New Error Definitions*

It isn't clear what errors will be returned if the collective limit on
xattr size is exceeded, as opposed to the limit on a single xattr.

*7.4   Added Operations*

I suggest deleting the first three paragraphs.

In the first sentence of the last paragraph, suggest adding, "In order
to meet these requirements,"  at the start.

Suggest adding the following new paragraph at the end:

Because some server may not be aware of the existence of these
operations, a client cannot always expect that issuing one of them
will either succeed or return NFS4ERR_NOTSUPP.  In some cases
NFS4ERR_OP_ILLEGAL will be returned or the request may encounter an
XDR decode error on the server. As a result, clients should only issue
these operations after determining that support is present.

*7.4.1.3.  DESCRIPTION **[GETXATTR]*

In the second sentence of the second paragraph, suggest adding "or
another error indicating the request was not understood" at the end of
the sentence.

*7.4.2.1.  ARGUMENTS [SETXATTR]*

Suggest replacing SETXATTR4_NONE by SETXATTR4_EITHER, since I think
this would be clearer.

*7.4.2.3.  DESCRIPTION **[SETXATTR]*

I'm concreted about the "SHOULD NOT" in the second sentence of the
second paragraph.  In particular, it isn't clear what the
interoperability consequences of violating the "SHOULD NOT" might be.

A possible replacement might be:

However, it is not NECESSARY to change these attributes if there has
been no actual change in the xattr value. Avoiding attribute change in
such situation is desirable as it avoids unnecessary cache
invalidation.

*7.4.4.3.  DESCRIPTION [REMOVEXATTR]*

I think the "SHOULD" in he second paragraph needs to be rethought, since:


   - There is no clear reason stated why this is not a "MUST".
   - According to RFC2119, a "SHOULD" differs from  MUST in that you
can violate a "SHOULD" if you are aware of the consequences but there
is no discussion to make clear what those consequences might be.

*7.4.5.  Valid Errors*

Suggest adding "and Section 11.2 of [RFC7862]" to the end of the last
sentence of the first paragraph.

*7.5.  Modifications to Existing Operations*

In the following, I will number paragraphs including all paragraphs,
whether bulleted, indented or not, or hanging.

Based on the discussion in *7.  Protocol Extensions* and *Acemask4
Extensions*, you might want to delete the eighth through eleventh
paragraphs.  On the other hand, if you retain these, you need to
change "two" to "three" in the eighth paragraph.

In the thirteenth through fifteenth paragraphs, suggest replacing
"SHOULD" by "should".

You might want to simply delete the sixteenth through twentieth
paragraphs. However if you don't, you need to address the following
issues in the sixteenth through eighteenth paragraphs:


   - It is incler what "check for it in the mode" means since these
bits don't appear in the mode.
   - It is not clear what "MUST only" means.
   - The most obvious meaning of "MUST only" is that the server may
not make this check in any other circumstances but that would be wrong
since the server *has to *check for permissions when doing SETXATTR,
GETXATTR, LISTXATTR, REMOVEXATTR.

*7.7.  Caching*

In the second sentence of the second paragraph, suggest replacing
"depend" by "be based".

Suggest the following replacement for the first sentence of the sixth paragraph:

An effective way to enable the client to make this determination
simply is for the client to serialize all xattr changes made to a
specific file.

*10.  References*

Because some of the references may have to shift between normative and
informative, I'm listing all those that need to be updated here.  In
particular:


   - [NFSv42] needs to be updated to [RFC7862].
   - [NFSv42-dot-x] needs to be updated to [RFC7863].
   - [NFSv4-vers] need to be updated to reference a current draft.
Presumably this will be draft-ietf-nfsv4-versioning-08.

*10.1.  Normative References*

The following issues need to be addressed:


   - It is unclear why RFC5662 is referenced normatively since it is a
subset of RFC7863.  Not clear if it needs to be be referenced
informatively in this dodument. o
   - I believe RFCs 7862 and 7863 should be referenced normatively.
   - The reference to NFSv4-ver only makes sense if it is made normative.