Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2021-11-16 at 09:34 -0500, Benjamin Coddington wrote:
> On 16 Nov 2021, at 9:17, Trond Myklebust wrote:
> 
> > On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote:
> > > On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
> > > > No, we really shouldn't need to care what the server thinks or
> > > > does.
> > > > The client is authoritative for the change attribute while it
> > > > holds
> > > > a
> > > > delegation, not the server.
> > > 
> > > My understanding of the intention of the code (which I had to
> > > sort of
> > > put
> > > together from historical patches in this area) is that we want to
> > > see
> > > ctime,
> > > mtime, and block size updates after copy/clone even if we hold a
> > > delegation,
> > > but without NFS_INO_INVALID_CHANGE, the client won't update those
> > > attributes.
> > > 
> > > If that's not necessary, we can drop this patch.
> > > 
> > 
> > We will still see the ctime/mtime/block size updates even if
> > NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status
> > are
> > tracked separately through their own NFS_INO_INVALID_* bits.
> > 
> > That said, there really is no reason why we shouldn't treat the
> > copy
> > and clone code exactly the same way we would treat a regular write.
> > Perhaps we can fix up the arguments of nfs_writeback_update_inode()
> > so
> > that it can be called here?
> 
> I wonder if that just kicks the problem down the road to when we
> return the
> delegation, and we'll see cases where ctime/mtime move backward.  And
> I
> don't think we can ever be authoritative for space_used, but maybe it
> doesn't
> matter if we're within the attribute timeouts.
> 

I don't understand your point. Mine is that we _should_ be setting the
NFS_INO_INVALID_MTIME, NFS_INO_INVALID_CTIME, NFS_INO_INVALID_BLOCKS...
flags here, and as far as I can tell, we are indeed doing so in
nfs42_copy_dest_done().

At least for clone(), we're then calling nfs_post_op_update_inode(),
which updates the attributes with whatever was retrieved in the CLONE
call. That will usually contain new values for mtime, ctime and blocks,
and so the cache is refreshed.

If the client failed to retrieve attributes as part of the CLONE or
COPY call, then we are indeed kicking the can of revalidating the
attributes down the road, but only as far as until the next call to
nfs_getattr(), or the delegation return, whichever comes first. The
fact that we do set the NFS_INO_INVALID_MTIME, ... means that we will
update those cached values before replying to a stat() or statx() call.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com