Subject: Re: [nfsv4] rfc5666bis-03.txt extensibility

From: Chuck Lever <[email-masked]>
Date: Mon, 1 Feb 2016 12:13:40 -0500

Hi Bill-

> On Jan 31, 2016, at 5:56 PM, William Allen Simpson <[email-masked]> wrote:
> 
> This was extensively re-written, and I didn't see it until now.
> 
> 
> 9.1.  Extending RPC-over-RDMA Header XDR
> 
>   The first four fields in the RPC-over-RDMA header must remain aligned
>   at the same fixed offsets for all versions of the RPC-over-RDMA
>   protocol.  The version number must be in a fixed place in order for
>   version mismatches to be detected.  For version mismatches to be
>   reported in a fashion that all future version implementations can
>   reliably decode, the rdma_proc field must be in a fixed place, the
>   value of RDMA_ERR_VERS must always remain the same, and the field
>   placement of the RDMA_ERR_VERS arm of the rpcrdma1_error union must
>   always remain the same.
> 
> So far, so good!
> 
> 
>   Given these constraints, one way to extend RPC-over-RDMA is to add
>   new values to the rdma_proc enumerated type and new components (arms)
>   to the rpcrdma1_body union.  New argument and result types may be
>   introduced for each new procedure defined this way.  These extensions
>   would be specified by new Internet Drafts with appropriate Working
>   Group and IESG review to ensure continued interoperation with
>   existing implementations.
> 
> This is a bit of a problem.  The recent change introduced:

>      union rpcrdma1_body switch (rpcrdma1_proc rdma_proc) {
>              case RDMA_MSG:
>              case RDMA_NOMSG:
>                rpcrdma1_chunks rdma_chunks;
>              case RDMA_MSGP:
>                uint32          rdma_align;
>                uint32          rdma_thresh;
>                rpcrdma1_chunks rdma_achunks;
>              case RDMA_DONE:
>                void;
>              case RDMA_ERROR:
>                rpcrdma1_error rdma_error;
>      };
> 
> First of all, RDMA_MSGP and RDMA_DONE are now reserved, and
> their old definitions should be removed.

These can be removed as part of Noveck's suggested XDR
rewrite (separate e-mail thread).

> Secondly, having the union body may be syntactically correct,
> but extensions really need the rpcrdma1_proc field to stand
> alone.  The above means the body has to be duplicated in
> each extension document.  As soon as we have two extensions,
> they will conflict and won't compile.

There's no compilation problem.

The actual problem is that this union would have to be
copied to the XDR definition in all specifications that
create Version One extensions. The copies of the union
would need to contain all previously specified extensions.
Therefore the extension documents would have to be
ordered and would depend on each other.

We'd like extensions to each stand by themselves, and
not to update rfc5666bis.

It's purely a matter of how extensions are specified by
RFCs, not an operational issue.

>   XDR extensions may introduce only optional features to an existing
>   RPC-over-RDMA protocol version.  To detect when an optional rdma_proc
>   value is supported by a receiver, it is desirable to have a specific
>   value of the rdma_err field, say, RDMA_ERR_PROC, that indicates when
>   the receiver does not recognize an rdma_proc value.
> 
> It should be added in this document.  Obviously missing protocol
> errors should be fixed.

Sure, they should be fixed. The question is how.

No-one implements ERR_PROC today. The way we are extending
Version One, it would have to be an extension, not part of
the base protocol, I think. Thus we'd need to rely on
ERR_CHUNK at least to boot strap the presence of ERR_PROC.
I don't see that extra complexity adding a lot of value.

> Older implementations won't recognize the error, but (to quote
> section 5.5.  Error Handling,
> 
>   To avoid an infinite loop, a receiver should drop an RDMA_ERROR
>   procedure that is malformed.

This is not normative, it's advice to implementers. And it's
a "should" not a "must."

Therefore protocol interoperation cannot rely on it.

