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". -- Jeff Layton <jlayton@xxxxxxxxxx>
