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

[David Noveck <[e-mail]>]

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



*1. Introduction:*



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



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



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.


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


· data server (DS)


I think this definition should be deleted together with *Section 1.2.
**Difference
Between a Data Server and a Storage Devic**e*. See my comments below (in *1.2.
Difference Between a Data Server and a Storage Device*)

for more details/ranting.


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


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


· layout stateid


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


   - 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


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



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


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.


   - recalling a layout

In the last sentence, suggest "could be able" by "has the opportunity".



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


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



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



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



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.



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