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

[Thomas Haynes <[e-mail]>]

I’ve lost my draft of my reply, so I’ll just wing it as I go along…

I.e., I made edits, recorded that fact in a draft reply and now have to clue as to my rationale.

I have addressed all of the issues in this part of the series and will be publishing them as draft-10.

I have also provided an approach to kerberized access under the loosely coupled model.


On May 21, 2016, at 7:00 AM, David Noveck <[e-mail]<mailto:[e-mail]>> wrote:

Review Structure

This is the first section of a multi-part review which has been broken up into multiple mail messages to avoid running into wg mailing list size restrictions.

This section contains the general comments and the first part of the per-section comments.  As a result, much of the material in the general comments will be referring to material that only appears in later review sections.

General Comments

Background of Review

The -06 of this document entered Working Group Last Call, according to the Datatracker, on 8/24/2015.

Soon after that I sent a long review (in two parts) to the authors, cc-ing the working group list.  The substance of my review was that the document had some major issues that needed to be addressed to get it into a WGLC-worthy state.

At the time I didn't get any response from Tom.  It now appears that Tom did send a response to the second part of my review on 10/16/2015, but it doesn't seem that I got it.  Although he did send it to the working group list, it can't be found in the mailing list archives.

I have not been able to find the email in which the WGLC was announced, so I'm not sure when it was supposed to end or any mail in which the end of the last call is discussed and next steps agreed upon. In any case, given the eight months since then and now, I presume the WGLC is now effectively over.  I'll leave it to the authors to resolve the administrative issues regarding the document state with Spencer.

In January 2016, and recently, Tom has posted updates to the document, although they don't address the major issues that I raised in my review of -06.  Recently, Tom forwarded me the lost response to the second part of my review, so I have a better picture of his views about my comments and his plans for the document going forward.

In any case, I've decided to send a review of -08.  Because the differences between  -06 and -08 are small, it will be based, in large part on my earlier review of -06, although there will be changes of the following sorts:

  *   In cases in which the -08 has a new issue, not present in the -06, I'll add a discussion of that issue, and note that it is a new issue in the -08.
  *   In cases in which there is an issue not noted in the -06 review but which applies to the -06 as well, I'll note that fact as well.
  *   Issues noted in the review of the -06, that have been addressed in -08 are simply eliminated.
  *   In cases in which I know from the forwarded response how Tom intends to deal with an issue, the discussion is simplified to reflect the likely approach.  In particular, the discussion of the handling of stateids is simplified given that Tom has decided on a particular approach.
  *   In cases in which I know from the forwarded response that Tom objects to a proposed change, I'll summarize the situation, but will try not to continue the argument, and leave resolution of the issue until later discussion of the document

General Evaluation

Basically the situation remains as it was when I sent the review of -06.  Although there has been some discussion of some of the issues raised in my review of -06, and some changes made, the major issues I noted have not been addressed by document changes.

There is a lot of valuable stuff in this document but significant work needs to be done to clarify things regarding the handling of locking state.  A lot of the problems derive from the fact that this document describes four different situations and is not careful about the differences among:

  *   Using NFSv3 as a data access protocol
  *   Using NFSv4.0 as a data access protocol
  *   Using NFSv4.1 as a data access protocol in the loose coupling case
  *   Using NFSv4.1 as a data access protocol in the tight coupling case

Issues Blocking Working Group Last Call

I'd characterize the following issues as being in this category:

  *   Resolving the contradiction in -08 between Section 2.3.  Locking Models and Section 5.1.  ff_layout4,
  *   Implementing some of the cleanup/clarification regarding locking discussed in 2.3.  Locking Models (as regards my current views regarding 08, this is addressed in the second part of the current review).
  *   Resolving the issue of the unused ffds_stateid field in Section 5.1.  ff_layout4,

My take is to just obsolete it and say that tightly coupled layouts are broken.



  *   Resolving the issue about FF_FLAGS_NO_IO_THROUH_MDS discussed in 5.1.  ff_layout4,

Other Noteworthy Issues

I think it makes sense to address RIck's proposal in his mail of March 8, Possible change to Flex Files layout. The response on the list was generally positive, so it makes sense to address this before working group last call.

Comments by Section (Through Section 2.2)

Abstract:

Suggest replacing "to allow" by "which allows"


Ack

1. Introduction:

Suggest replacing "the wire protocol of such a protocol" by "such a protocol".



Ack


1.1 Definitions:

• control protocol

This is not "a set of requirements".  Rather it is something that supports a set of requirements.  Unstated requirements with nothing to provide support them are not very helpful.


If it is not a set of requirements, then it is not a protocol.


