Re: [PATCH 01/19] fs: new API for handling inode->i_version

[Jeff Layton]

On Thu, 2017-12-14 at 09:04 +1100, NeilBrown wrote:
> On Wed, Dec 13 2017, Jeff Layton wrote:
> 
> > +/*
> > + * The change attribute (i_version) is mandated by NFSv4 and is mostly for
> > + * knfsd, but is also used for other purposes (e.g. IMA). The i_version must
> > + * appear different to observers if there was a change to the inode's data or
> > + * metadata since it was last queried.
> > + *
> > + * It should be considered an opaque value by observers. If it remains the same
> > + * since it was last checked, then nothing has changed in the inode. If it's
> > + * different then something has changed. Observers cannot infer anything about
> > + * the nature or magnitude of the changes from the value, only that the inode
> > + * has changed in some fashion.
> 
> I agree that it "should be" considered opaque, but I have a suspicion
> that NFSv4 doesn't consider it opaque.
> There is something about write delegations and the server performing a
> GETATTR callback to the delegated client so that it can answer GETATTR
> from other clients without recalling the delegation.
> 
> Specifically section "10.4.3 Handling of CB_GETATTR" of RFC5661 contains
> the text:
> 
>    o  The client will create a value greater than c that will be used
>       for communicating that modified data is held at the client.  Let
>       this value be represented by d.
> 
> "c" here is a 'change' attribute.
> 
> Then:
> 
>    While the change attribute is opaque to the client in the sense that
>    it has no idea what units of time, if any, the server is counting
>    change with, it is not opaque in that the client has to treat it as
>    an unsigned integer, and the server has to be able to see the results
>    of the client's changes to that integer.  Therefore, the server MUST
>    encode the change attribute in network order when sending it to the
>    client.  The client MUST decode it from network order to its native
>    order when receiving it, and the client MUST encode it in network
>    order when sending it to the server.  For this reason, change is
>    defined as an unsigned integer rather than an opaque array of bytes.
> 
> This all suggests that nfsd needs to be certain that "incrementing" the
> change id will produce a new changeid, which has not been used before,
> and also suggests that nfsd needs to be able to control the changeid
> stored after writes that result from a delegation being returned.
> 
> I'd just like to say that this is one of the most annoying dumb features
> of NFSv4, because it is trivial to fix and I suggested a fix before
> NFSv4.0 was finalized.  Grumble.
> 
> Otherwise the patch set looks good.  I haven't gone over the code
> closely, the but approach is spot-on.

I don't think we have to do that. There are really only two states with
a client holding a write delegation, as far as the server is concerned.
Either:

a) the client has done no writes to the file, in which case it'll return
the same i_version that the server has when issued a CB_GETATTR

...or...

b) it has written to the file while holding the delegation, in which
case it'll return a different CB_GETATTR to the server

The simplest thing for the server to do is to just increment the change
attribute _once_ when it gets back a CB_GETATTR with a different change
attr than it has.

That's sufficient to tell another client issuing a a GETATTR that the
file has changed without needing to recall the delegation.

Prior to the delegation being returned, the client will send at least
one WRITE RPC, and that's enough to ensure that the the next stat will
see the thing increase.

-- 
Jeff Layton <jlayton@xxxxxxxxxx>