Re: [PATCH v4 8/8] nfsd: freeze c/mtime updates with outstanding WRITE_ATTRS delegation

[Jeff Layton <jlayton@kernel.org>]

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 <jlayton@kernel.org>