As an example of the difficulties that this gives rise to, consider the phrase "requirements for the protocol" in the introduction.  If the control protocol is a "set of requirements" then the requirements for the protocol is "requirements for a set of requirements" :-(  This is making my head hurt.



And that is why we have the other document. RFC 5661 avoided defining what the control protocol was and even avoided defining the requirements for such a control protocol. Oh, we will let the vendor implementations do all that.


· data file

At the very least, should replace "E.g." by "I.e.".  my preference would be to simply say it is the file contents without using any Latin abbreviations.  So how about:

That part of a file system object that consists of the file contents.



I’ve made changes to this and the metadata file definitions.

• data server (DS)

I think this definition should be deleted together with Section 1.2. Difference Between a Data Server and a Storage Device. See my comments below (in 1.2. Difference Between a Data Server and a Storage Device)
for more details/ranting.



No

· file layout type

I think you mean here the layout type defined in chapter 13 of RFC5661.  As it is, with the current reference to 'the NFS protocol" this seems to include that and the flexible file layout.   If you need a term for that latter, suggest "NFS-based layout type".



Made a ref to Sec 13 of 5661.

• layout segment

The reference there is to chapter 13 of RFC5661 which deals with the (old-fashioned,  tightly-coupled) file layout type.  It makes more sense, in this document, to reference something specific to  the flex-files layout type.



I’m fine with it the way it is.

• layout stateid


In the last sentence suggest adding "of that document" after "Section 12.5.3".


Made a change.



  *   loose coupling
You seem to have gotten rid of the "no control protocol" stuff elsewhere.  it's time to do the same in the definitions section



I’m fine with what I have.


  *   metadata file

This is now confusing because, although the metadata file does not include the file contents, it does describe them.  So "describes the object and not the payload" is wrong.

Suggest the following replacement:

That part of a file system object which gives attributes for and the location of the file data rather than containing the file contents itself,  It can include such items as the times of  last modification or access, and security information such as an ACL.



I’m fine with what I have.


• mirror

There are a couple of issues I have with the second sentence (or the first sentence following the initial sentence fragment):
o It isn't clear to me how the two clauses relate.
o there needs to be more information about how the situation referred to in the first clause is arrived.
o None of this is really part of the definition of mirror.


Removed the 2nd sentence.


Some of this material needs to go into the section 8. Mirroring.


• tight coupling

Same issue as for "loose coupling"  You need to characterize the different types/strengths of potential control protocols, rather than pretending that a very thin one doesn't exist.



I’m fine with what I have.


  *   recalling a layout
In the last sentence, suggest "could be able" by "has the opportunity".



done

Also, I'm not sure what "etc" is referring to in the last sentence.  Should it be removed?


commit, update attributes if a write delegation is held, etc.


  *   revoking a layout


It seems to me that, for this layout type, revoking a layout and fencing are essentially identical and that this fact should be noted.



No, they are different.

recall is graceful and revoke is not.



1.2. Difference Between a Data Server and a Storage Device

The term data server appears eleven times in this document.  Nine of them are either in the definitions of "data server" and "data storage device" or in this section.  That leaves two:  the title of section 2.2 and the corresponding TOC entry.

There is no point in introducing two terms that essentially mean the same thing.  Instead of doing that and then backing away from it, it is  best to pick one and stick with it.




Again, I am fine with what I have.


2.1. LAYOUTCOMMIT:

There are a couple of issues in the first sentence:
•       "tightly coupled system" is not as clear/general as it should be.
•       The reference to the semantics of the File Layout Type is inappropriate given that the reference is to Chapter 12 of RFC5661.

Suggest the following replacement:

When tightly coupled storage devices are used, the metadata server has the responsibility, upon receiving a LAYOUTCOMMIT (see Section 18.42 of [RFC5661]<https://tools.ietf.org/html/rfc5661#section-18.42>), of ensuring that the semantics of pNFS are respected (see Section 12.5.4 of [RFC5661]<https://tools.ietf.org/html/rfc5661#section-12.5.4>).  These do not include a requirement that data written to data storage device be stable upon completion of the LAYOUTCOMMIT.


It makes sense to start a new paragraph at this point.

Ack and taken

With regard to the rest of material, I basically agree with Benny but I think we have to arrange things to make clearer that we are talking about the loosely coupled case here.  I think you need something like the following:

In the case of loosely coupled storage devices, it is the responsibility of the client to make sure the data file is stable before the metadata server begins to query the storage devices about the changes to the file. If any WRITE to a storage device did not result with stable_how equal to FILE_SYNC, a LAYOUTCOMMIT to the metadata server MUST be preceded by a COMMIT to the storage devices written to.


The last sentence of the paragraph is not very convincing as to the reason for this requirement.  It should explain why "the LAYOUTCOMMIT might not be synchronized to the last WRITE operation to the storage device".  I assume the worry is about a data storage device reboot immediately after the LAYOUTCOMMIT.



Ack

2.2. Fencing Clients from the Data Server:


If one replaces "Data Server" by "Data Storage Device" in the title, one gets rid of the last two uses of "Data server" in this document. Thus there is no need to discuss the differences/nuances between the two.

There's a problem in the fourth paragraph. Given that the first three paragraphs deal with the loosely coupled case, when you get to the tightly coupled case you need to say something about fencing clients from the data storage device. Instead you simply explain that the loose couplig fencing facilities are not used, leaving the fencing question up in the air.  Saying it's not your job is OK if you explain why that is.  For example, one might say:

In the case of tight coupling, such fencing facilities are the responsibility of the control protocol and are not described in detail here.  However, implementations of the tight coupling locking model (see Section 2.3), will need a way to prevent access by certain clients to specific files by invalidating the corresponding stateids on the data storage device.