Subject: [nfsv4] Review of draft-ietf-nfsv4-rfc5667bis-06

From: David Noveck <[email-masked]>
Date: Sat, 25 Feb 2017 07:57:32 -0500

*General Comments*

*Overall Evaluation*

This document is in pretty good shape.  Unless other people find additional
issues, I expect that -07 will be ready for WGLC.

*Comments by Section*

*Abstract*

In the second sentence, suggest deleting "Version One" at the end.  As
written, this suggests that ULBs are not or might not be required for
future transport versions.

*1.  Introduction*

The current first paragraph is misplaced.  I think it derives from rfc5667
where this was essentially the only function of ULBs and there were no
explicit ULB requirements.  I suggest replacing this paragraph by the
following two paragraphs but putting this material after the end of the
current material in this section:

The RPC-over-RDMA Version One transport may employ direct data
placement to convey data payloads associated with RPC transactions. To
enable successful interoperation, RPC client and server
implementations must agree as to which XDR data items in what
particular RPC procedures are eligible for direct data placement
(DDP).  Specifying those data items is a major component of a
protocol's Upper Layer Binding.

In addition, Upper Layer Bindings are required to include additional
information to assure that adequate resources are allocated to receive
RPC replies, and for a number of other reasons.

Suggest replacing the first paragraph (currently the second) by the
following to make sure all ULBs in this document are mentioned:

This document contains material required of Upper Layer Bindings, as
specified in [I-D.ietf-nfsv4-rfc5666bis], for the NFS protocol
versions listed below.  In addition, bindings are provided, when
necessary, for auxiliary protocols used together with NFS versions 2
and 3.

*2.  Reply Size Estimation*

The first paragraph as written is clumsy.  Suggest rewriting it as follows:

When using an  RPC-over-RDMA Version One transport, a requester,
during the construction of each RPC Call message, is responsible for
allocating appropriate resources for receiving the corresponding Reply
message.

There are a number of problems with the second paragraph;

   - Other than because of an astonishingly poor implementation, I don't
   see how you can have "An overrun of these resources"  resulting in
   "corruption of the Reply message".
   - The second sentence as written, implies that these horrible
   consequences (corruption or disconnection) are the reason for reply size
   estimation.  In fact, the reason you need a big enough buffer for the reply
   is that if you didn't you couldn't get the generated reply back and that is
   sufficient reason to require reply size estimation.

Suggest rewriting the second paragraph as follows:

When insufficient resources are available, it may be impossible to
provide the reply corresponding to the call. Therefore. reliable reply
size estimation is necessary to ensure successful interoperation.
This is of particular concern, for example, when deciding whether to
allocate a reply Chunk and when deciding on the proper size for a
Reply chunk.

Suggest adding a new bullet, as follows, following the current second:

The client specifies a reply size limit for the particular reply or
operation, as it does by setting the count field of READDIR request.

*2.1. Short Reply Chunk Retry*

A problem with this section as written is that this problem can arise even
if no reply chunk has been allocated. For example if the estimate is 900
bytes, the inline threshold is 1K, and the reply is 1028 bytes.

I don't see the point of the fourth (unbulleted) paragraph. The previous
paragraph and the associated bullets have already given the client two
choices of which he MAY choose one. Given that this is a standards track
document, that is all that is required. The last paragraph in this section
specifies that this technique is limited in scope. I don't see the need for
the editorializing here. Suggest deleting this paragraph.

*3.  Upper Layer Binding for NFS Versions 2 and 3*

The penultimate paragraph in this section is new in this version and I
don't understand the need for a new version 2/3 restriction so late in
the game.  If this is not a new restriction, the intent would be
clearer if it was rewritten as follows:

The structure of the NFS version 2 and  3 protocols and limitations on
DDP-eligible data items given above are such that:

   - It is impossible for a Legacy  NFS client to validly send a
reduced Payload stream in a Long Call.
   - It is impossible for a Legacy  NFS client to validly enable a
Legacy NFS server to send a reduced Payload stream in a Long Reply.

*4.1.  MOUNT, NLM, and NSM Protocols*

The second sentence of the last paragraph reads as follows: "In most
implementations this data item is never larger than 1024 bytes, making
reliable reply size estimation straightforward using the criteria
outlined in Section 2".  It appears that  the qualifying phrase "In
most implementations" has been ignored., leading to an incorrect
conclusion. This section needs to be revised to reflect the fact that
the client has no way to know whether the server is one within the set
of "most implementations" or one outside it.  Even if all existing
implementations acted within this limit, there is no way to prevent
new ones from being generated

