[PATCH 1/6] nfsd: drop inode parameter from nfsd4_change_attribute()

Jeff Layton posted 6 patches 1 month, 1 week ago
[PATCH 1/6] nfsd: drop inode parameter from nfsd4_change_attribute()
Posted by Jeff Layton 1 month, 1 week ago
Fix up nfs4_delegation_stat() to fetch STATX_MODE, and then drop the
inode parameter from nfsd4_change_attribute(), since it's no longer
needed.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c |  5 ++---
 fs/nfsd/nfs4xdr.c   |  2 +-
 fs/nfsd/nfsfh.c     | 11 ++++-------
 fs/nfsd/nfsfh.h     |  3 +--
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d753926db09eedf629fc3e0938f10b1a6fdb0245..2961a277a79c1f4cdb8c29a7c19abcb3305b61a1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5953,7 +5953,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
 	path.dentry = file_dentry(nf->nf_file);
 
 	rc = vfs_getattr(&path, stat,
-			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
+			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
 			 AT_STATX_SYNC_AS_STAT);
 
 	nfsd_file_put(nf);
@@ -6037,8 +6037,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		}
 		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
 		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
-		dp->dl_cb_fattr.ncf_initial_cinfo =
-			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
+		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
 	} else {
 		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 6286ad2afa069f5274ffa352209b7d3c8c577dac..da7ec663da7326ad5c68a9c738b12d09cfcdc65a 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3621,7 +3621,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
 		args.change_attr = ncf->ncf_initial_cinfo;
 		nfs4_put_stid(&dp->dl_stid);
 	} else {
-		args.change_attr = nfsd4_change_attribute(&args.stat, d_inode(dentry));
+		args.change_attr = nfsd4_change_attribute(&args.stat);
 	}
 
 	if (err)
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 4c5deea0e9535f2b197aa6ca1786d61730d53c44..453b7b52317d538971ce41f7e0492e5ab28b236d 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -670,20 +670,18 @@ fh_update(struct svc_fh *fhp)
 __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
-	struct inode *inode;
 	struct kstat stat;
 	__be32 err;
 
 	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
 		return nfs_ok;
 
-	inode = d_inode(fhp->fh_dentry);
 	err = fh_getattr(fhp, &stat);
 	if (err)
 		return err;
 
 	if (v4)
-		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
+		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
 
 	fhp->fh_pre_mtime = stat.mtime;
 	fhp->fh_pre_ctime = stat.ctime;
@@ -700,7 +698,6 @@ __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
 __be32 fh_fill_post_attrs(struct svc_fh *fhp)
 {
 	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
-	struct inode *inode = d_inode(fhp->fh_dentry);
 	__be32 err;
 
 	if (fhp->fh_no_wcc)
@@ -716,7 +713,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
 	fhp->fh_post_saved = true;
 	if (v4)
 		fhp->fh_post_change =
-			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
+			nfsd4_change_attribute(&fhp->fh_post_attr);
 	return nfs_ok;
 }
 
@@ -824,13 +821,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
  * assume that the new change attr is always logged to stable storage in some
  * fashion before the results can be seen.
  */
