Re: [nfsv4] I-D Action: draft-ietf-nfsv4-layoutwcc-01.txt

[Trond Myklebust <[e-mail]>]

As I've said many times before, LAYOUTCOMMIT is not only unnecessary, but
is a terrible idea. Any O_SYNC workload is literally sunk when you require
LAYOUTCOMMIT in order to fully commit the metadata, because each write()
syscall needs to go both to the data server and the metadata server.

No proposal that attempts to expand the use of LAYOUTCOMMIT is going to be
welcome on the Linux client.

On Tue, Apr 11, 2023 at 4:14 PM Rick Macklem <[e-mail]> wrote:

> On Tue, Apr 11, 2023 at 6:40 AM Trond Myklebust <[e-mail]> wrote:
> >
> > LAYOUTCOMMIT sort of makes sense for block based layouts, since you do
> need to persist updates of the blocks with the metadata server. It makes
> zero sense for files and flex files layout types, since you already have
> persistence in the form of the regular COMMIT operation. While you could
> argue that LAYOUTCOMMIT can be seen as just providing hints in those cases,
> there is nothing in the protocol spec that allows you to conclude that. It
> is not worth introducing a new operation that needs to be called every time
> that the client wants to ensure persistence, just in order to resolve a
> server reboot issue.
> >
> First, note the following sentence:
>     The LAYOUTCOMMIT operation indicates that the client has completed
>     writes using a layout obtained by a previous LAYOUTGET.
> In particular, note that it says nothing like "when data needs to be
> committed to stable storage".
> Also, I cannot find anywhere where the RFC discusses the relationship
> between LAYOUTCOMMIT
> and COMMIT.  Therefore, I do not believe any relationship should be
> assumed.
>
> Now, here's the back story:
> When I implemented the pNFS server for FreeBSD, I assumed that the
> client would do a
> LayoutCommit after a series of writes and before it expected the MDS
> to have up-to-date
> attributes for the file that was written to the DS (using a RW layout).
> --> At the time of LayoutCommit, the MDS did a Getattr on the DS(s)
> for the layout to
>      update the MDS's copy of the attributes that change when writing
> occurs.
>
> This worked fine for the FreeBSD client, which does a LayoutCommit
> whenever a
> process did a POSIX fsync or close syscall.
>
> Then I started testing using a Linux client and found out that it got
> confused by
> stale attributes acquired from the MDS after doing write(s) to the DS(s).
> Looking at the packet trace I found that, for the case where the Linux
> client
> did writes to the DS(s) with File_Sync set, the Linux client did not do a
> LayoutCommit. (It, of course, did not do a Commit, but that was what I
> expected.)
> --> I looked in the RFC for a statement that would make it clear that the
> Linux
>       client was broken and should do a LayoutCommit after these write(s)
> but...
>       I could not find one.
>        So, to work around the problem I added a tunable called
>        vfs.nfsd.pnfsgetdsattr which, when enabled, makes the pNFS server
> do a
>        Getattr against the DS(s) whenever it receives a Getattr from a
> client and
>        a RW layout has been issued for the file.
>        --> This resulted in a lot of MDS->DS Getattrs, but made the Linux
> client
>              function well against the FreeBSD pNFS server.
>
> It looks to me like the proposed Layout_WCC operation is mostly just
> trying to solve
> the problem that can be solved by requiring a pNFS client to do a
> LayoutCommit
> after a series of writes are done to DS(s) based on a RW layout and before
> it
> expects the MDS to have up-to-date attributes for the file that was
> written.
> (For my client, that seems to be whenever a POSIX close or fsync is done on
>   the file.)
>
> Since LayoutCommits do not happen that frequently, the overhead of doing
> a Getattr against a DS for each of them is not particularily large.
>
> Then there is the separate issue of how a client reports errors encountered
> when doing write(s) to a DS if the MDS reboots, making the layout stateid
> stale.
> I'd rather see this handled via a non-empty loca_layoutupdate, since
> that handles
> the problem for both NFSv4.1 and NFSv4.2 via a layout specific driver.
> (NFSv4.1 is deficient, since it does not have LayoutError and this could
> work
>  around that.) A new NFSv4.2 operation only fixes the issue for NFSv4.2 and
> only after it is adopted and only if it is clearly specified when the
> client must
> do the operation.
>
> It seems to me that a non-empty loca_layoutupdate is only needed for the
> rare second case, although it might be useful for the maintenance of
> up-to-date attributes on the MDS.
>
> Finally, I will note that it is not obvious how the client can provide a
> Change
> attribute to the MDS after doing Writes to a NFSv3 DS.  I think that the
> client can provide a boolean that indicates if a write to a DS for the file
> has been done since the last LayoutCommit (or Layout_WCC if you
> choose to go that way).
>
> > Secondly, as you pointed out, the repurposing of LAYOUTCOMMIT for
> propagating layout stats and layout errors requires a protocol change in
> any case. While that protocol change would be limited to the flex files
> protocol, it requires a whole new flex files v2 spec, since there is no
> procedure for amending the layout protocols in the way that exists for the
> NFSv4.2 protocol itself.
>
> First, I do not consider this "repurposing". I think you have assumed
> a relationship
> between LayoutCommit and Commit that does not exist and, although the RFC
> seems
> vague w.r.t. when LayoutCommits are to be performed by a pNFS client,
> I do think that
> they were intended for this use. Among other things, they are allowed
> to be done during
> Grace using a stale layout stateid.
>
> Second, what's the difference between an amended RFC for Flexible File
> Layout and
> an RFC for a new NFSv4.2 operation? All that changes is the
> loca_layoutupdate becomes
> optionally non-empty. (If everyone is really convinced that is not
> permitted, then define
> Flexible File Layout 2 with that minimal change, to fix the "how to
> report DS errors when
> the MDS reboots.)
>
> >
> > Lastly, while LAYOUTCOMMIT could potentially carry a layout stats and
> layout error payload in the same way that LAYOUTRETURN can, that payload
> size is limited by the negotiated 'ca_maxrequestsize' session value. This
> is in part why we have dedicated LAYOUTSTATS and LAYOUTERROR operations in
> the first place: so that the client can report the full information in
> multiple RPC calls when necessary, without being limited by the payload
> limitations of a single RPC call.
>
> The arguments that come before loca_layoutupdate for LayoutCommit
> are (at a glance, so I might be off a little) 68bytes vs 20bytes for
> Layout_WCC.
> I doubt 48bytes is going to make a difference. I also believe that
> up-to-date
> attributes and errors received from write(s) to DS(s) while the MDS reboots
> are what matters. I do not see stats as important for the case where the
> MDS
> has rebooted and the client cannot do a LayoutStats with the old layout
> stateid.
> (After all, LayoutStats does not even exist in NFSv4.1.)
>
> rick
>
> >
> > On Mon, Apr 10, 2023 at 7:11 PM Rick Macklem <[e-mail]>
> wrote:
> >>
> >> If Tom commented w.r.t. the use of LAYOUTCOMMIT for this,
> >> I missed it.
> >>
> >> Here are some snippets from RFC8881 Sec. 18.42 plus comments:
> >>
> >>    The LAYOUTCOMMIT operation indicates that the client has completed
> >>    writes using a layout obtained by a previous LAYOUTGET.  The client
> >>    may have only written a subset of the data range it previously
> >>    requested.  LAYOUTCOMMIT allows it to commit or discard provisionally
> >>    allocated space and to update the server with a new end-of-file.
> >> Note that the arguments loca_last_write_offset, loca_time_modify and
> >> loca_layoutupdate can be used by the client to inform the MDS about
> >> file changes due to writing to the DS.
> >>
> >>    If the loca_reclaim field is set to TRUE, this indicates that the
> >>    client is attempting to commit changes to a layout after the restart
> >>    of the metadata server during the metadata server's recovery grace
> >>    period (see Section 12.7.4).  This type of request may be necessary
> >>    when the client has uncommitted writes to provisionally allocated
> >>    byte-ranges of a file that were sent to the storage devices before
> >>    the restart of the metadata server.
> >> This explains how LayoutCommit can be used after an MDS reboot
> >> to update the MDS w.r.t. writes done to the DS.
> >>
> >>    The loca_last_write_offset field specifies the offset of the last
> >>    byte written by the client previous to the LAYOUTCOMMIT.  Note that
> >>    this value is never equal to the file's size (at most it is one byte
> >>    less than the file's size) and MUST be less than or equal to
> >>    NFS4_MAXFILEOFF.  Also, loca_last_write_offset MUST overlap the range
> >>    described by loca_offset and loca_length.  The metadata server may
> >>    use this information to determine whether the file's size needs to be
> >>    updated.  If the metadata server updates the file's size as the
> >>    result of the LAYOUTCOMMIT operation, it must return the new size
> >>    (locr_newsize.ns_size) as part of the results.
> >>
> >>    The loca_time_modify field allows the client to suggest a
> >>    modification time it would like the metadata server to set.  The
> >>    metadata server may use the suggestion or it may use the time of the
> >>    LAYOUTCOMMIT operation to set the modification time.  If the metadata
> >>    server uses the client-provided modification time, it should ensure
> >>    that time does not flow backwards.  If the client wants to force the
> >>    metadata server to set an exact time, the client should use a SETATTR
> >>    operation in a COMPOUND right after LAYOUTCOMMIT.  See Section 12.5.4
> >>    for more details.  If the client desires the resultant modification
> >>    time, it should construct the COMPOUND so that a GETATTR follows the
> >>    LAYOUTCOMMIT.
> >> Here are details on how loca_last_write_offset and loca_time_modify
> >> are used to update the MDS.
> >>
> >> Although the Flex Files layout does not define any loca_layout_update,
> >> it seems that this could be added more easily than new NFSv4.2
> operations
> >> if additional client->MDS information is deemed necessary.
> >>
> >> Now, I do agree that:
> >>    The LAYOUTCOMMIT operation commits changes in the layout represented
> >>    by the current filehandle, client ID (derived from the session ID in
> >>    the preceding SEQUENCE operation), byte-range, and stateid.
> >> "commits changes in the layout" is kinda weird and does not clarify when
> >> LayoutCommit should be done by a client. I think this should be
> clarified,
> >> at least for Flex File Layout, as whenever the client has completed a
> series
> >> of writes to DS(s) using the layout and expects the MDS to return
> >> up-to-date Getattr information for the file to clients.
> >>
> >> With the above clarification (or whatever works for you), I do not see
> why
> >> new operations are needed?
> >>
> >> rick
> >>
> >> On Thu, Mar 30, 2023 at 5:57 PM <[e-mail]> wrote:
> >> >
> >> >
> >> > A New Internet-Draft is available from the on-line Internet-Drafts
> >> > directories. This Internet-Draft is a work item of the Network File
> System
> >> > Version 4 (NFSV4) WG of the IETF.
> >> >
> >> >    Title           : Add LAYOUT_WCC to NFSv4.2's Flex File Layout Type
> >> >    Authors         : Thomas Haynes
> >> >                      Trond Myklebust
> >> >    Filename        : draft-ietf-nfsv4-layoutwcc-01.txt
> >> >    Pages           : 11
> >> >    Date            : 2023-03-30
> >> >
> >> > Abstract:
> >> >    The Parallel Network File System (pNFS) Flexible File Layout allows
> >> >    for a file's metadata (MDS) and data (DS) to be on different
> servers.
> >> >    It does not provide a mechanism for the data server to update the
> >> >    metadata server of changes to the data part of the file.  The
> client
> >> >    has knowledge of such updates, but lacks the ability to update the
> >> >    metadata server.  This document presents a refinement to RFC8435 to
> >> >    allow the client to update the metadata server to changes on the
> data
> >> >    server.
> >> >
> >> > The IETF datatracker status page for this Internet-Draft is:
> >> > https://datatracker.ietf.org/doc/draft-ietf-nfsv4-layoutwcc/
> >> >
> >> > There is also an HTML version available at:
> >> > https://www.ietf.org/archive/id/draft-ietf-nfsv4-layoutwcc-01.html
> >> >
> >> > A diff from the previous version is available at:
> >> >
> https://author-tools.ietf.org/iddiff?url2=draft-ietf-nfsv4-layoutwcc-01
> >> >
> >> > Internet-Drafts are also available by rsync at rsync.ietf.org:
> :internet-drafts
> >> >
> >> >
> >> > _______________________________________________
> >> > nfsv4 mailing list
> >> > [e-mail]
> >> > https://www.ietf.org/mailman/listinfo/nfsv4
> >>
> >> _______________________________________________
> >> nfsv4 mailing list
> >> [e-mail]
> >> https://www.ietf.org/mailman/listinfo/nfsv4
>