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

[David Noveck <[e-mail]>]

I'm OK as far as my suggested changes

On Wed, Aug 30, 2017 at 1:53 PM, Chuck Lever <[e-mail]> wrote:

>
> > 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
>
>
>
> _______________________________________________
> nfsv4 mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/nfsv4
>