> We continue:
> 
>   In RPC-over-RDMA Version One, a receiver can indicate that it does
>   not recognize an rdma_proc enum value only by returning an RDMA_ERROR
>   procedure with the rdma_err field set to RDMA_ERR_CHUNK (see
>   Section 5.5.2).  This is indistinguishable from a situation where the
>   receiver does indeed support the procedure, but the XDR is malformed.
> 
> Correct.  Also, RDMA_ERR_CHUNK causes the connection to terminate.

There's no normative requirement in rfc5666bis to terminate
a connection after sending ERR_CHUNK.

If a requester has provided junk in the chunk lists, the
responder's NIC will very likely close the connection because
of an RDMA error. But sending a nonsense Upper Layer request
does not cause loss of connection as long as a responder is
able to see there is a problem before it posts any RDMA
operations.

>   To resolve this problem, an RPC-over-RDMA Version One sender uses the
>   following convention.  If the first time the sender uses an optional
>   rdma_proc value the receiver returns an RDMA_ERROR procedure with
>   RDMA_ERR_CHUNK in the rdma_err field, the sender simply marks that
>   feature as unsupported and does not send it again on the current
>   connection instance.  Subsequent to an initial successful result,
>   receiving RDMA_ERR_CHUNK retains its more relaxed meaning of "generic
>   XDR parsing error."
> 
> The threads could very well return out of order.  I've got no
> indicators about the order the requests originally arrived.

Message ordering works the same way it works for all other
RPC-over-RDMA messages.

An RDMA_ERROR procedure still has an XID in the RPC-over-RDMA
header. The receiver that generates the error copies the XID
field from the bad header to the header of the RDMA_ERROR
procedure. The peer that receives RDMA_ERROR matches that XID
to the XID of the request that is waiting for a reply.

There's a discussion of how this works in Section 5.5, just
before the text you quote above about avoiding infinite loops.

> Overloading this doesn't seem like a good idea.

>   To ensure backwards compatibility when such an extension mechanism is
>   in place, the value of RDMA_ERR_CHUNK must remain the same for all
>   versions of the RPC-over-RDMA protocol.
> 
> Agreed, but not for the reasons stated.

--
Chuck Lever

---

From: William Allen Simpson <[email-masked]>
Date: Mon, 1 Feb 2016 15:38:44 -0500

> rewrite (separate e-mail thread).
That would be good.

> ordered and would depend on each other.
That's the compilation problem.  An extension that only
implements one won't have the others.  If it has them and
they are ordered, then they aren't optional.

> not to update rfc5666bis.
I'm on board with this.

> I don't see that extra complexity adding a lot of value.
Nobody implements new semantics for ERR_CHUNK, either.

I don't see the new semantics for ERR_CHUNK as adding any value.

The simpler ERR_PROC adds less complexity.

It is more obvious and easier to debug than a mere semantic
change to an existing value.

> Therefore protocol interoperation cannot rely on it.
That language is new, not in 5666 itself.

So change our language.  We used MUST in other standards.  We
also use the term "silently discard" instead of "drop".  Let's
adopt the usual standardese:

RFC 1661 "MUST be silently discarded."

    silently discard
              The implementation discards the packet without further
              processing.  The implementation SHOULD provide the
              capability of logging the error, including the contents of
              the silently discarded packet, and SHOULD record the event
              in a statistics counter.

Yeah, that's my own language used in most of my RFCs.  But also
thousands of times in many other RFCs.

> a connection after sending ERR_CHUNK.
Harrumph.  Aha, I see that the termination is only by the receiver,
and we used "should" instead of "SHOULD".

    An RDMA_ERROR procedure indicates a permanent error.  When receiving
    an RDMA_ERROR procedure, a requester should attempt to terminate the
    RPC transaction if it recognizes the XID in the reply's rdma_xid
    field, and return an error to the application to prevent retrying the
    failed RPC transaction.

