Re: [PATCH v3 1/7] iversion: update comments with info about atime updates

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-08-30 at 15:46 -0400, J. Bruce Fields wrote:
> On Tue, Aug 30, 2022 at 03:30:13PM -0400, Jeff Layton wrote:
> > On Tue, 2022-08-30 at 14:32 -0400, J. Bruce Fields wrote:
> > > On Tue, Aug 30, 2022 at 01:02:50PM -0400, Jeff Layton wrote:
> > > > The fact that NFS kept this more loosely-defined is what allowed us to
> > > > elide some of the i_version bumps and regain a fair bit of performance
> > > > for local filesystems [1]. If the change attribute had been more
> > > > strictly defined like you mention, then that particular optimization
> > > > would not have been possible.
> > > > 
> > > > This sort of thing is why I'm a fan of not defining this any more
> > > > strictly than we require. Later on, maybe we'll come up with a way for
> > > > filesystems to advertise that they can offer stronger guarantees.
> > > 
> > > Yeah, the afs change-attribute-as-counter thing seems ambitious--I
> > > wouldn't even know how to define what exactly you're counting.
> > > 
> > > My one question is whether it'd be worth just defining the thing as
> > > *increasing*.  That's a lower bar.
> > > 
> > 
> > That's a very good question.
> > 
> > One could argue that NFSv4 sort of requires that for write delegations
> > anyway. All of the existing implementations that I know of do this, so
> > that wouldn't rule any of them out.
> > 
> > I'm not opposed to adding that constraint. Let me think on it a bit
> > more.
> > 
> > > (Though admittedly we don't quite manage it now--see again 1631087ba872
> > > "Revert "nfsd4: support change_attr_type attribute"".)
> > > 
> > 
> > Factoring the ctime into the change attr seems wrong, since a clock jump
> > could make it go backward. Do you remember what drove that change (see
> > 630458e730b8) ?
> > 
> > It seems like if the i_version were to go backward, then the ctime
> > probably would too, and you'd still see a duplicate change attr.
> 
> See the comment--I was worried about crashes: the change attribute isn't
> on disk at the time the client requests it, so after a crash the client
> may see it go backward.  (And then could see it repeat a value, possibly
> with different file contents.)
> 
> Combining it with the ctime means we get something that behaves
> correctly even in that case--unless the clock goes backwards.
> 

Yeah ok, I vaguely remember discussing this.

That seems like it has its own problem though. If you mix in the ctime
and the clock jumps backward, then you could end up with the same issue
(a stale changeid, different contents). No crash required.
-- 
Jeff Layton <jlayton@kernel.org>