[PATCH v2] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.

[PATCH v2] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.

[NeilBrown]

Failing OP_SETCLIENTID or OP_EXCHANGE_ID should only happen if there is
memory allocation failure.  Putting a hard limit on the number of
clients is really helpful as it will either happen too early and prevent
clients that the server can easily handle, or too late and allow clients
when the server is swamped.

The calculated limit is still useful for expiring courtesy clients where
there are "too many" clients, but it shouldn't prevent the creation of
active clients.

Testing of lots of clients against small-mem servers reports repeated
NFS4ERR_DELAY responses which doesn't seem helpful.  There may have been
reports of similar problems in production use.

Also remove an outdated comment - we do use a slab cache.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d585c267731b..0791a43b19e6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2218,21 +2218,16 @@ 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) >= 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;
-- 
2.46.0

Re: [PATCH v2] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.

[Chuck Lever]

On Thu, Oct 24, 2024 at 09:10:42AM +1100, NeilBrown wrote:
> 
> Failing OP_SETCLIENTID or OP_EXCHANGE_ID should only happen if there is
> memory allocation failure.  Putting a hard limit on the number of
> clients is really helpful as it will either happen too early and prevent

Do you mean "is not really helpful" ?

I recall that Dai had suggested something similar in this area,
while he was working on the courteous server patches. Dai, do you
have any comments about this change? If not, can you send a R-b ?


> clients that the server can easily handle, or too late and allow clients
> when the server is swamped.
> 
> The calculated limit is still useful for expiring courtesy clients where
> there are "too many" clients, but it shouldn't prevent the creation of
> active clients.
> 
> Testing of lots of clients against small-mem servers reports repeated
> NFS4ERR_DELAY responses which doesn't seem helpful.  There may have been
> reports of similar problems in production use.
> 
> Also remove an outdated comment - we do use a slab cache.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d585c267731b..0791a43b19e6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2218,21 +2218,16 @@ 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) >= 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;
> -- 
> 2.46.0
> 

-- 
Chuck Lever

Re: [PATCH v2] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.

[Jeff Layton]

On Thu, 2024-10-24 at 09:10 +1100, NeilBrown wrote:
> Failing OP_SETCLIENTID or OP_EXCHANGE_ID should only happen if there is
> memory allocation failure.  Putting a hard limit on the number of
> clients is really helpful as it will either happen too early and prevent

"unhelpful" ?

> clients that the server can easily handle, or too late and allow clients
> when the server is swamped.
> 
> The calculated limit is still useful for expiring courtesy clients where
> there are "too many" clients, but it shouldn't prevent the creation of
> active clients.
> 
> Testing of lots of clients against small-mem servers reports repeated
> NFS4ERR_DELAY responses which doesn't seem helpful.  There may have been
> reports of similar problems in production use.
> 
> Also remove an outdated comment - we do use a slab cache.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d585c267731b..0791a43b19e6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2218,21 +2218,16 @@ 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) >= 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;

Do we even need to check nn->nfs4_max_clients at all?

Maybe we should just kick the laundromat whenever 
nfsd_courtesy_clients > 0. I would suggest just removing
nfs4_max_clients altogether, but it looks like
nfs4_get_client_reaplist() uses it and it's not clear to me what would
better replace it.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

On Thu, 24 Oct 2024 09:10:42 +1100, NeilBrown wrote:                                              
> Failing OP_SETCLIENTID or OP_EXCHANGE_ID should only happen if there is
> memory allocation failure.  Putting a hard limit on the number of
> clients is really helpful as it will either happen too early and prevent
> clients that the server can easily handle, or too late and allow clients
> when the server is swamped.
> 
> The calculated limit is still useful for expiring courtesy clients where
> there are "too many" clients, but it shouldn't prevent the creation of
> active clients.
> 
> [...]                                                                        

Applied to nfsd-next for v6.13, thanks!                                                                

[1/1] nfsd: Don't fail OP_SETCLIENTID when there are too many clients.
      commit: c7b8826b41906db1c930cbb10abb94eb24247f20                                                                      

--                                                                              
Chuck Lever