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

From: Chuck Lever <[email-masked]>
Date: Thu, 3 Aug 2017 13:06:04 -0400

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

---

From: Thomas Haynes <[email-masked]>
Date: Tue, 15 Aug 2017 18:51:22 +0000

> On Aug 3, 2017, at 10:06 AM, Chuck Lever <[email-masked]> wrote:
> 

No, thank you for the review!

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

> "as a whole and those specifically directed”

Ugh, thanks

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

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

>    storage devices from processing I/O from a specific client to a

Went with <def>: [is | are] and complete sentences.

> adequate, perhaps another subsection of 2 can be added.

This had already been fixed.

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

> whether the access is direct to a storage device or through the

I’m fine with I/O.

>   protocol are not data servers.”
> 

Ouch, in isolation this reads very badly.

> servers.”

Taken.

> referring to a storage device that does not simultaneously provide

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. 

> 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]).  Particular implementations may
   satisfy this requirement in any manner they choose and the mechanism
   chosen may not be described as a protocol.  Specifications defining
   layout types need to clearly show how implementations can meet the
   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.

> storage”

Fixed elsewhere

> "for metadata servers and storage devices”

Fixed elsewhere

> is still allowed to access file data via the MDS.

Stating that is redundant to point 1.

> semantics MUST be respected." Otherwise, this is not an enforceable

Already reworked.

> constructed from solely the client's view.

I’m fine with the current POV.

> would expose file attribute information to clients.

Oh, good catch!

> It's not immediately obvious to me what you are getting at in this

If a client has a READ iomode, then it cannot WRITE using it.

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

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

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?

> ... "at the time each access is performed.”

Ack

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

Ack and thanks

> section in a more generic form?

I’m fine with it as is.

> 
> --
> Chuck Lever
> 
> 
> 
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>

---

From: David Noveck <[email-masked]>
Date: Tue, 15 Aug 2017 16:30:41 -0400

> the correct use of RFC 2119 terms.

Neither is the IESG, in my experience.

In any case, my review mentioned this issue.  As it is not really clear
what the right answer is on this question
(RFC2119 contradicts itself), Tom agreed to make clearer how he is using
these terms and for me that resolved the issue.  Chuck will see the
clarifying paragraph when -06 is published.

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 16 Aug 2017 11:04:44 +0200

I don't have a whole lot of time to spare, so this is a very
narrow review with the focus on interactions with the scsi
and rdma layout types.

The first thing I noticed is that section 3 claims:

"There have been no published specifications for control protocols as yet."

which I don't think is true any more.  The SCSI layout fully specifies
the interaction between the MDS and the storage device, so it should
be considered to include a control protocol.

Note that in terms of specifications the it only uses SCSI and thus
the storage protocol, but it uses different features in the SCSI
specifications that are designed to be a control protocol, and the
particular way in the MDS has to use SCSI persistent reservations
could be considered a protocol by itself.  So the paragraph below
about loose coupling doesn't really apply in this case either.

All of that is pretty editorial though as it doesn't change any of
the fundamentals of the section.

I'm not sure what the point of

"Whether the metadata server allows access over other protocols
 (e.g., NFSv3, Server Message Block (SMB), etc) is strictly an
 implementation choice, just as it is in the case of any other
 (i.e., non-pNFS-supporting) NFSv4.1 server."

is.  While the statement is true nothing in NFS talks about
cross-protocol access to servers, so I don't think it matters.

Section 4.1 probably needs an update to include the SCSI layout
given that it claims to document the existing layout types.  The
block layout section is more or less valid for the SCSI layout
as well, so I would suggest to handle the SCSI layout in the
same section.

Another aspect that might be worth mentioning somewhere is
that layouts may revoke access to the whole storage device
for fencing when a layout recall is not acted upon.

---

From: Thomas Haynes <[email-masked]>
Date: Wed, 16 Aug 2017 17:56:48 +0000

> On Aug 16, 2017, at 2:04 AM, Christoph Hellwig <[email-masked]> wrote:
> 

RFC8154 doesn’t quite say that:

   The Server to Storage System protocol, called the "Control Protocol",
   is not of concern for interoperability, although it will typically be
   the same SCSI-based storage protocol.

I read that to say that it is possible for someone to implement the SCSI
layout without using the SCSI-based storage protocol. 

Going to the new terms:

  control communication requirements:  are 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.

   control protocol:  is the particular mechanism that an implementation
      of a layout type would use to meet the control communication
      requirement for that layout type.  This need not be a protocol as
      normally understood.  In some cases the same protocol may be used
      as a control protocol and data access protocol.

I would say that RFC8154 does fully meet the "control communication requirements”.

But based on the sentence above, I see a SHOULD on the use of the
SCSI-based storage protocol as a control protocol and not a MUST.

And given that RFC5663 also states:

   While the Server to
   Storage System protocol, called the "Control Protocol", is not of
   concern for interoperability here, it will typically also be a
   block/volume protocol when clients use block/ volume protocols.

and we don’t consider that layout type to have a control protocol.

> about loose coupling doesn't really apply in this case either.

I think you are arguing that since RFC5663 did not address directly
how the mds would fence the client and RFC8154 does, that
a full control protocol does exist in RFC8154.  And that the
sentence in question should read:

   The Server to Storage System protocol, called the "Control Protocol",
   is the same SCSI-based storage protocol.

> cross-protocol access to servers, so I don't think it matters.

The point I was trying to make is that if the client wants
access to the storage devices by a non-supported layout type,
then it has to access the data through the mds.

