Re: [nfsv4] AD review: draft-ietf-nfsv4-delstid-03

[Thomas Haynes <[e-mail]>]

The first pass was on Nov 7th, the second pass was today.

Note that during the first pass, I rearranged the sections as suggested at one point. As such, when asked where something is, I will either state the section name or point to the next version of the draft.


On Oct 5, 2023, at 3:43 AM, Zaheduzzaman Sarker <[e-mail]> wrote:

Hi all,

Now I have done my AD review of draft-ietf-nfsv4-delstid-03. I think this document needs some rewrites and clarifications before we can proceed further. See my comments below.

Comments :

# We need a section describing what are we updating in a nutshell. The way it is now written makes it is extremely difficult to understand what has been modified/amended/updated compared to RFC 8881.

# Please clearly mention that this specification will update RFC8881 in the abstract.

Does not this provide that?

  This document presents several extensions to RFC8881for both the opening and delegating of the file to the client.

Or do you want:

  This document updates RFC8881 by extending both the opening and delegating of the file to the client.


A broader issue I have with this (which I think you address later), is whether this document really updates RFC8881 in the sense that the updates tag In the <rfc> means?

PASS 2: I spoke with the RFC Editor and we agreed to the use of extends and not updates.



# Why are we defining delegation and stateid again here? What is the change in the definition that requires the redefinition ? Can't we use what is defined in RFC8881?

I agree with stateid.  I can’t find a short definition of delegation in Sec 1.7 of RFC8881.

My question is whether this document is supposed to stand on its own for legibility or assumes the reader has the time to go to RFC8881 to learn terms?

PASS 2: Will assume reader can go to RFC8881.



# Proxy definition : does this mean the client is proxying to itself? Can this proxy be somewhere in the network between the client and server. RFC8881 states server could also act as proxy hence this proxy definition needs to be clear about who can be proxy. I would also like to understand the difference between delegation and proxying.



The client is the proxy.

A delegation grants the client the right to be the proxy.

In terms of the definition I provided, the delegation establishes the authority to proxy.

PASS 2: Actually, in retrospect, I think we are trying to differentiate between delegation as defined in RFC 8881 and here. We could remove the term proxy altogether and state that we have modified what can cached on the client. However,  the key difference between delegate and proxy is that the delegated metadata is never passed back to metadata server and the proxied metadata is passed back.

# I would strongly suggest to define/describe "offline files". I have hard time understanding of a file being offline from whos perspective? Will a file be considered offline if the client for some reason cannot access the cloud storage or is this only about archived files?


Added a definition.


# Please explain or describe the code blocks, otherwise they seems to come out of nowhere and does not fit themselves well in the sections.

# I think we should gather all the new flags defined in one section or sub-section and describe them. This will also help to see what are we updating.


PASS 2: This is in contradiction as to how we have handled this in the past. We consistently put the new XDR in the section that defines it. If we want to put it all in one file, we provide a second document.

PASS 2: I’m not saying that change is bad or the good old days are best, but that when we embed the XDR in documents, we tend to “ignore” the XDR and allow the definition to occur within the context of the document.


# As we are defining new procedure and new flag, how are we handling the errors (we not have number of MUSTs, RECOMMENDs and SHOULDs)?


PASS 2: We are not determining a new procedure, but we are determining a new flag.

PASS 2: Also, the only possible errors are the server does not support the new functionality. That is however addressed in the document already:

    Since fattr4_open_arguments is a <bcp14>RECOMMENDED</bcp14> attribute, if the
    server informs the client that it does not support this new
    attribute, the client <bcp14>MUST</bcp14> take this to mean that
    the additional new <bcp14>OPTIONAL</bcp14> functionality to OPEN
    is also not supported.

PASS 2: Made a change:

    Since fattr4_open_arguments is a <bcp14>RECOMMENDED</bcp14> attribute, if the
    server informs the client via NFS4ERR_ATTRNOTSUPP that it does not support this new
    attribute, the client <bcp14>MUST</bcp14> take this to mean that
    the additional new <bcp14>OPTIONAL</bcp14> functionality to OPEN
    is also not supported.



# Section 3:
    - It says -
         "If the client gets both a open and a delegation stateid as part of the OPEN, then it has to return them both."
    Return to where?

