Subject: [nfsv4] Tsvart early review of draft-ietf-nfsv4-rpcrdma-version-two-06

From: Jana Iyengar via Datatracker <[email-masked]>
Date: Sun, 13 Mar 2022 16:37:57 -0700

Reviewer: Jana Iyengar
Review result: On the Right Track

Chuck, all,

Thanks much for the updates to the draft to reflect a working flow control
mechanism. I think the spirit is correct, but the description is unclear, my
comments below are an attempt to clarify the text. Otherwise, this is almost
there! Thanks and apologies for the delay in reviewing.

Comments:

1/ “A peer tracks a few critical values for each connection.” —> “A sender
tracks a few critical values for each connection.”

2/ For each of the variables described, replace “peer” with “sender” or
receiver” as appropriate. Flow control functions require actions at the send
side and at the receive side, and articulating which side is maintaining a
variable or performing an action is very important to not introduce ambiguity
and potential deadlocks.

3/ “Remote credits” —> I would suggest changing this to “Advertised Credits”,
to avoid confusion about which endpoint uses this.

4/ “The sender MUST NOT post this message if the sender's "Send message
counter" is greater than the current "Remote credits" value.” —> Two issues:
(i) What is the “send message counter”? Is this supposed to be “Sent message
count”? Or a new variable? (ii) each endpoint has its “Remote credits” value,
which one does this sentence refer to?

5/ “A receiver MAY adjust its credit limit” —> “A receiver MAY adjust its
advertised credit limit”

6/ “For instance, a peer may” —> “For instance, a receiver may”

- jana

---

From: Chuck Lever III <[email-masked]>
Date: Mon, 14 Mar 2022 20:01:41 +0000

Hi Jana-

> On Mar 13, 2022, at 7:37 PM, Jana Iyengar via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Jana Iyengar

Thanks for your comments!

> tracks a few critical values for each connection.”

Each peer acts as both a sender and receiver, because
the credit limit values are piggybacked on messages
coming in the other direction. So the original statement,
though generic, seems correct as it stands.

However, coming back to this section after some time away,
I agree that it is not entirely clear.

> variable or performing an action is very important to not introduce ambiguity

With a few minor textual corrections it seems like the
intent is now unambiguous, based on the RDMA operation
being counted:

- The peer posts an RDMA Send: the peer is acting as a sender

- An RDMA Receive completes: the peer is acting as a receiver

But looks like the text needs to tie these counters to a
particular connection. They are not peer-wide counters.

> to avoid confusion about which endpoint uses this.

Below you used "Advertised credits" to mean a peer's "Credit limit"
which is not the same as "Remote credits". Therefore...

I will replace "Credit limit" with "Advertised credits" and
"Remote credits" with "Received credits". Hopefully that will
avoid the confusion you pointed out.

> count”? Or a new variable?

That's a typo. "Sent message counter" was intended. Fixed.

> (ii) each endpoint has its “Remote credits” value,

The local peer's "Remote credits" value, which is defined as the
most recent "Credit limit" received from the remote peer.

The text assumes that the peer makes sending decisions based only
on information that is local to it (the four critical values).

> 5/ “A receiver MAY adjust its credit limit” —> “A receiver MAY adjust its

The peer adjusts its "Credit limit" value in this case, not its
"Received credits". Adjusted to use the new term.

> 6/ “For instance, a peer may” —> “For instance, a receiver may”

Again, the peer is acting in both capacities. It adjusts its
"Credit limit" here, meaning it is changing the value it advertises
to the other peer. I'm adjusted this text slightly to help clarify.

So, "peer" seems unambiguous to me, and "sender/receiver" seems
more confusing rather than less. It's just ones and zeros, though,
so please let me know if I've made things worse.

https://chucklever.github.io/i-d-rpcrdma-version-two/draft-ietf-nfsv4-rpcrdma-version-two.html#name-reliable-message-delivery

--
Chuck Lever