On Tue, 16 Jul 2024, Jeff Layton wrote:
> On Mon, 2024-07-15 at 17:14 +1000, NeilBrown wrote:
> > The heuristic for limiting the number of incoming connections to nfsd
> > currently uses sv_nrthreads - allowing more connections if more threads
> > were configured.
> >
> > A future patch will allow number of threads to grow dynamically so that
> > there is no need to configure sv_nrthreads. So we need a different
> > solution for limiting connections.
> >
> > It isn't clear what problem is solved by limiting connections (as
> > mentioned in a code comment) but the most likely problem is a connection
> > storm - many connections that are not doing productive work. These will
> > be closed after about 6 minutes already but it might help to slow down a
> > storm.
> >
> > This patch add a per-connection flag XPT_PEER_VALID which indicates
> > that the peer has presented a filehandle for which it has some sort of
> > access. i.e the peer is known to be trusted in some way. We now only
> > count connections which have NOT be determined to be valid. There
> > should be relative few of these at any given time.
> >
> > If the number of non-validated peer exceed as limit - currently 64 - we
> > close the oldest non-validated peer to avoid having too many of these
> > useless connections.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/nfsd/netns.h | 4 ++--
> > fs/nfsd/nfsfh.c | 8 ++++++++
> > include/linux/sunrpc/svc.h | 2 +-
> > include/linux/sunrpc/svc_xprt.h | 4 ++++
> > net/sunrpc/svc_xprt.c | 33 +++++++++++++++++----------------
> > 5 files changed, 32 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index 238fc4e56e53..0d2ac15a5003 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -128,8 +128,8 @@ struct nfsd_net {
> > unsigned char writeverf[8];
> >
> > /*
> > - * Max number of connections this nfsd container will allow. Defaults
> > - * to '0' which is means that it bases this on the number of threads.
> > + * Max number of non-validated connections this nfsd container
> > + * will allow. Defaults to '0' gets mapped to 64.
> > */
> > unsigned int max_connections;
> >
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 0b75305fb5f5..08742bf8de02 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -391,6 +391,14 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access)
> > goto out;
> >
> > skip_pseudoflavor_check:
> > + if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags) &&
> > + !test_and_set_bit(XPT_PEER_VALID, &rqstp->rq_xprt->xpt_flags)) {
> > + struct svc_serv *serv = rqstp->rq_server;
> > + spin_lock(&serv->sv_lock);
> > + serv->sv_tmpcnt -= 1;
> > + spin_unlock(&serv->sv_lock);
> > + }
> > +
>
> This is the only place you set XPT_PEER_VALID, but this change affects
> more services than just nfsd. What about lockd? Do we need a similar
> change there?
Lockd calls nlmsvc_ops->fopen which is nlm_fopen() which calls
nfsd_open() which calls fh_verify(). So lockd is safe.
The nfs callback handler might need help, but it sets ->sv_maxconn=1024,
so I think it is safe for now.
(lockd defaults nlm_max_connections to 1024, so it is also safe without
calling fh_verify. Maybe I should clean up)
Thanks,
NeilBrown
