[nfsv4] Re: question around CB_GETATTR handling

[Jeff Layton <[e-mail]>]

On Wed, 2024-09-04 at 17:33 -0700, Rick Macklem wrote:
> 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?
> 

Sure. A ctime bump should do it.

It's just that that won't reflect what the client is reporting. Maybe
that's OK though. The best outcome is that the change attribute after
the DELEGRETURN outpaces the cached version that we are reporting after
a CB_GETATTR.

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

There are a number of mitigation tactics. In practice, Linux nfsd also
folds the ctime into the change attribute that the server reports,
which should guarantee uniqueness in most cases.

I think I'm convinced that doing incrementing every time is probably
safest, at least with RFC 5661/8881 style CB_GETATTR.

That brings me to the next question:

Do delegated timestamps (a'la the delstid draft) change how we should
manage the change attribute? IOW, if the delegated size and mtime in a
CB_GETATTR response indicate no changes, should the server still
increment the change attr?
-- 
Jeff Layton <[e-mail]>