Re: [nfsv4] Review of deaft-ietf-nfsv4-delsrid-01

[Thomas Haynes <[e-mail]>]

> On Sep 10, 2022, at 5:24 AM, Noveck, David <[e-mail]> wrote:


Hi Dave,

Sorry about the lag in responding, got caught up in my day job.

Also, as pithy as my responses may be below, I had to spend a considerable amount of time internally debating my responses and subsequent changes to the document(s).

While I can argue that the LAYOUT_WCC belongs to the same document as the OPEN changes, in the end I did decide to make a new document.

Thanks for the review.
Tom


> 
> General Comments
>  
> Overall evaluation
> 
> This document contains useful extensions to NFSv4.2 which the working group needs to review and get published.  As it was three years ago, the document is ready to be adopted as a working group document.
>  
> Unfortunately little progress has been made during these three years so work needs to be done::
> Since there was little review of the document during six months that it was an unexpired working group document, that work needs to be done now,
> A number of things have changed in the interim, requiring further document updates.  For example, all references to RFC5661 now need to be changed to RFC8881.  These changes are not noted in the per-section comments  but they to need to be done.

Done

> Relation to flexible files layout
> 
> This document contains a mixture of extensions, some of which are usable by all of NFSv4 where others are limited to pNFS or to pNFS using a restricted set of layout types.   More clarity is needed to distinguish these classes of extensions.
>  
> Confusion about extension model
> 
> This document is less clear than it needs to be about the specific OPTIONAL protocol extensions proposed. At this point it seems as if all of the OPEN parameters have been revised en masse.    More detailed comments will appear later but it has to be made clear that the primary flag additions to the open arguments flags simply define unused bits and that the supported-flags attribute is a separate, ancillary feature that allows the client to determine which optional features the sever supports.
>  
> Compatibility Issues for the Flexible Files Layout Type
> 
> As discussed in more detail in 5.5.  Flex Files Layout Type, there are change mandated to flexible-file layout data structures that raise serious compatibility issues.  As a result, it may not be possible to fully define WCC_LAYOUT in this document.
> 

I don’t understand this, the LAYOUT_WCC is OPTIONAL and not “mandated”.

I added a Section to call this out, but this new operation can easily be ignored in all existing NFSv4.2 and flex file implementations.


> Comments by Section 
>  
> Abstract
> 
> The first sentence present a random fact about NFSv4 and should be deleted.  I'm not exactly sure what the second sentence says or if it is precisely true. 


It says that delegations allow a client to cache metadata on the file. See Section 10 of RFC8881, "Client-Side Caching”.



> For now, let's delete it.
>  
> I will propose the following replacement abstract:
>  
> This document proposes several extensions to NFSv4.2 relating to opening a file and providing delegations to files.  Included is an attribute defining what optional features of open are supported by a server.
>  
> 1. Introduction
> 
> Suggest combining the first two sentences with the following replacement.
>  
> In NFSv4, i.e. the Network File System version 4, granting a client a delegation allows it to act as the authority for the file's data and metadata.
>  
> 1.1 Definitions
> 
> What appears as the definition of weak cache consistency is not a definition but a series of observations.  
>  
> I note that:
> All of the discussion is about NFSv3 while this is an NFSv4 extension.
> There seems to be little point in citing this since does not provide consistency and you explicitly say the client is free to ignore the data.


Fixed it to be more of a definition.

Despite it being a NFSv3 term, it is pertinent to the discussion.


>  
> 1.2 Requirements Language
> 
> There are a number of matters that need to be addressed now rather than waiting for AUTH48.
>  
> Rather than citing only rfc2119, should reference BCP14, rfc2119, and rfc8174.
>  
> The keywords need to be wrapped in <bcp14></bcp14>.   Similarly, this needs to be done wherever in the document that these keywords appear.


I had already fixed this by stealing the appropriate wording from one of Chuck’s documents.

Needed to fix up all occurances of the terms in the text though.


>  
> 2. Offline Files
> 
> This is an excellent idea for an extension but I have issues with the presentation.
>  
> In the first sentence, the term “locally” is confusing.  In addition, the data can be brought online when opened and not necessarily thrown away.  Suggest rewriting  the first paragraph as follows:
>  
> When files are offline, the server has immediate high-performance access to the files’ attribute but not to the files’ content.  The server has to be able to present to the client enough information to describe each such file, even though the corresponding content is not readily available.  The cost of retrieving the data is substantial, so that the content should only be retrieved when it is to be used.  For example, graphical file managers (e.g.  OSX’s Finder) may only want to  access file content under certain circumstances, such as when a user is hovering his pointer over the file name,
>  

