Subject: [nfsv4] review of draft-ietf-nfsv4-flex-files-06 (part two of two)

From: David Noveck <[email-masked]>
Date: Thu, 3 Sep 2015 13:21:24 -0400

*General comments*

I agree with most of Benny's comments.  II'll try to avoid duplication and
only mention them when there is something about them that I disagree with.

*Issue Summary *

Most of the stuff in this part of the review consists of minor editorial
issues.

One exception is that there is there seem to be a significant
administrative issue blocking acceptance of this draft in its current
form.  For details, see *CB_RECALL_ANY*.

Also there is a major technical issue in which *ff_layout* and *State and
Locking Models* contradict each other.  The discussion of this is
consolidated with the review of *State and Locking Models* in the first
review part.

There is another relatively minor technical issue discussed within
*ff_layout4.*  It concerns whether ffds_stateid should be made an array.

For an overall summary of this review, see the *General Comments* section
in the first review email.

*Comments by Section (From Section 3):*

*4.1. ff_device_addr4:*

In the third non-CODE paragraph suggest, the following changes, primarily
to reflect the fact that pNFS client use of layouts is never mandatory:

   - In the first sentence, suggest replacing, "NFS versions and/or minor
   versions" by "choices as to NFS version, minor version, and coupling
   strength.
   - In the penultimate sentence, suggest adding "and ffdv_tightly_coupled
   to false" after "ffdv_minorversion to 0".
   - In the penultimate sentence, suggest replacing "MUST access the
   storage device" by "MAY ONLY access the storage device directly"
   - In the last sentence, suggest replacing "MUST access the storage
   device using NFSv4" by "MAY ONLY access the storage device directly using
   the corresponding minor version of NFSv4"

In the first sentence of the fourth paragraph, suggest the following
changes:

   - replacing "can not" by "cannot"
   - replacing "ffdv_version or ffdv_minorversion" by "combinations of
ffdv_version,
   ffdv_minorversion, and ffdv_tightly_coupled"

In the second sentence of the fourth paragraph suggest adding a comma after
"however"

In the second sentence of the last paragraph suggest replacing "no control
protocol is present" by "loose coupling is in effect".

*4.2. Storage Device Multipathing*

In the second sentence of the first paragraph, suggest replacing "case" by
"event".

A number of the suggested changes reflect the fact that there are not
degrees of optimality.  Something is optimal or it isn't.  For example:

   - In the second sentence of the third paragraph suggest replacing
   "optimal" by "desirable".
   - In the third sentence of the third paragraph suggest replacing
   "optimal" by "desirable".

In the second sentence of the last paragraph, suggest the following changes:

   - Adding "a" before "higher"
   - adding a comma after "version"

*5. Flexible File Layout Type:*

The RFC editor note in the enum layouttype4 is out-of-date and should be
removed.

In the penultimate sentence of the last paragraph, suggest "obviously must
be" by "is".

In the last sentence of the last paragraph, suggest replacing "opaque" by
"otherwise opaque".

*5.1. ff_layout4:*

With regard to the seventh paragraph, I suggest the following changes:

   - In the third sentence suggest replacing "each ffdv_version and
   ffdv_minorversion" by "a particular combination of ffdv_version,
   ffdv_minorversion, and ffdv_tightly_coupled".
   - In the fourth sentence, suggest replacing "version and minor version
   combination" by "combination of version, minor version, and coupling
   strength"

It is at the least highly anomalous that the protocol makes arrangement for
each protocol to have its own fh but has only a single ffds_stateid.  I
suggest this be made an array in parallel with ffds_fh_vers.

The material regarding loose coupling in the eighth paragraph is a flat-out
contradiction of what is said in the section *S**t**ate and Locking Models.*
It's time to pick one store and stick to it, at least until it becomes
untenable. I've decided that anything that I have to say about the choice
between these two models (not very much) belongs in the section *S**t**ateid
Choices* (in part one of the review).

On the other hand, if we follow the approach regarding loosely-coupled
stateids that appears in this section, as opposed to the one in *S**t**ate
and Locking Models*, we have the following situation:

   - There is no use for ffds_stateid forNFSv3.
   - For loosely-coupled NFSv4, ffds_statid would always have only an
   anonymous stateid.
   - In the case f tight coupling there is no need for ffds_srateid, since
   the global stateid model would require you to use the one you got from
   OPEN, LOCK, etc.

