On Tue, 2023-07-18 at 16:38 +1000, NeilBrown wrote:
> Most svc threads have no interest in a timeout.
> nfsd sets it to 1 hour, but this is a wart of no significance.
>
> lockd uses the timeout so that it can call nlmsvc_retry_blocked().
> It also sometimes calls svc_wake_up() to ensure this is called.
>
> So change lockd to be consistent and always use svc_wake_up() to trigger
> nlmsvc_retry_blocked() - using a timer instead of a timeout to
> svc_recv().
>
> And change svc_recv() to not take a timeout arg.
>
> This makes the sp_threads_timedout counter always zero.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/lockd/svc.c | 11 ++++++++---
> fs/lockd/svclock.c | 5 +++--
> fs/nfs/callback.c | 2 +-
> fs/nfsd/nfssvc.c | 2 +-
> include/linux/lockd/lockd.h | 4 +++-
> include/linux/sunrpc/svc.h | 1 -
> include/linux/sunrpc/svcsock.h | 2 +-
> net/sunrpc/svc.c | 2 --
> net/sunrpc/svc_xprt.c | 27 ++++++++++++---------------
> 9 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index a43b63e46127..4f55cd42c2e6 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -55,6 +55,11 @@ static DEFINE_MUTEX(nlmsvc_mutex);
> static unsigned int nlmsvc_users;
> static struct svc_serv *nlmsvc_serv;
> unsigned long nlmsvc_timeout;
> +static void nlmsvc_request_retry(struct timer_list *tl)
> +{
> + svc_wake_up(nlmsvc_serv);
> +}
> +DEFINE_TIMER(nlmsvc_retry, nlmsvc_request_retry);
>
> unsigned int lockd_net_id;
>
> @@ -130,18 +135,17 @@ lockd(void *vrqstp)
> * NFS mount or NFS daemon has gone away.
> */
> while (!kthread_should_stop()) {
> - long timeout = MAX_SCHEDULE_TIMEOUT;
>
> /* update sv_maxconn if it has changed */
> rqstp->rq_server->sv_maxconn = nlm_max_connections;
>
> - timeout = nlmsvc_retry_blocked();
> + nlmsvc_retry_blocked();
>
> /*
> * Find any work to do, such as a socket with data available,
> * and do the work.
> */
> - svc_recv(rqstp, timeout);
> + svc_recv(rqstp);
> }
> if (nlmsvc_ops)
> nlmsvc_invalidate_all();
> @@ -375,6 +379,7 @@ static void lockd_put(void)
> #endif
>
> svc_set_num_threads(nlmsvc_serv, NULL, 0);
> + timer_delete_sync(&nlmsvc_retry);
> nlmsvc_serv = NULL;
> dprintk("lockd_down: service destroyed\n");
> }
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index c43ccdf28ed9..3d7bd5c04b36 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -1008,7 +1008,7 @@ retry_deferred_block(struct nlm_block *block)
> * picks up locks that can be granted, or grant notifications that must
> * be retransmitted.
> */
> -unsigned long
> +void
> nlmsvc_retry_blocked(void)
> {
> unsigned long timeout = MAX_SCHEDULE_TIMEOUT;
> @@ -1038,5 +1038,6 @@ nlmsvc_retry_blocked(void)
> }
> spin_unlock(&nlm_blocked_lock);
>
> - return timeout;
> + if (timeout < MAX_SCHEDULE_TIMEOUT)
> + mod_timer(&nlmsvc_retry, jiffies + timeout);
> }
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 914d2402ca98..c47834970224 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -82,7 +82,7 @@ nfs4_callback_svc(void *vrqstp)
> /*
> * Listen for a request on the socket
> */
> - svc_recv(rqstp, MAX_SCHEDULE_TIMEOUT);
> + svc_recv(rqstp);
> }
>
> svc_exit_thread(rqstp);
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 5bf48c33986e..b536b254c59e 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -979,7 +979,7 @@ nfsd(void *vrqstp)
> * Find a socket with data available and call its
> * recvfrom routine.
> */
> - svc_recv(rqstp, 60*60*HZ);
> + svc_recv(rqstp);
> validate_process_creds();
> }
>
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index f42594a9efe0..0f016d69c996 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -204,6 +204,8 @@ extern unsigned long nlmsvc_timeout;
> extern bool nsm_use_hostnames;
> extern u32 nsm_local_state;
>
> +extern struct timer_list nlmsvc_retry;
> +
> /*
> * Lockd client functions
> */
> @@ -280,7 +282,7 @@ __be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
> struct nlm_host *, struct nlm_lock *,
> struct nlm_lock *, struct nlm_cookie *);
> __be32 nlmsvc_cancel_blocked(struct net *net, struct nlm_file *, struct nlm_lock *);
> -unsigned long nlmsvc_retry_blocked(void);
> +void nlmsvc_retry_blocked(void);
> void nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
> nlm_host_match_fn_t match);
> void nlmsvc_grant_reply(struct nlm_cookie *, __be32);
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index d51ae1e109b6..f3df7f963653 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -41,7 +41,6 @@ struct svc_pool {
> struct percpu_counter sp_messages_arrived;
> struct percpu_counter sp_sockets_queued;
> struct percpu_counter sp_threads_woken;
> - struct percpu_counter sp_threads_timedout;
> struct percpu_counter sp_threads_starved;
> struct percpu_counter sp_threads_no_work;
>
> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> index fb5c98069356..8da31799affe 100644
> --- a/include/linux/sunrpc/svcsock.h
> +++ b/include/linux/sunrpc/svcsock.h
> @@ -64,7 +64,7 @@ static inline u32 svc_sock_final_rec(struct svc_sock *svsk)
> * Function prototypes.
> */
> void svc_close_net(struct svc_serv *, struct net *);
> -void svc_recv(struct svc_rqst *, long);
> +void svc_recv(struct svc_rqst *);
> void svc_send(struct svc_rqst *rqstp);
> void svc_drop(struct svc_rqst *);
> void svc_sock_update_bufs(struct svc_serv *serv);
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index f09b0cce041c..170eabc03988 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -513,7 +513,6 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
> percpu_counter_init(&pool->sp_messages_arrived, 0, GFP_KERNEL);
> percpu_counter_init(&pool->sp_sockets_queued, 0, GFP_KERNEL);
> percpu_counter_init(&pool->sp_threads_woken, 0, GFP_KERNEL);
> - percpu_counter_init(&pool->sp_threads_timedout, 0, GFP_KERNEL);
> percpu_counter_init(&pool->sp_threads_starved, 0, GFP_KERNEL);
> percpu_counter_init(&pool->sp_threads_no_work, 0, GFP_KERNEL);
>
> @@ -593,7 +592,6 @@ svc_destroy(struct kref *ref)
> percpu_counter_destroy(&pool->sp_messages_arrived);
> percpu_counter_destroy(&pool->sp_sockets_queued);
> percpu_counter_destroy(&pool->sp_threads_woken);
> - percpu_counter_destroy(&pool->sp_threads_timedout);
> percpu_counter_destroy(&pool->sp_threads_starved);
> percpu_counter_destroy(&pool->sp_threads_no_work);
> }
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 67825eef8646..44a33b1f542f 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -735,10 +735,9 @@ rqst_should_sleep(struct svc_rqst *rqstp)
> return true;
> }
>
> -static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
> +static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp)
> {
> struct svc_pool *pool = rqstp->rq_pool;
> - long time_left = 0;
>
> /* rq_xprt should be clear on entry */
> WARN_ON_ONCE(rqstp->rq_xprt);
> @@ -756,7 +755,7 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
> smp_mb__after_atomic();
>
> if (likely(rqst_should_sleep(rqstp)))
> - time_left = schedule_timeout(timeout);
> + schedule();
> else
> __set_current_state(TASK_RUNNING);
>
> @@ -770,8 +769,6 @@ static struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
> goto out_found;
> }
>
> - if (!time_left)
> - percpu_counter_inc(&pool->sp_threads_timedout);
> if (kthread_should_stop())
> return NULL;
> percpu_counter_inc(&pool->sp_threads_no_work);
> @@ -856,7 +853,7 @@ static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
> * organised not to touch any cachelines in the shared svc_serv
> * structure, only cachelines in the local svc_pool.
> */
> -void svc_recv(struct svc_rqst *rqstp, long timeout)
> +void svc_recv(struct svc_rqst *rqstp)
> {
> struct svc_xprt *xprt = NULL;
> struct svc_serv *serv = rqstp->rq_server;
> @@ -870,7 +867,7 @@ void svc_recv(struct svc_rqst *rqstp, long timeout)
> if (kthread_should_stop())
> goto out;
>
> - xprt = svc_get_next_xprt(rqstp, timeout);
> + xprt = svc_get_next_xprt(rqstp);
> if (!xprt)
> goto out;
>
> @@ -1437,14 +1434,14 @@ static int svc_pool_stats_show(struct seq_file *m, void *p)
> return 0;
> }
>
> - seq_printf(m, "%u %llu %llu %llu %llu %llu %llu\n",
> - pool->sp_id,
> - percpu_counter_sum_positive(&pool->sp_messages_arrived),
> - percpu_counter_sum_positive(&pool->sp_sockets_queued),
> - percpu_counter_sum_positive(&pool->sp_threads_woken),
> - percpu_counter_sum_positive(&pool->sp_threads_timedout),
> - percpu_counter_sum_positive(&pool->sp_threads_starved),
> - percpu_counter_sum_positive(&pool->sp_threads_no_work));
> + seq_printf(m, "%u %llu %llu %llu 0 %llu %llu\n",
> + pool->sp_id,
> + percpu_counter_sum_positive(&pool->sp_messages_arrived),
> + percpu_counter_sum_positive(&pool->sp_sockets_queued),
> + percpu_counter_sum_positive(&pool->sp_threads_woken),
> + /* prevously pool->sp_threads_timedout */
> + percpu_counter_sum_positive(&pool->sp_threads_starved),
> + percpu_counter_sum_positive(&pool->sp_threads_no_work));
>
> return 0;
> }
>
>
More simplifications. I like it!
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
