[nfsv4] review of draft-naik-xattrs-02

[David Noveck <[e-mail]>]

*Overall comments*

I think this document takes a fundamentally correct approach to the issue.
Also, it is fairly far along.

I believe it should become a working group document relatively soon.   This
is so even though it needs progress on other documents before it could be
considered as a standards track document by the IESG.

I have a whole bunch of comments/corrections to be considered listed in the
sections below.  Almost all of these are quite minor and the majority are
editorial issues.  I've tried to make sure we have the option of making
xattrs an extension to v4.2 if there is significant progress on
draft-ietf-nfsv4-versioning, while maintaining a fallback to put this into
a small v4.3 if that should be necessary.

The only cases in which I discuss potential substantive changes to the
protocol are:

   - I think you need to have SETXATTR and REMOVEXATTR return the current
   change time as of the update done by the xattr operation.
   - Proposed changes regarding handling of setxattr_type4.
   - Some sort of revision is needed regarding  ACE4_{GET,SET}_XATTRS

There are a couple of cases where I indicate issues that might not need to
be dealt with right away but might turn into issues when this goes to the
IESG.

   - Internationalization considerations for xattr names :-(
   - The issue of a potential IANA registry for extended attribute names,

*Comments by Section*

*Abstract:*

Suggest replacing the first sentence by:

This document proposes extensions to the NFSv4 protocol which allow
file extended attributes (hereinafter also referred to as xattrs) to
be manipulated using NFSv4.

My stab at addressing the singular/plural conundrum in the second sentence
replaces the sentence by the following two sentences:


Support for xattrs is a file system feature that allows opaque metadata,
not interpreted by the file system, to be associated with files and
directories. Such support is present in many modern local file systems.


Suggest adding "are provided" to the end of the last sentence.

*Introduction:*

In the last sentence of the first paragraph suggest replacing "read from,
and write to" by "interrogate, and modify".

In the second sentence of the second paragraph, suggest replacing "on the
underlying file system"
by  "within the underlying file system".

Suggest replacing the third paragraph by the following:

Extended attributes have previously been considered unsuitable for portable
use because some aspects of their handling are not precisely defined and
they are not formally documented by any standard (such as POSIX).
Nevertheless, it appears that xattrs are widely deployed and their support
in modern disk-based file systems is nearly universal.


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

There is no clear specification of how xattrs could be mapped to any
existing file attributes defined in RFC 5661 [2].  As a result, most
NFSv4 client implementations ignore application-specified xattrs.

Suggest replacing in the second sentence of the fourth paragraph, "This" by
"This state of affairs".

Suggest the following in place of the existing fifth paragraph:

There is thus a need to provide a means by which such data loss can be
avoided.  This will involve exposing xattrs within the NFSv4 protocol,
despite the lack of completely compatible file system implementations.

Suggest the following in place of the existing sixth paragraph.

This document discusses (in Section 5) the reasons that NFSv4 named
attributes as currently standardized in [2
<https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2>], are
unsuitable for representing xattrs.  Instead, it proposes a separate
protocol mechanism to support xattrs.  As a consequence, xattrs and
named attributes will both be optional features with servers free to
support either, both, or neither.


*Use cases:*

In the last sentence of the second paragraph suggest the following changes:

   - Replace "jettison" by "dispense with".
   - End the sentence where the colon now appears and put the material
   after it into a new sentence.
   - Replace "these" by "this form of access is"
   - Replace "faster to access" by "provides faster access"
   - Replace "readily accessible by any application" by "is readily
   accessible by applications".


*File system Support:*

In the first sentence of the second paragraph, suggest adding a comma after
"file systems".


*Namespaces:*

Suggest replacing the second paragraph by the following:

This document only provides for namespaces supported user-managed
metadata only, thus avoiding the need to specify the semantics
applicable particular system-interpreted xattrs.


The values of xattrs are considered application data just as the
contents of named attributes, files, and symbolic links are.  Servers
have a responsibility to store whatever value the client specifies and
to return it on demand.


xattr keys and values MUST not be interpreted by the NFS clients and
servers, as such behavior would lead to non-interoperable
implementations.  If there is a need to specify attributes that
servers need to be act upon, the appropriate semantics need to be
specified by adding a new attribute for the purpose as provided by
[RFC5661] and [draft-ietf-nfsv4-versioning].


I know that I promised to somehow "toughen up" the language. I was unable
to do that. Given that the original already used "MUST", there was no real
place to go while still staying within the scope of RFC2119. In any case, I
hope the above moves things in the right direction

  *Differences with named Attributes:*

Suggest, in the title, replacing "with" by "from".

In the first paragraph, the appropriate reference for the definition of
named attributes should be to  RFC7530.

In the first sentence of the second paragraph, suggest replacing "define
individual xattrs 'get' and 'set' operations" by "operations to get and set
indivdual xatrrs".

*Protocol Enhancements:*

Suggest, in the title, replacing "enhancements" by "extensions".

In the second sentence, suggest deleting "to bitmap4 data type.

In the fourth sentence, suggest replacing "new operations, namely" by "the
new operations".

Also in the fourth sentence suggest replacing "xattr key/value" by "xattr
keys and values".

Suggest deleting the third sentence and adding a new second paragraph,
along the lines of the following:

These changes follow applicable guidelines for valid NFSv4 protocol
extension, whether the extensions occur in a minor version (as
specified in [2]) or as an extension to an existing minor version (as
specified in [draft-ietf-nfsv4-versioning]).


I'm thinking that the reference to the versioning doc would be
informational for now but could eventually be made normative, most likely
after that document is submitted to the IESG but before approval.

I note that you specify an attribute number and values
for certain flags/value but do not specify opcodes for your new
operations.  What you might want to do to address that issue is to remove
all numerical assignments from where they are now and put them all into a
new section 6.x, entitled something like "New values associated with
protocol extensions".  Having those in one place would facilitate the
process of approving this document as a working group document assumng we
follow the procedures now described in draft-ietf-nfsv4-versioning-01.

*New Attributes:*

In the first sentence suggest adding "per-fs read-only" before "attribute".

Also in the first sentence suggest deleting "with GETATTR".

*Attribute 82: xattr_support:*

You might want to:

   - Change the title to "xattr_support attribute"
   - In the table replace "82" by "--".

Suggest adding a new paragraph after the current first one, as follows:

Since xattr_support is not a REQUIRED attribute, server need not support
it.  However, a client may reasonably assume that a server (or fs) that
does not support the xattr_support attribute does not provide xattr support
and act on that basis.


*New operations:*

Suggest replacing the first two sentences by the following:

Individual xattrs generally represent separate items of metadata.  For
various reasons, combining them into single attribute results in
clumsy implementations which contain significant functional deficits.
In consequence, adding a new attribute to represent the set of xattrs
for an object is not an appropriate way to provide support for xattrs.


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


Such a read-modify-write cycle is subject to updates being lost in the case
of simultaneous updates by multiple clients.  In addition, two clients
might simultaneously add the same xattr key to the same file with each
concluding that it did the inital creation for the common xattr key, when
the semantic model implies that only one could have done so.


You need a paragraph to deal with interactions between xattr and stateful
operations. Please look at the following attempt and see if it needs any
changes:

Xattr operations operate, for the most part, separate from operations
related to locking state. Xattrs can be interrogated and modified without a
corresponding open file. An open denying write or read does not prevent
access to or modification of xattrs. Only in the case of delegations is
there an interaction with the state logic. Xattrs cannot be modified when a
delegation for the corresponding file is held by another client. On the
other hand, xattrs can be interrogated despite the holding of a write
delegation by another client.

There are a number of issues that arise repeatedly in the subsections of
section 6 but are not specifically mentioned below:

   - In many cases, the word "MUST" is used in contexts it would not be
   used in corresponding description of READ and WRITE, for example.
   - The indenting of the titles of sections named "ARGUMENTS" seems wrong
   and could be fixed.

*New Definitions:*

The first sentence of the second paragraph consists of two independent
clauses joined by a comma.  To address that and also to simplify
capitalization issues, I suggest rewriting it as follows:


An xattr4 consists of an xattrname4 which is a UTF-8 string denoting
the xattr key name and an attrvalue4 which is a variable-length string
that identifies the value of the xattr.


I don't see the point of the third sentence of the second paragraph. The
concept of the combined size of an xattr does not seem to be used anywhere
else. I'd suggest deleting this sentence.

In the fourth sentence of the second paragraph, I'd suggest replacing
"array of xattr4" by "set of extended attributes".

There are a number of issues with the last sentence of the second paragraph
that should be considered:

   - If you do have separate ACE bit for getting and setting xattrs, then
   ACCESS and OPEN will give you no useful information as to access.
   - ACCESS is the operation designed to determine whether access is
   permissible. If OPEN does allow you to infer permission that can be
   mentioned but it should not be on the same level as open.

There are some issues that relate to internationalization that need to be
addressed. While you could (implicitly) inherit the treatment of file names
specified in RFC7530, there are a number of considerations that argue for
something different:

   - The fact that the strings are defined as capitalization-insensitive
   means that the string cannot be anything other than UTF-8, as opposed to
   the case of file names in which UTF-8 is merely the preferred encoding.
   - You may be closer to a clean slate in having fewer internationalized
   strings to deal with.

This leads to a number of potential issues that you may have to deal with
eventually:

   - Given that this data has to be UTF8, we face the issue of mandating
   checking for valid UTF8 strings, either as a "MUST" or a "SHOULD". For file
   names, this is a "MAY".
   - The fact that capitalization is inherently language-specific, even
   though the practical effect of this fact may be limited in the context in
   which you are dealing (i.e. you may have few or no Turkish or Azeri xattr
   names to deal with). In any case, you'll eventually need some specification
   language to deal with this issue.
   - The fact that some OS's don't like to do normalization-related
   processing in the kernel raises some issues. Given that we are in a
   UTF8-only environment may mean that we may no longer be able to simply
   ignore normalization issues as is now allowed for file names. That may be
   something the IESG is unwilling to accept. If we can't depend on the server
   to convert to a preferred normalization form, we may have to mandate that
   the client provides xattr names in the preferred normalization form. If the
   client could require the same of the applications, it can avoid having to
   do normalization itself. You might have to dance around the issue of
   verifying correct normalization. Sigh!

*Caching:*

In the first sentence of the second paragraph,, suggest replacing "an
operation" by "some operations".

The way the third paragraph is written is confusing.  I suggest separating
the statements about synchronously doing modifications from those about
caching per se.  The result would be something like the following:

All modifications to xattrs should be done by requests to the server, since
the protocol has no support for writeback caching of of xattrs.  The server
should perform such updates synchronously.


Xattrs obtained from the server and xattrs sent to the server may be
cached and clients can use them to avoid subsequent GETXATTR requests,
provided that the client ensure that the cached value ha not been
subsequently modified by another client.  Such assurance can depend on
the client holding a delegation for the file in question or the client
can interrogate the change time, to make sure that any cached value is
still valid.  Such caching may be read-only or writethrough.


I think that speaking of "incoherency" give the wrong impression.  I
suggest rewriting the last paragraph as follows:

The result of local caching is that the individual xattrs maintained
on clients may not be up-to-date. Changes made in one order on the
server may be seen in a different order on one client and in a third
order on another client. In order to limit problems that might arise
because by separate operations to obtain individual xattrs and other
file attributes, a client should treat xattrs just like other file
attributes with respect to caching as detailed in section 10.6 of RFC
<https://tools.ietf.org/html/rfc5661> 5661
<https://tools.ietf.org/html/rfc5661> [2
<https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2>]. A
client may validate its cached version of an xattr for a file by
fetching the change attribute and assuming that if the change
attribute has the same value as it did when the attribute was cached,
then the xattr has not changed.  If the client holds a delegation that
ensures that the change attribute cannot be modified, that it can
dispense with actual interrogation of the change attribute.