*5.2.1.  Reply Size Estimation for Minor Version 0*

With regard to the second sentence of the second paragraph the
introductory phrase  "As discussed in Section 2," is incorrect since
the referenced material does not appear in section 2.

With regard to the first sentence of the last paragraph, WRITE is not
actually non-idempotent.  Better examples would be RENAME or REMOVE.

*5.4.2.  Multiple DDP-eligible Data Items*

With regard to the last sentence of the second paragraph, it is not
clear what within the following material are "additional
restrictions".  I can't see how all of it can so characterized.  This
needs to be clarified.

*5.5.2.  NFS Version 4.1 Callback*

With regard to the second sentence of the first paragraph, it is not
clear which particular "mechanism" within the bidirection document is
being referred to.  Suggest replacing the word by  "mechanisms" or
"approach".

*5.7.1.  Congestion Avoidance*

Given that the section about RFC56661 is referencing a different
document and is about a different protocol, suggest replacing
"further" by "also".

---

From: Chuck Lever <[email-masked]>
Date: Sat, 25 Feb 2017 09:48:30 -0800

> On Feb 25, 2017, at 4:57 AM, David Noveck <[email-masked]> wrote:
> 
> 3.  Upper Layer Binding for NFS Versions 2 and 3
> The penultimate paragraph in this section is new in this version and I don't understand the need for a new version 2/3 restriction so late in the game.

RFC 5667 Section 4 says:

> Similarly, a single RDMA Read list entry MAY be posted by the client
> to supply the opaque file data for a WRITE request or the pathname
> for a SYMLINK request.  The server MUST ignore any Read list for
> other NFS procedures, as well as additional Read list entries beyond
> the first in the list.

I take "Read list entry" to mean Read chunk, composed of
multiple list entries that share the same XDR position.
This comports with similar language describing Write
chunks where a single list entry is indeed allowed to
have multiple segments.

However, the original intent might have been "single
Read segment". In that case, this language clearly states
that Legacy clients can send only one contiguous memory
region in Legacy NFS READ and SYMLINK requests.

At your behest I replaced the RFC 5667 language with
just the definitions of which XDR data items are DDP
eligible, and removed the discussion about Read list
entries.

This new language permits a Legacy client to reduce the
Payload stream of an NFS READ or SYMLINK request and put
that stream into a Position-Zero Read chunk. The client
would then send the PZRC and the non-zero position chunk:
two distinct Read chunks, which is not allowed by the
RFC 5667 language discussing NFS READ or SYMLINK.

To remain interoperable with RFC 5667-compliant
implementations, only one Read chunk is allowed, which
excludes the use of reduced Payload streams in a PZRC,
even though RPC-over-RDMA Version One allows this
construction.

There isn't a need to support sending a reduced Payload
stream this way in these NFS versions, so I added the new
paragraph to forbid it explicitly. This should now have
the same basic meaning as Section 4 of RFC 5667.

RFC 5667 does not restrict sending a Write chunk and a
Reply chunk together. However IIRC current implementations
do not support this (at least with NFSv2 and NFSv3) and
there shouldn't ever be a need to for these NFS versions.

> If this is not a new restriction, the intent would be clearer if it was rewritten as follows:
> The structure of the NFS version 2 and  3 protocols and limitations on DDP-eligible data items given above are such that:
> 	• It is impossible for a Legacy  NFS client to validly send a reduced Payload stream in a Long Call.  
> 	• It is impossible for a Legacy  NFS client to validly enable a Legacy NFS server to send a reduced Payload stream in a Long Reply.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Sat, 25 Feb 2017 18:54:22 -0500

> > for a SYMLINK request.

Part of the problem here is that, as you discuss later, this statement is
ambiguous, as the meaning of "read list entry" is not clear.

> > The server MUST ignore any Read list for
> > other NFS procedures,

As I understand it, this statement cannot apply to PZRCs, and rfc5666bis
has already dealt with that issue.  So, if one tried to maintain this
paragraph,
in something like the RFC5667-form, some modification would have been
necessary to avoid essentially preventing any use of PZRCs

> > as well as additional Read list entries beyond
> > the first in the list.

> chunks where a single list entry is indeed allowed to

Makes sense to me.

> Read segment".

It might have been but there is no way to be sure.  In some
cases, one might look to how existing implementations behave,
to come up with an interpretation even though it might not
have been what the author intended.

> In that case, this language clearly states
> region

I think you mean that,  if that were the case, this language would
mean that.

