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

[Chuck Lever <[e-mail]>]

Thanks to Tom for writing this one.

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?


== Abstract ==

"as a whole and those those specifically directed"

Consider:

"as a whole and those specifically directed"


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

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

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.

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

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

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.


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

Consider:

... "is a storage device, but storage devices which use protocols
which are not file access protocols (such as NFS) are not data
servers."

"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?

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

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"

"for metadata serverss and storage devices"

Consider:

"for metadata servers and storage devices"

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

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

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

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.


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

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

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

"The client the has the opportunity to re-acquire the layout"

Consider:

"The client has a subsequent opportunity to re-acquire the layout"

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?


--
Chuck Lever