Extending the Opening of Files in NFSv4.2
draft-ietf-nfsv4-delstid-08

# Gunter Van de Velde, RTG AD, comments for draft-ietf-nfsv4-delstid-05

Many thanks for writing this document. 

Please find next some observations and non-blocking comments encountered during my processing of this work.
I am not very familiar with NFS technologies, hence maybe some of my observations and comments could be due to unexperienced.

Please find https://www.ietf.org/blog/handling-iesg-ballot-positions/ documenting the handling of ballots.

#DETAILED COMMENTS
#=================
##classified as [minor] and [major]

10	Abstract
11
12	   The Network File System v4 (NFSv4) allows a client to both open a
13	   file and be granted a delegation of that file.  This delegation
14	   provides the client the right to authoritatively cache metadata on
15	   the file locally.  This document presents several extensions for both
16	   the opening and delegating of the file to the client.  This document
17	   extends both NFSv4.1 (see RFC8881) and NFSv4.2 (see RFC7863).

[major]
The title of the document says "Extending the Opening of Files in NFSv4.2" however 
the abstract says that this works extends both NFSv4.1 (see RFC8881) and NFSv4.2 (see RFC7863)

88	   authority for the file's metadata and data.  This document presents a
89	   number of extensions which enhance the functionality of opens and
90	   delegations.  These allow the client to:

[minor]
cosmetic rewrite textblob proposal

"
This document introduces a set of extensions designed to enhance the functionality of file opens and delegations. These extensions enable the client to:
"

98	   *  during the OPEN procedure, get either the open or delegation
99	      stateids, but not both.
100
101	   *  cache both the access and modify times, reducing the number of
102	      times the client needs to go to the server to get that
103	      information.

[minor]
stateid is speciefied in https://datatracker.ietf.org/doc/html/rfc7530#section-9.1.4

[minor]
cosmetic rewrite textblob proposal

"
* During the OPEN procedure, retrieve either the open stateid or delegation stateid, but not both simultaneously.

* Cache both the access and modify timestamps, thereby reducing the frequency with which the client must query the server for this information.
"

105	   Using the process detailed in [RFC8178], the revisions in this
106	   document become an extension of NFSv4.2 [RFC7862].

[major]
See earlier in the abstract where is indicated that NFS4.1 is extended too

147	   A compound with a GETATTR or READDIR can report the file's attributes
148	   without bringing the file online.  However, either an OPEN or a
149	   LAYOUTGET might cause the file server to retrieve the archived data
150	   contents, bringing the file online.  For non-pNFS systems, the OPEN
151	   operation requires a filehandle to the data content.  For pNFS
152	   systems, the filehandle retrieved from an OPEN need not cause the
153	   data content to be retrieved.  But when the LAYOUTGET operation is
154	   processed, a layout type specific mapping will cause the data content
155	   to be retrieved from offline storage.

[minor]
I am not familiar with terminology as GETATTR, READDIR, LAYOUTGET, pNFS
Should refereces be added if these are not wellknown in the technology area?

157	   If the client is not aware that the file is offline, it might
158	   inadvertently open the file to determine what type of file it is
159	   accessing.  By interrogating the new attribute FATTR4_OFFLINE, a

[minor]
Is FATTR4_OFFLINE a particular abbreviation for something of meaning?
(i have no NFS experience)

170	   /// typedef bool            fattr4_offline;

[minor]
Here lower case is used, while earlierand later the same with upper case is used? Is this intentional?

189	   The fattr4_open_arguments attribute is a new XDR extension which

[minor]
Should this attribute be upper case FATTR4_OPEN_ARGUMENTS ?

465	   only supports NFSv4.2.  An implementation could add a NFSv3 server
466	   which is a NFSv4.2 client gateway the two incompatible systems.  As

[minor]
The phrase seems wrong? What about:

"
An implementation may introduce an NFSv3 server that functions as an NFSv4.2 client, serving as a gateway between the two otherwise incompatible systems.
"

Thanks for an easy review. One tiny nit: in Section 4.1 you refer to “this draft” when talking about the document. That won’t age well when it’s published as an RFC. “This specification” is probably better.

# Orie Steele, ART AD, comments for draft-ietf-nfsv4-delstid-05 
CC @OR13

* line numbers:
  - https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-nfsv4-delstid-05.txt&submitcheck=True

* comment syntax:
  - https://github.com/mnot/ietf-comments/blob/main/format.md

* "Handling Ballot Positions":
  - https://ietf.org/about/groups/iesg/statements/handling-ballot-positions/

## Comments

### Extends or Updates?