> in Legacy NFS READ and SYMLINK requests.

I think we are talking about WRITE and SYMLINK, given
that read chunks are mentioned.

While I can easily believe that existing implementations
limit SYMLINKs to a single contiguous buffer, I have trouble
believing the same about WRITEs.

> eligible, and removed the discussion about Read list

One benefit of that is that we now know what the spec says, while,
as with the section 4 text you quoted, we weren't.

> RFC 5667 language discussing NFS READ or SYMLINK.

It is not clear that section 4 would disallow that.  Beyond the
uncertainty about what "read list entry" means and a possible need
to treat PZRCs separately as done in rfc5666bis, the issue is that
you are only supposed to ignore "additional Read list entries beyond
the first in the list."  To me, this would allow a PZRC, if it is the first
in the
list, which it might well be.

Anyway, your view and my view of this obscure text doesn't really
count.  We have to work off the behavior of implementations.  If existing
implementations did or didn't do something because of this presumed
restriction, that would provide an answer.  Unfortunately, I don't think
this
provides an answer either,

> even though RPC-over-RDMA Version One allows this

True but it is not clear to me me how this situation could ever
arise given:

   - The XDR of legacy NFS protocols
   - The definition of what is DDP-eligible in the ULB

> paragraph to forbid it explicitly.

If there is no way for this situation to arise, there is no need to
forbid it explicitly.

> This should now have

As the meaning of that section is so unclear, I can't
be sure.

On Sat, Feb 25, 2017 at 12:48 PM, Chuck Lever <[email-masked]>
wrote:

> don't understand the need for a new version 2/3 restriction so late in the
> game.
>
> RFC 5667 Section 4 says:
>
> >       • It is impossible for a Legacy  NFS client to validly enable a

---

From: Chuck Lever <[email-masked]>
Date: Sun, 26 Feb 2017 12:29:18 -0800

> On Feb 25, 2017, at 3:54 PM, David Noveck <[email-masked]> wrote:
> 
> > RFC 5667 Section 4 says:
> 
> has already dealt with that issue.  So, if one tried to maintain this paragraph,
> It might have been but there is no way to be sure.

We can ask Tom Talpey. If he does not recall, then
we have no way to be sure.

> In some

All server implementations I'm aware of allow WRITE
payloads with multiple segments. That suggests the
former interpretation is correct.

> the first in the list."  To me, this would allow a PZRC, if it is the first in the

A PZRC is allowed. A non-zero position Read chunk is
allowed. What might not be allowed is both at the
same time.

> restriction, that would provide an answer.  Unfortunately, I don't think this

At least Linux does not currently support a reduced
Payload stream in a PZRC. Solaris is not likely to
either.

Either the original implementers interpreted the
original specification as not allowing this
construction, or so far there hasn't been a need
to support it.

> 	• The XDR of legacy NFS protocols
> 	• The definition of what is DDP-eligible in the ULB

The situation MAY arise because it is not specifically
excluded by the new language.

Unless the Transport header is exceptionally large
or there is exceptional authentication material,
a reduced NFS WRITE or SYMLINK can always fit inline.

However, a client MAY still choose to reduce the
Payload stream and send it in a PZRC, even if it
could be sent inline. There is nothing that says
an implementation cannot use RDMA_NOMSG for all
transactions.

The point here is that if an NFSv3 WRITE is large,
there are only two legal ways to send it:

 - Reduce the payload into a Read chunk and send
the rest inline

 - Send the whole request in a PZRC (RDMA_NOMSG)

> If there is no way for this situation to arise, there is no need to

There is a need to forbid it explicitly:

- The new language definitely does not forbid this
construction.

- It is possible a client might use this
construction for reasons we can't predict.

- As far as I know the extant implementations do
not support this construction, and we have to
preserve interoperation with them.

That reasoning could also apply in the NFSv4
case, where it is almost a certainty that typical
NFSv4 WRITE requests can be larger than the
inline threshold.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Sun, 26 Feb 2017 19:54:38 -0500

>  As far as I know the extant implementations do

I can see that the relevant factor is the need
to preserve interoperability, which is just as well,
since, whenever we try to interpret what RFC5667
really meant, my head starts to hurt.

If the reason for forbidding something is driven by
interoperability concerns, it would make the spec
more clear if this were said explicitly

> NFSv4 WRITE requests can be larger than the

Perhaps so but note that section 5.4.1 of -06 says:

Therefore, an NFS version 4 client MAY send a reduced Payload stream
in a Long Call.  An NFS version 4 client MAY enable an NFS version 4
server to send a reduced Payload stream in a Long Reply.

