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

["Frank Filz" <[e-mail]>]

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:[e-mail]] On Behalf Of David Noveck
Sent: Monday, August 3, 2015 1:04 PM
To: Manoj Naik; Marc Eshel; [e-mail]
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:

 

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

 

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

 


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


 


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.