Early Review of draft-ietf-nfsv4-rpcrdma-version-two-05
review-ietf-nfsv4-rpcrdma-version-two-05-tsvart-early-iyengar-2021-11-02-00

I've read up through Section 4, specifically to understand and comment on the
Flow Control aspects of this draft, as requested by Chuck Lever.

As I understand it, FC works as follows:

- A receiver advertises the number of unacknowledged messages it is willing to
  receive (`credit window`), and also indicates the number of messages received so far
  (`received`). The underlying transport is assumed to provide ordered and
  reliable delivery.

- A sender is required to not send more than receiver-advertized `received +
  credit window` messages.

- These advertisements are typically piggy-backed on payload-bearing
  messages. Optionally, a receiver can send a message with a `Grants Credit`
  header type to advertise its window.

- The receiver can increase or decrease the advertised window.
    
- Each message has a max message size (`Inline Threshold`) which is indicated
  via `Transport Properties`, and defaults to 4096 bytes.

    
I have a few comments specific to Flow Control:

- The document allows for advertised window to increase or decrease, meaning
  that a receiver can renege on credit it advertises. Specifically, this allows
  a receiver to say it will accept a particular message and then reduces its
  window to cause that message to be now out of its window.

  This choice is rife with pain. I would urge, in the strongest manner possible,
  to avoid allowing reneging. That is, do not allow a receiver to arbitrarily
  reduce advertised credit so as to allow reneging. If you really really need
  it, prepare for some blood and sweat, and eventually, inevitable tears.

  To clarify, I do not mean that the window size must remain the same. A
  receiver can reduce its window size, but do it so that if a message was
  in-window before, it remains in-window after.

  For example, here is a sequence of FC messages that reduces the window and
  reneges on a previously allowed message (this is bad):

  received = 10, window = 5  (allowed = 11-15)
  received = 11, window = 2  (allowed = 12-13, reneged on 14, 15)

  And here is a sequence of FC messages that reduces the window without
  reneging (this is good):

  received = 10, window = 5  (allowed = 11-15)
  received = 11, window = 4  (allowed = 12-15)
  received = 12, window = 3  (allowed = 13-15)
    
- The current mechanism has a receiver advertising both `received` and `credit
  window` and the sender computes the limit. It is more direct for a receiver to
  simply advertise the limit -- basically the maximum message that the sender
  can send. This makes for fewer fields in the exchange, is more intuitive, and
  importantly, it is easier to describe a mechanism that avoids reneging
  (advertise the total message limit, and never advertise a lower number).

- Are the expected limits at a receiver in bytes or in operations? I imagine
  that the answer is bytes. If so, the FC mechanisms should be tied to this
  resource. At the moment, a receiver has to figure out what to advertise based
  on available memory and the maximum message size it advertised. Note that
  there will be inefficiency because messages are likely to be smaller than the
  max limit. It is likely to be more direct and efficient if the unit is all
  bytes.

- When are transport properties exchanged? FC credit needs to be available at
  the beginning of a connection for any progress to be made at all, so either
  both endpoints need to advertise it right at the start, or some default needs
  to be assumed. I don't see either in the document.

- Finally, `credit window` is a strange term. What the document currently uses
  is arguably a window, and the term `window` here applies to messages that can
  be received, not to flow control credit.

General comments (I skimmed the rest of the document):

- I understand the value of the XDR descriptions for the protocol, but I haven't
  encountered this in the past. Is this adequate for an IETF RFC? This is not a
  request to change anything, so perhaps all I'm looking for is that the ADs are
  ok with this.

- It would be useful to have forward references from the earlier sections to the
  later wire format and detailed error sections. For example, when a connection
  might be closed due to FC violations.

- I might recommend a little text in the intro section laying out the structure
  of the document.

- I see lowercase and uppercase MUSTs and SHOULDs. For clarity, consider
  rewriting the text to avoid the lowercase keywords.