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

[Jeff Layton]

On Sat, 2017-12-16 at 15:17 +1100, NeilBrown wrote:
> On Wed, Dec 13 2017, Jeff Layton wrote:
> 
> > 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.
> 
> "increment" and "increase" are not words that mean anything for an
> "opaque value".
> NFSd is, presumably, an "observer" of i_version (as it isn't the
> filesytem that controls it), so your text says it must treat i_version as
> opaque.  That means it cannot detect an "increase" (only a change), and
> it certainly cannot "increment" the value.
> 
> I think you need to allow observers to treat i_version as a 64 bit number
> which will monotonically increase.  Any change to the file will result
> in an increment of at least '1'.
> 

One thing here...

I'm currently doing this:

static inline s64                                                               
inode_cmp_iversion(const struct inode *inode, const u64 old)                    
{                                                                               
        return (s64)inode_peek_iversion(inode) - (s64)old;                      
}                                                                               

But I don't think that'll handle wraparound correctly if we want to 
allow people to determine whether it's older or newer. I'll probably
change this to shift the old value left by one bit, and mask off the low
bit of the current inode->i_version.

That'll always give you a difference of 2 or more if they're different,
but it should return the correct sign, which is really all we care about
anyway.

Granted, we're unlikely to wrap around with a 64 bit value, but it's
hard to know for sure what values might be stored on disk on existing
filesystems.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>