GETXATTR - Get an extended attribute of a file


As currently written the first and third sentence of the second
paragraph of DESCRIPTION are contradictory in that the first is not
conditional on being able to do the operation while third makes due
allowance for failure conditions.  Suggest replacing, in the first
sentence "MUST obtain" by "wiil fetch".


In the last sentence of that paragraph, suggest replacing "will exceed
the channel's negotiated maximum response size" by "is such as to
cause the channel's negotiated maximum response size to be exceeded".


The IMPLEMENTATION section is confusing in that it jams client and
server considerations together.  It would be better to start with the
client and proceed like the following:


Clients which have a cached xattr value may avoid the need to do a
GETXATTR by determining if the change attribute is the same as it was
when the xattr was fetched.  If the client does not hold a delegation
for the file in question it can do this by do a GETATTR fetching the
change attribute and comparing the value to a change attribute value
fetched when the xattr value was obtained. This handling is similar to
how a client would revalidate other file attributes such as ACLs.


When responding to such a GETATTR, the server will, if there is an
OPEN_DELEGATE_WRITE delegation held by another client for the file in
question, either obtain the actual current value of these attributes
from the client holding the delegation by using the CB_GETATTR
callback, or revoke the delegation. See Section 18.7.4 of RFC 5661
<https://tools.ietf.org/html/rfc5661#section-18.7.4> for details [2
<https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2>].


