[nfsv4] Re: AD review : draft-ietf-nfsv4-layoutwcc-02

[Thomas Haynes <[e-mail]>]

> On Oct 3, 2024, at 9:55 AM, Zaheduzzaman Sarker <[e-mail]> wrote:
> 
> 
> 
> On Tue, Sep 24, 2024 at 10:01 AM Thomas Haynes <[e-mail] <mailto:[e-mail]>> wrote:
>> 
>> 
>>> On Aug 14, 2024, at 8:55 AM, Zaheduzzaman Sarker <[e-mail] <mailto:[e-mail]>> wrote:
>>> 
>>> Hi,
>>>  
>>> I have now reviewed the draft-ietf-nfsv4-layoutwcc and I think this document need to improve the writing. Hence please respond to the comments and resolve them.
>>>  
>>> I have done this review as If I don't really understand NFSv4 and NFSv3 (which might be anyway true) and tried to reflect on things that was unclear in the description.
>>>  
>>> I hope this review helps and feel free to take the opportunity to educate me.
>>>  
>>> //Zahed
>>>  
>> 
>> 
>> Zahed,
>> 
>> The review does help.
>> 
>> I am sending out a revised copy of the draft to see if my changes tackle the issues you have raised.
>> 
>> I think we will have another round after this one.
>> 
>> 
>> Thanks,
>> Tom
>> 
>> 
>> 
>> 
>>>  
>>>  
>>> My section by section comments are below -
>>> ====================================
>>>  
>>> # Abstract
>>>     s/This document presents a refinement to RFC8435 to allow the client to update the metadata server to changes on the data server/ This document updates RFC8435 to allow the client to update the metadata server to changes on the data server.
>>>  
>>>     This makes it clear that the current specification will update RFC 8435.
>>>  
>>>     However, to me it is still not super clear if we are updating RFC 8435 or extending RFC 7862. What is the rationale here for the update tag?
>> 
>> 
>> Good catch!
>> 
>> We removed it from delstid after discussion. I have removed it.
>> 
>> Also, we have this:
>> 
>> This document presents a
>>       refinement to RFC8435 to allow the client to update the metadata
>>       server to changes on the data server.
>> 
>> Would you prefer:
>> 
>> This document presents an
>>       extension to RFC8435 to allow the client to update the metadata
>>       server to changes on the data server.
> 
> Yes. So are you saying you don't need the update tag? 


Yes,



>> 
>> 
>> 
>>>  
>>> # Section 1
>>>  
>>>     My initial assumption was that we have a useful feature with WCC for NFv3 that we would like to allow for NFv4 with LAYOUT-WCC. However, the more I read Section 1 and Section 2, I am getting more confused as the description of NFSv4 and NFv3 is very intertwined. Either my assumption is wrong or the text should be improved, either case I think the following would help.
>>>         - let’s assume the reader will read this as a starting document (I think this will be true for the most of the reviewers in the process) hence, let's describe the scenario better for them and not assume they understand and know the difference between NFv4 and NFSv3 by heart. Hence, providing right reference is the right tools here. Also make sure we write
>>>             - the version of NFS the metadata server is running or does it even matter
>>>             - short description of the WCC and reference the RFC for the details
>>>         - Lets keep all the NFv3 related details in the Introduction section. This means describe some of the basic concepts those are described in different sub-sections of section 2 in Section 1, and focus on NFSv4 specifics in section 2 and 3. This means already in introduction we write the relation of NFSv3 GETATTR to NFv4 GETATRR and who sends them , etc. as a base to understand the extension are doing in the rest of the sections.
>>>         - Make sure we are specific about the extension we are doing in this specification - are we writing this specification only for NFSv4 client or the client's NFV version does not matter? If the client learns gid and uid from via NFSv3 how that is done and why that is important here. This pops up if I read the following sentence -
>>>  
>>>                        "In this document, we introduce a new operation called LAYOUT_WCC which allows the client to periodically report the attributes of the data files to the metadata server."
>>>  
>>>  
>>> # Section 2.1
>>>  
>>>     Please provide the reference to the data types of the structs
>> 
>> Done
> 
> OK 
>> 
>>>  
>>> # Section 2.3
>>>  
>>>     It not clear if the extension is for NFSv4 only or it can also replace WCC for NFSv3. 
>>>  
>>>     Why are first 3 paragraph relevant at all? what is the relation of this description to the extension?
>> 
>> 
>> It is the motivation for the changes. Pulled into a new section as per your other comments.
> 
> Good. 
>> 
>> 
>> 
>>>     
>>>     Overall, sometimes it is not clear in the text on who does what. I think we can do a better job of stating what is client's responsibility and what is metadata server's. This might be get cleared automatically when we have a better split between the section 1 and section 2
>>>  
>>> # Section 2.4.1
>>>  
>>>     - Please provide reference to mtime, atime and all the attribute mentioned (size, space_used, change, time_access, time_metadata, and time_modify.).
>> 
>> 
>> Done
>> 
>>>     - it says - 
>>>  
>>>             Whenever it sends an NFS4ERR_ACCESS error via LAYOUTRETURN or LAYOUTERROR - it could have already gotten the NFSv3 uid and gid values back in the WCC of the WRITE, READ, or COMMIT operation which got the error.
>>>     
>>>         I am a bit confused here, just reading the text here, are you saying the client had got some attributes via NFSv3 but when error occurs it uses NFv4 LAYOUT-WCC to report? 
>> 
>> Yes. 
>> 
>> 
>> But I will need to check to see if it can get those values on an error.
>  
> Please check and update accordingly.
>> 

3.1 General comments on attributes and consistency data on failure

   For those procedures that return either post_op_attr or wcc_data
   structures on failure, the discriminated union may contain the
   pre-operation attributes of the object or object parent
   directory.  This depends on the error encountered and may also
   depend on the particular server implementation. Implementors are
   strongly encouraged to return as much attribute data as possible
   upon failure, but client implementors need to be aware that

      
      
Callaghan, el al             Informational                     [Page 29]
^L    
RFC 1813                 NFS Version 3 Protocol                June 1995
      

   their implementation must correctly handle the variant return
   instance where no attributes or consistency data is returned.


I added a small change: 

            Whenever it sends an NFS4ERR_ACCESS error via LAYOUTRETURN or LAYOUTERROR - it could
            have already gotten the NFSv3 uid and gid values back in the WCC of the WRITE,
-           READ, or COMMIT operation which got the error.
+           READ, or COMMIT operation which got the error. Thus it could report that information
+           back to the metadata server, saving it from querying that information via a NFSv3 GETATTR.


>> 
>> 
>>>  
>>> # Section 2.5
>>>  
>>>     We should be very clear that we are not defining any new ERROR for LAYOUT_WCC if that is the case and reference to where the errors are defined. Otherwise, we need to explain each and every errors here in this document, that would be cumbersome or repeating.
>> 
>> Agreed and done.
> 
> OK. 
>> 
>>>  
>>> # Section 5 IANA considerations : 
>>>  
>>>     Please provide more details on which IANA registry to be updated/amended for the new entries. 
>> 
>> 
>> Marked the section as not to be included in the final draft.
> 
> Why? don't you want to update the registry with new entries? 

There are no new entries for any registry.


>> 
>> I think historically we use this to show that RFC-TBD means the assigned RFC number.
> 
> But you would need to say what registries need to be updated with what entries.
> 
> 
> //Zahed
>  
>> 
>> 
>>> _______________________________________________
>>> nfsv4 mailing list -- [e-mail] <mailto:[e-mail]>
>>> To unsubscribe send an email to [e-mail] <mailto:[e-mail]>