That should probably be a SHOULD.

This is also new language, not in 5666.

So I'd move it from here to 3 places:

The RDMA_ERR_VERS value indicates a permanent error.  A
requester MUST cease using the unrecognized version.
Further transactions on the same connection MUST use an
indicated version. If it
recognizes the XID in the RDMA_ERR_VERS rdma_xid field, a
requester SHOULD
return an error to the application to prevent retrying the
failed RPC transaction.

The RDMA_ERR_CHUNK value indicates a permanent error. If it
recognizes the XID in the RDMA_ERR_CHUNK rdma_xid field, a
requester SHOULD attempt to terminate the RPC transaction and
return an error to the application to prevent retrying the
failed RPC transaction.

The RDMA_ERR_PROC value indicates a permanent error. A
requester MUST cease using the optional procedure. If it
recognizes the XID in the RDMA_ERR_PROC rdma_xid field, a
requester SHOULD attempt to
return an error to the application to prevent retrying the
failed RPC transaction.

(That is, only ERR_CHUNK should terminate.)

> able to see there is a problem before it posts any RDMA
Correct.  Agreed.

> before the text you quote above about avoiding infinite loops.
But that xid match isn't guaranteed to be ordered.

You could keep an ordered list of outstanding xids and decide
that it was the first that had an error.

But this complication could be avoided by using ERR_PROC.

---

From: Chuck Lever <[email-masked]>
Date: Mon, 1 Feb 2016 19:17:09 -0500

> On Feb 1, 2016, at 3:38 PM, William Allen Simpson <[email-masked]> wrote:
> 

The current meaning of ERR_CHUNK is not changing. It
means "I can't parse the XDR in the RPC-over-RDMA
header". That's exactly the meaning we need.

Unfortunately the "old" semantics aren't implemented
either, and RFC 5666 is ambiguous about what those
might be exactly.

Section 4.2 says:

"When other decoding errors are detected in the header
or chunks, either an RPC decode error MAY be returned
or the RPC/RDMA error code ERR_CHUNK MUST be returned."

My reading is that implementations can return either
RPC_GARBAGEARGS or ERR_CHUNK, but one of those MUST be
returned.

To send an RPC_GARBAGEARGS reply, the sender uses
RDMA_MSG, not RDMA_ERROR.

The current set of RPC-over-RDMA Version One servers
treat an unrecognized procedure as an RDMA_MSG.

Thus they are likely to interpret the content of an
optional procedure as junk, and return RPC_GARBAGEARGS
with an RDMA_MSG reply when presented with an optional
procedure. Even if they treated it as an RPC-over-RDMA
decoding error, they are still permitted to return
RPC_GARBAGEARGS.

RPC_GARBAGEARGS is a very small reply, so at least
the Linux RPC-over-RDMA client reply handler could be
changed to recognize that too when an optional
procedure is in play.

Or, our clients could get clever and poke a server
with an unsupported procedure type; and if it responds
with RPC_GARBAGEARGS, we know it is very old school.

> The simpler ERR_PROC adds less complexity.

Maybe, but it still has the same problem: legacy
servers treat unrecognized procedures as RDMA_MSG
and can reply with RPC_GARBAGEARGS.

Is it a stretch to add ERR_PROC on top of this mess?
That would have to be answered by those with more WG
history. I think there are extensibility concerns
around introducing another ERR_ value.

> change to an existing value.

The question is whether we need to add another RDMA_ERROR
error value, and whether it has to be done by violating
basic laws of interoperability. Convenience has to take
a back seat.

Remember we are not trying to create a generic extension
mechanism in Version One. Just something that is good
enough and that interoperates with current implementations.

> thousands of times in many other RFCs.

Current implementations probably don't discard illegal
ERR_ values, but they should. That's why this is
"should" and not MUST. It's just good advice.

The specification does not prevent an implementer from
choosing good behavior (discarding), and so far there
have been no consequences for any implementation that
has chosen not to discard malformed RDMA_ERROR
procedures.

