| Message ID | 20240715074657.18174-10-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | support automatic changes to nfsd thread count | expand |
On Mon, 2024-07-15 at 17:14 +1000, NeilBrown wrote: > If there are more non-courteous clients than the calculated limit, we > should fail the request rather than report a soft failure and > encouraging the client to retry indefinitely. > > If there a courteous clients which push us over the limit, then expedite > their removal. > > This is not known to have caused a problem is production use, but > testing of lots of clients reports repeated NFS4ERR_DELAY responses > which doesn't seem helpful. > > Also remove an outdated comment - we do use a slab cache. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index a20c2c9d7d45..88936f3189e1 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2212,21 +2212,20 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn) > return 1; > } > > -/* > - * XXX Should we use a slab cache ? > - * This type of memory management is somewhat inefficient, but we use it > - * anyway since SETCLIENTID is not a common operation. > - */ > static struct nfs4_client *alloc_client(struct xdr_netobj name, > struct nfsd_net *nn) > { > struct nfs4_client *clp; > int i; > > - if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients) { > + if (atomic_read(&nn->nfs4_client_count) - > + atomic_read(&nn->nfsd_courtesy_clients) >= nn->nfs4_max_clients) > + return ERR_PTR(-EREMOTEIO); > + nit: I know it gets remapped, but why EREMOTEIO? From nfsd's standpoint this would seem to imply a problem on the client. Maybe: #define EUSERS 87 /* Too many users */ ...instead? > + if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients && > + atomic_read(&nn->nfsd_courtesy_clients) > 0) > mod_delayed_work(laundry_wq, &nn->laundromat_work, 0); > - return NULL; > - } > + > clp = kmem_cache_zalloc(client_slab, GFP_KERNEL); > if (clp == NULL) > return NULL; > @@ -3115,8 +3114,8 @@ static struct nfs4_client *create_client(struct xdr_netobj name, > struct dentry *dentries[ARRAY_SIZE(client_files)]; > > clp = alloc_client(name, nn); > - if (clp == NULL) > - return NULL; > + if (IS_ERR_OR_NULL(clp)) > + return clp; > > ret = copy_cred(&clp->cl_cred, &rqstp->rq_cred); > if (ret) { > @@ -3498,6 +3497,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > new = create_client(exid->clname, rqstp, &verf); > if (new == NULL) > return nfserr_jukebox; > + if (IS_ERR(new)) > + return nfserr_resource; > status = copy_impl_id(new, exid); > if (status) > goto out_nolock; > @@ -4416,6 +4417,8 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > new = create_client(clname, rqstp, &clverifier); > if (new == NULL) > return nfserr_jukebox; > + if (IS_ERR(new)) > + return nfserr_resource; > spin_lock(&nn->client_lock); > conf = find_confirmed_client_by_name(&clname, nn); > if (conf && client_has_state(conf)) { Patch looks fine otherwise though. Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index a20c2c9d7d45..88936f3189e1 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -2212,21 +2212,20 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn) return 1; } -/* - * XXX Should we use a slab cache ? - * This type of memory management is somewhat inefficient, but we use it - * anyway since SETCLIENTID is not a common operation. - */ static struct nfs4_client *alloc_client(struct xdr_netobj name, struct nfsd_net *nn) { struct nfs4_client *clp; int i; - if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients) { + if (atomic_read(&nn->nfs4_client_count) - + atomic_read(&nn->nfsd_courtesy_clients) >= nn->nfs4_max_clients) + return ERR_PTR(-EREMOTEIO); + + if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients && + atomic_read(&nn->nfsd_courtesy_clients) > 0) mod_delayed_work(laundry_wq, &nn->laundromat_work, 0); - return NULL; - } + clp = kmem_cache_zalloc(client_slab, GFP_KERNEL); if (clp == NULL) return NULL; @@ -3115,8 +3114,8 @@ static struct nfs4_client *create_client(struct xdr_netobj name, struct dentry *dentries[ARRAY_SIZE(client_files)]; clp = alloc_client(name, nn); - if (clp == NULL) - return NULL; + if (IS_ERR_OR_NULL(clp)) + return clp; ret = copy_cred(&clp->cl_cred, &rqstp->rq_cred); if (ret) { @@ -3498,6 +3497,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, new = create_client(exid->clname, rqstp, &verf); if (new == NULL) return nfserr_jukebox; + if (IS_ERR(new)) + return nfserr_resource; status = copy_impl_id(new, exid); if (status) goto out_nolock; @@ -4416,6 +4417,8 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, new = create_client(clname, rqstp, &clverifier); if (new == NULL) return nfserr_jukebox; + if (IS_ERR(new)) + return nfserr_resource; spin_lock(&nn->client_lock); conf = find_confirmed_client_by_name(&clname, nn); if (conf && client_has_state(conf)) {
If there are more non-courteous clients than the calculated limit, we should fail the request rather than report a soft failure and encouraging the client to retry indefinitely. If there a courteous clients which push us over the limit, then expedite their removal. This is not known to have caused a problem is production use, but testing of lots of clients reports repeated NFS4ERR_DELAY responses which doesn't seem helpful. Also remove an outdated comment - we do use a slab cache. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)