On Mon, Jul 01, 2024 at 08:01:30AM +1000, NeilBrown wrote:

On Sun, 30 Jun 2024, Chuck Lever wrote:

quoted

On Fri, Jun 28, 2024 at 05:10:53PM -0400, Mike Snitzer wrote:

quoted

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: [ref]

Linus would not be impressed.  He likes links that you can click on and
follow.

I've read email that suggests he doesn't like those either. Another
day ending in "y", I guess.

So
   Link: https://lore.kernel.org/171918165963.14261.959545364150864599@noble.neil.brown.name (local)

is preferred (at least I think that is the current state of the
conversation

see https://lore.kernel.org/all/CAHk-=wiD9du3fBHuLYzwUSdNgY+hxMZEWNZpqJXy-=wD2wafdg@mail.gmail.com/

As I read it, this refers to using Message-Id: to link to a patch
submission. I'm linking to a discussion thread, not to a patch.
Just to be clear.

NeilBrown

quoted

quoted

Suggested-by: NeilBrown <neil@brown.name>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 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

-- 
Chuck Lever