Re: [PATCH v9 07/19] nfs/localio: fix nfs_localio_vfs_getattr() to properly support v4

[Chuck Lever <chuck.lever@oracle.com>]

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@noble.neil.brown.name>


> 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