It could be hardened to MUST. Does it need to be?

> That should probably be a SHOULD.

Not terminating has no interoperability consequence.
"should" is appropriate to use for implementation
advice. Also, see below regarding advice about RPC
application APIs.

> requester MUST cease using the unrecognized version.

Again, MUST is not needed for interoperation. There is
no consequence if the sender continues to pelt a receiver
with messages containing bad version numbers, and it's
already obvious that the sender will not make forward
progress in this case.

A receiver can choose to drop the connection in that
case to mitigate a DoS. But that's entirely up to
implementers, and the specification allows that
flexibility.

> indicated version.

Likewise, this MUST is unneeded. It is already required
that a receiver cannot respond to messages with
unsupported version numbers in any way but to return
ERR_VERS.

> If it

This document can say nothing normative about the API
presented to applications, so SHOULD is right out as it
applies to returning an application error.

> return an error to the application to prevent retrying the

Note that "terminate the RPC transaction" does not mean
"terminate the connection." Was that the original
conundrum?

> (That is, only ERR_CHUNK should terminate.)

The proposed replacement misuses RFC 2119 terms, and
we haven't agreed that ERR_PROC is the approach we
will use. I'm going to set this aside for the moment.

> But that xid match isn't guaranteed to be ordered.

If needed, a sender can issue an optional procedure
serially until it has received a non-error response.

Ordering does not apply to optional procedures that
are one-shot. So far that's been the only kind of
extension we've discussed.

The expectation here is that senders are constructed
to send only valid XDR, and optional procedures that
need to respond in a more complex fashion should
build in their own status codes.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Tue, 2 Feb 2016 05:45:52 -0500

> treat an unrecognized procedure as an RDMA_MSG.

OUCH!  I had managed to forget about this piece of legacy
server (mis)-behavior.

Such a server has a good chance of treating random data as a chunk,
making it possible that it will try to do some RDMA operation that does
cause the connection to drop.

If that doesn't happen, it would treat the random data as a reply or a
request.
I expect it doesn't do anything if it thinks it's a reply.  In the case in
which it
looks like a request, it is likely to run into an XDR decode error although
there is
an infinitesimal chance it will do something else.

It doesn't seem like we want to (or can safely) test such servers' support
of
extensions.  It seems better to identify such servers first and decide that
they
support  no extensions.

> with RPC_GARBAGEARGS, we know it is very old school.

Not sure of he relationship between "very old school" and "legacy" as I use
it
above.

In any case, it makes to use a reserved/unsupported header type to and to
format it
as if it were an RDMA_MSG sending an RPC NULL request.  Then,

   - If you get back a NULL response, the server is legacy.
   - If it responds with RPC_GARBAGEARGS the server is old school but not
   legacy.
   - Or if it responds RDMA_ERR_CHUNK (or any new code we decide on), we
   can rely on its error handling being new-style.

For servers identified as legacy, it makes sense to not bother testing
individually for extension
support and to treat every extension as unsupported by that server.

On Mon, Feb 1, 2016 at 7:17 PM, Chuck Lever <[email-masked]> wrote:

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

---

From: Chuck Lever <[email-masked]>
Date: Tue, 2 Feb 2016 10:55:25 -0500

> On Feb 2, 2016, at 5:45 AM, David Noveck <[email-masked]> wrote:
> 

Yes, but only if the header's chunk lists are populated.

> If that doesn't happen, it would treat the random data as a reply or a request.
> I expect it doesn't do anything if it thinks it's a reply.  In the case in which it 
> looks like a request, it is likely to run into an XDR decode error although there is
> It doesn't seem like we want to (or can safely) test such servers' support of 
> extensions.  It seems better to identify such servers first and decide that they 
> support  no extensions.

I agree.