But having thought about it, I don’t see the value of leaving that
sentence in place.

> given that it claims to document the existing layout types.

This was taken care of earlier by changing the title to:

4.  Specifications of Original Layout Types

>  The

I thought this covered that?

   In the context of fencing off of the client upon revocation of a
   layout, these limitations come into play again, i.e., the granularity
   of the fencing can only be at the host/logical-unit level.  Thus, if
   one of a client's layouts is revoked by the server, it will
   effectively revoke all of the client's layouts for files located on
   the storage units comprising the logical volume.  This may extend to
   the client's layouts for files in other file systems.  Clients need
   to be prepared for such revocations and reacquire layouts as needed.

---

From: Thomas Haynes <[email-masked]>
Date: Wed, 16 Aug 2017 17:56:49 +0000

Going to the new terms:

   While the Server to

---

From: Thomas Haynes <[email-masked]>
Date: Wed, 16 Aug 2017 18:11:48 +0000

> On Aug 16, 2017, at 10:56 AM, Thomas Haynes <[email-masked]> wrote:
> 
> 

Okay, off of my high horse.

I had already removed the claim above and I have just added a new
paragraph in Section 3.0:

   An example of this last case is the SCSI layout type [RFC8154], which
   extends the block layout type.  The block layout type had a
   requirement for fencing of clients, but did not present a way for the
   control protocol (in this case the SCSI storage protocol) to fence
   the client.  The SCSI layout type remedies that in [RFC8154] and in
   effect has a tightly coupled model.

I think this addresses your point.

---

From: Chuck Lever <[email-masked]>
Date: Wed, 16 Aug 2017 18:49:52 -0400

> On Aug 15, 2017, at 2:51 PM, Thomas Haynes <[email-masked]> wrote:
> 
> 

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.

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

It might be easier to parse if you broke it into two or
three sentences.

>   Particular implementations may
>   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."

>   Specifications defining

Last nit picked, I promise: "to show clearly"

>   requirements being updated rather than the specification of the file

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

> Hmm, the old Section 5 could be the new Section 5.0.

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.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Wed, 16 Aug 2017 20:27:54 -0400

> organization of the material.

It could be better and it could be worse.   It is hard to say before  one
tries
it out.  It is certainly not necessary in order to make the document
understandable.

I never noticed the issue with section 4 in my review.  However, the fact
that
Chuck found this confusing indicates that a clarification is in order and I
can
see that his proposed text makes the intent clearer.

Making such a big chnge at this point is liable to be disruptive,  Unless
the
existing organization is unworkable, it is best to keep the fundamental
structure, already reviewed, as it is.  A big reorganization would trigger
a new WGLC of -07, once it is produced.

Mu suggestion is to incorporate Chuck's suggestions in a very small -07.
Then it
is a simple matter of quickly verifying that all the requested changes have
been
made and then layout-types can move forward

---

From: hch <[email-masked]>
Date: Thu, 17 Aug 2017 09:32:21 +0200

> layout without using the SCSI-based storage protocol. 

Looks like that was a bad copy and paste job from the block layout.
I need to have a word with the lead author ;-)

> SCSI-based storage protocol as a control protocol and not a MUST.

In terms of interop it's indeed not a MUST - just the observable
behavior MUST be as if it was used, which is very close to the same.

> and we don’t consider that layout type to have a control protocol.

The big difference between the block and the scsi layout is that
the former does not specify a fencing protocol - and it turns out
implementing one is basically impossible except for a corner case.

>    is the same SCSI-based storage protocol.

Yes.

>    to be prepared for such revocations and reacquire layouts as needed.

Ok.

---

From: Thomas Haynes <[email-masked]>
Date: Tue, 29 Aug 2017 20:59:09 +0000

On Aug 16, 2017, at 3:49 PM, Chuck Lever <[email-masked]<mailto:[email-masked]>> wrote:

On Aug 15, 2017, at 2:51 PM, Thomas Haynes <[email-masked]<mailto:[email-masked]>> wrote:

On Aug 3, 2017, at 10:06 AM, Chuck Lever <[email-masked]<mailto:[email-masked]>> wrote:

Document status:

== Abstract ==

Consider:

Ugh, thanks

== 1. Introduction ==

== 2. Definitions ==

Consider:

Consider:

Consider:

And so on.

I’m fine with I/O.

Consider:

Taken.

Changed it to:

Consider:

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

"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

Last nit picked, I promise: "to show clearly”

Over the lifetime of this document or the author?

:-)

Fixed elsewhere

Consider:

Fixed elsewhere

Already reworked.

Oh, good catch!

illuminate the updates made to RFC5661 Section 12.”

I am fine with this change.

Section 3.0
Section 4.0
Section 4.1
Section 4.2
Section 4.3

Thoughts?

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.

== 5. Summary ==

Consider:

Ack

Consider:

Ack and thanks

I’m fine with it as is.

--
Chuck Lever

_______________________________________________
nfsv4 mailing list
[email-masked]<mailto:[email-masked]>
https://www.ietf.org/mailman/listinfo/nfsv4

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Wed, 30 Aug 2017 13:53:44 -0400

> On Aug 29, 2017, at 4:59 PM, Thomas Haynes <[email-masked]> wrote:
> 
>> 
>> On Aug 16, 2017, at 3:49 PM, Chuck Lever <[email-masked]> wrote:
>> 
>>> 

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

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Wed, 30 Aug 2017 14:22:34 -0400

I'm OK as far as my suggested changes