Re: [nfsv4] Review comments for WGLC of draft-ietf-nfsv4-layout-types

[Chuck Lever <[e-mail]>]

> On Aug 29, 2017, at 4:59 PM, Thomas Haynes <[e-mail]> wrote:
> 
>> 
>> On Aug 16, 2017, at 3:49 PM, Chuck Lever <[e-mail]> wrote:
>> 
>>> 
>>> On Aug 15, 2017, at 2:51 PM, Thomas Haynes <[e-mail]> wrote:
>>> 
>>> 
>>>> On Aug 3, 2017, at 10:06 AM, Chuck Lever <[e-mail]> wrote:
>>>> 
>>>> Thanks to Tom for writing this one.
>>> 
>>> No, thank you for the review!
>>> 
>>> 
>>>> 
>>>> I'm not qualified to address the technical issues covered by this
>>>> document, thus my comments are limited to editorial and mechanical
>>>> suggestions. 
>>>> 
>>>> It would be great if current authors of new layouts (other than
>>>> the author of this document) could review this one. Christoph,
>>>> can you take a look at this one? It's short.
>>>> 
>>>> The document is a valuable addition to the canon of pNFS
>>>> specifications, but is still rough around the edges IMO. Nothing
>>>> that is impossible to fix, but seems like section 4 needs some
>>>> face lifting.
>>>> 
>>>> 
>>>> Document status:
>>>> 
>>>> This is a "Standards Track" document. Section 4 appears to update
>>>> three RFCs which are Standards Track. Only one of these is mentioned
>>>> in the document header and Abstract.
>>>> 
>>>> Section 3 appears to be advice to authors of layout specifications,
>>>> and advice to the WG on how to evaluate new layouts. Is the use of
>>>> RFC 2119 terms appropriate for this section of this document?
>>>> 
>>> 
>>> I think it is clear I am not a paragon of virtue when it comes to 
>>> the correct use of RFC 2119 terms.
>>> 
>>> 
>>>> 
>>>> == Abstract ==
>>>> 
>>>> "as a whole and those those specifically directed"
>>>> 
>>>> Consider:
>>>> 
>>>> "as a whole and those specifically directed”
>>> 
>>> Ugh, thanks
>>> 
>>>> 
>>>> 
>>>> == 1. Introduction ==
>>>> 
>>>> "As a consequence, new internet drafts (see [FlexFiles] and [Lustre])
>>>> may struggle to meet the requirements to be a pNFS layout type."
>>>> 
>>>> The Lustre document referenced here does not appear to be an RFC or an
>>>> active I-D:
>>>> 
>>>> https://datatracker.ietf.org/doc/draft-faibish-nfsv4-pnfs-lustre-layout/
>>>> 
>>>> Perhaps the reference should be removed or replaced.
>>> 
>>> 
>>> Which is why it is not normative. Note that the URL is stable.
>>> 
>>> This may come out during the last editorial phase. It lays the groundwork
>>> for why we want the document.
>>> 
>>> 
>>>> 
>>>> "This document specifies the layout type independent requirements placed
>>>> on all layout types, whether one of the original three or any new variant."
>>>> 
>>>> Consider this simplification:
>>>> 
>>>> "This document specifies layout type-independent requirements placed on
>>>> all layout types.”
>>> 
>>> We have after Dave’s review:
>>> 
>>>  This document specifies the requirements placed on all layout types
>>>  independent of the particular type chosen, whether one of the
>>>  original three or any new additional one.
>>> 
>>> and I think you would argue for:
>>> 
>>>  This document specifies the requirements placed on all layout types
>>>  independent of the particular type chosen.
>>> 
>>> And taking in mind your last point about the Intro, how about this as a
>>> last paragraph:
>>> 
>>>  As a consequence, new internet drafts (see [FlexFiles] and [Lustre])
>>>  may struggle to meet the requirements to be a pNFS layout type.  This
>>>  document gathers the requirements from all of the original layout
>>>  type standard documents and then specifies the requirements placed on
>>>  all layout types independent of the particular type chosen.
>>> 
>>> 
>>> 
>>>> 
>>>> The introduction should also announce what is happening in section 4,
>>>> which provides updates to existing layout types.
>>>> 
>>>> 
>>>> == 2. Definitions ==
>>>> 
>>>> In general the syntax of each definition should be consistent. You
>>>> have "defines yada", "is yada", and sometimes complete sentences.
>>>> Some examples follow:
>>>> 
>>>> "control communication requirements:  define for a layout type the
>>>>  details regarding information on layouts, stateids, file metadata,
>>>>  and file data which must be communicated between the metadata
>>>>  server and the storage devices."
>>>> 
>>>> Consider:
>>>> 
>>>> "control communication requirements:  layout type requirements
>>>>  regarding information on layouts, stateids, file metadata,
>>>>  and file data which must be communicated between the metadata
>>>>  server and storage devices."
>>>> 
>>>> 
>>>> "(file) data:  is that part of the file system object which contains
>>>>  the data to read or writen.  It is the contents of the object and
>>>>  not the attributes of the object."
>>>> 
>>>> Consider:
>>>> 
>>>> "(file) data:  data content that is opaque to storage services.
>>>>  File data that is written is expected to be unaltered when it is
>>>>  subsequently read back. In particular, data content is not the
>>>>  file's attributes."
>>>> 
>>>> "fencing:  is the process by which the metadata server prevents the
>>>>  storage devices from processing I/O from a specific client to a
>>>>  specific file.
>>>> 
>>>> Consider:
>>>> 
>>>> "fencing:  the mechanism by which a metadata server prevents
>>>>  storage devices from processing I/O from a specific client to a
>>>>  specific file."
>>>> 
>>>> And so on.
>>> 
>>> Went with <def>: [is | are] and complete sentences.
>>> 
>>> 
>>>> 
>>>> "layout iomode:  see Section 1."
>>>> 
>>>> Strikes me as needing expansion. If a single paragraph is not
>>>> adequate, perhaps another subsection of 2 can be added.
>>> 
>>> This had already been fixed.
>>> 
>>> 
>>>> 
>>>> "loose coupling:  describes when the control protocol, between a
>>>>  metadata server and storage device, is a storage protocol."
>>>> 
>>>> Loose and tight coupling might be better served in a separate
>>>> subsection. This definition strikes me as vague for a first time
>>>> reader. At least refer to section 3 here.
>>>> 
>>>> The terms "layout" and "layout type" are key to the entire
>>>> discussion. They might benefit greatly from an independent
>>>> subsection in section 2, or even some discussion in section 1.
>>>> 
>>> 
>>> 
>>> I think there is a difference of opinion on what the definitions
>>> section means. To me, the section provides the smallest
>>> possible definition that the reader can look at to
>>> get the gist of the term. It is not a comprehensive
>>> definition of the term.
>>> 
>>> When the reader comes across “layout type” later in
>>> the document, they can flip back to the definitions
>>> section to see the broad intent of the term.
>> 
>> I accept your definition of "definition".
>> 
>> There are certain terms, however, that you've already
>> chosen to elaborate on. Elaborating on one or two
>> other terms could be helpful, but I'm not going to
>> be insistent.
>> 
>> 
>>>> You might find it handy to have distinct definitions of "file
>>>> access protocols" and "block access protocols" as separate classes
>>>> of storage protocols.
>>>> 
>>>> Throughout the document, you might consider using a more abstract
>>>> term than "I/O operation". "File data access" for example might
>>>> be more independent of which storage protocol is in use, or
>>>> whether the access is direct to a storage device or through the
>>>> MDS.
>>>> 
>>> 
>>> I’m fine with I/O.
>>> 
>>> 
>>>> 
>>>> == 2.1. Use of the terms ... ==
>>>> 
>>>> "Note that every data server is a storage device but
>>>> that storage devices which use protocols which are not file access
>>>> protocol are not data servers.”
>>>> 
>>> 
>>> Ouch, in isolation this reads very badly.
>>> 
>>> 
>>>> Consider:
>>>> 
>>>> ... "is a storage device, but storage devices which use protocols
>>>> which are not file access protocols (such as NFS) are not data
>>>> servers.”
>>>> 
>>> 
>>> 
>>> Taken.
>>> 
>>> 
>>>> "while simultaneously acting as a non-data-server storage device for
>>>> others."
>>>> 
>>>> I'm not quite sure what you are saying here. I don't see a definition
>>>> for "non-data-server storage device". In context, perhaps you are
>>>> referring to a storage device that does not simultaneously provide
>>>> file access?
>>> 
>>> 
>>> A storage device can be a data server for one storage protocol
>>> and at the same time, not a data server for another storage
>>> protocol.
>>> 
>>> Changed it to:
>>> 
>>>     Since a given storage device may support multiple layout types, a
>>>     given device can potentially act as a data server for some set of
>>>     storage protocols while simultaneously acting as a storage device
>>>     for others. 
>>> 
>>> 
>>>> 
>>>> This discussion could benefit from adding definitions of "file
>>>> data access" and "block data access", for instance.
>>>> 
>>>> 
>>>> == 3. The Control Protocol ==
>>>> 
>>>> "In Section 12.2.6 of [RFC5661], the control protocol was introduced."
>>>> 
>>>> Consider:
>>>> 
>>>> "In Section 12.2.6 of [RFC5661], the concept of a layout type control
>>>> protocol was introduced.”
>>> 
>>> I’m struggling with adding this differentiation and I think I have a fix:
>>> 
>>>  One of the key requirements of a layout type is the need for a
>>>  mechanism to be used to meet the requirements that apply to the
>>>  interaction between the metadata server and the storage device such
>>>  that they present a consistent interface to the client
>>>  (Section 12.2.6 of [RFC5661]).
>> 
>> I'm having some trouble with this. Mechanically, the word
>> "requirements" is repeated. The last bit doesn't make sense
>> to me: NFS and the storage protocols already make up a
>> "consistent interface to clients," so I think you mean
>> something else, like "a consistent view of stored data and
>> lock state”.
> 
> 
> I see why you are struggling with it, but replacing it is a struggle for me.
> 
> What you have is that each of the:
> 
> 1) NFS protocol between client and mds
> 2) Storage protocol between client and storage device
> 
> are already consistent interfaces.
> 
> What I am saying is that the union of the two has to
> be consistent.
> 
> Argh, I can read mine in yours but not yours in mine:
> 
> 
>    A layout type has to meet the requirements that apply to the
>    interaction between the metadata server and the storage device such
>    that they present to the client a consistent view of stored data and
>    lock state (Section 12.2.6 of [RFC5661]).
> 
> 
>> 
>> It might be easier to parse if you broke it into two or
>> three sentences.
> 
> 
> 
> 
> 
> 
>> 
>> 
>>>  Particular implementations may
>>>  satisfy this requirement in any manner they choose and the mechanism
>>>  chosen may not be described as a protocol.
>> 
>> In normal English, "may not be described as a protocol" can mean
>> "please do not describe it as a protocol." A less ambiguous wording
>> might be:
>> 
>> "The chosen control mechanism is not required to be described as
>> [or perhaps, specified as] a protocol.”
>> 
> 
> 
> In fixing the above, I actually struggled over this before reading this, I had gone with:
> 
>    Particular implementations
>    may satisfy these requirements in any manner they choose and the
>    mechanism chosen need not be described as a protocol. 
> 
> But now I am thinking of
> 
>    Particular implementations
>    may satisfy these requirements in any manner they choose and the
>    mechanism chosen need not be specified as a protocol. 
> 
> 
> 
>> 
>>>  Specifications defining
>>>  layout types need to clearly show how implementations can meet the
>> 
>> Last nit picked, I promise: "to show clearly”
>> 
> 
> Over the lifetime of this document or the author?
> 
> :-)
> 
> 
> 
>> 
>>>  requirements discussed below, especially with respect to those that
>>>  have security implications.  In addition, such specifications may
>>>  find it necessary to impose requirements on implementations of the
>>>  layout type to ensure appropriate interoperability.
>>> 
>>> Notice I also deleted a some sentences.
>>> 
>>> 
>>>> 
>>>> This text is repeated in front of the third and fourth paragraphs:
>>>> 
>>>> "In some cases, there may be no control protocol other than the
>>>> storage”
>>> 
>>> Fixed elsewhere
>>> 
>>> 
>>> 
>>>> 
>>>> "for metadata serverss and storage devices"
>>>> 
>>>> Consider:
>>>> 
>>>> "for metadata servers and storage devices”
>>> 
>>> Fixed elsewhere
>>> 
>>>> 
>>>> "(2)  The metadata server MUST be able to restrict access to a file on
>>>>    the storage devices when it revokes a layout."
>>>> 
>>>> This section should make clear that after layout revocation, a client
>>>> is still allowed to access file data via the MDS.
>>> 
>>> Stating that is redundant to point 1.
>>> 
>>> 
>>>> 
>>>> "(4)  Locking MUST be respected."
>>>> 
>>>> This section feels like it needs expansion. Even "NFSv4.1 file locking
>>>> semantics MUST be respected." Otherwise, this is not an enforceable
>>>> MUST.
>>> 
>>> 
>>> Already reworked.
>>> 
>>>> 
>>>> "(5)  The metadata server and the storage devices MUST agree on
>>>>      attributes like modify time, the change attribute, and the end-
>>>>      of-file (EOF) position."
>>>> 
>>>> It would be better to attempt to provide a list of the attributes where
>>>> this requirement is necessary instead of providing examples.
>>>> 
>>>> I think this section would be more clear if the requirement is
>>>> constructed from solely the client's view.
>>> 
>>> I’m fine with the current POV.
>>> 
>>>> 
>>>> This item and its discussion don't make sense for storage protocols that
>>>> are not file access protocols. It needs to be rephrased to apply strictly
>>>> to storage devices that are accessed via a file access protocol, which
>>>> would expose file attribute information to clients.
>>>> 
>>> 
>>> Oh, good catch!
>>> 
>>> 
>>>> 
>>>> == 3.2. Undocumented ... ==
>>>> 
>>>> "(4)  Clients MUST NOT perform I/O operations which would be
>>>>    inconsistent with the iomode specified in the layout segments it
>>>>    holds."
>>>> 
>>>> It's not immediately obvious to me what you are getting at in this
>>>> item.
>>>> 
>>> 
>>> If a client has a READ iomode, then it cannot WRITE using it.
>>> 
>>> 
>>>> "(5)  The metadata server MUST be able to do allocation and
>>>>    deallocation of storage.  I.e., creating and deleting files."
>>>> 
>>>> Perhaps the language here could be refined. "storage" can mean anything
>>>> and hasn't been previously defined.
>>> 
>>> I think the rewording from Dave’s review satisfies this?
>>> 
>>>  (3)  The metadata server MUST be able to do storage allocation,
>>>       whether that is to create, delete, extend, or truncate files.
>>> 
>>> 
>>>> 
>>>> I think you are talking about managing layout extents on storage
>>>> devices?
>>>> 
>>>> 
>>>> == 4.1 File Layout Type ==
>>>> 
>>>> Throughout this section, the discussion is not clear when it switches
>>>> between describing behavior specified by RFC 5661, current
>>>> implementation behavior, and fresh advice to new implementers.
>>> 
>>> 
>>> I’m torn here, despite this statement:
>>> 
>>>  This section is not normative with regards to each of the presented
>>>  types.  This document does not update the specification of either the
>>>  block layout type (see [RFC5663]) or the object layout type (see
>>>  [RFC5664]).  Nor does it update Section 13 of [RFC5661], but rather
>>>  Section 12 of that document.  In other words, it is the pNFS
>>>  requirements being updated rather than the specification of the file
>>>  layout type.
>> 
>> Now I understand what you're going for. I think it would be more
>> clear if you start with what _is_ being updated. How about:
>> 
>> "This section updates Section 12 of [RFC5661], which enumerates
>> the requirements of pNFS layout type specifications. It is not
>> normative with regards to the layout type presented in Section
>> 13 of RFC5661, the block layout type [RFC5663], and the object
>> layout type [RFC5664]. These are discussed here only to
>> illuminate the updates made to RFC5661 Section 12.”
>> 
> 
> I am fine with this change.
> 
> 
> 
>> 
>>> Section 4 is not updating the pNFS requirements. That occurs in
>>> Section 3.
>>> 
>>> I can see a rewhack of the draft to have in order:
>>> 
>>> Section 3.0
>>> Section 4.0
>>> Section 4.1
>>> Section 4.2
>>> Section 4.3
>>> A new Section 5 to introduce the following
>>> Section 3.1 as Section 5.1
>>> Section 3.2 as Section 5.2
>>> Section 3.3 as Section 5.3
>>> Section 5 as Section 6
>>> …
>>> 
>>> So the new Section 3 declares the main issue.
>>> The new Section 4 uses the existing layout types to illustrate the issues.
>>> The new Section 5 presents the resulting requirements.
>>> 
>>> Hmm, the old Section 5 could be the new Section 5.0.
>>> 
>>> Thoughts?
>> 
>> My feeble mind can't imagine the result with this brief
>> description, but your proposal could very well be a better
>> organization of the material.
>> 
>> 
> 
> Hah, “feeble”, now I have to try to remember what I wrote!
> 
> I am going to follow Dave’s suggestion here, apply the edits above, publish
> v7 and see if the WG can flow with the changes.
> 
> 
>>>> There are no reasons given about why these changes are being made (was
>>>> the spec language inadequate, or are you addressing bugs in the protocol,
>>>> for example)?
>>>> 
>>>> Please make it clear what the original spec says, what it should now
>>>> say, and what is new advice to implementers of the file layout type.
>>>> And separately discuss why the update is necessary.
>>>> 
>>>> See Noveck's migration update doc series for examples of how to
>>>> clearly describe an update to specification language.
>>>> 
>>>> Similar comments apply to sections 4.2 and 4.3: it's not clear what
>>>> is being updated.
>>>> 
>>>> I'm not at all arguing about the technical content of this section.
>>>> These are strictly observations about how this material is presented.
>>>> 
>>>> 
>>>> == 5. Summary ==
>>>> 
>>>> I'm not sure what purpose this section would play once this document
>>>> becomes an RFC.
>>>> 
>>>> 
>>>> == 6. Security Considerations ==
>>>> 
>>>> "It may do this directly, by providing that appropriate checks be
>>>> performed at the time the access is performed."
>>>> 
>>>> Consider:
>>>> 
>>>> ... "at the time each access is performed.”
>>> 
>>> Ack
>>> 
>>>> 
>>>> "The client the has the opportunity to re-acquire the layout"
>>>> 
>>>> Consider:
>>>> 
>>>> "The client has a subsequent opportunity to re-acquire the layout”
>>>> 
>>> 
>>> Ack and thanks
>>> 
>>> 
>>>> There is a strong discussion of client-side access checking in
>>>> section 4.2. Would that be more appropriate to move into this
>>>> section in a more generic form?
>>>> 
>>> 
>>> 
>>> I’m fine with it as is.
>>> 
>>> 
>>>> 
>>>> --
>>>> Chuck Lever
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> nfsv4 mailing list
>>>> [e-mail]
>>>> https://www.ietf.org/mailman/listinfo/nfsv4
>>>> 
>>> 
>> 
>> --
>> Chuck Lever

Tom, all updates look good to me. Thanks for responding.



--
Chuck Lever