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

[Zaheduzzaman Sarker <[e-mail]>]

On Thu, Oct 5, 2023 at 1:16 PM David Noveck <[e-mail]> wrote:

>
>
> On Thu, Oct 5, 2023 at 6: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.
>>
>
> Since this is an extension to NFSv4.2,, the comparison should be to
> RFC7862/
>

The point here is to be clear about what we are doing.


>
>
>> # Please clearly mention that this specification will update RFC8881 in
>> the abstract.
>>
>
> It does not update really RFC8881, even though the header says it does.
> Although the title says "NFSv4.2", it does not need to update RFC7862
> either.  Please see RFC 8178  regarding this issue.
>
So this means this document should not really say it is updating any of
those RFCs, or?

>
>
>>
>> # 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?
>>
>> # 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.
>>
>> # 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?
>>
>> # 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.
>>
>> # As we are defining new procedure and new flag, how are we handling the
>> errors (we not have number of MUSTs, RECOMMENDs and SHOULDs)?
>>
>> # 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?
>>     - Where is OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION and
>> OPEN4_RESULT_NO_OPEN_STATEID  delegation flags defined ?
>>
>> # Section 3.1:
>>    - Put a reference to the "CLOSE operation".
>>    - Where is OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION flag
>> defined?
>>    - What is DELEGRETURN? yes, refer to RFC8881.
>>
>> # 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?.
>>     - "These new attributes are invalid GETATTR, VERIFY, and NVERIFY" -
>> there is something missing in this statement. "invalid to be used with" ?
>>     - Please describe/explain/reference - "old times", "past the current
>> time", and "original time". also put references for "access time", "change
>> time", "modify time".
>>     - Where is FATTR4_TIME_DELEG_ACCESS and FATTR4_TIME_DELEG_MODIFY
>> defined?
>>     - Give a ref to NFS4ERR_DELAY.
>>
>> # 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.
>>
>> # 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?
>>
>> # 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?
>>
>
> I don't think it should do so.  That hasn't been the practice with other
> extensions to NFSv4.2.
>

Then that referred statement need to be elaborated to reflect what it
actually does.  Like could append but not update the original RFC7863 and
refer to "the practice"

//Zahed


>
>> # 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.
>>
>> //Zahed
>>
>>
>>
>>
>>
>>
>>