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

[Thomas Haynes <[e-mail]>]

Dave,

I couldn’t find a suggestion you cited in your review of Section 8.3 below. Not ducking it, just couldn’t find it.


On May 21, 2016, at 4:01 PM, David Noveck <[e-mail]<mailto:[e-mail]>> wrote:

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

It is a standard variant of the If Thus Else clause. As there is no Else, it was shortened.

“As such”
“It surely follows”
“This means”

Oh wait, I’m looking at Section 15.1.2 - stupid vi /s…

And, bah I wrote that paragraph and I can’t decode the intent.

The title and the first 3 sentences are all valid statements, just not tied together.

On the third review, I still can not make out my original and surely cohesive intent with this section.

I’ve rewritten it into something which at least is logically connected.


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



My observation is that once a draft gets past the working group, uses of “may”, “should”, and “must” are all replaced by editors because of a belief of confusion with “MAY”, “SHOULD”, and “MUST”.

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.


I’ve rewritten it as:

    Although the client implementation has the option  to propagate
    a corresponding error to the application that initiated the I/O
    operation and drop any unwritten data, the client SHOULD attempt to
    retry the original I/O operation by either requesting a new layout or
    sending the I/O via regular NFSv4.1+ READ or WRITE operations to the
    metadata server.  The client SHOULD attempt to retrieve a new layout
    and retry the I/O operation using the storage device first and only if
    the error persists, retry the I/O operation via the metadata server.

I.e., took out the first SHOULD and left the remaining two.

The client can do X when it gets an error or it can do either A or B, both are valid by RFC 5661.

Which argues the first SHOULD in my above. And it looks like I am closer to what you proposed...




8. Mirroring

Suggest making the following changes in the last sentence of the first  paragraph:
•  replace "construct" by "consequence"
•  replace "that" by "in which"


Ack


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


Ack


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

I’d argue that most of my previous SHOULDs are referring to what is in the implementer’s head.

And “invalid” <=> “MUST NOT” according to RFC 2119:

MUST   This word, or the terms "REQUIRED" or "SHALL", mean that the
   definition is an absolute requirement of the specification.


Stating something is invalid is equivalent to stating it is an absolute requirement that the specification not do it.



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

Ack

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.


I agree thar a MUST is wrong here. I’ll even go further and say a SHOULD is also wrong here.

I’ve gone with:

      If the client issues a LAYOUTGET for a writable layout segment which
      is in the process of being resilvered, then the metadata server
      can deny that request with a NFS4ERR_LAYOUTUNAVAILABLE.  The client
      would then have to perform the I/O through the metadata server.

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


I can’t find where you make this proposal.


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.


I think you meant UNDULY here?

(And in case my humor is as subtle as it normally is, I’m making fun of myself here.)

Humor aside, it reads perfectly fine until I got contaminated by your comment.

I’ve add the enum definition and then changed the first sentence to be:

    If the lora_layout_type layout type is LAYOUT4_FLEX_FILES and the
    lr_returntype is LAYOUTRETURN4_FILE, then




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.



Actually, a NFSv4.1 MDS which implements the flex file layout can legally support these strutures within LAYOUTRETURN. The version difference is that it cannot accept the NFSv4.2 stand alone variants of these operations.

Further note that Sections 10 and 11 call out that the stand alone operations can only be used with a base NFSv4.2 MDS.

I’ve rewritten the last paragraph of Section 9.0 to be:

   If the lora_layout_type layout type is LAYOUT4_FLEX_FILES and the
   lr_returntype is LAYOUTRETURN4_FILE, then the lrf_body opaque value
   is defined by ff_layoutreturn4 (See Section 9.3).  It allows the
   client to report I/O error information or layout usage statistics
   back to the metadata server as defined below.  Note that while the
   data strucures are built on concepts introduced in NFSv4.2, the
   effective discriminated union (lora_layout_type combined with
   ff_layoutreturn4) allows for a NFSv4.1 metadata server to utilize the
   data.


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.


see above

In the first post-CODE paragraph suggest making the following changes:
•  In the first sentence, delete the "the" in "the data transfers".


Ack

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


Ack

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



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



Ack

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


Ack


13. Recalling layouts:

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



I’m pretty sure this was from Benny’s original draft and I’m also confident it means your first point below.

So unless it can be pointed out to mean something different, I’m taking the changes below...

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.


I’ve added sufficiently weaselly words to indicate the authors of RFC 5661 were not forward thinkers.

(And for those of you catching on, my subtle humor is at play again.)

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.


Wait, the one time I use “should” and it isn’t clear?

I think the wording of "suitable authorization credentials” makes it appear I’m hiding behind implementation details. It doesn’t. I simply mean either use AUTH_SYS style credentials or Kerberos tickets.

Reworded to point out these are RPC creds.
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


I took both of these and then cut the paragraph into two and removed some of the strung on sentences (origin of the stringing was me).


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.



I rewrote the first paragraph to be get rid of the “i.e.,” and then added a new paragraph describing a model that could be made to work with the loosely coupled model. I have been pretty confident this document would be torn to pieces in the IESG if it did not present at least one approach that was viable.



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



If I leave it at “review”, then I need not fear any more!


•  Changing "call" to "calls"


What is the plural that suggests two and only two?



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


It was “lead” because later it was "sharpened"!