PASS 2: The server - changed,,,,


    - Where is OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION and OPEN4_RESULT_NO_OPEN_STATEID  delegation flags defined ?


PASS 2: In the section on supported open arguments. These are now defined before use….




# Section 3.1:
   - Put a reference to the "CLOSE operation".

Updated

   - Where is OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION flag defined?
   - What is DELEGRETURN? yes, refer to RFC8881.

Updated


# Section 4:
   - I am not sure I understand the statement - "While it is the authority for these values, it has no way to transfer these values to the server when the delegation is being returned and the server reclaims its authority with regard to these values."
     I thought DELEGRETURN is a way to transfer or return those values. RFC 8881 says - "Delegations may be returned voluntarily (i.e., before the server has recalled them) or when recalled. In either case, the client must properly propagate state changed under the context of the delegation to the server before returning the delegation". Can we please explain it in a better way, like what values we are taking about and why they cant be transferred?.


PASS 2: You are correct in that we could send a compound with SEQ, PUTFH, SETATTR (time_modify | time_access), DELEGRETURN to achieve a similar effect.

PASS 2:  Wow, I had to really dig back to  figure out why we wanted this distinction. It turns out that if we do a SETATTR with either time_modify_set or time_access_set, then we haver to change both the change and time_metadata attributes, which will cause clients to believe that there had been a modification to the file.

Thank you very much for this comment in your review!


    - "These new attributes are invalid GETATTR, VERIFY, and NVERIFY" - there is something missing in this statement. "invalid to be used with" ?


PASS 2: I independently went with “in’, but I like your suggestion better.


    - Please describe/explain/reference - "old times", "past the current time", and "original time". also put references for "access time", "change time", "modify time".

PASS 2: I can see the confusion and I’ve tried to clarify this, but I suspect we will need to go back and forth on it.

PASS 2: I now have:

    When the client presents either
    FATTR4_TIME_DELEG_ACCESS or FATTR4_TIME_DELEG_MODIFY attributes
    to the server, the server <bcp14>MUST</bcp14> decide for both of
    them whether the time presented is before the corresponding
    FATTR4_TIME_ACCESS or FATTR4_TIME_MODIFY attribute
    on the file or past the current server time.
    When the time presented is before the original time, then the update
    is ignored. When the time presented is in the future, the server
    can either clamp the new time to the current time, or it may return
    NFS4ERR_DELAY to the client, allowing it to retry.  Note that if the
    clock skew is large, the delay approach would result in access to
    the file being denied until the clock skew is exceeded.


    - Where is FATTR4_TIME_DELEG_ACCESS and FATTR4_TIME_DELEG_MODIFY defined?

PASS 2: Please verify I made a pointer on first refenence.

    - Give a ref to NFS4ERR_DELAY.

PASS 2: Done


# Section 4.1 seems to me a good motivation for this extension, may be we should uplift if to a separate motivation section.

# Section 4.2: If we want someone to review this document, how would one read this section? please describe the code blocks, what they do or mean or related to the sections where these blocks are described.

# May be section 5 is misplaced, may be we should have it before section 3 and 4, so we define them and then show how the are used. the alternative would be to refer them from section 3 and section 4.


I am fine with making section 5 the new Section 3.


# Section 5:
    - It says - " client MUST determine that the additional new OPTIONAL functionality to OPEN is also not supported". what is the procedure for the client to determine that?


I think this is lack of communication in the word “determine”.

My intent here is for: the client has to take this to mean that the additional new OPTIONAL functionality to OPEN is also not supported.

Which when normative: the client MUST take this to mean that the additional new OPTIONAL functionality to OPEN is also not supported.

I.e, the lack of support for fattr4_open_arguments determines that the client MUST not use the OPTIONAL functionality.

Updated


# Section 6:
    - It says - "While the XDR can be appended to that from [RFC7863<https://www.ietf.org/archive/id/draft-ietf-nfsv4-delstid-03.html#RFC7863>], the various code snippets belong in their respective areas of the that XDR.". Does this mean this document also should update RFC7863?



Thanks for this point.



# Section 7: in this specification we are extending some capabilities for client delegation. I think we need more convincing description that there is not security concerns here.



Tried to provide some convincing text.

Updated.



//Zahed