> Not sure of he relationship between "very old school" and "legacy" as I use it
> above.  
> 
> In any case, it makes to use a reserved/unsupported header type to and to format it
> 	• If you get back a NULL response, the server is legacy.

Correct, it treated the new header type as RDMA_MSG.

> 	• If it responds with RPC_GARBAGEARGS the server is old school but not legacy.

I might lump that behavior in the same category as bullet one.
It's essentially a server that does not perform adequate checking
of RPC-over-RDMA header XDR, and thus cannot support extensions.

Both these cases use RDMA_MSG to send their response. It should
be trivial for the transport layer to identify such servers
without looking at the encapsulated RPC message.

Even if it turns out that peeking at the RPC message is necessary,
both a NULL reply and an RPC_GARBAGEARGS reply are short,
fixed-length messages.

> 	• Or if it responds RDMA_ERR_CHUNK (or any new code we decide on), we can rely on its error handling being new-style.

Yes. We know that particular header type is not recognized by
this server, and that it is safe to check for other optional
header types.

> For servers identified as legacy, it makes sense to not bother testing individually for extension 

I agree.

--
Chuck Lever

---

From: William Allen Simpson <[email-masked]>
Date: Tue, 2 Feb 2016 12:25:01 -0500

> treat an unrecognized procedure as an RDMA_MSG.
What-the-heck?!?!?  (Mine does not!)

That needs to be documented, even as incorrect.  Wow, that's
seriously borked.

Meanwhile, we are talking past each other on some points.

Because we are talking about extensibility, what I meant is
any implementation that implements _ADDITIONAL_ procedures
MUST implement ERR_PROC.

It's totally meaningless for those that don't implement
additional procedures.  They'll never see it, so there's
no expectation they'll implement it.

As to normative language, you're advocating a very limited
viewpoint.  We certainly can say SHOULD for an API.  That's
why we have SHOULD as well as MUST.  If there's a good
reason -- as in your API sucks -- then SHOULD is exactly the
correct language.

MUST means I don't care how badly your API sucks, go fix it.

I'll try to cover the long message later, after I've had a
chance to read the later messages in this thread....

---

From: Chuck Lever <[email-masked]>
Date: Tue, 2 Feb 2016 13:35:34 -0500

> On Feb 2, 2016, at 12:25 PM, William Allen Simpson <[email-masked]> wrote:
> 

If we were designing a protocol from scratch I
would have no objection.

We really really want to avoid adding new protocol
elements if that can be avoided. In this case, it
seems like it certainly can be.

If it's not avoidable, then the authors of
rfc5666bis and nfsv4 WG have to be prepared to
make valid technical arguments to anyone who might
object to a mid-stream change to XDR protocol
elements.

Is there a strong technical reason (like, an
existing conflicting use of ERR_CHUNK) that makes
an ERR_PROC necessary? Explain why ERR_CHUNK is
not adequate for this purpose.

Do any existing deployed implementations use
ERR_PROC? If not, it's something that is out of
scope for rfc5666bis, according to the many
previous protocol changes we've considered.

Do we need to have interoperating prototypes of
ERR_PROC before we can move rfc5666bis?

If we add this protocol element, what's to stop
us from adding more? I'd like to have a very good
reason, or maybe two, to set a precedent like this.

The bar for this kind of change should be high.

> reason -- as in your API sucks -- then SHOULD is exactly the

My comment about normative API guidance was not a
general statement. It referred only to RFC 5666 and
rfc5666bis.

These documents do not define or alter an API. If
they did, then they would use SHOULD and MUST and
specify exactly what error code is to be returned
from which function calls.

I prefer to remove the "return an error to the
application" language. That clause is just an aside,
and a vague one at that. There's no good way to
strengthen it without veering into API specifics.

Terminating the RPC transaction is clear enough,
and without the "return an error" clause I might
consider using SHOULD as now the text refers
strictly to the implementation of the RPC-over-RDMA
protocol.

--
Chuck Lever