Subject: [nfsv4] Barry Leiba's Discuss on draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: (with DISCUSS and COMMENT)

From: Barry Leiba via Datatracker <[email-masked]>
Date: Wed, 12 Feb 2020 21:45:34 -0800

Barry Leiba has entered the following ballot position for
draft-ietf-nfsv4-rpcrdma-cm-pvt-data-07: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-nfsv4-rpcrdma-cm-pvt-data/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Thanks for this document.  This is a simple DISCUSS point that should be very
easy to resolve:

— Section 5.2 —

   A sender computes the encoded
   value by dividing the buffer size, in octets, by 1024 and subtracting
   one from the result.

Is the buffer size necessarily a multiple of 1024?  If so, where is that
specified?  If not, what is the encoded value when the buffer size is, say,
2000?  Is it zero?  Or one?

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Some purely editorial comments:

— Abstract —
The abstract needs to stand alone, so you should expand the term RDMA-CM in the
abstract.  (RPC doesn’t need expanding, so once you’ve expanded RDMA-CM,
RPC-over-RDMA should be OK.)

— Introduction —
Please expand “XDR” on first use.

   Section 7 of the current document

“of this document” is better, I think.

— Section 3.2 —
Please expand “RNIC” and “STag”.

   invalidation without the need for additional protocol to be defined.

Either “an additional protocol” or “additional protocols”.

— Section 4.1 —

   Realizing these goals
   require that implementations of this extension follow the practices

The subject is “realizing”, which is singular.  So, “requires’.

— Section 5.1 —

   Bits 14 - 8:  These bits are reserved and are always zero when the
      Version field contains 1.

In other protocols, leaving it unspecified as to what happens if not all
reserved bits are zero has caused interoperability problems.  If you know
that’s not a concern here, that’s fine.  Otherwise, it might be good to say
explicitly that either they are ignored on receipt or non-zero bits result in
an error.

---

From: Chuck Lever <[email-masked]>
Date: Thu, 13 Feb 2020 09:24:09 -0500

Hi Barry-

> On Feb 13, 2020, at 12:45 AM, Barry Leiba via Datatracker <[email-masked]> wrote:
> 

Good catch! Buffer sizes are not constrained to 1024-byte length alignment.

Further, if a sender posts, say, a 2032-byte message to a receiver that uses
a 2000-byte buffer, a Receive error occurs that typically results in
connection loss.

IMO Section 5.2 should instruct the sender to round the actual buffer length
down to the nearest 1024-byte multiple before encoding. Would that clarify
the issue for you?

Agreed to all.

> explicitly that either they are ignored on receipt or non-zero bits result in

Gotcha. I can add a sentence that states "When the Version field contains 1,
receivers MUST ignore reserved bits".

I will address all these comments in a subsequent revision.

--
Chuck Lever

---

From: Barry Leiba <[email-masked]>
Date: Thu, 13 Feb 2020 09:30:44 -0500

Hi, Chuck, and thanks for the quick response.

> down to the nearest 1024-byte multiple before encoding. Would that clarify

Yes, that would be quite clear.

And thanks for addressing my other comments as well.

Barry

---

From: Chuck Lever <[email-masked]>
Date: Thu, 13 Feb 2020 10:02:05 -0500

> On Feb 13, 2020, at 9:30 AM, Barry Leiba <[email-masked]> wrote:
> 

Looking at Section 5.2 now, it appears that the encoding computation performs
the rounding implicitly. IMO instructing that the sender rounds down first
would be redundant. Here's what I've come up with instead:

   Inline threshold sizes from 1KB to 256KB can be represented in the
   Send Size and Receive Size fields.  These inline threshold values
   provide a pair of 1024-octet-aligned maximum message lengths that
   guarantee Send and Receive operations do not fail due to length
   errors.

   A sender computes the encoded value by dividing its actual buffer
   size, in octets, by 1024 and subtracting one from the result.  A
   receiver decodes this value by performing the inverse set of
   operations: it adds one to the encoded value and then multiplies that
   result by 1024.

   The client uses the smaller of its own send size and the server's
   reported receive size as the client-to-server inline threshold.  The
   server uses the smaller of its own send size and the clients's
   reported receive size as the server-to-client inline threshold.

Perhaps not as clear as one could hope. Comments/suggestions are welcome.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Thu, 13 Feb 2020 10:30:21 -0500

> And thanks for addressing my other comments as well.

Please see

https://chucklever.github.io/i-d-rpcrdma-cm-pvt-data/#go.draft-ietf-nfsv4-rpcrdma-cm-pvt-data.diff

for an rfcdiff of changes intended to address your comments.

--
Chuck Lever

---

From: Barry Leiba <[email-masked]>
Date: Thu, 13 Feb 2020 11:04:32 -0500

The diff looks good to me; thanks!

Barry