[nfsv4] Review of draft-ietf-nfsv4-flex-files-08 (part three of three)

[David Noveck <[e-mail]>]

*Review Structure*

This email is the third part of a three-part review.

Note that the overall comments are contained in the first part of this
review.  These contain:

   - *Background of Review*
   -
*General Evaluation *
   - *Issues Blocking Working Group Last Call*
   - *Other Noteworthy Issues*

*Per-section Comments (From Section 5.1.2 Onwards)*

*5.1.2.  Client Interactions with FF_FLAGS_NO_IO_THRU_MDS*

There's some issues with the second sentence that should be noted:


   - As there appears to be no logical connection between the first
two sentences, i suggest deleting "Thus, ".
   - The clause "it can recall the layout and either not set the flag
on the new layout or not provide a layout"  seems to be written
implying that by doing either of these things, the metadata server
somehow makes it OK for the client to o IO through the metadata
server.  In the pNFS model it is always OK to do IO through the
metadata server.  One the should-vs.-SHOULD question about this flag
is resolved, this should be rewritten.

I'm unclear as to the implications of the last sentence:

   - In the NFS4_OK case, what the client can do is dependent on
resolution of the should-vs.-SHOULD issue.
   - In the NFS4ERR_LAYOUTUNAVALIABLE case, it is clear
metadara-server irected IO will be done.
   - The NFS4ERR_{LAYOUTTRYLATER, DELAY} case is not clear since
*Section 5.1.1.  Error Codes from LAYOUTGET* only speaks, for these
errors, about cases in which the client has a layout, an, in this
case, it has already been returned.

*7. Recovering from Client I/O Errors:*



The last paragraph is quite problematic with regard to use of the RFC2119
terms, for the following reasons:

·  In a number of places, we are told that he client MAY do X and SHOULD do
Y, where X and Y are mutually exclusive.  While this might make sense with
"may" and "should", it doesn't make sense with "MAY" and "SHOULD".  If you
say you SHOULD do Y, then you can only do something else (e.g. X), under
some very restrictive conditions.  On the other hand, if you say you MAY do
X, you can just choose to do X, with no such restrictions.

·  In some cases the thing that is introduced by "MAY" or "SHOULD" is not
something  that an implementation is actually doing.  For example, in "MAY
be acceptable to propagate", propagating an error is something that the
client can do, but since it relates to the interface between the
application and the client, it is not in-scope for this document.  In this
case,, something being acceptable or not is an attitude issue which is not
an appropriate target of an RFC2119 term.


Because of these issues, I suggest rewriting this paragraph as follows:


Although a client always has the option to propagate a corresponding error
to the application that initiated the I/O operation and drop any unwritten
data, most clients will require that retry be attempted.  In this case, the
client will either retry the original I/O operation by requesting a new
layout using LAYOUTGET and retrying  the I/O operation(s) using the new
layout, or by retrying the I/O operation(s) using regular NFS READ or WRITE
operations via the metadata server.  When doing such retries, the client
SHOULD attempt retrieving a new layout and retry the I/O operation using
the storage device first and only if the error persists or a layout is
unavailable, retry the I/O operation via the metadata server.


*8. Mirroring*



Suggest making the following changes in the last sentence of the first
 paragraph:

·  replace "construct" by "consequence"

·  replace "that" by "in which"


Suggest making the following changes to the last paragraph:

·  In the second sentence, replace "get to" by "are made on"

·  In the fourth sentence, delete "e.g. using an earlier version of the
protocol,"


*8.2. Writing to **Mirrors*



I think the third sentence of the second paragraph is not using the RFC2119
terms correctly.  These terms are about what implementations may or
may not *do.
 *When you say "MUST NOT make assumptions", you are basically referring to
what is in the implementer's head.  Since the following sentence clearly
states the real REQUIREMENT as to what the client must do, I would rewrite
the sentence in question to being "It is invalid for the client to assume".



In the last sentence sentence, suggest  replacing "committed" by "committed
to all mirrors"



*8.3. Metadata Server Resilvering of the File*



There are some issues that relate to the last two sentences:

·  It doesn't seem right to me for the client to respond to
NFS4ERR_LAYOUTTRYLATER by giving up and trying something else.  While it
allowed to do that, it should not be encouraged or presented as the
only/preferred way to do something.  This also appear to conflict with the
last sentence of *Section 7.  **Recovering from Client Errors.*

·  NFS4ERR_LAYOUTUNAVAIL should be mentioned as a possible valid return
here.  In fact, this is the one that makes it valid desirable to do the
retry to the metadata server and I have proposed that the last
sentence of *Section
7. **Recovering from Client Errors *be rewritten to allow that.


*9. Flexible Files Layout Type Return*



This is unduly confusing.  The document need to be more explicit that the
layoutreturn_type4 has to be LAYOUTRETURN4_FILE and that there is no
LAYOUTRETURN4_FLEXFILE.



