Re: [RFC PATCH v2 2/8] NFSD: Move the fh_getattr() helper

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-07-08 at 12:06 -0400, Mike Snitzer wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Clean up: The fh_getattr() function is part of NFSD's file handle
> API, so relocate it.
> 
> I've made it an un-inlined function so that trace points and new
> functionality can easily be introduced. That increases the size of
> nfsd.ko by about a page on my x86_64 system (out of 26MB; compiled
> with -O2).
> 

Weird. I would have expected making it uninlined would decrease the
size of the binary not increase it. There are quite a few fh_getattr()
calls.

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfsd/nfsfh.c | 23 +++++++++++++++++++++++
>  fs/nfsd/nfsfh.h |  1 +
>  fs/nfsd/vfs.h   | 13 -------------
>  3 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 16e6b4428d55..f4a3cc9e31e0 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -663,6 +663,29 @@ fh_update(struct svc_fh *fhp)
>  	return nfserr_serverfault;
>  }
>  
> +/**
> + * fh_getattr - Retrieve attributes on a local file
> + * @fhp: File handle of target file
> + * @stat: Caller-supplied kstat buffer to be filled in
> + *
> + * Returns nfs_ok on success, otherwise an NFS status code is
> + * returned.
> + */
> +__be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
> +{
> +	struct path p = {
> +		.mnt		= fhp->fh_export->ex_path.mnt,
> +		.dentry		= fhp->fh_dentry,
> +	};
> +	u32 request_mask = STATX_BASIC_STATS;
> +
> +	if (fhp->fh_maxsize == NFS4_FHSIZE)
> +		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
> +
> +	return nfserrno(vfs_getattr(&p, stat, request_mask,
> +				    AT_STATX_SYNC_AS_STAT));
> +}
> +
>  /**
>   * fh_fill_pre_attrs - Fill in pre-op attributes
>   * @fhp: file handle to be updated
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 6f5255d1c190..5ef7191f8ad8 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -222,6 +222,7 @@ extern char * SVCFH_fmt(struct svc_fh *fhp);
>  __be32	fh_verify(struct svc_rqst *, struct svc_fh *, umode_t, int);
>  __be32	fh_verify_local(struct net *, struct svc_cred *, struct auth_domain *,
>  			struct svc_fh *, umode_t, int);
> +__be32	fh_getattr(const struct svc_fh *fhp, struct kstat *stat);
>  __be32	fh_compose(struct svc_fh *, struct svc_export *, struct dentry *, struct svc_fh *);
>  __be32	fh_update(struct svc_fh *);
>  void	fh_put(struct svc_fh *);
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 4007dcbbbfef..0c0292611c6d 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -160,17 +160,4 @@ __be32		nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
>  
>  void		nfsd_filp_close(struct file *fp);
>  
> -static inline __be32 fh_getattr(const struct svc_fh *fh, struct kstat *stat)
> -{
> -	u32 request_mask = STATX_BASIC_STATS;
> -	struct path p = {.mnt = fh->fh_export->ex_path.mnt,
> -			 .dentry = fh->fh_dentry};
> -
> -	if (fh->fh_maxsize == NFS4_FHSIZE)
> -		request_mask |= (STATX_BTIME | STATX_CHANGE_COOKIE);
> -
> -	return nfserrno(vfs_getattr(&p, stat, request_mask,
> -				    AT_STATX_SYNC_AS_STAT));
> -}
> -
>  #endif /* LINUX_NFSD_VFS_H */

In any case, I don't see a real benefit in keeping this as a static
inline.

Reviewed-by: Jeff Layton <jlayton@kernel.org>