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

[David Noveck <[e-mail]>]

On Thu, Oct 5, 2023, 7:42 AM Zaheduzzaman Sarker <
[e-mail]> wrote:

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

Fair enough.



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

Yes.

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
>

Yes.

and refer to "the practice"
>

Not sure what that means.


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