SETXATTR - Set an extended attribute of a file


As this is written now, handling the conditional create case (I.e.
create the attribute if it does not currently exist) is unduly
difficult. One is forced to retry after an error and there is no
guarantee that the situation has not changed in the meantime.  Some
ways of addressing this are:


   - Adding a new value (e.g. SETXATTR4_CONDCR) to setxattr4_type for
the conditional create case.
   - Getting rid of setaxattr4_type, making the server always doing a
conditional create, and adding a boolean to the response indicating
whether the xattr existed before the setxattr was done.

I'm also proposing that you add a changetime value to the NFS4_OK case
of SETXATTR4res.  doing a GETATTR before the SETXATTR provides a
useless value while doing a gETATTR after the SETXATTR leaves a race
open in which you can wind up consideriing an out-of-date xattr value
as valid.

Which regard to the second paragraph of DESCRIPTION:

   - In the first sentence, suggest replacing "If the xattr key and/or
value contained in the client request exceeds" by "If the xattr and
value contained in the client request are such that the request would
exceed".
   - In the second sentence, suggest you pick another error since
NFS4ERR_ENOSPC would most likely indicate you are out of disk space.

The two sentences of the third paragraph of DESCRIPTION disagree
regarding the case in which you do a SETXATTR that sets the xattr
value to what it currently is.  Suggest the following as a
replacement:


