[nfsv4] Re: question around CB_GETATTR handling

[Rick Macklem <[e-mail]>]

On Wed, Sep 4, 2024 at 4:54 PM Jeff Layton <[e-mail]> wrote:
>
> On Wed, 2024-09-04 at 14:15 -0700, Rick Macklem wrote:
> > On Wed, Sep 4, 2024 at 2:05 PM Rick Macklem <[e-mail]> wrote:
> > >
> > > On Wed, Sep 4, 2024 at 1:09 PM Jeff Layton <[e-mail]> wrote:
> > > >
> > > > On Wed, 2024-09-04 at 19:47 +0000, Trond Myklebust wrote:
> > > > > On Wed, 2024-09-04 at 15:33 -0400, Jeff Layton wrote:
> > > > > > On Wed, 2024-09-04 at 19:21 +0000, Trond Myklebust wrote:
> > > > > > > On Wed, 2024-09-04 at 12:23 -0400, Jeff Layton wrote:
> > > > > > > > RFC 8881, section 10.4.3 has this pseudo code to describe how to
> > > > > > > > handle
> > > > > > > > the size and change attr:
> > > > > > > >
> > > > > > > > ------------------8<-------------------
> > > > > > > >     if (!modified) {
> > > > > > > >         do CB_GETATTR for change and size;
> > > > > > > >
> > > > > > > >         if (cc != sc)
> > > > > > > >             modified = TRUE;
> > > > > > > >     } else {
> > > > > > > >         do CB_GETATTR for size;
> > > > > > > >     }
> > > > > > > >
> > > > > > > >     if (modified) {
> > > > > > > >         sc = sc + 1;
> > > > > > > >         time_modify = time_metadata = current_time;
> > > > > > > >         update sc, time_modify, time_metadata into file's
> > > > > > > > metadata;
> > > > > > > >     }
> > > > > > > > ------------------8<-------------------
> > > > > > > >
> > > > > > > >
> > > > > > > > The line that shows sc = sc + 1 seems to indicate that we need to
> > > > > > > > increment "sc" on every CB_GETATTR, even if nothing changed since
> > > > > > > > the
> > > > > > > > last time.
> > > > > > > >
> > > > > > > > Is that correct or should we only increment it if it wasn't
> > > > > > > > incremented
> > > > > > > > before? The latter would make more sense, given that it doesn't
> > > > > > > > query
> > > > > > > > for change if the file is already considered to be modified.
> > > > > > >
> > > > > > > I believe RFC5661 says
> > > > > > >
> > > > > > >    For simplicity of implementation, the client MAY for each
> > > > > > > CB_GETATTR
> > > > > > >    return the same value d.  This is true even if, between
> > > > > > > successive
> > > > > > >    CB_GETATTR operations, the client again modifies the file's data
> > > > > > > or
> > > > > > >    metadata in its cache.  The client can return the same value
> > > > > > > because
> > > > > > >    the only requirement is that the client be able to indicate to
> > > > > > > the
> > > > > > >    server that the client holds modified data.  Therefore, the
> > > > > > > value of
> > > > > > >    d may always be c + 1.
> > > > > > >
> > > > > > > Which is how the Linux client tries to implement it.
> > > > > > >
> > > > > >
> > > > > > Right, 8881 is similar.
> > > > >
> > > > > Yeah, but nobody has coded to that.
> > > > >
> > > > > >
> > > > > > In this case though, I'm asking about the server-side implementation.
> > > > > > That has its own algorithm (illustrated above in the pseudocode), and
> > > > > > it seems to indicate that once the server detects that the client has
> > > > > > made a single modification to the file, that it needs to increment
> > > > > > "sc"
> > > > > > on every subsequent CB_GETATTR.
> > > > > >
> > > > > > That makes no sense on its face, so I suspect this is a mistake in
> > > > > > the
> > > > > > pseudocode above. Or am I wrong and this is actually necessary?
> > > > > > --
> > > > >
> > > > > Oh... No, as far as the server is concerned, I think it is indeed
> > > > > supposed to increment the change attribute and timestamps on every
> > > > > reply. That is reinforced here:
> > > > >
> > > > >    As discussed earlier in this section, the client MAY return the same
> > > > >    cc value on subsequent CB_GETATTR calls, even if the file was
> > > > >    modified in the client's cache yet again between successive
> > > > >    CB_GETATTR calls.  Therefore, the server must assume that the file
> > > > >    has been modified yet again, and MUST take care to ensure that the
> > > > >    new nsc it constructs and returns is greater than the previous nsc it
> > > > >    returned.
> > > > >
> > > > > The spec also says that "committing the constructed attribute values to
> > > > > stable storage is OPTIONAL". That seems misleading or possibly just
> > > > > incorrect. The fact that the client declares that it is caching data
> > > > > doesn't necessarily mean that it will actually manage to write that
> > > > > data back.
> > > I think the server does have the option of not saving the change attribute
> > > on stable storage and then, after a server reboot, if the client does a
> > > OPEN/CLAIM_PREVIOUS for the delegation, the server can require
> > > an immediate recall of the delegation. (By setting recall == true in the
> > > open_read_delegation4/open_write_delegation4.)
> > >
> > > At least I think that works?
> > Hmm. I now think it takes more than the above. I think it does need to
> > commit nsc to stable storage, just as it would for changes done via
> > write(s) to the server, as an updated change.
> > (It is the sc value it can forget on a reboot, if
> > it does the immediate recall for CLAIM_REVIOUS for a write delegation.)
> >
> > Does this sound correct to others? rick
> >
>
> The Linux server doesn't always have ultimate control over the change
> attribute, and we can't necessarily commit a specific value to the
> metadata that the client gives us. The change attr is sort of like the
> ctime that way.
If there is something "harmless" that the nfsd can do to the file which
changes change, you might be able to do that every time other clients
do a GETATTR. (Such as setting ctime, maybe?)

