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

[Benjamin Coddington <bcodding@redhat.com>]

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?

Ben