On Thu, Aug 29, 2024 at 12:01:12PM -0400, Mike Snitzer wrote:
> On Thu, Aug 29, 2024 at 11:57:20AM -0400, Jeff Layton wrote:
> > On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> > > Introduce nfsd_serv_try_get and nfsd_serv_put and update the nfsd code
> > > to prevent nfsd_destroy_serv from destroying nn->nfsd_serv until any
> > > caller of nfsd_serv_try_get releases their reference using nfsd_serv_put.
> > >
> > > A percpu_ref is used to implement the interlock between
> > > nfsd_destroy_serv and any caller of nfsd_serv_try_get.
> > >
> > > This interlock is needed to properly wait for the completion of client
> > > initiated localio calls to nfsd (that are _not_ in the context of nfsd).
> > >
> > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/netns.h | 8 +++++++-
> > > fs/nfsd/nfssvc.c | 39 +++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 46 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index 238fc4e56e53..e2d953f21dde 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -13,6 +13,7 @@
> > > #include <linux/filelock.h>
> > > #include <linux/nfs4.h>
> > > #include <linux/percpu_counter.h>
> > > +#include <linux/percpu-refcount.h>
> > > #include <linux/siphash.h>
> > > #include <linux/sunrpc/stats.h>
> > >
> > > @@ -139,7 +140,9 @@ struct nfsd_net {
> > >
> > > struct svc_info nfsd_info;
> > > #define nfsd_serv nfsd_info.serv
> > > -
> > > + struct percpu_ref nfsd_serv_ref;
> > > + struct completion nfsd_serv_confirm_done;
> > > + struct completion nfsd_serv_free_done;
> > >
> > > /*
> > > * clientid and stateid data for construction of net unique COPY
> > > @@ -221,6 +224,9 @@ struct nfsd_net {
> > > extern bool nfsd_support_version(int vers);
> > > extern unsigned int nfsd_net_id;
> > >
> > > +bool nfsd_serv_try_get(struct nfsd_net *nn);
> > > +void nfsd_serv_put(struct nfsd_net *nn);
> > > +
> > > void nfsd_copy_write_verifier(__be32 verf[2], struct nfsd_net *nn);
> > > void nfsd_reset_write_verifier(struct nfsd_net *nn);
> > > #endif /* __NFSD_NETNS_H__ */
> > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > > index defc430f912f..e43d440f9f0a 100644
> > > --- a/fs/nfsd/nfssvc.c
> > > +++ b/fs/nfsd/nfssvc.c
> > > @@ -193,6 +193,30 @@ int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change
> > > return 0;
> > > }
> > >
> > > +bool nfsd_serv_try_get(struct nfsd_net *nn)
> > > +{
> > > + return percpu_ref_tryget_live(&nn->nfsd_serv_ref);
> > > +}
> > > +
> > > +void nfsd_serv_put(struct nfsd_net *nn)
> > > +{
> > > + percpu_ref_put(&nn->nfsd_serv_ref);
> > > +}
> > > +
> > > +static void nfsd_serv_done(struct percpu_ref *ref)
> > > +{
> > > + struct nfsd_net *nn = container_of(ref, struct nfsd_net, nfsd_serv_ref);
> > > +
> > > + complete(&nn->nfsd_serv_confirm_done);
> > > +}
> > > +
> > > +static void nfsd_serv_free(struct percpu_ref *ref)
> > > +{
> > > + struct nfsd_net *nn = container_of(ref, struct nfsd_net, nfsd_serv_ref);
> > > +
> > > + complete(&nn->nfsd_serv_free_done);
> > > +}
> > > +
> > > /*
> > > * Maximum number of nfsd processes
> > > */
> > > @@ -392,6 +416,7 @@ static void nfsd_shutdown_net(struct net *net)
> > > lockd_down(net);
> > > nn->lockd_up = false;
> > > }
> > > + percpu_ref_exit(&nn->nfsd_serv_ref);
> > > nn->nfsd_net_up = false;
> > > nfsd_shutdown_generic();
> > > }
> > > @@ -471,6 +496,13 @@ void nfsd_destroy_serv(struct net *net)
> > > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > struct svc_serv *serv = nn->nfsd_serv;
> > >
> > > + lockdep_assert_held(&nfsd_mutex);
> > > +
> > > + percpu_ref_kill_and_confirm(&nn->nfsd_serv_ref, nfsd_serv_done);
> > > + wait_for_completion(&nn->nfsd_serv_confirm_done);
> > > + wait_for_completion(&nn->nfsd_serv_free_done);
> > > + /* percpu_ref_exit is called in nfsd_shutdown_net */
> > > +
> > > spin_lock(&nfsd_notifier_lock);
> > > nn->nfsd_serv = NULL;
> > > spin_unlock(&nfsd_notifier_lock);
> > > @@ -595,6 +627,13 @@ int nfsd_create_serv(struct net *net)
> > > if (nn->nfsd_serv)
> > > return 0;
> > >
> > > + error = percpu_ref_init(&nn->nfsd_serv_ref, nfsd_serv_free,
> > > + 0, GFP_KERNEL);
> > > + if (error)
> > > + return error;
> > > + init_completion(&nn->nfsd_serv_free_done);
> > > + init_completion(&nn->nfsd_serv_confirm_done);
> > > +
> > > if (nfsd_max_blksize == 0)
> > > nfsd_max_blksize = nfsd_get_default_max_blksize();
> > > nfsd_reset_versions(nn);
> >
> > A little hard to review this one at this point in the series, as there
> > are no callers of get/put yet, but the concept seems reasonable.
> >
> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>
> Thanks, yeah Chuck asked that I factor this interlock interface out to
> a separate patch because it was a bit much buried in the next patch
> that actually consumes it.
Yes, and to add some rationale for it. I know folks don't like the
addition of new functions before their callers are introduced.
Thanks!
--
Chuck Lever