That would presumably also get the change onto stable storage?

>
> > >
> > > >
> > > > Huh, ok.
> > > >
> > > > So imagine that the client gets a write deleg and makes a single small
> > > > change to the file, but then holds on to the write deleg for a long
> > > > time. Other clients are issuing CB_GETATTRs against the server.
> > > > Eventually the delegation is returned.
> > > >
> > > > Could you end up in a change attr reuse situation due to all of this
> > > > faux change attr incrementing by the server?
> > > Since change is 64bits, I think this will take a very very long time?
> > >
>
> The scenario I was thinking of is:
>
> Client gets a write delegation and the change attr is 1. Client does a
> (cached) modification and sends a response of 2 to the server's first
> CB_GETATTR (cc + 1).
>
> Next, we get GETATTR requests from more clients. The server increments
> "sc" for every one, and we end up with a change attribute of 5 in the
> delegation record. The exported file still has a change attr of 1 as
> there has been no writeback from the client yet.
>
> Finally, the client writes the data back and does a DELEGRETURN. The
> local file lands at a value of 3. More data is written from another
> client, and we eventually get to a change attribute of 5. The client
> that saw a value of 5 earlier does another GETATTR and is now confused.
> It doesn't realize that there have been more writes done by another
> client and the size is completely different from what it expects.
Since you have 64bits to work with, maybe the first increment of sc
could be by a large amount, so that the normal updates will "never" get
there. (Or something like putting the file system's change value in the
low order 63 bits with the high order bit always 0 vs always set the
high order bit in sc,
so you have bisected the change number space.)
I'm sure there are others. The increment by 1 in the RFC algorithm
is just an example of making it change each GETATTR.

rick

>
> As a practical matter, other clients can't do much with the file while
> there is a write delegation outstanding (aside from GETATTR), so that
> may mean that the above doesn't make any difference. The client doing
> the GETATTR can't do a READ or anything, and since we did at least one
> change attr increment, other clients will need to invalidate their
> caches.
> --
> Jeff Layton <[e-mail]>