Re: [PATCH 6/9] nfsd: allow admin-revoked NFSv4.0 state to be freed.

[Chuck Lever <chuck.lever@oracle.com>]

On Mon, Nov 20, 2023 at 09:47:03AM +1100, NeilBrown wrote:
> On Sat, 18 Nov 2023, Chuck Lever wrote:
> > On Fri, Nov 17, 2023 at 01:18:52PM +1100, NeilBrown wrote:
> > > For NFSv4.1 and later the client easily discovers if there is any
> > > admin-revoked state and will then find and explicitly free it.
> > > 
> > > For NFSv4.0 there is no such mechanism.  The client can only find that
> > > state is admin-revoked if it tries to use that state, and there is no
> > > way for it to explicitly free the state.  So the server must hold on to
> > > the stateid (at least) for an indefinite amount of time.  A
> > > RELEASE_LOCKOWNER request might justify forgetting some of these
> > > stateids, as would the whole clients lease lapsing, but these are not
> > > reliable.
> > > 
> > > This patch takes two approaches.
> > > 
> > > Whenever a client uses an revoked stateid, that stateid is then
> > > discarded and will not be recognised again.  This might confuse a client
> > > which expect to get NFS4ERR_ADMIN_REVOKED consistently once it get it at
> > > all, but should mostly work.  Hopefully one error will lead to other
> > > resources being closed (e.g.  process exits), which will result in more
> > > stateid being freed when a CLOSE attempt gets NFS4ERR_ADMIN_REVOKED.
> > > 
> > > Also, any admin-revoked stateids that have been that way for more than
> > > one lease time are periodically revoke.
> > > 
> > > No actual freeing of state happens in this patch.  That will come in
> > > future patches which handle the different sorts of revoked state.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >  fs/nfsd/netns.h     |  4 ++
> > >  fs/nfsd/nfs4state.c | 97 ++++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 100 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index ec49b200b797..02f8fa095b0f 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -197,6 +197,10 @@ struct nfsd_net {
> > >  	atomic_t		nfsd_courtesy_clients;
> > >  	struct shrinker		nfsd_client_shrinker;
> > >  	struct work_struct	nfsd_shrinker_work;
> > > +
> > > +	/* last time an admin-revoke happened for NFSv4.0 */
> > > +	time64_t		nfs40_last_revoke;
> > > +
> > >  };
> > 
> > This hunk doesn't apply to nfsd-next due to v6.7-rc changes to NFSD
> > to implement a dynamic shrinker. So I stopped my review here for
> > now.
> 
> I didn't a rebase onto nfsd-next and there were no conflicts!
> 
> I guess the change from
>   	struct work_struct	nfsd_shrinker_work;
> to
>   	struct work_struct	*nfsd_shrinker_work;
> 
> was technically a conflict but I'm surprised your tool complained..

That was the change I noticed when it failed to apply. "stg import"
is pretty picky, unfortunately...


-- 
Chuck Lever