Took some of this suggestion, parts of it are stylistic.


> In the last sentence of the last paragraph, suggest replacing “mean that” by “involve situations in which”.
>  

Done	

> 3. Determining the Arguments to OPEN
> 
> I found this section confusing and feel it needs considerable organizational work, for the following reasons:
> ·        The title of the section, "Determining the Arguments to OPEN", suggests that there has been a change in the way the arguments to OPEN are determined, when in fact there can be no such change.
>  
> A better title might be XDR extensions to OPEN, currently the title of Section 3.1.
>  
> ·        The section combines, in a not very straightforward way, information about specific optional extensions to OPEN with a separate mechanism (a new attribute) to determine what optional extensions to OPEN a server supports.
>  
> I would like to propose the following changes.
> ·        devote section 3 to the open extensions while putting the detailed description of the attribute to determine support in its own top-level section.
> ·        limit the attribute description to optional open features.
>  

I’ve pushed this to be after the proxying section, so now section 5.


> With regard to the existing text of this section, propose replacing “Either type of stateid is suffucient to keep the file open” by “Either type of stateid is sufficient to enable the server to treat the file as if it were open.”
>  



Done


> In the last sentence, the use of “should” is problematic.  Do you want “SHOULD”?  Since I can’t think of valid reasons to not do this, perhaps “MUST” is right.  If that makes you sound like a control freak, maybe you should rewrite it to not use these terms since this is just the way the protocol work and say something like the following:
>  
> In this case, the open stateid field, OPEN4resok.stateid (see Section 18.16.2 of [RFC5661]), will also be set to the special all zero stateid


Done


>  
> 3.1 XDR Modifications to OPEN
> 
> As discussed above, I am proposing moving this material into its own top-level section which I will refer to in this document as 3’.  I suggest Determining Open Feature Support.
>  
> The first paragraph needs to be revised, in large part because of its use of the confusing/nowhere-defined term “microversion”.  Suggest the following replacement:
>  
> [RFC8178] (see Section 4.4.2) allows for extending a particular minor version of the NFSv4 protocol without requiring the definition of a new minor version.  The client can probe the capabilities of the server and based on the  result, determine if both it and the server support optional features not previously specified as part of the minor version.
>  

Agreed


> With regard to the first sentence of the second paragraph, suggest the following replacement, which I believe is more accurate:
> 
> The XDR extensions presented in this section provide helpful support when  the OPEN   procedure is  extended in such a fashion.
>  


Done


> With regard to the second sentence of the second paragraph, I feel this is flat-out wrong:
> ·        It does describe all the parameters to OPEN (e.g. file name, etc.) and there is no need to include them since they are REQUIRED parameter.
> ·        Similarly there is no need to include other REQUIRED parameters since the function of this is to determine which OPTIONAL parameters are supported.
>  

I don’t understand these points.


> With regard to the bulleted items:
>  
> With regard to   the third  paragraph, suggest the following replacement, which I believe is more accurate:
>  
> Subsequent extensions can use this framework when introducing new OPTIONAL functionality to OPEN, by creating a new flags for each OPTIONAL parameter.
>  


Done

> With regard to the XDR code, I’d like to suggest:
> ·        That you delete material that only applies to REQUIRED parameters to avoid, unnecessary implementation complexity.

I would find that hard - it was pretty easy to plop this down as is into my .x file.


> ·        That you change “OPEN4” to “OPEN” in the introductory contents.

Done

> ·        That you move the definitions of OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION and OPEN4_RESULT_NO_OPEN_STATEID to Section 3.
>  
> 4.  Proxying of Times
> 
> With regard to the penultimate sentence of the first paragraph, there is confusion generated by the use of the word “guarantee” since there is nothing done in this document to assure the continued validity of these time after the delegation is returned.  Suggest the following replacement:
>  
> While it is the authority for these values, it has no way to transfer these values to the server when the delegation is  being  returned and the server reclaims its authority with regard to these values.
>  

Done

> With regard to the last sentence of the second paragraph, it needs to be made clearer that this applies only as long as the delegation is held, especially since the previous sentence refers to delegation return.


I fine with it the way it is.


>  
> In the first sentence of the third paragraph, the phrase “it MUST be able to query” is problematic since it is addressing the server and the required action is to be performed by the client.   I think you need to rephrase. With regard to the last sentence, it needs to be clearer that the SETATTRs need to preceed the DELEGRETURN since ding them after is race-prone.
>  




> 4.1.  Use case
> 
> I found this section hard to understand since there is no definition of “proxy”.    Not sure whether it needs to be here or in the definitions section but it needs to be somewhere.
>  

Took a stab at a definiton.