```
16	   the opening and delegating of the file to the client.  This document
17	   extends both NFSv4.1 (see RFC8881) and NFSv4.2 (see RFC7863).
```

### What are stateids

```
98	   *  during the OPEN procedure, get either the open or delegation
99	      stateids, but not both.
```

Consider a reference to Section 8.2 of RFC8881.

### Introduce "compound"

```
147	   A compound with a GETATTR or READDIR can report the file's attributes
148	   without bringing the file online.  However, either an OPEN or a
```

Perhaps add a reference to Section 2.3 of RFC8881?

### Introduce "pNFS" / "non-pNFS"

```
150	   contents, bringing the file online.  For non-pNFS systems, the OPEN
151	   operation requires a filehandle to the data content.  For pNFS
152	   systems, the filehandle retrieved from an OPEN need not cause the
153	   data content to be retrieved.  But when the LAYOUTGET operation is
154	   processed, a layout type specific mapping will cause the data content
155	   to be retrieved from offline storage.
```

Expand on first use, reference Section 12 of RFC 8881?

### can -> SHOULD / will -> MUST?

```
339	   The client is already prepared to not get a delegation stateid even
340	   if requested.  In order to not send an open stateid, the server can
341	   indicate that fact with the result flag of
342	   OPEN4_RESULT_NO_OPEN_STATEID.  The open stateid field,
343	   OPEN4resok.stateid (see Section 18.16.2 of [RFC8881]), will also be
344	   set to the special all zero stateid.
```

Should this be made normative?

### maybe race conditions

```
425	   Failure to properly sequence the operations may lead to race cases.
```

Can this be avoided with normative language?

Its possible this is addressed with the normative paragraphs that follow.


### 2008 reference for LEGAL

Is this necessary? Is there a more recent reference?

```
540	   Both the XDR description and the scripts used for extracting the XDR
541	   description are Code Components as described in Section 4 of "Legal
542	   Provisions Relating to IETF Documents" [LEGAL].  These Code
543	   Components are licensed according to the terms of that document.
```

```
594	   [LEGAL]    IETF Trust, "Legal Provisions Relating to IETF Documents",
595	              November 2008, <http://trustee.ietf.org/docs/IETF-Trust-
596	              License-Policy.pdf>.
```

** Section 6.1
-- What is the purpose of Section 6.1 and is it needed? The text appears to be re-explaining the licensing terms that already apply.

** Per the following reference used in the above Section 6.1 text:
   [LEGAL]    IETF Trust, "Legal Provisions Relating to IETF Documents",
              November 2008, <http://trustee.ietf.org/docs/IETF-Trust-
              License-Policy.pdf>.

The above URL redirects to a landing page at https://trustee.ietf.org/documents/trust-legal-provisions/.  Is there a particular TLP in mind?  Based on the 2008 date, is TLP v1.0 being referenced?

Thank you for writing this document...

I am **so** no an NFS person, but I found the document a fascinating read, and it sounds like it solves a real issue, in a reasonable fashion...

I especially liked S 4.1.  Implementation Experience, because I was wondering if this has actually been implemented :-)  

Thanks,
W

# Éric Vyncke, INT AD, comments for draft-ietf-nfsv4-delstid-05

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

Special thanks to Christopher Inacio for the shepherd's detailed write-up including the WG consensus and the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric

# COMMENTS (non-blocking)

## Abstract

The text about `Discussion of this draft...` should not be part of the abstract but be in its own dedicated section.

## Section 1

While the abstract contains `This document extends both NFSv4.1 (see RFC8881) and NFSv4.2 (see RFC7863)`, this section only has `the revisions in this document become an extension of NFSv4.2 [RFC7862]`. I.e., there is a discrepancy at first sight.

Some explanations about `delegation stateids` will be welcome for the reader.

## Section 2

Should there be informative reference (or a warning in section 1.1 to re-use terms from RFC xyz) for GETATTR, ... ?

## Section 3

What is `bitmap4`, while I am not a NFS expert, I would expect having some terminology defined (see comment to section 2).

## Section 3.1

I am not familiar with XDR, but I wonder why the specifications are split in multiple CODE brackets rather than a single one. Also puzzled by the use of `///` as I read them as comments. Even section 6 does not explain why there is a need for a sentinel.

## Section 5.1

Suggest to mention NFSv3 in the section title.

## Section 6.1

It does not really hurt repeating the code licensing but it is redundant with "Copyright notice"

## Section 8

As I am not a NFS expert, I find strange that IANA registries are not used in this document. But, if this is the authors/WG choice, then let it be.