This wasn't in -05. It doesn't seem to be needed, but if it is wrong,
simply deleting it would not be enough. You might have to put in
something forbidding it, as is done in the legacy case.

---

From: Tom Talpey <[email-masked]>
Date: Mon, 27 Feb 2017 08:16:57 -0500

> we have no way to be sure.

I agree the paragraph in question could have been more clear. I'll
hazard a guess that it should have been written as "Read list" instead
of "Read list entry", meaning, an entire scatter list is provided.
This woud certainly match the semantic for the result of an ordinary
NFS Read.

I will also observe that the statement is a MAY. That is, it prescribes
no behavior, and offers a choice to the implementer. It does not rule
out the option of posting a list.

I think you guys need to stop worrying about writing these "rules"
down so literally. The only goal of RFC5667 was to isolate the tidbits
of NFS behaviors separate from the core rpcrdma transport. The
document makes relatively few MUST requirements.

Tom.

---

From: Chuck Lever <[email-masked]>
Date: Mon, 27 Feb 2017 08:33:00 -0800

> On Feb 26, 2017, at 4:54 PM, David Noveck <[email-masked]> wrote:
> 
> Therefore, an NFS version 4 client MAY send a reduced Payload stream in a Long Call.  An NFS version 4 client MAY enable an NFS version 4 server to send a reduced Payload stream in a Long Reply.

Earlier I had believed we had some historical reasons
for matching the intent of RFC 5667. But if we are
interested only in documenting the behavior of
existing implementations, note that:

- None of them have support for more than one chunk
in a Read or Write list

- None of them support PZRC plus a non-zero Read
chunk

- None of them support multiple Write chunks

- None of them support multiple NFS READ or WRITE
requests per COMPOUND 

This could be an argument to, in addition, remove a 
large piece of text that discusses multiple Write chunks.
However, it might be interesting to some to leave
some flexibility for future use.

I would like to hear opinions about this.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Mon, 27 Feb 2017 11:38:12 -0500

One of the issues that Chuck has to deal with is the need
to make new implementations interoperable with existing
implementation.  That has been the source of some new
MUSTs.

Regarding the use of MUSTs, I think these terms are overused
in general and that RFC2119's suggestion that these be used
sparingly (which ironically uses a "MUST") is too often ignored,
including by  the IESG.

Regarding your suggestion that 5667bis is using "MUST" too
much, a comparison with RFC5667 is instructive.  Including
"MUST NOT"s, the RFC2119 term "MUST" Is  used:

   - 22 times in RFC5667, a 10-page spec.
   - 14 times in rf5667bis-06, an 18-page spec.

Part of Chuck's advantage here is that he deleted a lot of the
duplication in which RFC5667 ether repeated what was in
RFC5666 specialized to NFS (useless) or contradicted it (which
really had to go).

On Mon, Feb 27, 2017 at 8:16 AM, Tom Talpey <[email-masked]> wrote:

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

---

From: David Noveck <[email-masked]>
Date: Mon, 27 Feb 2017 21:09:16 -0500

> for matching the intent of RFC 5667.

I think we do, where that intent is clear.

> But if we are
> existing implementations,

For some of the cases you mention, there is no possible
ambiguity.  The clear intent of RFC5666 and RFC5667
is that multiple READs and WRITEs were supposed to be
allowed and considerable effort was expended in explaining
how they would work.

So it appears that some the things on your liist are bugs.  We
may be forced, for practical reasons, to turn a blind eye to these
bugs for some period of time, but this is not the same sort of
situation as the PZRC-read-chunk issue which is a fairly minor nit
based on text with rfc56667 that defies clear interpretation.  Sigh!

> note that:

interesting/depressing list deleted

> large piece of text that discusses multiple Write chunks.

Perhaps it could.

> some flexibility for future use.

Do you mean future use within Version One?  Or we
could limit Version One to what it is now., in the expectation
that it will not exist for very long. Then we
could use base Version Two to implement what
Version One was supposed to be and get a small set
of performance goodies at the same time.

If we on;t want to write off Version One in that way,
we need a reliable means of distinguishing servers
with full COMPOUND support  from those that you
describe.

For example, when you say a server only supports a single READ
operation, what does it do if it gets a COMPOUND with more than
one?  We could have something to work with if:

   - All such servers did the same thing.
   - It didn't result in disconnection or memory corruption.

I thought about an extension adding a full_rdma_compound_support
attribute but that doesn't work for V4.0.