So the situation is that if you continue with the approach in this section,
there is no need for ffds_stateid, while, if you follow the approach in *S*
*t**ate and Locking Models*, ffds_stateid should probably be an array. The
one thing it shouldn't be is what is there now, a single strateid.

Regarding what to do with this paragraph if you decide to stick with this,
I have the following suggestions:

   - As regards the penultimate sentence, given the approach that is taken
   in *S**t**ate and Locking Models*, the argument that is given there no
   longer makes sense. Note that is true even if you decide, for other
   reasons, that that approach is not workable. Suggest you replace the second
   sentence with the following sentence and clause to be prepended to the
   final sentence:

For loose coupling with an NFSv4.x storage device, the client may use
an anonymous stateid to perform I/O on the storage device.  Given the
difficulties attendant upon providing a useful stateid ...

   - In the last sentence, since the stateid is an array, "zero" is not
   appropriate. If you don't want to say "anonymous stateid" here, say "a
   stateid consisting of all zero bytes".

*5.3. Handling Version Errors:*

In the first sentence, suggest replacing "ffdv_version and
ffdv_minorversion combinations" by "combination of ffdv_version,
ffdv_minorversion, and ffdv_tightly_coupled."

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 the section *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
   the section *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.1.3. ff_iostat4*

This is another place where an NFSv4.2 structure. is imported.  The issues
are the same as noted in *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 ingesting layout 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 in the foreseeable future.  Just saying
"RFC5661bis"is enough to induce distress in some quarters.  Please note
that "RFC-bis dysphoria" is not in the DSM -:)

One could add this stuff in a minor version but 4.2 has already gone
through too many last calls 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 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 implements 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:*

Some references need to be updated:

   - NFSv42 needs to point at the -38 draft.
   - RFCNFSv4 needs to point at RFC7530.
   -

   pNFSLayouts needs to point at the -03 draft.

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 there.  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 last paragraph, suggest changing "lead" to "led".

---

From: Benny Halevy <[email-masked]>
Date: Sun, 6 Sep 2015 13:54:02 +0300

Thanks Dave,  my comments below...

On Thu, Sep 3, 2015 at 8:21 PM, David Noveck <[email-masked]> wrote:
> General comments
> Issue Summary
> One exception is that there is there seem to be a significant administrative
> issue blocking acceptance of this draft in its current form.  For details,
> see CB_RECALL_ANY.
>
> Also there is a major technical issue in which ff_layout and State and
> Locking Models contradict each other.  The discussion of this is
> consolidated with the review of State and Locking Models in the first review
> part.
>
> ff_layout4.  It concerns whether ffds_stateid should be made an array.
>
> For an overall summary of this review, see the General Comments section in
> the first review email.
>
> Comments by Section (From Section 3):
>
> 4.1. ff_device_addr4:
>
> In the third non-CODE paragraph suggest, the following changes, primarily to
> reflect the fact that pNFS client use of layouts is never mandatory:
>
> In the first sentence, suggest replacing, "NFS versions and/or minor
> In the penultimate sentence, suggest adding "and ffdv_tightly_coupled to
> false" after "ffdv_minorversion to 0".
> In the penultimate sentence, suggest replacing "MUST access the storage
> device" by "MAY ONLY access the storage device directly"
> In the last sentence, suggest replacing "MUST access the storage device
> using NFSv4" by "MAY ONLY access the storage device directly using the
> corresponding minor version of NFSv4"
>
> replacing "can not" by "cannot"
> replacing "ffdv_version or ffdv_minorversion" by "combinations of
> ffdv_version, ffdv_minorversion, and ffdv_tightly_coupled"
>
> 4.2. Storage Device Multipathing
>
> In the second sentence of the third paragraph suggest replacing "optimal" by
> "desirable".
> In the third sentence of the third paragraph suggest replacing "optimal" by
> "desirable".
>
> Adding "a" before "higher"
> adding a comma after "version"
>
> 5. Flexible File Layout Type:
>
> 5.1. ff_layout4:
> In the third sentence suggest replacing "each ffdv_version and
> In the fourth sentence, suggest replacing "version and minor version
> contradiction of what is said in the section State and Locking Models. It's
> time to pick one store and stick to it, at least until it becomes untenable.
> I've decided that anything that I have to say about the choice between these
> two models (not very much) belongs in the section Stateid Choices (in part
> one of the review).
>
> stateids that appears in this section, as opposed to the one in State and
> Locking Models, we have the following situation:
>
> There is no use for ffds_stateid forNFSv3.
> For loosely-coupled NFSv4, ffds_statid would always have only an anonymous
> stateid.
> In the case f tight coupling there is no need for ffds_srateid, since the
> global stateid model would require you to use the one you got from OPEN,
> LOCK, etc.
>
> there is no need for ffds_stateid, while, if you follow the approach in
> State and Locking Models, ffds_stateid should probably be an array. The one
> thing it shouldn't be is what is there now, a single strateid.

