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

[Chuck Lever III <[e-mail]>]

Hi Jana-

> On Mar 13, 2022, at 7:37 PM, Jana Iyengar via Datatracker <[e-mail]> wrote:
> 
> 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.

Thanks for your comments!


> Comments:
> 
> 1/ “A peer tracks a few critical values for each connection.” —> “A sender
> 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.


> 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.

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.


> 3/ “Remote credits” —> I would suggest changing this to “Advertised Credits”,
> 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.


> 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?

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


> (ii) each endpoint has its “Remote credits” value,
> which one does this sentence refer to?

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
> advertised credit limit”

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