[nfsv4] AD Review of draft-ietf-nfsv4-rfc5666bis-08

[Spencer Dawkins at IETF <[e-mail]>]

Hi, Spencer (S, where the "S" stands for "Shepherd),

My apologies for a slow AD evaluation for this draft. I've been down with
something flu-ish since December 24, and am only now surfacing.

I found this specification to be pretty clear, but I did have some
questions and suggested wording changes that I'd like for people to
consider before I request Last Call.

Could you let me know when you think the document is good to go?

In the meantime, I'll do the AD Evaluation for
draft-ietf-nfsv4-rpcrdma-bidirection-05.

Thanks!

Spencer (D)

AD Evaluation follows ---

This could certainly be OK, but I note that RFC 5666 uses the pre-2008 text
IPR version, while the bis draft does not. Is that intentional? (If you
folks were able to obtain releases that allow you to use the default IPR
version, I applaud your initiative. It's never going to be easier to do
that, than now).

Just so that I understand what's being said, is

   This document obsoletes RFC 5666.  However, the protocol specified by
   this document is based on existing interoperating implementations of
   the RPC-over-RDMA Version One protocol.

saying

   This document obsoletes RFC 5666.  However, the protocol specified by
   this document is based on experience gained from existing
                             ^^^^^^^^^^^^^^^^^^^^^^
   interoperating implementations of the RPC-over-RDMA Version One protocol.

? (If so, again, I applaud that!)

I would be a bit more comfortable with

   Open Network Computing Remote Procedure Call (ONC RPC, or simply,
   RPC)

if the document said

   Open Network Computing Remote Procedure Call (ONC RPC, often shortened
   in this document as RPC)

(or, more broadly, "often shortened in NFSv4 documents", if that's
correct), because I'm thinking that "RPC" doesn't even refer to ONC RPC in
all IETF-stream RFCs.

Is this text

   Although the RDMA transport described herein can provide relatively
   transparent support for any RPC application, this document also
   describes mechanisms that can optimize data transfer even further,
   given more active participation by RPC applications.

saying

   Although the RDMA transport described herein can provide relatively
   transparent support for any RPC application, this document also
   describes mechanisms that can optimize data transfer even further,
   for RPC applications that are willing to exploit awareness of
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   RDMA transport.
   ^^^^^^^^^^^^^^

?

In this text,

   o  Section 2 has been expanded to introduce and explain key RPC, XDR,
      and RDMA terminology.  These terms are now used consistently
      throughout the specification.

I wonder if it's helpful to add a reference for XDR.

I found myself reading

   o  The specification of the optional Connection Configuration
      Protocol has been removed from the specification.

and wondering "where did it go, and why?". Is this going to appear in a
separate document, or was this so optional that it wasn't much implemented
or deployed? For most of the items in the bullet list in Section 2.1, I
don't think you need any explanation, but if you're removing something that
was in the obsoleted RFC, it might be nice to explain what's behind that.

In this text,

   o  A subsection describing the use of RPCSEC_GSS with RPC-over-RDMA
      Version One has been added.

it might be helpful to add a reference for RPCSEC_GSS.

In this text,

   o  Support for the Read-Read transfer model has been removed.  Read-
      Read is a slower transfer model than Read-Write.  As a result,
      implementers have chosen not to support it.  Removal simplifies
      explanatory text, and support for the RDMA_DONE procedure is no
      longer necessary.

"is no longer necessary" says, to me, that it can still be present, and
used. Is that about right, or is "the RDMA_DONE procedure is no longer
supported" more accurate? I'm just asking because the description of the
RDMA_MSGP message type in the next bullet sounds much blunter, and both of
those seem weaker than the descriptions in Section 5.6. If you're able to
use the same language in all these places, I'd be clearer on what your
intent is.

It seems to me that

   The protocol version number has not been changed because the protocol
   specified in this document fully interoperates with implementations
   of the RPC-over-RDMA Version One protocol specified in [RFC5666].

belongs a lot earlier in the document - at least in the Introduction, and
possibly the Abstract as well. It's nearly five pages into the current
draft.

I'm not clear on the relationship between RDMA and DDP, and I'm a bit
surprised that Section 3.2.1 is called Direct Data Placement, but most of
the contents are describing RDMA, only one paragraph mentions DDP, and that
paragraph says that RDMA isn't necessary or sufficient for DDP, unless I'm
misreading it. I suspect that my hangup is that it's not clear where RDMA
and DDP are moving the RPC message, but I'm not sure of that. Could you
take a look at this section and make sure it's as clear for an implementer
as it needs to be?

I'm noticing that "pre-posted" is used for the first time without a
reference or definition, and "pinned" is used only once, without a
reference or definition.

In this text,

    RDMA Read
       The RDMA provider supports an RDMA Read operation to directly
       place peer source data in the read initiator's memory.  The local
       host initiates an RDMA Read, and completion is signaled there; no
       completion is signaled on the remote.

is "remote" short for "remote peer"? And I'm wondering how this is
different from the next paragraph, which begins,

      The remote peer receives no notification of RDMA Read completion.

On

    [RFC5666] specifies the use of both the Read-Read and the Read-Write
    Transfer Model.  All current RPC-over-RDMA Version One
    implementations use only the Read-Write Transfer Model.  Therefore
    the use of the Read-Read Transfer Model within RPC-over-RDMA Version
    One implementations is no longer supported.  Transfer Models other
    than the Read-Write model may be used in future versions of RPC-over
       RDMA.

I'm still stumbling over my previous point about Read-Read, but this time,
it's "no longer supported", which seems different from "no longer
necessary" in my previous comment.

In this text,

  4.4.2.  DDP-Eligibility

    Only an XDR data item that might benefit from Direct Data Placement
    may be reduced.  The eligibility of particular XDR data items to be
    reduced is independent of RPC-over-RDMA, and thus is not specified by
    this document.

is there a pointer/reference you can give, for where this is specified? Is
this describing the same "not specified in this document" as the text in
5.4.2. "Communicating DDP-Eligibility"? I had the same question there.

For this text,

   Except in special cases, a chunk contains exactly one XDR data item.

is there a pointer/reference/clue you can provide about chunks with more
than one XDR data item?

I struggled with this text a bit (and similar text appears in Section
4.4.6.1. Write Chunk Round-up):

3.5.  XDR Decoding with Read Chunks
       XDR requires each encoded data item to start on four-byte alignment.
    When an odd-length data item is encoded, its length is encoded
    literally, while the data is padded so the next data item in the XDR
    stream can start on a four-byte boundary.

I would think "odd-length" covers one-byte and three-byte fields, but not
two-byte fields. If you mean "a data item with a length that's not a
multiple of four bytes", that would be more accurate.

I might have said "When an odd-length data item is encoded, its unpadded
length is encoded". I think I understand what you meant by "encoded
literally", but I was guessing.

Thanks for getting the code component licensing right (I think) - this is
my first RFC to shepherd with a code component, so I'm reading
https://trustee.ietf.org/license-info/IETF-TLP-4.htm in parallel.

In this text:

  The details of reporting and recovery from RDMA link layer errors are
    outside the scope of this protocol specification.

are these details described anywhere? If so, a reference would be nice.

My apologies for asking about text that's unchanged from RFC 5666, but I
wasn't sure what you meant here:

   RPC services normally register with a portmap or rpcbind [RFC1833]
   service, which associates an RPC Program number with a service
   address.  (In the case of UDP or TCP, the service address for NFS is
   normally port 2049.)  This policy is no different with RDMA
   transports, although it may require the allocation of port numbers
   appropriate to each Upper Layer Protocol that uses the RPC framing
   defined here.

Is this saying that you need multiple port numbers if you use multiple
ULPs, or that specific ULPs have different port conventions, or something
else? I'm guessing a couple of other possibilities, but I'm guessing.

It didn't jump out at me, that this paragraph was introducing two cases
that result in similar problems, until I read the rest of the section:

  A DDP-eligibility violation occurs when a requester forms a Call
    message with a non-DDP-eligible data item in a Read chunk.  A
    violation occurs when a responder forms a Reply message without
    reducing a DDP-eligible data item when there is a Write list provided
    by the requester.

I was at least partially confused because the first case is for a
"DDP-eligibility violation", and the second is for "a violation" - is the
second also for a "DDP-eligibility violation"?

If I'm getting that right, this text might be clearer as

  A DDP-eligibility violation occurs when a requester forms a Call
    message with a non-DDP-eligible data item in a Read chunk, or
    when a responder forms a Reply message without
    reducing a DDP-eligible data item when there is a Write list provided
    by the requester.

Nice work on Section 8. You may note that Mirja and I have been including
migration and coexistence discussions at TSVAREA, the past couple of IETFs.

In Section 8.1, I wonder if you'll get pushback on naming a theoretically
short-lived working group (yeah, I know the nfsv4 working group is older
than 1998, and that's version 4, for the oldest charter in the datatracker)
in a "forever RFC":

  Such extensions must be specified in a Standards Track document with
    appropriate review by the nfsv4 Working Group and the IESG.

Let's leave this for now, and see if anyone objects.