> In the last sentence of the last paragraph, the combination of support for CB_GETATTR nd  SETATTR need to i be fixed.  It makes sense for a client to support CB_GETATTR, but a client does not support SETATTR. Rather, it uses support provide by the server. 


This makes sense tp me. It isn’t support of SETATTR, it is support of the new attributes in SETATTR.


> Need to clarify.
>  
> The last sentence does not seem to be correct.  If a clienr/proxy holds a delegation, then it is, as you say elsewhere, the authority for those values and there is nothing to stop him returning those values in response to a GETATTR, rather than asking the server.   It might be more natural to do so with your extensions, but that is not what the documents says.
>  


I’ll need to come back to this/


> 4.2.  XDR for Proxying of Times
> 
> I propose the following changes:
> ·        Since it isn’t clear what client in quote marks means, please be more explicit. 



I’m fine with leaving this as is.



>  Since section 4 talks about using these attributes even apart from pNFS layouts, this use should be included as well.


I don’t understand this one, but I am fine without talking about pNFS here.


> ·        Use of the term “RECOMMENDED” is not in accord with RFC2119, it should not be used here.


It actually fits in the context of the XDR code that can be extracted from the minor versions.


> ·        The definition of OPEN4_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS should be moved to section 3.


No, it belongs here.


> 
> 5.3.  DESCRIPTION
> 
> I think you need some introductory material for this new open, especially if it is intended to be limited to use with a pNFS when a single mapping type is in use, as I believe is the case here.   IIUC, all the other extensions are for general NFSv4.2 use.   Since layouts are mentioned, it is clear that pNFS use is assumed but clarification is needed on two other points:
> 
> ·        Much of the text is written assuming this being used by the flexible file layout type.    If that is the case, it is best to say so explicitly, up front.  If there is any possibility of it being used by other layout types, this needs further discussion, especially since new layout-type-specific data structures would need to be defined.
> 
> ·        All of the text assumes the data access protocol is NFSv3, while, IIUC other versions of NFS are supported.  It needs to be clearer whether this operation is limited to the case in which NFSv3 is the data access protocol. 
> 
> I really don’t see how the third paragraph adds to the discussion.  Further it talls about a cache consistency protocol which I don’t believe either NFSv3 or NFSv4 supports.
> 

No, it does not talk about a new protocol. I can kinda see why you raise an issue here.

I hope I’ve addressed the issue with the new draft.


> 5.5.  Flex Files Layout Type
> 
> Given the nature of the material in this section, this document should be marked as updating rfc8435, although, as discussed below, that wouldn’t work either.
> The use of “SHOULD” in the first paragraph following the XDR is not appropriate for a number of reasons:
> ·        There can be no “valid reason” to ignore the supposed recommendation.
> ·        The nature of this recommendation/requirement is such that use of RFC2119 terms are not appropriate, since the goal is not to tell implementations what to do.
>  
> It appeared at first that what was meant was something like “The data structure specified above have been designed so that there is a on-to-one correspondence between:”
> However, if you look at rfc8435, and compare ff_data_server4 with  ff_data_server_wcc4, you find that the can be no one-to-one-correspondence despite the fact thar the wcc structure is a semantic extension of the structure defined in rfc8435, these two data structure are otw incompatible so that an implementation of the flex-files mapping type defined in rfc8435 cannot interoperate with the one assumed in this document.  Sigh!
>  

The claim is not that the structures directly correspond to each other, but that there has to be a correspondence.

I suspect that I am not communicating the requirements correctly in the document. I clearly have a working prototype of a server and a client which implement these new structures and which would cleanly work with opposing machines which only spoke RFC8435.


> I think you need two different flexible files layout types and a new document, obsoleting rfc8435, defining both of them.   WCC_LAYOUT needs to bemoved to that new document.
>  
>  

I don’t think we need a new flexible file layout type.

This is a new operation for NFSv4.2. It can be used by a Flex File Layout type implementation, but there are existing mechanisms in place which:
1) allow a server without this support to cleanly tell the client it is not supported.
2) allow a client without this support to never send it to a server which supports it.




> 7.  Security Considerations
> 
> A couple of issues:
> ·        Given the quality of the Security Considerations in RFC7862 and the fact work is being done to address these, suggest referring, for now, to “documents addressing NFSv4.2 security in general”
> ·        Feel that there may be additional security considerations that arise from giving those with delegations the authority to change modify times.  Fpr example, ransomware might encrypt file data and hide the fact that a modification has been made by restoring the old modification time.


Note that it is perfectly legal already to change time_modify via SETATTR.


>  
> 8. IANA Considerations
> 
> A better way to express this would be “This document does not require any actions by IANA”