On 28 May 2025, at 16:16, Trond Myklebust wrote: > 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@xxxxxxxxxxxxxxx> >>>>> >>>>> 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@xxxxxxxxxxxxxxx> >>>>> Signed-off-by: Lance Shelton <lance.shelton@xxxxxxxxxxxxxxx> >>>>> Signed-off-by: Trond Myklebust >>>>> <trond.myklebust@xxxxxxxxxxxxxxx> >>>>> --- >>>>> 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@xxxxxxxxxx/ >>> >>> 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. Ok - I'll resend this on v3 adding BTIME to NFS_INO_INVALID_ATTR. Ben
