Subject: [nfsv4] review of draft-ietf-nfsv4-xattrs-00

From: David Noveck <[email-masked]>
Date: Mon, 3 Aug 2015 16:04:10 -0400

Since the document is unchanged from draft-naik-xattrs-02, the review is
almost identical to my earlier one.  There are a few differences that
reflect  the discussions we have had, the new situation, and something I
just thought of.  Any changed text is in red.

*Overall comments*

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

I think this was a good time to turn this into a working group document.  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 pre- and
   post- change times for  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
   - Possible additions to deal with ACCESS and 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 assuming 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-related 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:*

In the first sentence, suggest replacing "NFS xattr" by "NFSv4 xattr4."

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 this as follows:

An xattr4 consists of an xattrname4 which is a string denoting the
xattr key name and an attrvalue4 which is a variable-length string
that identifies the value of the xattr.  The xattr4name's handling of
internationalization-related issues is the same as that for NFSv4 file
names and named attribute names, as described in [RFC7530].

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 may be some other issues that relate to internationalization that
need to be addressed. You are best off, if, as previously discussed, you
remove any indication that the names are case insensitive. This will enable
you to inherit the internationalization-related language from RFC7530,
which applies to the names of files and named attributes. Some issues to
consider:

   - Although there has been mention of defining xattrname as uft8str_cs,
   it might be better to define this as component4, since this will make it
   clearer that everything internationalization-related in RFC7530, applies to
   xatttr names as well.
   - You are probably going to have to choose whether xattr names are
   (always) case-sensitive or whether they follow the case-sensitivity of the
   filesystem of which they are a part. Right now, named attributes use the
   latter approach but I'm not clear whether this is right of whether this
   matters.

I'm suggesting you define a structure for pre- and post-operation
attributes to be returned by SETXATTR and REMOVEXATTR. Text like the
following makes sense at the end of the existing section:

The xachange4 structure is used to communicate the values of the change
attribute immediately before and immediately after an operation which
modifies an xattr. Having this information available allows a client to
validate and retain cached xattr's while setting/removing the value of
another xattr.

struct xachange4 {

changeid4 xac_pre;

changeid4 xac_post;

};

*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 an xachange4 structure 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 an xachange4 structure
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

If you decide there is to be finer-grained access control, you might
want to consider the implications for ACCESS.  Some possibilities;

   - Adding bits ACCESS4_XAREAD and ACCESS4_XAWRITE
   - Creating a new ACESS_XATTR operation.  This would allow you
distinguish permissions for adding, modifying, and deleting xattrs to
a file.
   - Creating a new ACCESS_PLUS operation that returns a pair of
acemask4's.  If this were done, it would be best defined as a feature
separate from xattrs since someone not supporting xattr's might still
find it useful.

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

---

From: Nico Williams <[email-masked]>
Date: Mon, 3 Aug 2015 16:52:33 -0500

