On Sun, 30 Jun 2024, Chuck Lever wrote: > On Fri, Jun 28, 2024 at 05:10:53PM -0400, Mike Snitzer wrote: > > This is nfs-localio code which blurs the boundary between server and > > client... > > > > The change_attr is used by NFS to detect if a file might have changed. > > This code is used to get the attributes after a write request. NFS > > uses a GETATTR request to the server at other times. The change_attr > > should be consistent between the two else comparisons will be > > meaningless. > > > > So nfs_localio_vfs_getattr() should use the same change_attr as the > > one that would be used if the NFS GETATTR request were made. For > > NFSv3, that is nfs_timespec_to_change_attr() as was already > > implemented. For NFSv4 it is something different (as implemented in > > this commit). > > > > [above header derived from linux-nfs message Neil sent on this topic] > > Instead of this note, I recommend: > > Message-Id: <171918165963.14261.959545364150864599@xxxxxxxxxxxxxxxxxxxxx> Linus would not be impressed. He likes links that you can click on and follow. So Link: https://lore.kernel.org/171918165963.14261.959545364150864599@xxxxxxxxxxxxxxxxxxxxx is preferred (at least I think that is the current state of the conversation see https://lore.kernel.org/all/CAHk-=wiD9du3fBHuLYzwUSdNgY+hxMZEWNZpqJXy-=wD2wafdg@xxxxxxxxxxxxxx/ NeilBrown > > > > Suggested-by: NeilBrown <neil@xxxxxxxxxx> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> > > --- > > fs/nfs/localio.c | 48 +++++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 39 insertions(+), 9 deletions(-) > > > > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c > > index 0f7d6d55087b..fe96f05ba8ca 100644 > > --- a/fs/nfs/localio.c > > +++ b/fs/nfs/localio.c > > @@ -364,21 +364,47 @@ nfs_set_local_verifier(struct inode *inode, > > verf->committed = how; > > } > > > > +/* Factored out from fs/nfsd/vfs.h:fh_getattr() */ > > +static int __vfs_getattr(struct path *p, struct kstat *stat, int version) > > +{ > > + u32 request_mask = STATX_BASIC_STATS; > > + > > + if (version == 4) > > + request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE); > > + return vfs_getattr(p, stat, request_mask, AT_STATX_SYNC_AS_STAT); > > +} > > + > > +/* > > + * Copied from fs/nfsd/nfsfh.c:nfsd4_change_attribute(), > > + * FIXME: factor out to common code. > > + */ > > +static u64 __nfsd4_change_attribute(const struct kstat *stat, > > + const struct inode *inode) > > +{ > > + u64 chattr; > > + > > + if (stat->result_mask & STATX_CHANGE_COOKIE) { > > + chattr = stat->change_cookie; > > + if (S_ISREG(inode->i_mode) && > > + !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) { > > + chattr += (u64)stat->ctime.tv_sec << 30; > > + chattr += stat->ctime.tv_nsec; > > + } > > + } else { > > + chattr = time_to_chattr(&stat->ctime); > > + } > > + return chattr; > > +} > > + > > static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb) > > { > > struct kstat stat; > > struct file *filp = iocb->kiocb.ki_filp; > > struct nfs_pgio_header *hdr = iocb->hdr; > > struct nfs_fattr *fattr = hdr->res.fattr; > > + int version = NFS_PROTO(hdr->inode)->version; > > > > - if (unlikely(!fattr) || vfs_getattr(&filp->f_path, &stat, > > - STATX_INO | > > - STATX_ATIME | > > - STATX_MTIME | > > - STATX_CTIME | > > - STATX_SIZE | > > - STATX_BLOCKS, > > - AT_STATX_SYNC_AS_STAT)) > > + if (unlikely(!fattr) || __vfs_getattr(&filp->f_path, &stat, version)) > > return; > > > > fattr->valid = (NFS_ATTR_FATTR_FILEID | > > @@ -394,7 +420,11 @@ static void nfs_local_vfs_getattr(struct nfs_local_kiocb *iocb) > > fattr->atime = stat.atime; > > fattr->mtime = stat.mtime; > > fattr->ctime = stat.ctime; > > - fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime); > > + if (version == 4) { > > + fattr->change_attr = > > + __nfsd4_change_attribute(&stat, file_inode(filp)); > > + } else > > + fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime); > > fattr->du.nfs3.used = stat.blocks << 9; > > } > > > > -- > > 2.44.0 > > > > -- > Chuck Lever >