BTW, do any of these old-fashioned servers with these bugs, recognize
and report DDP-eligibility violations?  If not this could be a fool proof
way
to distinguish servers who may have these implememtation gaps from newer
ones that should have full COMPOUND support for RDMA.

---

From: Chuck Lever <[email-masked]>
Date: Tue, 28 Feb 2017 09:46:58 -0800

> On Feb 27, 2017, at 6:09 PM, David Noveck <[email-masked]> wrote:
> 

If you're referring to handling multiple Read or
Write chunks per RPC, there is no clear intent.
That text in RFC 5667 is clearly unfinished, and
its state is a major reason the WG embarked on
this update.

> based on text with rfc56667 that defies clear interpretation.  Sigh!

Again, the WG still has one author of that document
who is available to us to confirm the intent of
that text.

The problem is existing implementations do not
match the capabilities described in RFC 5667.

> interesting/depressing list deleted

Not depressing. Basically this list means that the
world has survived adequately with incomplete
implementations of RPC-over-RDMA and NFSv4 for quite
some time. There has been no need to support multiple
NFSv4 READ and WRITE operations in a COMPOUND on RDMA
or on any other transport.

The issue is whether the WG believes rectifying that
situation is an immediate need, or something that
would be nice to allow for the future, or something
that is not necessary for the remaining future of
RPC-over-RDMA Version One.

> that it will not exist for very long.

Again I am at odds with your view. Version One
has plenty of capability to last for quite a
while, and Version Two is years away from
appearing in storage products in a robust form.

> Then we 

> 	• All such servers did the same thing.
> 	• It didn't result in disconnection or memory corruption.
> and report DDP-eligibility violations?  If not this could be a fool proof way

Given the experience we had trying to detect
the completeness of protocol features with
RPC-over-RDMA, I really don't relish going
down that path.

As far as I am aware, no client I'm aware of
sends such COMPOUNDs. There is some support in
the Linux NFS server to handle such COMPOUNDS,
but as you might guess, there's been no testing
of this facility with real clients.

> 	• 22 times in RFC5667, a 10-page spec.
> 	• 14 times in rf5667bis-06, an 18-page spec.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Tue, 28 Feb 2017 15:41:03 -0500

> who is available to us to confirm the intent of

Tom could weigh in here, but this is text that was written over ten
years ago.  I'm sure Tom would have interesting things to say about this,
but, if he is anything like me, he wouldn't remember his precise intent
in writing any particular sentence.  He would look at the same evidence
as we have available to us, and draw conclusions about intent in the
same way we would, albeit with more knowledge of the circumstances
of composition.

In particular, I expect  he would look at the following facts in determining
intent, just as you or I might:

   - That there a reference to a write list and that the XDR thus is
   specifically designed to accommodate a variable number of items, rather
   being an optional item (a max-length-one variable-length array)..  True,
   this is in RFC5666, but given the centrality of NFS to this effort, it is
   hard to make the case that this was intended to support protocols other
   than NFS.
   - That the fourth paragraph of Section 5 of RFC5667 specifies a matching
   procedure which would make no sense if the intent was other than to support
   multiple write chunks.
   - That section 5 has an example include a write list of three elements.

Presumably those items and similar evidence led you to write section 5,4,2
in rfc5667bis.  I will listen to what Tom has to say, but it would be very
hard for him to convince me that you somehow misunderstood the intent of
RFCs 5666 an 5667.

> while,

There is no point in us in arguing "very long" vs. "quite a while".
Regardless of how long one expects Version One to be useful,  the question
is whether we anticipate sticking strictly with the capabilities of the
existing servers  or provide some way to provide within the scope of
Version One support for the features that I believe to have been intended
in RFC 5666 and 5667.

I'm not sure where you stand on this issue.  On the one hand, you say,
"However, it might be interesting to some to leave some flexibility for
future use".  Given your preference for the per-version ULB model, I
assumed that meant we would have to have some way of distinguishing servers
that provided the current level of support from other servers.  I made some
suggestions in that regard which you didn't like.  So the question is
whether you have some other way of doing this in mind or you whether you
have decided that these limitations will endure for  the entirety of
Version One, however one might characterize its duration.

> and Version Two is years away from

Probably so, but to me, the fact that something is going to take a
while to do makes it more appropriate to push forward, rather than
less.  And whatever you think about Version One,, it lacks:

   - General remote invalidation support
   - A default inline threshold size that is appropriate to a protocol that
   has neither remote invalidation support nor message continuation.
   - The ability to decide on and use a threshold bigger than the default.