| Message ID | 168966228862.11075.7544295807519851006.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SUNRPC svc thread code, and use llist | expand |
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@suse.de> > --- > 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@kernel.org>
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; }
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@suse.de> --- 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(-)