Re: [PATCH v2 2/3] nfs: Add timecreate to nfs inode

[Trond Myklebust <trondmy@kernel.org>]

On Wed, 2025-05-28 at 11:27 -0400, Jeff Layton wrote:
> On Wed, 2025-05-28 at 11:13 -0400, Benjamin Coddington wrote:
> > On 28 May 2025, at 8:56, Jeff Layton wrote:
> > 
> > > On Wed, 2025-05-28 at 08:50 -0400, Benjamin Coddington wrote:
> > > > From: Anne Marie Merritt <annemarie.merritt@primarydata.com>
> > > > 
> > > > Add tracking of the create time (a.k.a. btime) along with
> > > > corresponding
> > > > bitfields, request, and decode xdr routines.
> > > > 
> > > > Signed-off-by: Anne Marie Merritt
> > > > <annemarie.merritt@primarydata.com>
> > > > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > > > Signed-off-by: Trond Myklebust
> > > > <trond.myklebust@hammerspace.com>
> > > > ---
> > > >  fs/nfs/inode.c          | 28 ++++++++++++++++++++++------
> > > >  fs/nfs/nfs4proc.c       | 14 +++++++++++++-
> > > >  fs/nfs/nfs4xdr.c        | 24 ++++++++++++++++++++++++
> > > >  fs/nfs/nfstrace.h       |  3 ++-
> > > >  include/linux/nfs_fs.h  |  7 +++++++
> > > >  include/linux/nfs_xdr.h |  3 +++
> > > >  6 files changed, 71 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > > index 160f3478a835..fd84c24963b3 100644
> > > > --- a/fs/nfs/inode.c
> > > > +++ b/fs/nfs/inode.c
> > > > @@ -197,6 +197,7 @@ void nfs_set_cache_invalid(struct inode
> > > > *inode, unsigned long flags)
> > > >  		if (!(flags & NFS_INO_REVAL_FORCED))
> > > >  			flags &= ~(NFS_INO_INVALID_MODE |
> > > >  				   NFS_INO_INVALID_OTHER |
> > > > +				   NFS_INO_INVALID_BTIME |
> > > >  				   NFS_INO_INVALID_XATTR);
> > > >  		flags &= ~(NFS_INO_INVALID_CHANGE |
> > > > NFS_INO_INVALID_SIZE);
> > > >  	}
> > > > @@ -522,6 +523,7 @@ nfs_fhget(struct super_block *sb, struct
> > > > nfs_fh *fh, struct nfs_fattr *fattr)
> > > >  		inode_set_atime(inode, 0, 0);
> > > >  		inode_set_mtime(inode, 0, 0);
> > > >  		inode_set_ctime(inode, 0, 0);
> > > > +		memset(&nfsi->btime, 0, sizeof(nfsi->btime));
> > > >  		inode_set_iversion_raw(inode, 0);
> > > >  		inode->i_size = 0;
> > > >  		clear_nlink(inode);
> > > > @@ -545,6 +547,10 @@ nfs_fhget(struct super_block *sb, struct
> > > > nfs_fh *fh, struct nfs_fattr *fattr)
> > > >  			inode_set_ctime_to_ts(inode, fattr-
> > > > >ctime);
> > > >  		else if (fattr_supported &
> > > > NFS_ATTR_FATTR_CTIME)
> > > >  			nfs_set_cache_invalid(inode,
> > > > NFS_INO_INVALID_CTIME);
> > > > +		if (fattr->valid & NFS_ATTR_FATTR_BTIME)
> > > > +			nfsi->btime = fattr->btime;
> > > > +		else if (fattr_supported &
> > > > NFS_ATTR_FATTR_BTIME)
> > > > +			nfs_set_cache_invalid(inode,
> > > > NFS_INO_INVALID_BTIME);
> > > >  		if (fattr->valid & NFS_ATTR_FATTR_CHANGE)
> > > >  			inode_set_iversion_raw(inode, fattr-
> > > > >change_attr);
> > > >  		else
> > > > @@ -1900,7 +1906,7 @@ static int
> > > > nfs_inode_finish_partial_attr_update(const struct nfs_fattr
> > > > *fattr,
> > > >  		NFS_INO_INVALID_ATIME | NFS_INO_INVALID_CTIME
> > > > |
> > > >  		NFS_INO_INVALID_MTIME | NFS_INO_INVALID_SIZE |
> > > >  		NFS_INO_INVALID_BLOCKS | NFS_INO_INVALID_OTHER
> > > > |
> > > > -		NFS_INO_INVALID_NLINK;
> > > > +		NFS_INO_INVALID_NLINK | NFS_INO_INVALID_BTIME;
> > > >  	unsigned long cache_validity = NFS_I(inode)-
> > > > >cache_validity;
> > > >  	enum nfs4_change_attr_type ctype = NFS_SERVER(inode)-
> > > > >change_attr_type;
> > > > 
> > > > @@ -2219,10 +2225,13 @@ static int nfs_update_inode(struct
> > > > inode *inode, struct nfs_fattr *fattr)
> > > >  	nfs_fattr_fixup_delegated(inode, fattr);
> > > > 
> > > >  	save_cache_validity = nfsi->cache_validity;
> > > > -	nfsi->cache_validity &= ~(NFS_INO_INVALID_ATTR
> > > > -			| NFS_INO_INVALID_ATIME
> > > > -			| NFS_INO_REVAL_FORCED
> > > > -			| NFS_INO_INVALID_BLOCKS);
> > > > +	nfsi->cache_validity &=
> > > > +		~(NFS_INO_INVALID_ATIME | NFS_INO_REVAL_FORCED
> > > > |
> > > > +		  NFS_INO_INVALID_CHANGE |
> > > > NFS_INO_INVALID_CTIME |
> > > > +		  NFS_INO_INVALID_MTIME | NFS_INO_INVALID_SIZE
> > > > |
> > > > +		  NFS_INO_INVALID_OTHER |
> > > > NFS_INO_INVALID_BLOCKS |
> > > > +		  NFS_INO_INVALID_NLINK | NFS_INO_INVALID_MODE
> > > > |
> > > > +		  NFS_INO_INVALID_BTIME);
> > > > 
> > > 
> > > The delta above is a little curious. This patch is just adding
> > > NFS_INO_INVALID_BTIME, but the patch above adds the clearing of
> > > several
> > > other bits as well. Why does this logic change?
> > 
> > Good point, I wonder if it was based on other attribute work at the
> > time,
> > the original was here:
> > https://lore.kernel.org/linux-nfs/20211227190504.309612-3-trondmy@kernel.org/
> > 
> > So I think what we're doing here is clearing what we know we don't
> > have to
> > check/update - I think we can drop this entire hunk, its a minor
> > optimization, but hopefully we can get some expert opinion.  
> > Trond, do you
> > want me to test with this hunk removed?
> 
> Looking little closer, I think what happened there is that
> NFS_INO_INVALID_ATTR got unrolled into individual attr flags. So
> there
> may be no net change here (other than adding BTIME).
> 
> Still, it'd be best to do that in a separate patch, IMO, esp. since
> this is in nfs_update_inode(), which is already so "fiddly".

NFS_INO_INVALID_ATTR is a dinosaur that I'd like to see extinct. We
have fine grained attribute tracking these days, and so keeping the old
coarse grained one around is just confusing.

That said, I'm fine with this being a separate patch.

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