Re: [nfsv4] Review of draft-ietf-nfsv4-fex-files-13 (part two of two)

[Thomas Haynes <[e-mail]>]

On Aug 22, 2017, at 4:45 AM, David Noveck <[e-mail]<mailto:[e-mail]>> wrote:

Review Structure

This review has been split up into multiple emails to avoid running afoul of mailing list size restrictions.

This is the second section.

Previous Section

This section contains all the general (non-section-specific) comments and the first section of the detailed per-section comments.   Generally it should be read first.

Current Section

Contains the bulk of the per-section comments, including those related to security considerations.

Comments by Section (from Section 2.1 onward)

2.1.  LAYOUTCOMMIT

The reference to section 12.5.4 of RFC5661 may have to be updated to point to something within layout-types, given that that Section 12 of RFC5661 is updated by layout-types.

ACK


2.3.  State and Locking Models

The sentence "The choice of locking models is governed by the following rules", is confusng since it is actually the server implementation who makes this choice, when there is a choice

Suggest replacing this by the following:

A client can determine which couplig model has been implemented as follows:

No, the client can just look at ffdv_tightly_coupled in ff_device_versions4.


But, agreed that the section appears aimless. I changed the first sentence to:

      An implementation can always be deployed as a loosely
      coupled model. There is however no way for a storage
      device to indicate over a NFS protocol that it can
      definitively participate in a tightly coupled model:

And after the points, added:

      A storage device would have to either be discovered or
      advertised over the control protocol to enable a tight
      coupling model.




4.1.  ff_device_addr4

In the first sentence, need to replace "storage protool specific "by "layout-type-specific”.

Ack but without hyphens



5.  Flexible File Layout Type

The placement of the last sentence in the section is confusing.  It should either be moved to Section 5.1 or "This" should be changed to "The next”.

A section is a major number.

A subsection is a minor number of a major number.





5.1.  ff_layout4

The first pargraph, as currently written, seems to suggest that mirroring is always in effect.  It needs to be clarified.

Went with:

   The ff_layout4 structure specifies a layout in that portion of the
   data file described in the current layout segment.  It is either a
   single instance or a set of mirrored copies of that portion of the
   data file.  When mirroring is in effect, it protects against loss of
   data in layout segments.  Note that while not explicitly shown in the
   above XDR, each layout4 element returned in the logr_layout array of
   LAYOUTGET4res (see Section 18.43.1 of [RFC5661]) describes a layout
   segment.  Hence each ff_layout4 also describes a layout segment.


There are a number of issues in the ninth (non-code) paragraph.  Sucggest replacing the last two sentences by the following:

In the case of loose coupling when accessing an NFSv4 storage device, the client will use an anonymous stateid to perform I/O on the storage device as there is no use for the metadata server stateid in the absence of a control protocol to provide a global stateid model. In such cases, the server MUST set the ffds_stateid to be the anonymous stateid.

The sentences were too complex, rewrote as:


   For tight coupling, ffds_stateid provides the stateid to be used by
   the client to access the file.  For loose coupling and a NFSv4
   storage device, the client will have to use an anonymous stateid to
   perform I/O on the storage device.  With no control protocol, the
   metadata server stateid can not be used to provide a globale stateid
   model.  Thus the server MUST set the ffds_stateid to be the anonymous
   stateid.




There are a number of issues in the thirteenth (non-code) paragraph:

  *   It is unclear what the referent of the intial "These" is.
  *   The statement that "it can not be fixed" is not correct. It could be fixed although there are good resons not to fix it. Those reason should be explained.
  *   The statement that an implementation might require the protocol to be changed sounds backward to me and the IESG is likely to find it unacceptable.

Suggest the following replcement paragraph:

A number of issues stem from a mismatch between the fact that ffds_stateid is defined as a single item while ffds_fh_vers is defined as an array.  It is possible for each open file on the storage device to require its own open stateid.  Because there are established loosely coupled implementations of the version of the protocol described in this document, such potential issues have not been addressed here.  It is possible for future layout types to be defined that adress these issues, should it become important to provide multiple stateids for the same underlying file.

Ack



5.1.2.  Client Interactions with FF_FLAGS_NO_IO_THRU_MDS

Suggest replacing the second sentence by "The flag functions as a hint”.

Ack


To clarify the third sentence suggest replacing it by the following:

The flag indicates to the client that the metadata server prefers to separate  the MDS I/O from the data I/O, most likely for peformance reasons.

Ack


The material beyond the current third sentece is just a complicated way of saying that the MDS has no effective way to deal with a client that ignores this flag.  It would be better to just delete that material

Complicated? Sure.

But you miss the point that the mds can deal with a client which ignores the flag. It throws a hissy fit and refuses to proceed until the client does as requested.

Ack


14.  Client Fencing

In the first sentece,suggest deleting the phrase "at the least”.

Ack


15.  Security Considerations

The way the material before section 15.1 is written, it isn't clear that the first pargraph is dscussing security for pNFS as whole, while the following two paragraphs are about security with regard to the loose coupling model of the flex-files layout type.

In order to clarify, suggest:

  *   not describing pNFS as an extension since it is inconsistent with the watythis term is used in RFC8178.  "Feature" would be better.

Ack

  *   shortening the first pargraph to put the stress on the fct that this issue was ealt with in RFC5661.

No, I’d rather get feedback during the Sec Dir review.


  *   Adding some introductory material to the second paragraph to make clear what you are talkig about.

Went with:

    The metadata server is primarily responsible for securing the
    data path. It has to authenticate the client access and
    provide appropriate credentials to the client to access
    data files on the storage device. Finally, it is responsible
    for revoking access for a client to the storage device.




15.1.  RPCSEC_GSS and Security Services

There needs to be some sort of introductory material to introduce this section as a whole.  Suggest the following:

Because of the special use of principals within the loose coupling model, the  issues are different in the case of loose coupling (Section 15.1.1) and tight coupling (Section 15.1.2)

Went with:

      Because of the special use of principals within the loose coupling
      model, the issues are different depending on the coupling model.



15.1.1.  Loosely Coupled

Although I understand why RPCSEC_GSSv3 is not being used, many readers, not being familiar with the discussions about it, may well be confused, especually given the reference to "the intent of the loosely coupled model that the storage device not be modified" which is mentioned nowhere else in this document.  Suggest the following replacement:

RPCSEC_GSS version 3 (RPCSEC_GSSv3) [RFC7861] contains facilities that would allow it to be used to authorize the client to the storage device on behalf of the metadata server.  Doing so  would require that each of the metadata server, storage device, and client would need to implement RPCSEC_GSSv3 using an RPC-application-defined structured privilege assertion in a manner described in Section 4.9.1 of [RFC7862].  The specifics necessary to do so are not described in this document.  This is principally because any such specification would require extensive implementation work on a wide range of data servers, which would be unlikely  to result in a widely usable specification for a considerable time.

As a result, the layout type described in this document will not provide support for use of RPCSEC_GSS togther with the loosely coupled model.  However, future layout types could be specified which would allow such support, either through the use of RPCSEC_GSSv3, or in other ways.

While true, nothing prevents an implementation from doing this without a standard’s document.


Anyway,

Ack

17.1.  Normative References

The reference for [pNFSLayouts] needs to be updated.

A moving window that will probably be fixed by the two documents becoming a cluster-pair.



I believe RFC7861 belongs here (see below).

17.2.  Informative References

Given how this is used in Section 15.1.1, I believe the reference to RFC 7861 is more appropriate as a normative reference.


Ack