>    - Internationalization considerations for xattr names :-(

That's settled now, yes?  It should be the same as for NFSv4
case-sensitive filenames.

>    - The issue of a potential IANA registry for extended attribute names,

Oof.  Can we impose this on the rest of the world?

We could carve out a bit of the namespace for Internet standards though.
That'd be good.

It might actually be a good thing to have a FCFS for everything outside
an Internet standards prefix, and Expert review for within that prefix.

> [RFC5661] and [draft-ietf-nfsv4-versioning].

I thought there already are xattrs that are interpreted by the
filesystem/OS.  No?

Nico
--

---

From: "Frank Filz" <[email-masked]>
Date: Mon, 3 Aug 2015 14:22:50 -0700

It seems to me that it would be useful to have a new ACCESS_PLUS that took the ACE4 flags instead of the flags inherited from NFS v3. That would allow the client it indicate more closely what it was looking for, and for the server to indicate exactly what it would grant access to. Of course I’m not a client developer so I’m not sure how much more would be useful beyond the traditional ACCESS4 flags.

Frank

From: nfsv4 [mailto:[email-masked]] On Behalf Of David Noveck
Sent: Monday, August 3, 2015 1:04 PM
To: Manoj Naik; Marc Eshel; [email-masked]
Subject: [nfsv4] review of draft-ietf-nfsv4-xattrs-00

Since the document is unchanged from draft-naik-xattrs-02, the review is almost identical to my earlier one.  There are a few differences that reflect  the discussions we have had, the new situation, and something I just thought of.  Any changed text is in red.

Overall comments

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

I think this was a good time to turn this into a working group document.  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 pre- and post- change times for  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

*         Possible additions to deal with ACCESS and 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:

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.

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"

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

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.

This document discusses (in Section 5) the reasons that NFSv4 named attributes as currently standardized in [ <https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2> 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:

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

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:

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:

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 assuming we follow the procedures now described in draft-ietf-nfsv4-versioning-01.

New Attributes:

Attribute 82: xattr_support:

You might want to:

*         Change the title to "xattr_support attribute"

*         In the table replace "82" by "--".

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:

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.

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-related 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 this as follows:

An xattr4 consists of an xattrname4 which is a string denoting the xattr key name and an attrvalue4 which is a variable-length string that identifies the value of the xattr.  The xattr4name's handling of internationalization-related issues is the same as that for NFSv4 file names and named attribute names, as described in [RFC7530].

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 may be some other issues that relate to internationalization that need to be addressed. You are best off, if, as previously discussed, you remove any indication that the names are case insensitive. This will enable you to inherit the internationalization-related language from RFC7530, which applies to the names of files and named attributes. Some issues to consider:

*	Although there has been mention of defining xattrname as uft8str_cs, it might be better to define this as component4, since this will make it clearer that everything internationalization-related in RFC7530, applies to xatttr names as well.
*	You are probably going to have to choose whether xattr names are (always) case-sensitive or whether they follow the case-sensitivity of the filesystem of which they are a part. Right now, named attributes use the latter approach but I'm not clear whether this is right of whether this matters.

I'm suggesting you define a structure for pre- and post-operation attributes to be returned by SETXATTR and REMOVEXATTR. Text like the following makes sense at the end of the existing section:

The xachange4 structure is used to communicate the values of the change attribute immediately before and immediately after an operation which modifies an xattr. Having this information available allows a client to validate and retain cached xattr's while setting/removing the value of another xattr.

struct xachange4 {

changeid4 xac_pre;

changeid4 xac_post;

};

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>   <https://tools.ietf.org/html/rfc5661> 5661 [ <https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2> 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.

 

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 <https://tools.ietf.org/html/rfc5661#section-18.7.4>  18.7.4 of RFC 5661 for details [2 <https://tools.ietf.org/html/draft-naik-nfsv4-xattrs-02#ref-2> ].

 

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 an xachange4 structure 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:

 

As with SETXATTR, I believe you should return an xachange4 structure in the response.

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

Valid Errors:

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.         

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

*         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
If you decide there is to be finer-grained access control, you might want to consider the implications for ACCESS.  Some possibilities;
*         Adding bits ACCESS4_XAREAD and ACCESS4_XAWRITE
*         Creating a new ACESS_XATTR operation.  This would allow you distinguish permissions for adding, modifying, and deleting xattrs to a file.
*         Creating a new ACCESS_PLUS operation that returns a pair of acemask4's.  If this were done, it would be best defined as a feature separate from xattrs since someone not supporting xattr's might still find it useful.

New Section 6.x:

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:

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.

---

From: David Noveck <[email-masked]>
Date: Mon, 3 Aug 2015 21:05:10 -0400

> case-sensitive filenames.

I agree with you and many others do as well.  It'll only be settled when
this document gets through WGLC.

There is a remaining issue regarding capitalization-insensitive fs's.  sigh!

> Oof.  Can we impose this on the rest of the world?

Probably not but the issue we have to deal with is whether the same
logic applies to the registry for named attributes.

> an Internet standards prefix, and Expert review for within that prefix.

It might be good, but I'm not sure if the  whole thing is workable.  Let's
let the
author's make their decision and then we can attack them from all sides :-)

> I thought there already are xattrs that are interpreted by the

No. No.  I've stopped at two no's rather than 1,000,003 :-)

This document deals only with xattrs in the user namespace, i.e those
not interpreted by the filesystem.  whether they are/can be interpreted by
the OS depends on how you use the term "OS", but the way I use it, no.
(Now up to three no's :-)

---

From: "Manoj Naik" <[email-masked]>
Date: Tue, 18 Aug 2015 22:11:46 -0700

Thanks for the review. I have just published a new revision of the draft 
that incorporates most, if not all of your editorial changes. 
Additional responses below.

> 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 assuming we follow the procedures now described in draft-
> ietf-nfsv4-versioning-01.

See Section 7.5 of xattrs-01 draft that documents the new 
attributes/operations
in XDR form.

> 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:

Added Section 7.7 specific to xattrs and file locking (and delegations).

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

I think I got some of these but not sure I got all. 

> There may be some other issues that relate to internationalization 
> that need to be addressed. You are best off, if, as previously 
> discussed, you remove any indication that the names are case 
> insensitive. This will enable you to inherit the 
> internationalization-related language from RFC7530, which applies to
> the names of files and named attributes. Some issues to consider:
> Although there has been mention of defining xattrname as uft8str_cs,
> it might be better to define this as component4, since this will 
> make it clearer that everything internationalization-related in 
> RFC7530, applies to xatttr names as well.
> You are probably going to have to choose whether xattr names are 
> (always) case-sensitive or whether they follow the case-sensitivity 
> of the filesystem of which they are a part. Right now, named 
> attributes use the latter approach but I'm not clear whether this is
> right of whether this matters.

I think I've done this, let me know if more changes are needed.

> The xachange4 structure is used to communicate the values of the 
> change attribute immediately before and immediately after an 
> operation which modifies an xattr. Having this information available
> allows a client to validate and retain cached xattr's while setting/
> removing the value of another xattr.
> 
> struct xachange4 {
> changeid4 xac_pre;
> changeid4 xac_post;
> };

The pre/post values are only useful if the server can guarantee the 
atomicity of the SETXATTR/REMOVEXATTR operation. For this, the existing
change_info4 seems appropriate, so I've used that instead.

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

The use of create/replace flags for SETXATTR follows the BSD setxattr() 
call,
that allows conditional create based on whether the xattr being set
exists (XATTR_REPLACE) or not (XATTR_CREATE). I think it's fairly rare 
for a setxattr() to repeatedly fail due to contention. Even if it does, 
the reported error is consistent with how the setxattr() system call 
behaves.
 
> I'm also proposing that you add an xachange4 structure 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.

Done with change_info4. See above. I've added detailed description of
how clients can validate the cached xattrs after a SETXATTR call using
change_info4 in Section 7.6.

> ·     In the second sentence, suggest you pick another error since 

Using NFS4ERR_NAMETOOLONG based on the discussion on the list.

> ·         Adding bits ACCESS4_XAREAD and ACCESS4_XAWRITE
> ·         Creating a new ACESS_XATTR operation.  This would allow 
> you distinguish permissions for adding, modifying, and deleting 
> xattrs to a file.
> ·         Creating a new ACCESS_PLUS operation that returns a pair 
> of acemask4's.  If this were done, it would be best defined as a 
> feature separate from xattrs since someone not supporting xattr's 
> might still find it useful.

I am not convinced about the usefulness of an ACCESS call that checks
a client's access rights to perform xattr operations, given that there
is no guarantee that the operation will succeed even if the ACCESS does.
Instead, the ACE4 bits do allow a server to provide fine-grained
permission checks against xattr operations. However, I can see how
additional ACCESS bits may be useful, so I've added these. See section 
7.4.

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

Do we need a new IANA registry for xattr names? Currently, the draft does
not say it does. This just opens up a can of worms :)

Manoj.

---

From: Benny Halevy <[email-masked]>
Date: Wed, 19 Aug 2015 08:52:35 +0300

> not say it does. This just opens up a can of worms :)

Since the use case is for storing user-defined metadata I see no
requirement for cross-user (organization) interoperability (unlike, maybe
system level xattrs), thus no justification for IANA registration.
Similarly, it's like requiring users to register their filenames in a
directory.

Benny

---

From: Matt Benjamin <[email-masked]>
Date: Wed, 19 Aug 2015 12:12:44 -0400 (EDT)

Agree.

Matt

-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-761-4689
fax.  734-769-8938
cel.  734-216-5309

----- Original Message -----
> From: "Benny Halevy" <[email-masked]>
> To: "Manoj Naik" <[email-masked]>
> Cc: [email-masked]
> Sent: Wednesday, August 19, 2015 1:52:35 AM
> Subject: Re: [nfsv4] review of draft-ietf-nfsv4-xattrs-00
> 
> 
> 
> 
> > > IANA Considerations:
> > > 
> Since the use case is for storing user-defined metadata I see no requirement
> for cross-user (organization) interoperability (unlike, maybe system level
> xattrs), thus no justification for IANA registration. Similarly, it's like
> requiring users to register their filenames in a directory.
> 
> Benny
> 
> > 
> > Manoj.
> > 
> > 
> 
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>