[nfsv4] Re: question around CB_GETATTR handling

[Rick Macklem <[e-mail]>]

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

>
> >
> > 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?
>
> rick
>
> > --
> > Jeff Layton <[e-mail]>
> >
> > _______________________________________________
> > nfsv4 mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]