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

[Zaheduzzaman Sarker <[e-mail]>]

On Tue, Sep 24, 2024 at 10:01 AM Thomas Haynes <[e-mail]> wrote:

>
>
> On Aug 14, 2024, at 8:55 AM, Zaheduzzaman Sarker <
> [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?

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

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

>
> 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]
> To unsubscribe send an email to [e-mail]
>
>
>