*9.1.1. ff_ioerr4*



This is one place where an NFSv4.2 structure is imported.  As a result it
isn't very clear what approach you take to using a NFSv4.1 MDS.  It may be
clear to the authors but the reader is left to wonder. There should
probably be something in *Introduction* to address this.



*9.2.3. ff_iostat4*



This is another place where an NFSv4.2 structure. is imported.  The issues
are the same as noted in* 9.1.1.  ff_ioerr4* above.



In the first post-CODE paragraph suggest making the following changes:

·  In the first sentence, delete the "the" in "the data transfers".

·  In the second sentence, replace "no visibility to the I/O stream" by "no
direct knowledge of the I/O operations being done"

·  Also in the second sentence, replace "cannot use any" by "and thus
cannot create on its own"

·  In the third sentence, change "infeasible" to "not feasible".

In the first sentence of the second post-CODE paragraph, suggest replacing
"identify" by "identifies".



*13. Recalling layouts:*



It is not clear exactly what is meant by "when there are sharing conflicts."



I suggest you be a little more clear about what kind(s) of conflicts you
mean. Some possibilities:

·  When existing layouts are inconsistent with the need to enforce locking
constraints.

·  When existing layouts are inconsistent with the requirements regarding
resilvering as described in Section 8.3.


*13.1. CB_RECALL_ANY:*



With regard to AI13, I don't see any reasonable prospect of these constants
being added to RFC5661.  Also, the first paragraph of this section says
that these are in [RFC5661].



One could add this stuff in a minor version but v4.2 iis no longer open to
change and v4.3 looks a long way off.



One possibility is to do this stuff as an extension as described in
draft-ietf-nfsv4-versioning-01.  If the stuff in this section is not
critical to going forward, perhaps it could dropped and this stuff would be
defined in a separate extension document.  That document would have to
reference draft-ietf-nfsv4-versioning (or a possible successor) normatively
so there might be a considerable delay.



*15.  Security Considerations*



This is a big improvement from the -04.



In the second paragraph, it isn't clear how the authorization used at
LAYOUTGET time, relates to the one at done at OPEN/ACCESS time.  I realize
that in a security considerations section one might want to indicate to the
IESG that you have belt  and suspenders working.  However, implementers may
need to understand what is actually necessary to be safe.



There are a couple of issues that arise from recalling of layouts being a
SHOULD.  Some possible changes:

·  In the sixth sentence suggest rewriting all the material after the word
"and it" as follows:

then MUST fence off any clients still holding outstanding layouts forr the
respective files by implicitly invalidating the previously distributed
credential on all data file comprising the file in question.  It is
REQUIRED that this be done before committing to the new ACL and/or
permissions.

·  suggest rewriting the penultimate sentence as follows:

Doing this will require the clients reauthorize access by requesting new
layouts, to be obtained according to the modified access control metadata,
before doing any further IO operations



*15.1.1 Loosely coupled*



I'm going to propose a number of stylistic changes with the goal of
eliminating sentences beginning with "I.e".  In some cases, I'm taking the
liberty of  guessing about your motivations in cases in which these are
simply left unexpressed.  My main point is that the motivations need to be
clearer and am not suggesting that my interpretation is the correct one.

·  In the first paragraph, suggest replacing "I.e." by "Given that,"

·  In the second paragraph, suggest replacing "I.e." by "In this case,there
would be a need for the metadata server, storage device, and client to each
implement RPCSEC_GSSV3.  As that process would unreasonably extend the
deployment process, it must, for the moment, be considered out of scope.


I also suggest it would be clearer to rewrite the final paragraph as
follows:



To summarize the current situation, methods to automatically authenticate
principals or use RPCSEC_GSSv3-aware clients, metadata server, and storage
devices could be designed and deployed.  However, the specifics of such
approaches are not addressed in this document.  Currently, if more secure
authentication is desired using the flex-files layout type, tight coupling
should be used,  as described in the next section.



*15.1.2 Tightly coupled*


In the second sentence, suggest replacing "Thus" by "As a result,".



*17.1. Normative References:*



Are you sure you want to make the reference to pNFSLayouts normative?
Although the treatment of these issues is not ideal in RFC5661 (and I think
the pNFSLayouts work should go forward as soon as possible), a number of
layout types have gone forward so far under what is stated in RFC5661.
Given that we now have a better understanding of the issues from drafting
the layouts draft, it could be argued that publication of the corresponding
RFC as a Proposed Standard is not a requirement for flex-files to go
forward, and that the reference could be Informative.



*Acknowledgements:*



In the fourth paragraph suggest:

·  Changing "a comprehensive review" to "comprehensive reviews".

·  Changing "call" to "calls"


In the penultimate paragraph, propose changing "lead" to "led".
Alternatively, you might want to "replace led the charge" by "made a
convincing case".