Good point.  I'm inclined towards the simple model of having no stateid
in the layout, but I'd like to hear Tom and others weighing in on this...

> Regarding what to do with this paragraph if you decide to stick with this, I
> have the following suggestions:
>
> As regards the penultimate sentence, given the approach that is taken in
> State and Locking Models, the argument that is given there no longer makes
> sense. Note that is true even if you decide, for other reasons, that that
> approach is not workable. Suggest you replace the second sentence with the
> following sentence and clause to be prepended to the final sentence:
>
> For loose coupling with an NFSv4.x storage device, the client may use an
> anonymous stateid to perform I/O on the storage device.  Given the
> In the last sentence, since the stateid is an array, "zero" is not
> 5.3. Handling Version Errors:
>
> In the first sentence, suggest replacing "ffdv_version and ffdv_minorversion
> combinations" by "combination of ffdv_version, ffdv_minorversion, and
> ffdv_tightly_coupled."
>
> 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
> 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.
>
> 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.

makes sense

> replace "construct" by "consequence"
> replace "that" by "in which"
>
> 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
>
>
> 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
> 8.3. Metadata Server Resilvering of the File
>
> It doesn't seem right to me for the client to respond to
> last sentence of the section 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 the
> section Recovering from Client Errors be rewritten to allow that.

The problem is that NFS4ERR_LAYOUTUNAVAIL is typically a permanent error.
While don't have a specific error code for this case, NFS4ERR_LAYOUTTRYLATER
Is defined as "Layouts are temporarily unavailable for the file."
which is certainly true in
this case. Whether the client performs I/O against the MDS or waits and retries
LAYOUTGET before resuming I/O is an implementation choice and the spec
may provide
guidelines to cases where it's more desirable to do the former vs. the latter.

> 9. Flexible Files Layout Type Return
>

LAYOUTRETURN4_FILE refers to the operation scope (returning layout for
a single FILE, a FSID, or ALL), not to the layout type.

> 9.1.1. ff_ioerr4
> clear to the authors but the reader is left to wonder. There should probably
> be something in Introduction to address this.
>
> 9.1.3. ff_iostat4
>
> are the same as noted in ff_ioerr4 above.
>
> 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".
>
> 13. Recalling layouts:
> When existing layouts are inconsistent with the need to enforce locking
> constraints.
> When ingesting layout are inconsistent with the requirements regarding
> 13.1. CB_RECALL_ANY:
> "RFC5661bis"is enough to induce distress in some quarters.  Please note that
> "RFC-bis dysphoria" is not in the DSM -:)
>
> One could add this stuff in a minor version but 4.2 has already gone through
> too many last calls and v4.3 looks a long way off.
>

Whatever direction we take, let's strive for a streamlined process... :)

Thanks,

Benny

> 15.  Security Considerations
>
> In the sixth sentence suggest rewriting all the material after the word "and
> it" as follows:
>
> 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:
>
> 15.1.1 Loosely coupled
> 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
> implements RPCSEC_GSSV3.  As that process would unreasonably extend the
> deployment process, it must, for the moment, be considered out of scope.
>
> 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
>
> 17.1. Normative References:
>
> NFSv42 needs to point at the -38 draft.
> RFCNFSv4 needs to point at RFC7530.
>
> Acknowledgements:
> Changing "a comprehensive review" to "comprehensive reviews".
> Changing "call" to "calls"
>