A successful SETXATTR MUST change the file time_modify and change
attributes if the xattr is created or the value assigned to xattr is
actually changes. However, these attributes SHOULD NOT be changed in
case in which SETXATTR Cause no actual change in the xattr value.


*LISTXATTR - List extended attributes of a file:*

The struct LISTXATTR4args is missing the definition of la_cookieverf.

REMOVEXATTR - Remove an extended attribute of a file:


As with SETXATTR, I believe you should return a change time in the response.


With regard to the second paragraph of DESCRIPTION:


   - Suggest replacing "SHOULD" by "MUST".
   - Suggest deleting the second sentence.

*Valid Errors:*

This does not address the issues with NFS4ERR_ENOSPC mentioned elsewhere.

With regard to GETXATTR:

   - Don't see why NFS4ERR_GRACE is included.
   - Believe NFS4ERR_{IS,NOT}DIR should be deleted.

With regard to SETXATTR:


   - Believe NFS4ERR_ADMIN_REVOKED should be deleted.
   - Believe NFS4ERR_ATTRNOTSUPP should be deleted.
   - Believe NFS4ERR_BADOWNER should be deleted.
   - Believe NFS4ERR_BAD_RANGE should be deleted.
   - Believe NFS4ERR_BAD_STATEID  should be deleted.
   - Believe NFS4ERR_DELEG_REVOKED should be deleted.
   - Believe NFS4ERR_EXPIRED should be deleted.
   - Believe NFS4ERR_FBIG should be deleted.
   - Don't see why NFS4ERR_GRACE is included.
   - Believe NFS4ERR_LOCKED should be deleted.
   - Believe NFS4ERR_NOTDIR should be deleted.
   - Believe NFS4ERR_OLD_STATEID should be deleted.
   - Believe NFS4ERR_OPENMODE should be deleted.
   - Believe NFS4ERR_UNKNOWN_LAYOUTTYPE should be deleted.

