Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2024-02-19 at 08:57 +1100, NeilBrown wrote:
> On Fri, 16 Feb 2024, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > behaviour
> > when doing a SETATTR rpc call by stripping out the calls to
> > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > 
> > Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
> > fh_(un)lock for file operations")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 4 ++++
> >  fs/nfsd/vfs.c      | 9 +++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 14712fa08f76..e6d8624efc83 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  	};
> >  	struct inode *inode;
> >  	__be32 status = nfs_ok;
> > +	bool save_no_wcc;
> >  	int err;
> >  
> >  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  
> >  	if (status)
> >  		goto out;
> > +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> > +	cstate->current_fh.fh_no_wcc = true;
> >  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> >  				0, (time64_t)0);
> > +	cstate->current_fh.fh_no_wcc = save_no_wcc;
> 
> This looks clumsy.
> I think the background is that NFSv3 needs atomic wcc attributes for
> file operations, but NFSv4 doesn't - it only has them for directory
> ops.
> So NFSv4, like NFSv2, doesn't want fh_fill_pre_attrs() to be called
> by
> nfsd_setattr().
> 
> NFSv2 avoids it by always setting ->fh_no_wcc.  Here you temporarily
> set
> fh_no_wcc to true for the same effect.  So the code is correct.
> But it is not obvious to the casual reader why this is happening.
> 
> I would rather a "wcc_wanted" flag or similar, but that can be done
> in a
> separate clean-up patch later.

That is in theory what the fh_no_wcc flag is for, however the issue is
that it got overloaded to also mean 'change_info4 wanted' when we added
support for NFSv4 to knfsd.
NFSv4 does not have a concept of weak cache consistency, but it does
try to track updates to the change attribute atomically (ideally) for
most operations that change the directory contents.

IOW: I think a better clean up would be to separate out 'wcc' and
'change_info4' as representing different functionality.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com