Re: [PATCH v5 09/10] nfsd: handle delegated timestamps in SETATTR

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2024-12-12 at 16:06 -0500, Chuck Lever wrote:
> On 12/9/24 4:14 PM, Jeff Layton wrote:
> > Allow SETATTR to handle delegated timestamps. This patch assumes that
> > only the delegation holder has the ability to set the timestamps in this
> > way, so we allow this only if the SETATTR stateid refers to a
> > *_ATTRS_DELEG delegation.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++++++++++++---
> >   fs/nfsd/nfs4state.c |  2 +-
> >   fs/nfsd/nfs4xdr.c   | 20 ++++++++++++++++++++
> >   fs/nfsd/nfsd.h      |  5 ++++-
> >   4 files changed, 53 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index f8a10f90bc7a4b288c20d2733c85f331cc0a8dba..fea171ffed623818c61886b786339b0b73f1053d 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1135,18 +1135,43 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >   		.na_iattr	= &setattr->sa_iattr,
> >   		.na_seclabel	= &setattr->sa_label,
> >   	};
> > +	bool save_no_wcc, deleg_attrs;
> > +	struct nfs4_stid *st = NULL;
> >   	struct inode *inode;
> >   	__be32 status = nfs_ok;
> > -	bool save_no_wcc;
> >   	int err;
> >   
> > -	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > +	deleg_attrs = setattr->sa_bmval[2] & (FATTR4_WORD2_TIME_DELEG_ACCESS |
> > +					      FATTR4_WORD2_TIME_DELEG_MODIFY);
> > +
> > +	if (deleg_attrs || (setattr->sa_iattr.ia_valid & ATTR_SIZE)) {
> > +		int flags = WR_STATE;
> > +
> > +		if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS)
> > +			flags |= RD_STATE;
> > +
> >   		status = nfs4_preprocess_stateid_op(rqstp, cstate,
> >   				&cstate->current_fh, &setattr->sa_stateid,
> > -				WR_STATE, NULL, NULL);
> > +				flags, NULL, &st);
> >   		if (status)
> >   			return status;
> >   	}
> > +
> > +	if (deleg_attrs) {
> > +		status = nfserr_bad_stateid;
> > +		if (st->sc_type & SC_TYPE_DELEG) {
> > +			struct nfs4_delegation *dp = delegstateid(st);
> > +
> > +			/* Only for *_ATTRS_DELEG flavors */
> > +			if (deleg_attrs_deleg(dp->dl_type))
> > +				status = nfs_ok;
> > +		}
> > +	}
> > +	if (st)
> > +		nfs4_put_stid(st);
> > +	if (status)
> > +		return status;
> > +
> >   	err = fh_want_write(&cstate->current_fh);
> >   	if (err)
> >   		return nfserrno(err);
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c882eeba7830b0249ccd74654f81e63b12a30f14..a76e35f86021c5657e31e4fddf08cb5781f01e32 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5486,7 +5486,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
> >   static inline __be32
> >   nfs4_check_delegmode(struct nfs4_delegation *dp, int flags)
> >   {
> > -	if ((flags & WR_STATE) && deleg_is_read(dp->dl_type))
> > +	if (!(flags & RD_STATE) && deleg_is_read(dp->dl_type))
> >   		return nfserr_openmode;
> >   	else
> >   		return nfs_ok;
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 0561c99b5def2eccf679bf3ea0e5b1a57d5d8374..ce93a31ac5cec75b0f944d288e796e7a73641572 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -521,6 +521,26 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
> >   		*umask = mask & S_IRWXUGO;
> >   		iattr->ia_valid |= ATTR_MODE;
> >   	}
> > +	if (bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
> > +		fattr4_time_deleg_access access;
> > +
> > +		if (!xdrgen_decode_fattr4_time_deleg_access(argp->xdr, &access))
> > +			return nfserr_bad_xdr;
> > +		iattr->ia_atime.tv_sec = access.seconds;
> > +		iattr->ia_atime.tv_nsec = access.nseconds;
> > +		iattr->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET | ATTR_DELEG;
> > +	}
> > +	if (bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
> > +		fattr4_time_deleg_modify modify;
> > +
> > +		if (!xdrgen_decode_fattr4_time_deleg_modify(argp->xdr, &modify))
> > +			return nfserr_bad_xdr;
> > +		iattr->ia_mtime.tv_sec = modify.seconds;
> > +		iattr->ia_mtime.tv_nsec = modify.nseconds;
> > +		iattr->ia_ctime.tv_sec = modify.seconds;
> > +		iattr->ia_ctime.tv_nsec = modify.seconds;
> > +		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG;
> > +	}
> >   
> >   	/* request sanity: did attrlist4 contain the expected number of words? */
> >   	if (attrlist4_count != xdr_stream_pos(argp->xdr) - starting_pos)
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 004415651295891b3440f52a4c986e3a668a48cb..f007699aa397fe39042d80ccd568db4654d19dd5 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -531,7 +531,10 @@ static inline bool nfsd_attrs_supported(u32 minorversion, const u32 *bmval)
> >   #endif
> >   #define NFSD_WRITEABLE_ATTRS_WORD2 \
> >   	(FATTR4_WORD2_MODE_UMASK \
> > -	| MAYBE_FATTR4_WORD2_SECURITY_LABEL)
> > +	| MAYBE_FATTR4_WORD2_SECURITY_LABEL \
> > +	| FATTR4_WORD2_TIME_DELEG_ACCESS \
> > +	| FATTR4_WORD2_TIME_DELEG_MODIFY \
> > +	)
> >   
> >   #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \
> >   	NFSD_WRITEABLE_ATTRS_WORD0
> > 
> 
> Hi Jeff-
> 
> After this patch is applied, I see failures of the git regression suite
> on NFSv4.2 mounts.
> 
> Test Summary Report
> -------------------
> ./t3412-rebase-root.sh                             (Wstat: 256 (exited 
> 1) Tests: 25 Failed: 5)
>    Failed tests:  6, 19, 21-22, 24
>    Non-zero exit status: 1
> ./t3400-rebase.sh                                  (Wstat: 256 (exited 
> 1) Tests: 38 Failed: 1)
>    Failed test:  31
>    Non-zero exit status: 1
> ./t3406-rebase-message.sh                          (Wstat: 256 (exited 
> 1) Tests: 32 Failed: 2)
>    Failed tests:  15, 20
>    Non-zero exit status: 1
> ./t3428-rebase-signoff.sh                          (Wstat: 256 (exited 
> 1) Tests: 7 Failed: 2)
>    Failed tests:  6-7
>    Non-zero exit status: 1
> ./t3418-rebase-continue.sh                         (Wstat: 256 (exited 
> 1) Tests: 29 Failed: 1)
>    Failed test:  7
>    Non-zero exit status: 1
> ./t3415-rebase-autosquash.sh                       (Wstat: 256 (exited 
> 1) Tests: 27 Failed: 2)
>    Failed tests:  3-4
>    Non-zero exit status: 1
> ./t3404-rebase-interactive.sh                      (Wstat: 256 (exited 
> 1) Tests: 131 Failed: 15)
>    Failed tests:  32, 34-43, 45, 121-123
>    Non-zero exit status: 1
> ./t1013-read-tree-submodule.sh                     (Wstat: 256 (exited 
> 1) Tests: 68 Failed: 1)
>    Failed test:  34
>    Non-zero exit status: 1
> ./t2013-checkout-submodule.sh                      (Wstat: 256 (exited 
> 1) Tests: 74 Failed: 4)
>    Failed tests:  26-27, 30-31
>    Non-zero exit status: 1
> ./t5500-fetch-pack.sh                              (Wstat: 256 (exited 
> 1) Tests: 375 Failed: 1)
>    Failed test:  28
>    Non-zero exit status: 1
> ./t5572-pull-submodule.sh                          (Wstat: 256 (exited 
> 1) Tests: 67 Failed: 2)
>    Failed tests:  5, 7
>    Non-zero exit status: 1
> Files=1007, Tests=30810, 1417 wallclock secs (11.18 usr 10.17 sys + 
> 1037.05 cusr 6529.12 csys = 7587.52 CPU)
> Result: FAIL
> 
> The NFS client and NFS server under test are running the same v6.13-rc2
> kernel from my git.kernel.org nfsd-testing branch.
> 
> 

I'm not seeing these failures. I ran the gitr suite under kdevops with
your nfsd-testing branch (6.13.0-rc2-ge9a809c5714e):

All tests successful.
Files=1007, Tests=30695, 10767 wallclock secs (13.87 usr 16.86 sys + 1160.76 cusr 17870.80 csys = 19062.29 CPU)
Result: PASS

...and looking at the results of those specific tests, they did run and
they did pass.

I'm rerunning the tests now. It's possible the underlying fs matters.
Mine is exporting xfs. Yours?
-- 
Jeff Layton <jlayton@kernel.org>