-u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
+u64 nfsd4_change_attribute(const struct kstat *stat)
 {
 	u64 chattr;
 
 	if (stat->result_mask & STATX_CHANGE_COOKIE) {
 		chattr = stat->change_cookie;
-		if (S_ISREG(inode->i_mode) &&
+		if (S_ISREG(stat->mode) &&
 		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
 			chattr += (u64)stat->ctime.tv_sec << 30;
 			chattr += stat->ctime.tv_nsec;
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
 	fhp->fh_pre_saved = false;
 }
 
-u64 nfsd4_change_attribute(const struct kstat *stat,
-			   const struct inode *inode);
+u64 nfsd4_change_attribute(const struct kstat *stat);
 __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
 __be32 fh_fill_post_attrs(struct svc_fh *fhp);
 __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);

-- 
2.47.0
Re: [PATCH 1/6] nfsd: drop inode parameter from nfsd4_change_attribute()
Posted by Chuck Lever 1 month, 1 week ago
Hey Jeff -

On Mon, Oct 14, 2024 at 03:26:49PM -0400, Jeff Layton wrote:
> Fix up nfs4_delegation_stat() to fetch STATX_MODE,

The patch description isn't clear about why this change is needed.
After reading through the other patches, I'm not sure I'm any more
enlightened about it ;-)


> and then drop the
> inode parameter from nfsd4_change_attribute(), since it's no longer
> needed.

Since nfsd4_change_attribute() expects @stat to be filled in by the
caller, it needs a kdoc-style comment that documents that part of
the API contract.

I can add one when applying this patch, unless you would like to
resend this one or send me something to squash into this change.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c |  5 ++---
>  fs/nfsd/nfs4xdr.c   |  2 +-
>  fs/nfsd/nfsfh.c     | 11 ++++-------
>  fs/nfsd/nfsfh.h     |  3 +--
>  4 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d753926db09eedf629fc3e0938f10b1a6fdb0245..2961a277a79c1f4cdb8c29a7c19abcb3305b61a1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5953,7 +5953,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>  	path.dentry = file_dentry(nf->nf_file);
>  
>  	rc = vfs_getattr(&path, stat,
> -			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> +			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
>  			 AT_STATX_SYNC_AS_STAT);
>  
>  	nfsd_file_put(nf);
> @@ -6037,8 +6037,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		}
>  		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
>  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
> -		dp->dl_cb_fattr.ncf_initial_cinfo =
> -			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
> +		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
>  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
>  	} else {
>  		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 6286ad2afa069f5274ffa352209b7d3c8c577dac..da7ec663da7326ad5c68a9c738b12d09cfcdc65a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3621,7 +3621,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  		args.change_attr = ncf->ncf_initial_cinfo;
>  		nfs4_put_stid(&dp->dl_stid);
>  	} else {
> -		args.change_attr = nfsd4_change_attribute(&args.stat, d_inode(dentry));
> +		args.change_attr = nfsd4_change_attribute(&args.stat);
>  	}
>  
>  	if (err)
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 4c5deea0e9535f2b197aa6ca1786d61730d53c44..453b7b52317d538971ce41f7e0492e5ab28b236d 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -670,20 +670,18 @@ fh_update(struct svc_fh *fhp)
>  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
>  {
>  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> -	struct inode *inode;
>  	struct kstat stat;
>  	__be32 err;
>  
>  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
>  		return nfs_ok;
>  
> -	inode = d_inode(fhp->fh_dentry);
>  	err = fh_getattr(fhp, &stat);
>  	if (err)
>  		return err;
>  
>  	if (v4)
> -		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> +		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
>  
>  	fhp->fh_pre_mtime = stat.mtime;
>  	fhp->fh_pre_ctime = stat.ctime;
> @@ -700,7 +698,6 @@ __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
>  __be32 fh_fill_post_attrs(struct svc_fh *fhp)
>  {
>  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> -	struct inode *inode = d_inode(fhp->fh_dentry);
>  	__be32 err;
>  
>  	if (fhp->fh_no_wcc)
> @@ -716,7 +713,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
>  	fhp->fh_post_saved = true;
>  	if (v4)
>  		fhp->fh_post_change =
> -			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> +			nfsd4_change_attribute(&fhp->fh_post_attr);
>  	return nfs_ok;
>  }
>  
> @@ -824,13 +821,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
>   * assume that the new change attr is always logged to stable storage in some
>   * fashion before the results can be seen.
>   */
> -u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
> +u64 nfsd4_change_attribute(const struct kstat *stat)
>  {
>  	u64 chattr;
>  
>  	if (stat->result_mask & STATX_CHANGE_COOKIE) {
>  		chattr = stat->change_cookie;
> -		if (S_ISREG(inode->i_mode) &&
> +		if (S_ISREG(stat->mode) &&
>  		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
>  			chattr += (u64)stat->ctime.tv_sec << 30;
>  			chattr += stat->ctime.tv_nsec;
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
>  	fhp->fh_pre_saved = false;
>  }
>  
> -u64 nfsd4_change_attribute(const struct kstat *stat,
> -			   const struct inode *inode);
> +u64 nfsd4_change_attribute(const struct kstat *stat);
>  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
>  __be32 fh_fill_post_attrs(struct svc_fh *fhp);
>  __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);
> 
> -- 
> 2.47.0
> 

-- 
Chuck Lever
Re: [PATCH 1/6] nfsd: drop inode parameter from nfsd4_change_attribute()
Posted by Jeff Layton 1 month, 1 week ago
On Tue, 2024-10-15 at 14:29 -0400, Chuck Lever wrote:
> Hey Jeff -
> 
> On Mon, Oct 14, 2024 at 03:26:49PM -0400, Jeff Layton wrote:
> > Fix up nfs4_delegation_stat() to fetch STATX_MODE,
> 
> The patch description isn't clear about why this change is needed.
> After reading through the other patches, I'm not sure I'm any more
> enlightened about it ;-)
> 

My original thinking was that this patch was just a cleanup and
simplification, but now that I look, I think the inode we were passing
to this function in nfs4_open_delegation was wrong and that could throw
off the result in some cases.

Maybe we should add a Fixes: tag for this:

    bf92e5008b17 nfsd: fix initial getattr on write delegation

...since that should have fixed this call as well.

> 
> > and then drop the
> > inode parameter from nfsd4_change_attribute(), since it's no longer
> > needed.
> 
> Since nfsd4_change_attribute() expects @stat to be filled in by the
> caller, it needs a kdoc-style comment that documents that part of
> the API contract.
> 

Agreed. It needs STATX_MODE and STATX_CTIME. It can also make use of
STATX_CHANGE_COOKIE if present.

> I can add one when applying this patch, unless you would like to
> resend this one or send me something to squash into this change.
> 

That sounds great. A kdoc comment over that is a good idea.

> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c |  5 ++---
> >  fs/nfsd/nfs4xdr.c   |  2 +-
> >  fs/nfsd/nfsfh.c     | 11 ++++-------
> >  fs/nfsd/nfsfh.h     |  3 +--
> >  4 files changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index d753926db09eedf629fc3e0938f10b1a6fdb0245..2961a277a79c1f4cdb8c29a7c19abcb3305b61a1 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5953,7 +5953,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
> >  	path.dentry = file_dentry(nf->nf_file);
> >  
> >  	rc = vfs_getattr(&path, stat,
> > -			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> > +			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> >  			 AT_STATX_SYNC_AS_STAT);
> >  
> >  	nfsd_file_put(nf);
> > @@ -6037,8 +6037,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> >  		}
> >  		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
> >  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
> > -		dp->dl_cb_fattr.ncf_initial_cinfo =
> > -			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
> > +		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
> >  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> >  	} else {
> >  		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 6286ad2afa069f5274ffa352209b7d3c8c577dac..da7ec663da7326ad5c68a9c738b12d09cfcdc65a 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3621,7 +3621,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> >  		args.change_attr = ncf->ncf_initial_cinfo;
> >  		nfs4_put_stid(&dp->dl_stid);
> >  	} else {
> > -		args.change_attr = nfsd4_change_attribute(&args.stat, d_inode(dentry));
> > +		args.change_attr = nfsd4_change_attribute(&args.stat);
> >  	}
> >  
> >  	if (err)
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 4c5deea0e9535f2b197aa6ca1786d61730d53c44..453b7b52317d538971ce41f7e0492e5ab28b236d 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -670,20 +670,18 @@ fh_update(struct svc_fh *fhp)
> >  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
> >  {
> >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > -	struct inode *inode;
> >  	struct kstat stat;
> >  	__be32 err;
> >  
> >  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> >  		return nfs_ok;
> >  
> > -	inode = d_inode(fhp->fh_dentry);
> >  	err = fh_getattr(fhp, &stat);
> >  	if (err)
> >  		return err;
> >  
> >  	if (v4)
> > -		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> > +		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
> >  
> >  	fhp->fh_pre_mtime = stat.mtime;
> >  	fhp->fh_pre_ctime = stat.ctime;
> > @@ -700,7 +698,6 @@ __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
> >  __be32 fh_fill_post_attrs(struct svc_fh *fhp)
> >  {
> >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > -	struct inode *inode = d_inode(fhp->fh_dentry);
> >  	__be32 err;
> >  
> >  	if (fhp->fh_no_wcc)
> > @@ -716,7 +713,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
> >  	fhp->fh_post_saved = true;
> >  	if (v4)
> >  		fhp->fh_post_change =
> > -			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> > +			nfsd4_change_attribute(&fhp->fh_post_attr);
> >  	return nfs_ok;
> >  }
> >  
> > @@ -824,13 +821,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
> >   * assume that the new change attr is always logged to stable storage in some
> >   * fashion before the results can be seen.
> >   */
> > -u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
> > +u64 nfsd4_change_attribute(const struct kstat *stat)
> >  {
> >  	u64 chattr;
> >  
> >  	if (stat->result_mask & STATX_CHANGE_COOKIE) {
> >  		chattr = stat->change_cookie;
> > -		if (S_ISREG(inode->i_mode) &&
> > +		if (S_ISREG(stat->mode) &&
> >  		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
> >  			chattr += (u64)stat->ctime.tv_sec << 30;
> >  			chattr += stat->ctime.tv_nsec;
> > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
> > --- a/fs/nfsd/nfsfh.h
> > +++ b/fs/nfsd/nfsfh.h
> > @@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
> >  	fhp->fh_pre_saved = false;
> >  }
> >  
> > -u64 nfsd4_change_attribute(const struct kstat *stat,
> > -			   const struct inode *inode);
> > +u64 nfsd4_change_attribute(const struct kstat *stat);
> >  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
> >  __be32 fh_fill_post_attrs(struct svc_fh *fhp);
> >  __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);
> > 
> > -- 
> > 2.47.0
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>
Re: [PATCH 1/6] nfsd: drop inode parameter from nfsd4_change_attribute()
Posted by Chuck Lever 1 month, 1 week ago
On Tue, Oct 15, 2024 at 02:42:10PM -0400, Jeff Layton wrote:
> On Tue, 2024-10-15 at 14:29 -0400, Chuck Lever wrote:
> > Hey Jeff -
> > 
> > On Mon, Oct 14, 2024 at 03:26:49PM -0400, Jeff Layton wrote:
> > > Fix up nfs4_delegation_stat() to fetch STATX_MODE,
> > 
> > The patch description isn't clear about why this change is needed.
> > After reading through the other patches, I'm not sure I'm any more
> > enlightened about it ;-)
> > 
> 
> My original thinking was that this patch was just a cleanup and
> simplification, but now that I look, I think the inode we were passing
> to this function in nfs4_open_delegation was wrong and that could throw
> off the result in some cases.
> 
> Maybe we should add a Fixes: tag for this:
> 
>     bf92e5008b17 nfsd: fix initial getattr on write delegation
> 
> ...since that should have fixed this call as well.
> 
> > 
> > > and then drop the
> > > inode parameter from nfsd4_change_attribute(), since it's no longer
> > > needed.
> > 
> > Since nfsd4_change_attribute() expects @stat to be filled in by the
> > caller, it needs a kdoc-style comment that documents that part of
> > the API contract.
> > 
> 
> Agreed. It needs STATX_MODE and STATX_CTIME. It can also make use of
> STATX_CHANGE_COOKIE if present.
> 
> > I can add one when applying this patch, unless you would like to
> > resend this one or send me something to squash into this change.
> > 
> 
> That sounds great. A kdoc comment over that is a good idea.

I'll make adjustments before applying.


> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/nfsd/nfs4state.c |  5 ++---
> > >  fs/nfsd/nfs4xdr.c   |  2 +-
> > >  fs/nfsd/nfsfh.c     | 11 ++++-------
> > >  fs/nfsd/nfsfh.h     |  3 +--
> > >  4 files changed, 8 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index d753926db09eedf629fc3e0938f10b1a6fdb0245..2961a277a79c1f4cdb8c29a7c19abcb3305b61a1 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -5953,7 +5953,7 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
> > >  	path.dentry = file_dentry(nf->nf_file);
> > >  
> > >  	rc = vfs_getattr(&path, stat,
> > > -			 (STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> > > +			 (STATX_MODE | STATX_SIZE | STATX_CTIME | STATX_CHANGE_COOKIE),
> > >  			 AT_STATX_SYNC_AS_STAT);
> > >  
> > >  	nfsd_file_put(nf);
> > > @@ -6037,8 +6037,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > >  		}
> > >  		open->op_delegate_type = NFS4_OPEN_DELEGATE_WRITE;
> > >  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
> > > -		dp->dl_cb_fattr.ncf_initial_cinfo =
> > > -			nfsd4_change_attribute(&stat, d_inode(currentfh->fh_dentry));
> > > +		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
> > >  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> > >  	} else {
> > >  		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index 6286ad2afa069f5274ffa352209b7d3c8c577dac..da7ec663da7326ad5c68a9c738b12d09cfcdc65a 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -3621,7 +3621,7 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
> > >  		args.change_attr = ncf->ncf_initial_cinfo;
> > >  		nfs4_put_stid(&dp->dl_stid);
> > >  	} else {
> > > -		args.change_attr = nfsd4_change_attribute(&args.stat, d_inode(dentry));
> > > +		args.change_attr = nfsd4_change_attribute(&args.stat);
> > >  	}
> > >  
> > >  	if (err)
> > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > > index 4c5deea0e9535f2b197aa6ca1786d61730d53c44..453b7b52317d538971ce41f7e0492e5ab28b236d 100644
> > > --- a/fs/nfsd/nfsfh.c
> > > +++ b/fs/nfsd/nfsfh.c
> > > @@ -670,20 +670,18 @@ fh_update(struct svc_fh *fhp)
> > >  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > > -	struct inode *inode;
> > >  	struct kstat stat;
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> > >  		return nfs_ok;
> > >  
> > > -	inode = d_inode(fhp->fh_dentry);
> > >  	err = fh_getattr(fhp, &stat);
> > >  	if (err)
> > >  		return err;
> > >  
> > >  	if (v4)
> > > -		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> > > +		fhp->fh_pre_change = nfsd4_change_attribute(&stat);
> > >  
> > >  	fhp->fh_pre_mtime = stat.mtime;
> > >  	fhp->fh_pre_ctime = stat.ctime;
> > > @@ -700,7 +698,6 @@ __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  __be32 fh_fill_post_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > > -	struct inode *inode = d_inode(fhp->fh_dentry);
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc)
> > > @@ -716,7 +713,7 @@ __be32 fh_fill_post_attrs(struct svc_fh *fhp)
> > >  	fhp->fh_post_saved = true;
> > >  	if (v4)
> > >  		fhp->fh_post_change =
> > > -			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> > > +			nfsd4_change_attribute(&fhp->fh_post_attr);
> > >  	return nfs_ok;
> > >  }
> > >  
> > > @@ -824,13 +821,13 @@ enum fsid_source fsid_source(const struct svc_fh *fhp)
> > >   * assume that the new change attr is always logged to stable storage in some
> > >   * fashion before the results can be seen.
> > >   */
> > > -u64 nfsd4_change_attribute(const struct kstat *stat, const struct inode *inode)
> > > +u64 nfsd4_change_attribute(const struct kstat *stat)
> > >  {
> > >  	u64 chattr;
> > >  
> > >  	if (stat->result_mask & STATX_CHANGE_COOKIE) {
> > >  		chattr = stat->change_cookie;
> > > -		if (S_ISREG(inode->i_mode) &&
> > > +		if (S_ISREG(stat->mode) &&
> > >  		    !(stat->attributes & STATX_ATTR_CHANGE_MONOTONIC)) {
> > >  			chattr += (u64)stat->ctime.tv_sec << 30;
> > >  			chattr += stat->ctime.tv_nsec;
> > > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > > index 5b7394801dc4270dbd5236f3e2f2237130c73dad..876152a91f122f83fb94ffdfb0eedf8fca56a20c 100644
> > > --- a/fs/nfsd/nfsfh.h
> > > +++ b/fs/nfsd/nfsfh.h
> > > @@ -297,8 +297,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
> > >  	fhp->fh_pre_saved = false;
> > >  }
> > >  
> > > -u64 nfsd4_change_attribute(const struct kstat *stat,
> > > -			   const struct inode *inode);
> > > +u64 nfsd4_change_attribute(const struct kstat *stat);
> > >  __be32 __must_check fh_fill_pre_attrs(struct svc_fh *fhp);
> > >  __be32 fh_fill_post_attrs(struct svc_fh *fhp);
> > >  __be32 __must_check fh_fill_both_attrs(struct svc_fh *fhp);
> > > 
> > > -- 
> > > 2.47.0
> > > 
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever