On Fri, 2025-12-19 at 10:58 -0500, Olga Kornievskaia wrote:
> Hi Jeff,
>
> I narrowed down the upstream failure for generic/215 and generic/407
> to this commit.
>
> Let's consider first where the kernel is compiled with delegated
> attributes off (but it also fails just the same if the delegated
> attributes are compiled in).
>
> I don't understand why the code unconditionally changed to call
> nfsd4_finalize_deleg_timestamps() which I think the main driver behind
> the failure.
>
> Running generic/407 there is an OPEN (which gives out a write
> delegation) and returns a change id, then on this filehandle there is
> a SETATTR (with a getattr) which returns a new changeid. Then there is
> a CLONE where the filehandle is the destination filehandle on which
> there is a getattr which returns unchanged changeid/modify time (bad).
> Then there is a DELEGRETURN (with a getattr) which again returns same
> change id. Test fails.
>
> Prior to this commit. The changeid/modify time is different in CLONE
> and DELEGRETURN -- test passes.
>
> Now let me describe what happens with delegated attributes enabled.
> OPEN returns delegated attributes delegation, included getattr return
> a changeid. Then CLONE is done, the included gettattr returns a
> different (from open's) changeid (different time_modify). Then there
> is SETATTR+GEATTR+DELEGRETURN compound from the client (which carries
> a time_deleg_modify value different from above). Server in getattr
> replies with changeid same as in clone and mtime with the value client
> provided. So I'm not sure exactly why the test fails here but that's a
> different problem as my focus is on "delegation attribute off option"
> at the moment.
>
> I don't know if this is the correct fix or not but perhaps we
> shouldn't unconditionally be setting this mode? (note this fix only
> fixes the delegattributes off. however i have no claims that this
> patch is what broke 215/407 for delegated attributes on. Something
> else is in play there). If this solution is acceptable, I can send a
> patch.
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 81fa7cc6c77b..624cc6ab2802 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6318,7 +6318,8 @@ nfs4_open_delegation(struct svc_rqst *rqstp,
> struct nfsd4_open *open,
> dp->dl_ctime = stat.ctime;
> dp->dl_mtime = stat.mtime;
> spin_lock(&f->f_lock);
> - f->f_mode |= FMODE_NOCMTIME;
> + if (deleg_ts)
> + f->f_mode |= FMODE_NOCMTIME;
> spin_unlock(&f->f_lock);
> trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> } else {
>
>
That patch does look correct to me -- nice catch. Have you validated
that it fixes 215 and 407?
Thanks,
Jeff
--
Jeff Layton <[email protected]>