With regard to LISTXATTR:

   - Believe NFS4ERR_ADMIN_REVOKED should be deleted.
   - NFS4ERR_FHEXPIRED is not a valid error.
   - Don't see why NFS4ERR_GRACE is included.
   - Believe NFS4ERR_LOCKED should be deleted.
   - Believe NFS4ERR_{IS,NOT}DIR should be deleted.

With regard to REMOVEXATTR:

   - Believe NFS4ERR_ADMIN_REVOKED should be deleted.
   - Believe NFS4ERR_ATTRNOTSUPP should be deleted.
   - Believe NFS4ERR_BADOWNER should be deleted.
   - Believe NFS4ERR_BAD_RANGE should be deleted.
   - Believe NFS4ERR_BAD_STATEID  should be deleted.
   - Believe NFS4ERR_DELEG_REVOKED should be deleted.
   - NFS4ERR_FHEXPIRED is not a valid error.
   - Believe NFS4ERR_EXPIRED should be deleted.
   - Don't see why NFS4ERR_GRACE is included.
   - Believe NFS4ERR_NOTDIR should be deleted.
   - Believe NFS4ERR_UNKNOWN_LAYOUTTYPE should be deleted

Extensions to ACE Access Mask Attributes:


You might want to:

   - Delete the definition of the two new values and only indicate
that new values are defined.
   - Indicate that the actual value will appears in the new section 6.x.

I don't understand the statement "No additional granularity of control
is implied by these constants for server implementations".
Specifically,

   - The previous sentence, in indicating how these are used, does
imply a finer-grained access control arrangement.
   - If there were not finer-grained access control, there would be no
point in defining these additional bits

*New Section 6.x:*

The section title should be something like "Numeric values assigned".

The initial paragraph should be something like the following:


This section lists the numeric values assigned to implement the xattr
feature. To avoid inconsistent assignments, they have been checked against
the most recent minor version and all extensions currently approved as
working group documents. Given that, development of interoperable
prototypes should be possible using these value, although the possibility
exists that these assignments may be modified before eventual publication
as a standard-track document.

After that, I would list the necessary assignments in XDR-style format with
comments indicating where each XDR section would by placed in a
consolidated XDR for the protocol including the extension.

*pNFS Considerations:*

Would revise to be more consistent with the fact that xattrs are more
likely to be on the metadata server itself and not data servers aka data
storage devices. Something like:

All xattr operations are sent to the metadata server, which is
responsible for fetching data from and effecting necessary changes to
persistent storage.

*Security Considerations:*

If you are going to reference any previous NFSv4 protocol version, it
should be NFSv4.2.

I think it may make sense to say here that the issues are exactly the same
as those relating to the storing of file data and named attributes.  These
are all various sorts of application data and the fact that the means of
reference is slightly different in each case should not be considered
security-relevant.

*IANA Considerations:*

A better formulation would be:


This document does not require any actions by IANA.

The IESG may raise the issue of the existing IANA maintained registry of
named attributes. If you don't think one is required, you may may to
explain why this situation is different. In any case, you should refer to
the description in RFC7530 as the one in RFC5661 is not correct (may need
an errata for that sometime).


*References:*

I'm pretty sure you need a reference to RFC7530 and I tend to think it
should be normative.

I think you need to have a reference to the current NFSv4.2 draft. If this
becomes a working group document before the v4.2 RFC is out, you will need
to add a note asking the RFC Editor to make sure that the eventual RFC
number is inserted.