nfsd/sunrpc: add support for a workqueue-based nfsd
Subsystems (6)
v2 cover nfsd/sunrpc: add support for a workqueue-based nfsd
This is the second revision of the workqueue-based nfsd. Changes from the RFC version I posted earlier:
v2:
- found and fixed problem causing the delays between work. It was a bug in the design of the new code. I was queueing the svc_xprt's work to do everything and that necessarily serialized that work. This version adds a work_struct to the svc_rqst as well, and that's where the bulk of the work.
- no longer tries to use the thread count as a max_active setting for the workqueue. The workqueue max_active settings are left to the default. This means that /proc/fs/nfsd/threads is basically a binary switch that's either zero (not running) or non-zero (running). The actual value has no meaning.
- the handling of the fs_struct has been reworked. It's now allocated as part of the struct svc_rqst. Each svc_rqst has its own fs_struct now.
- CONFIG_SUNRPC_SVC_WORKQUEUE has been removed. Since the code must be enabled at runtime anyway, we don't need a switch to disable this code at build time.
- the new tracepoints have been reworked.
This new code is still a little slower (2-3%) than the older, thread based code in my testing, though my hope is that it may perform better than the older code on a NUMA machine. I don't have one at the moment to verify that, however.
For background, here's an excerpt from the original posting:
This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
Performance:
------------
As far as numbers go, I ran a modified version of the fio tiobench-example.fio test that just reduces the file size to 128M:
threads:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13740KB, aggrb=228KB/s, minb=57KB/s, maxb=57KB/s,
mint=60034msec, maxt=60049msec
Run status group 1 (all jobs):
WRITE: io=14652KB, aggrb=244KB/s, minb=60KB/s, maxb=61KB/s,
mint=60002msec, maxt=60028msec
Run status group 2 (all jobs):
READ: io=524288KB, aggrb=49639KB/s, minb=12409KB/s, maxb=12433KB/s,
mint=10542msec, maxt=10562msec
Run status group 3 (all jobs):
READ: io=524288KB, aggrb=50670KB/s, minb=12667KB/s, maxb=12687KB/s,
mint=10331msec, maxt=10347msec
workqueue:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13632KB, aggrb=226KB/s, minb=56KB/s, maxb=56KB/s,
mint=60010msec, maxt=60061msec
Run status group 1 (all jobs):
WRITE: io=14328KB, aggrb=238KB/s, minb=59KB/s, maxb=59KB/s,
mint=60023msec, maxt=60033msec
Run status group 2 (all jobs):
READ: io=524288KB, aggrb=47779KB/s, minb=11944KB/s, maxb=11955KB/s,
mint=10963msec, maxt=10973msec
Run status group 3 (all jobs):
READ: io=524288KB, aggrb=48379KB/s, minb=12094KB/s, maxb=12114KB/s,
mint=10819msec, maxt=10837msec
...a small performance decrease using workqueues (about 5% in the worst case). It varies a little between runs but the results over several runs are fairly consistent in showing a small perf decrease.
In an attempt to ascertain where that comes from, I added some tracepoints and wrote a perl script to scrape the results and figure out where the latency comes from. The numbers here are from the ones that produced the above results:
[jlayton@palma nfsd-wq]$ ./analyze.pl < threads.txt
-------------------------------------------------------------
rpcs=272437
queue=3.79952429455322 queued=10.0558330900775 receive=10.6196625266701 process=2325.8173229043 total=2350.2923428156
[jlayton@palma nfsd-wq]$ ./analyze.pl < workqueue.txt
-------------------------------------------------------------
rpcs=272329
queue=4.41979737859012
queued=8.124893049967 receive=11.3421082590608 process=2310.15945786277 total=2334.04625655039
Here's a legend, the numbers represent a delta between two tracepoints for a particular xprt or rqst. I can provide the script if it's helpful but it's definitely hacked together:
rpcs: total number of rpcs processed (just a count)
queue: time it took to queue the xprt.xi
trace_svc_xprt_enqueued - trace_svc_xprt_enqueue (in usecs)
queued: time between being queued and going "active"
trace_svc_xprt_active - trace_svc_xprt_enqueued (in usecs)
receive: time between going "active" and completing the receive
trace_svc_xprt_received - trace_svc_xprt_active (in usecs)
process: time between completing the receive and finishing processing
trace_svc_process - trace_svc_xprt_recevied (in usecs)
total: total time from enqueueing to finishing the processing
trace_svc_process - trace_svc_xprt_enqueued (in usecs)
The interesting bit is that according to these numbers, the workqueue RPCs ran more quickly on average (though most of that is in svc_process, for which I have zero explanation). So, why are we getting slower results?
One theory is that it's getting bitten by the fact that the workqueue queueing/dequeueing code uses spinlocks that disable bottom halves. Perhaps that adds up and causes more latency to softirq processing? I'm not sure how best to nail that down...
It's probably worth it to start considering this for v3.20, but I'd like to get some time on a larger scale test rig to see how it does first. We'll also need Al's ACK on the fs_struct stuff.
Thoughts or comments are welcome.
Jeff Layton (16):
sunrpc: add a new svc_serv_ops struct and move sv_shutdown into it
sunrpc: move sv_function into sv_ops
sunrpc: move sv_module parm into sv_ops
sunrpc: turn enqueueing a svc_xprt into a svc_serv operation
sunrpc: abstract out svc_set_num_threads to sv_ops
sunrpc: move pool_mode definitions into svc.h
sunrpc: factor svc_rqst allocation and freeing from sv_nrthreads
refcounting
sunrpc: set up workqueue function in svc_xprt
sunrpc: set up svc_rqst work if it's defined
sunrpc: add basic support for workqueue-based services
nfsd: keep a reference to the fs_struct in svc_rqst
nfsd: add support for workqueue based service processing
sunrpc: keep a cache of svc_rqsts for each NUMA node
sunrpc: add more tracepoints around svc_xprt handling
sunrpc: print the svc_rqst pointer value in svc_process tracepoint
sunrpc: add tracepoints around svc_sock handling
fs/fs_struct.c | 60 ++++++-- fs/lockd/svc.c | 7 +- fs/nfs/callback.c | 6 +- fs/nfsd/nfssvc.c | 100 +++++++++++-- include/linux/fs_struct.h | 4 + include/linux/sunrpc/svc.h | 93 ++++++++++--- include/linux/sunrpc/svc_xprt.h | 3 + include/linux/sunrpc/svcsock.h | 1 + include/trace/events/sunrpc.h | 88 ++++++++++-- net/sunrpc/Makefile | 2 +- net/sunrpc/svc.c | 141 +++++++++++-------- net/sunrpc/svc_wq.c | 302 ++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svc_xprt.c | 68 ++++++++- net/sunrpc/svcsock.c | 6 + 14 files changed, 758 insertions(+), 123 deletions(-) create mode 100644 net/sunrpc/svc_wq.c
Signature · 1 line
2.1.0
PATCH v2 1/16 sunrpc: add a new svc_serv_ops struct and move sv_shutdown into it
In later patches we'll need to abstract out more operations on a per-service level, besides sv_shutdown and sv_function.
Declare a new svc_serv_ops struct to hold these operations, and move sv_shutdown into this struct.
--- fs/lockd/svc.c | 6 +++++- fs/nfs/callback.c | 5 ++++- fs/nfsd/nfssvc.c | 6 +++++- include/linux/sunrpc/svc.h | 20 ++++++++++---------- net/sunrpc/svc.c | 18 +++++++++--------- 5 files changed, 33 insertions(+), 22 deletions(-)
fs/lockd/svc.c +5 -1
@@ -326,6 +326,10 @@ out_rqst:return error; } +static struct svc_serv_ops lockd_sv_ops = { + .svo_shutdown = svc_rpcb_cleanup, +}; +static struct svc_serv *lockd_create_svc (void ) {struct svc_serv *serv;
@@ -350,7 +354,7 @@ static struct svc_serv *lockd_create_svc(void)printk (KERN_WARNING "lockd_up: no pid, %d users??\n" , nlmsvc_users); - serv =svc_create (&nlmsvc_program,LOCKD_BUFSIZE , svc_rpcb_cleanup); + serv =svc_create (&nlmsvc_program,LOCKD_BUFSIZE , &lockd_sv_ops);if (!serv) {printk (KERN_WARNING "lockd_up: create service failed\n" );return ERR_PTR (-ENOMEM );
fs/nfs/callback.c +4 -1
@@ -306,6 +306,9 @@ err_bind:return ret; } +static struct svc_serv_ops nfs_cb_sv_ops = { +}; +static struct svc_serv *nfs_callback_create_svc (int minorversion) {struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
@@ -331,7 +334,7 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)printk (KERN_WARNING "nfs_callback_create_svc: no kthread, %d users??\n" , cb_info->users); - serv =svc_create (&nfs4_callback_program,NFS4_CALLBACK_BUFSIZE ,NULL ); + serv =svc_create (&nfs4_callback_program,NFS4_CALLBACK_BUFSIZE , &nfs_cb_sv_ops);if (!serv) {printk (KERN_ERR "nfs_callback_create_svc: create service failed\n" );return ERR_PTR (-ENOMEM );
fs/nfsd/nfssvc.c +5 -1
@@ -390,6 +390,10 @@ static int nfsd_get_default_max_blksize(void)return ret; } +static struct svc_serv_ops nfsd_sv_ops = { + .svo_shutdown = nfsd_last_thread, +}; +int nfsd_create_serv (struct net *net) {int error;
@@ -404,7 +408,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - nfsd_last_thread, nfsd,THIS_MODULE ); + &nfsd_sv_ops, nfsd,THIS_MODULE );if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +10 -10
@@ -54,6 +54,13 @@ struct svc_pool {unsigned long sp_flags; }____cacheline_aligned_in_smp ; +struct svc_serv ; + +struct svc_serv_ops { +/* Callback to use when last thread exits. */ +void (*svo_shutdown)(struct svc_serv *serv,struct net *net); +}; +/* * RPC service. *
@@ -85,13 +92,7 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ - -void (*sv_shutdown)(struct svc_serv *serv, -struct net *net); -/* Callback to use when last thread -* exits. -*/ - +struct svc_serv_ops * sv_ops;/* server operations */ struct module * sv_module;/* optional module to count when * adding threads */ svc_thread_fn sv_function;/* main function for threads */
@@ -429,13 +430,12 @@ int svc_rpcb_setup(struct svc_serv *serv, struct net *net);void svc_rpcb_cleanup (struct svc_serv *serv,struct net *net);int svc_bind (struct svc_serv *serv,struct net *net);struct svc_serv *svc_create (struct svc_program *,unsigned int , -void (*shutdown)(struct svc_serv *,struct net *net)); +struct svc_serv_ops *);struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -void (*shutdown)(struct svc_serv *,struct net *net), -svc_thread_fn ,struct module *); +struct svc_serv_ops *,svc_thread_fn ,struct module *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +9 -9
@@ -423,7 +423,7 @@ EXPORT_SYMBOL_GPL(svc_bind);*/ static struct svc_serv *__svc_create (struct svc_program *prog,unsigned int bufsize,int npools, -void (*shutdown)(struct svc_serv *serv,struct net *net)) +struct svc_serv_ops *ops) {struct svc_serv *serv;unsigned int vers;
@@ -440,7 +440,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools, bufsize =RPCSVC_MAXPAYLOAD ; serv->sv_max_payload = bufsize? bufsize :4096 ; serv->sv_max_mesg =roundup (serv->sv_max_payload +PAGE_SIZE ,PAGE_SIZE ); - serv->sv_shutdown = shutdown; + serv->sv_ops = ops; xdrsize =0 ;while (prog) { prog->pg_lovers = prog->pg_nvers-1 ;
@@ -486,21 +486,21 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,struct svc_serv *svc_create (struct svc_program *prog,unsigned int bufsize, -void (*shutdown)(struct svc_serv *serv,struct net *net)) +struct svc_serv_ops *ops) { -return __svc_create (prog, bufsize,/*npools*/ 1 , shutdown); +return __svc_create (prog, bufsize,/*npools*/ 1 , ops); }EXPORT_SYMBOL_GPL (svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -void (*shutdown)(struct svc_serv *serv,struct net *net), -svc_thread_fn func,struct module *mod) +struct svc_serv_ops *ops,svc_thread_fn func, +struct module *mod) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get (); - serv =__svc_create (prog, bufsize, npools, shutdown); + serv =__svc_create (prog, bufsize, npools, ops);if (!serv) goto out_err;
@@ -517,8 +517,8 @@ void svc_shutdown_net(struct svc_serv *serv, struct net *net) {svc_close_net (serv, net); -if (serv->sv_shutdown) - serv->sv_shutdown (serv, net); +if (serv->sv_ops->svo_shutdown) + serv->sv_ops->svo_shutdown (serv, net); }EXPORT_SYMBOL_GPL (svc_shutdown_net);
Signature · 1 line
2.1.0
PATCH v2 2/16 sunrpc: move sv_function into sv_ops
In reply to Jeff LaytonSince we now have a container for holding svc_serv operations, move the sv_function into it as well.
--- fs/nfsd/nfssvc.c | 3 ++- include/linux/sunrpc/svc.h | 11 +++-------- net/sunrpc/svc.c | 8 +++----- 3 files changed, 8 insertions(+), 14 deletions(-)
fs/nfsd/nfssvc.c +2 -1
@@ -392,6 +392,7 @@ static int nfsd_get_default_max_blksize(void)static struct svc_serv_ops nfsd_sv_ops = { .svo_shutdown = nfsd_last_thread, + .svo_function = nfsd, };int nfsd_create_serv (struct net *net)
@@ -408,7 +409,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops, nfsd,THIS_MODULE ); + &nfsd_sv_ops,THIS_MODULE );if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +3 -8
@@ -19,11 +19,6 @@#include <linux/wait.h> #include <linux/mm.h> -/* -* This is the RPC server thread function prototype -*/ -typedef int (*svc_thread_fn )(void *); -/* statistics for svc_pool structures */ struct svc_pool_stats {atomic_long_t packets;
@@ -58,7 +53,8 @@ struct svc_serv;struct svc_serv_ops {/* Callback to use when last thread exits. */ -void (*svo_shutdown)(struct svc_serv *serv,struct net *net); +void (*svo_shutdown)(struct svc_serv *,struct net *); +int (*svo_function)(void *); };/*
@@ -95,7 +91,6 @@ struct svc_serv {struct svc_serv_ops * sv_ops;/* server operations */ struct module * sv_module;/* optional module to count when * adding threads */ -svc_thread_fn sv_function;/* main function for threads */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -435,7 +430,7 @@ struct svc_rqst *svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -struct svc_serv_ops *,svc_thread_fn ,struct module *); +struct svc_serv_ops *,struct module *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +3 -5
@@ -34,7 +34,7 @@static void svc_unregister (const struct svc_serv *serv,struct net *net); -#define svc_serv_is_pooled (serv) ((serv)->sv_function) +#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function)/* * Mode for mapping cpus to pools.
@@ -494,8 +494,7 @@ EXPORT_SYMBOL_GPL(svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -struct svc_serv_ops *ops,svc_thread_fn func, -struct module *mod) +struct svc_serv_ops *ops,struct module *mod) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get ();
@@ -504,7 +503,6 @@ svc_create_pooled(struct svc_program *prog, unsigned int bufsize,if (!serv) goto out_err; - serv->sv_function = func; serv->sv_module = mod;return serv; out_err:
@@ -740,7 +738,7 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) }__module_get (serv->sv_module); - task =kthread_create_on_node (serv->sv_function, rqstp, + task =kthread_create_on_node (serv->sv_ops->svo_function, rqstp, node,"%s" , serv->sv_name);if (IS_ERR (task)) { error =PTR_ERR (task);
Signature · 1 line
2.1.0
PATCH v2 3/16 sunrpc: move sv_module parm into sv_ops
In reply to Jeff Layton...not technically an operation, but it's more convenient and cleaner to pass the module pointer in this struct.
--- fs/nfsd/nfssvc.c | 3 ++- include/linux/sunrpc/svc.h | 9 ++++++--- net/sunrpc/svc.c | 8 +++----- 3 files changed, 11 insertions(+), 9 deletions(-)
fs/nfsd/nfssvc.c +2 -1
@@ -393,6 +393,7 @@ static int nfsd_get_default_max_blksize(void)static struct svc_serv_ops nfsd_sv_ops = { .svo_shutdown = nfsd_last_thread, .svo_function = nfsd, + .svo_module =THIS_MODULE , };int nfsd_create_serv (struct net *net)
@@ -409,7 +410,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops,THIS_MODULE ); + &nfsd_sv_ops);if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +6 -3
@@ -54,7 +54,12 @@ struct svc_serv;struct svc_serv_ops {/* Callback to use when last thread exits. */ void (*svo_shutdown)(struct svc_serv *,struct net *); + +/* function for service threads to run */ int (*svo_function)(void *); + +/* optional module to count when adding threads (pooled svcs only) */ +struct module *svo_module; };/*
@@ -89,8 +94,6 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ -struct module * sv_module;/* optional module to count when -* adding threads */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -430,7 +433,7 @@ struct svc_rqst *svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -struct svc_serv_ops *,struct module *); +struct svc_serv_ops *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +3 -5
@@ -494,7 +494,7 @@ EXPORT_SYMBOL_GPL(svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -struct svc_serv_ops *ops,struct module *mod) +struct svc_serv_ops *ops) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get ();
@@ -502,8 +502,6 @@ svc_create_pooled(struct svc_program *prog, unsigned int bufsize, serv =__svc_create (prog, bufsize, npools, ops);if (!serv) goto out_err; - - serv->sv_module = mod;return serv; out_err:svc_pool_map_put ();
@@ -737,12 +735,12 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)break ; } -__module_get (serv->sv_module); +__module_get (serv->sv_ops->svo_module); task =kthread_create_on_node (serv->sv_ops->svo_function, rqstp, node,"%s" , serv->sv_name);if (IS_ERR (task)) { error =PTR_ERR (task); -module_put (serv->sv_module); +module_put (serv->sv_ops->svo_module);svc_exit_thread (rqstp);break ; }
Signature · 1 line
2.1.0
PATCH v2 4/16 sunrpc: turn enqueueing a svc_xprt into a svc_serv operation
In reply to Jeff LaytonFor now, all services use svc_xprt_do_enqueue, but once we add workqueue-based service support, we'll need to do something different.
--- fs/lockd/svc.c | 3 ++- fs/nfs/callback.c | 1 + fs/nfsd/nfssvc.c | 11 ++++++----- include/linux/sunrpc/svc.h | 3 +++ include/linux/sunrpc/svc_xprt.h | 1 + net/sunrpc/svc_xprt.c | 10 +++++----- 6 files changed, 18 insertions(+), 11 deletions(-)
fs/lockd/svc.c +2 -1
@@ -327,7 +327,8 @@ out_rqst: }static struct svc_serv_ops lockd_sv_ops = { - .svo_shutdown = svc_rpcb_cleanup, + .svo_shutdown = svc_rpcb_cleanup, + .svo_enqueue_xprt = svc_xprt_do_enqueue, };static struct svc_serv *lockd_create_svc (void )
fs/nfs/callback.c +1 -0
@@ -307,6 +307,7 @@ err_bind: }static struct svc_serv_ops nfs_cb_sv_ops = { + .svo_enqueue_xprt = svc_xprt_do_enqueue, };static struct svc_serv *nfs_callback_create_svc (int minorversion)
fs/nfsd/nfssvc.c +6 -5
@@ -390,10 +390,11 @@ static int nfsd_get_default_max_blksize(void)return ret; } -static struct svc_serv_ops nfsd_sv_ops = { - .svo_shutdown = nfsd_last_thread, - .svo_function = nfsd, - .svo_module =THIS_MODULE , +static struct svc_serv_ops nfsd_thread_sv_ops = { + .svo_shutdown = nfsd_last_thread, + .svo_function = nfsd, + .svo_enqueue_xprt = svc_xprt_do_enqueue, + .svo_module =THIS_MODULE , };int nfsd_create_serv (struct net *net)
@@ -410,7 +411,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops); + &nfsd_thread_sv_ops);if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +3 -0
@@ -58,6 +58,9 @@ struct svc_serv_ops {/* function for service threads to run */ int (*svo_function)(void *); +/* queue up a transport for servicing */ +void (*svo_enqueue_xprt)(struct svc_xprt *); +/* optional module to count when adding threads (pooled svcs only) */ struct module *svo_module; };
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -116,6 +116,7 @@ void svc_xprt_init(struct net *, struct svc_xprt_class *, struct svc_xprt *, struct svc_serv *);int svc_create_xprt (struct svc_serv *,const char *,struct net *,const int ,const unsigned short ,int ); +void svc_xprt_do_enqueue (struct svc_xprt *xprt);void svc_xprt_enqueue (struct svc_xprt *xprt);void svc_xprt_put (struct svc_xprt *xprt);void svc_xprt_copy_addrs (struct svc_rqst *rqstp,struct svc_xprt *xprt);
net/sunrpc/svc_xprt.c +5 -5
@@ -24,7 +24,6 @@ static int svc_deferred_recv(struct svc_rqst *rqstp);static struct cache_deferred_req *svc_defer (struct cache_req *req);static void svc_age_temp_xprts (unsigned long closure);static void svc_delete_xprt (struct svc_xprt *xprt); -static void svc_xprt_do_enqueue (struct svc_xprt *xprt);/* apparently the "standard" is that clients close * idle connections after 5 minutes, servers after
@@ -226,12 +225,12 @@ static void svc_xprt_received(struct svc_xprt *xprt) }/* As soon as we clear busy, the xprt could be closed and -* 'put', so we need a reference to call svc_xprt_do_enqueue with: +* 'put', so we need a reference to call svc_enqueue_xprt with: */ svc_xprt_get (xprt);smp_mb__before_atomic ();clear_bit (XPT_BUSY , &xprt->xpt_flags); -svc_xprt_do_enqueue (xprt); + xprt->xpt_server->sv_ops->svo_enqueue_xprt (xprt);svc_xprt_put (xprt); }
@@ -321,7 +320,7 @@ static bool svc_xprt_has_something_to_do(struct svc_xprt *xprt)return false; } -static void svc_xprt_do_enqueue (struct svc_xprt *xprt) +void svc_xprt_do_enqueue (struct svc_xprt *xprt) {struct svc_pool *pool;struct svc_rqst *rqstp =NULL ;
@@ -403,6 +402,7 @@ redo_search: out:trace_svc_xprt_do_enqueue (xprt, rqstp); } +EXPORT_SYMBOL_GPL (svc_xprt_do_enqueue);/* * Queue up a transport with data pending. If there are idle nfsd
@@ -413,7 +413,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt) {if (test_bit (XPT_BUSY , &xprt->xpt_flags))return ; -svc_xprt_do_enqueue (xprt); + xprt->xpt_server->sv_ops->svo_enqueue_xprt (xprt); }EXPORT_SYMBOL_GPL (svc_xprt_enqueue);
Signature · 1 line
2.1.0
PATCH v2 5/16 sunrpc: abstract out svc_set_num_threads to sv_ops
In reply to Jeff LaytonAdd an operation that will do setup of the service. In the case of a classic thread-based service that means starting up threads. In the case of a workqueue-based service, the setup will do something different.
--- fs/nfsd/nfssvc.c | 8 +++++--- include/linux/sunrpc/svc.h | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-)
fs/nfsd/nfssvc.c +5 -3
@@ -394,6 +394,7 @@ static struct svc_serv_ops nfsd_thread_sv_ops = { .svo_shutdown = nfsd_last_thread, .svo_function = nfsd, .svo_enqueue_xprt = svc_xprt_do_enqueue, + .svo_setup = svc_set_num_threads, .svo_module = THIS_MODULE, };
@@ -506,8 +507,8 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)/* apply the new numbers */ svc_get (nn->nfsd_serv);for (i =0 ; i < n; i++) { - err =svc_set_num_threads (nn->nfsd_serv, &nn->nfsd_serv->sv_pools[i], - nthreads[i]); + err = nn->nfsd_serv->sv_ops->svo_setup (nn->nfsd_serv, + &nn->nfsd_serv->sv_pools[i], nthreads[i]);if (err)break ; }
@@ -546,7 +547,8 @@ nfsd_svc(int nrservs, struct net *net) error =nfsd_startup_net (nrservs, net);if (error) goto out_destroy; - error =svc_set_num_threads (nn->nfsd_serv,NULL , nrservs); + error = nn->nfsd_serv->sv_ops->svo_setup (nn->nfsd_serv, +NULL , nrservs);if (error) goto out_shutdown;/* We are holding a reference to nn->nfsd_serv which
include/linux/sunrpc/svc.h +3 -0
@@ -61,6 +61,9 @@ struct svc_serv_ops {/* queue up a transport for servicing */ void (*svo_enqueue_xprt)(struct svc_xprt *); +/* set up thread (or whatever) execution context */ +int (*svo_setup)(struct svc_serv *,struct svc_pool *,int ); +/* optional module to count when adding threads (pooled svcs only) */ struct module *svo_module; };
Signature · 1 line
2.1.0
PATCH v2 6/16 sunrpc: move pool_mode definitions into svc.h
In reply to Jeff LaytonIn later patches, we're going to need to allow code external to svc.c to figure out what pool_mode is in use. Move these definitions into svc.h to prepare for that.
Also, make the svc_pool_map object available and exported so that other modules can peek in there to get insight into what pool mode is in use. Likewise, export svc_pool_map_get/put function to make it safe to do so.
--- include/linux/sunrpc/svc.h | 25 +++++++++++++++++++++++++ net/sunrpc/svc.c | 31 +++++++------------------------ 2 files changed, 32 insertions(+), 24 deletions(-)
include/linux/sunrpc/svc.h +25 -0
@@ -428,6 +428,29 @@ struct svc_procedure { };/* +* Mode for mapping cpus to pools. +*/ +enum { +SVC_POOL_AUTO =-1 ,/* choose one of the others */ +SVC_POOL_GLOBAL ,/* no mapping, just a single global pool +* (legacy & UP mode) */ +SVC_POOL_PERCPU ,/* one pool per cpu */ +SVC_POOL_PERNODE /* one pool per numa node */ +}; + +struct svc_pool_map { +int count;/* How many svc_servs use us */ +int mode;/* Note: int not enum to avoid +* warnings about "enumeration value +* not handled in switch" */ +unsigned int npools; +unsigned int *pool_to;/* maps pool id to cpu or node */ +unsigned int *to_pool;/* maps cpu or node to pool id */ +}; + +extern struct svc_pool_map svc_pool_map; + +/* * Function prototypes. */ int svc_rpcb_setup (struct svc_serv *serv,struct net *net);
@@ -438,6 +461,8 @@ struct svc_serv *svc_create(struct svc_program *, unsigned int,struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node);void svc_exit_thread (struct svc_rqst *); +unsigned int svc_pool_map_get (void ); +void svc_pool_map_put (void );struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int ,struct svc_serv_ops *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );
net/sunrpc/svc.c +7 -24
@@ -36,34 +36,17 @@ static void svc_unregister(const struct svc_serv *serv, struct net *net);#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function) -/* -* Mode for mapping cpus to pools. -*/ -enum { -SVC_POOL_AUTO =-1 ,/* choose one of the others */ -SVC_POOL_GLOBAL ,/* no mapping, just a single global pool -* (legacy & UP mode) */ -SVC_POOL_PERCPU ,/* one pool per cpu */ -SVC_POOL_PERNODE /* one pool per numa node */ -};#define SVC_POOL_DEFAULT SVC_POOL_GLOBAL/* * Structure for mapping cpus to pools and vice versa. * Setup once during sunrpc initialisation. */ -static struct svc_pool_map { -int count;/* How many svc_servs use us */ -int mode;/* Note: int not enum to avoid -* warnings about "enumeration value -* not handled in switch" */ -unsigned int npools; -unsigned int *pool_to;/* maps pool id to cpu or node */ -unsigned int *to_pool;/* maps cpu or node to pool id */ -} svc_pool_map = { - .count =0 , +struct svc_pool_map svc_pool_map = { .mode =SVC_POOL_DEFAULT }; +EXPORT_SYMBOL_GPL (svc_pool_map); +static DEFINE_MUTEX (svc_pool_map_mutex);/* protects svc_pool_map.count only */ static int
@@ -236,7 +219,7 @@ svc_pool_map_init_pernode(struct svc_pool_map *m)* vice versa). Initialise the map if we're the first user. * Returns the number of pools. */ -static unsigned int +unsigned int svc_pool_map_get (void ) {struct svc_pool_map *m = &svc_pool_map;
@@ -271,7 +254,7 @@ svc_pool_map_get(void)mutex_unlock (&svc_pool_map_mutex);return m->npools; } - +EXPORT_SYMBOL_GPL (svc_pool_map_get);/* * Drop a reference to the global map of cpus to pools.
@@ -280,7 +263,7 @@ svc_pool_map_get(void)* mode using the pool_mode module option without * rebooting or re-loading sunrpc.ko. */ -static void +void svc_pool_map_put (void ) {struct svc_pool_map *m = &svc_pool_map;
@@ -297,7 +280,7 @@ svc_pool_map_put(void)mutex_unlock (&svc_pool_map_mutex); } - +EXPORT_SYMBOL_GPL (svc_pool_map_put);static int svc_pool_map_get_node (unsigned int pidx) {
Signature · 1 line
2.1.0
PATCH v2 7/16 sunrpc: factor svc_rqst allocation and freeing from sv_nrthreads refcounting
In reply to Jeff LaytonIn later patches, we'll want to be able to allocate and free svc_rqst structures without monkeying with the serv->sv_nrthreads refcount.
Factor those pieces out of their respective functions.
--- include/linux/sunrpc/svc.h | 3 +++ net/sunrpc/svc.c | 58 ++++++++++++++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 20 deletions(-)
include/linux/sunrpc/svc.h +3 -0
@@ -458,8 +458,11 @@ void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net);int svc_bind (struct svc_serv *serv,struct net *net);struct svc_serv *svc_create (struct svc_program *,unsigned int ,struct svc_serv_ops *); +struct svc_rqst *svc_rqst_alloc (struct svc_serv *serv, +struct svc_pool *pool,int node);struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node); +void svc_rqst_free (struct svc_rqst *);void svc_exit_thread (struct svc_rqst *);unsigned int svc_pool_map_get (void );void svc_pool_map_put (void );
net/sunrpc/svc.c +38 -20
104 lines not on this page · raw message
Signature · 1 line
2.1.0
PATCH v2 8/16 sunrpc: set up workqueue function in svc_xprt
In reply to Jeff LaytonFor workqueue-based services we'll want to queue a workqueue job whenever a xprt needs to be serviced. Add a work_struct to svc_xprt and initialize it from a new work_func_t field in the svc_serv_ops.
--- include/linux/sunrpc/svc.h | 3 +++ include/linux/sunrpc/svc_xprt.h | 1 + net/sunrpc/svc.c | 2 +- net/sunrpc/svc_xprt.c | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-)
include/linux/sunrpc/svc.h +3 -0
@@ -58,6 +58,9 @@ struct svc_serv_ops {/* function for service threads to run */ int (*svo_function)(void *); +/* xprt work function */ +work_func_t svo_xprt_work; +/* queue up a transport for servicing */ void (*svo_enqueue_xprt)(struct svc_xprt *);
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -53,6 +53,7 @@ struct svc_xprt {struct kref xpt_ref;struct list_head xpt_list;struct list_head xpt_ready; +struct work_struct xpt_work;unsigned long xpt_flags;#define XPT_BUSY 0/* enqueued/receiving */ #define XPT_CONN 1/* conn pending */
net/sunrpc/svc.c +1 -1
@@ -34,7 +34,7 @@static void svc_unregister (const struct svc_serv *serv,struct net *net); -#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function) +#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function || (serv)->sv_ops->svo_xprt_work)#define SVC_POOL_DEFAULT SVC_POOL_GLOBAL
net/sunrpc/svc_xprt.c +2 -0
@@ -157,6 +157,8 @@ void svc_xprt_init(struct net *net, struct svc_xprt_class *xcl, xprt->xpt_ops = xcl->xcl_ops;kref_init (&xprt->xpt_ref); xprt->xpt_server = serv; +if (serv->sv_ops->svo_xprt_work) +INIT_WORK (&xprt->xpt_work, serv->sv_ops->svo_xprt_work);INIT_LIST_HEAD (&xprt->xpt_list);INIT_LIST_HEAD (&xprt->xpt_ready);INIT_LIST_HEAD (&xprt->xpt_deferred);
Signature · 1 line
2.1.0
PATCH v2 9/16 sunrpc: set up svc_rqst work if it's defined
In reply to Jeff LaytonWe want to queue most of the work in the context of the svc_rqst. Add a work_struct to the svc_rqst and a new operation for svc_rqst_work. If that's defined then initialize it when setting up the service.
--- include/linux/sunrpc/svc.h | 4 ++++ net/sunrpc/svc.c | 2 ++ 2 files changed, 6 insertions(+)
include/linux/sunrpc/svc.h +4 -0
@@ -61,6 +61,9 @@ struct svc_serv_ops {/* xprt work function */ work_func_t svo_xprt_work; +/* rqst work function */ +work_func_t svo_rqst_work; +/* queue up a transport for servicing */ void (*svo_enqueue_xprt)(struct svc_xprt *);
@@ -293,6 +296,7 @@ struct svc_rqst {struct svc_cacherep * rq_cacherep;/* cache info */ struct task_struct *rq_task;/* service thread */ spinlock_t rq_lock;/* per-request lock */ +struct work_struct rq_work;/* per-request work */ };#define SVC_NET (svc_rqst) (svc_rqst->rq_xprt->xpt_net)
net/sunrpc/svc.c +2 -0
@@ -595,6 +595,8 @@ svc_rqst_alloc(struct svc_serv *serv, struct svc_pool *pool, int node)spin_lock_init (&rqstp->rq_lock); rqstp->rq_server = serv; rqstp->rq_pool = pool; +if (serv->sv_ops->svo_rqst_work) +INIT_WORK (&rqstp->rq_work, serv->sv_ops->svo_rqst_work); rqstp->rq_argp =kmalloc_node (serv->sv_xdrsize,GFP_KERNEL , node);if (!rqstp->rq_argp)
Signature · 1 line
2.1.0
PATCH v2 10/16 sunrpc: add basic support for workqueue-based services
In reply to Jeff LaytonAdd a new "workqueue" pool mode setting. When that is configured, we'll set up a svc_pool for each NUMA node, but don't bother with the pool <=> cpu mapping arrays.
We use an unbound workqueue, which should naturally make the work be queued to a CPU within the current NUMA node.
The first iteration of this is quite simple. When a svc_xprt needs to be serviced we queue its work and return. In later patches, we'll optimize this a bit more.
--- include/linux/sunrpc/svc.h | 8 ++- include/linux/sunrpc/svc_xprt.h | 1 + include/linux/sunrpc/svcsock.h | 1 + net/sunrpc/Makefile | 2 +- net/sunrpc/svc.c | 13 ++++ net/sunrpc/svc_wq.c | 146 ++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svc_xprt.c | 47 ++++++++++++- 7 files changed, 215 insertions(+), 3 deletions(-) create mode 100644 net/sunrpc/svc_wq.c
include/linux/sunrpc/svc.h +7 -1
@@ -47,6 +47,7 @@ struct svc_pool {#define SP_TASK_PENDING (0)/* still work to do even if no * xprt is queued. */ unsigned long sp_flags; +struct work_struct sp_work;/* per-pool work struct */ }____cacheline_aligned_in_smp ;struct svc_serv ;
@@ -106,6 +107,7 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ +struct workqueue_struct *sv_wq;/* workqueue for wq-based services */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -442,7 +444,8 @@ enum {SVC_POOL_GLOBAL ,/* no mapping, just a single global pool * (legacy & UP mode) */ SVC_POOL_PERCPU ,/* one pool per cpu */ -SVC_POOL_PERNODE /* one pool per numa node */ +SVC_POOL_PERNODE ,/* one pool per numa node */ + SVC_POOL_WORKQUEUE, /* workqueue-based service */ }; struct svc_pool_map {
@@ -490,6 +493,9 @@ void svc_reserve(struct svc_rqst *rqstp, int space);struct svc_pool *svc_pool_for_cpu (struct svc_serv *serv,int cpu);char *svc_print_addr (struct svc_rqst *,char *,size_t ); +int svc_wq_setup (struct svc_serv *,struct svc_pool *,int ); +void svc_wq_enqueue_xprt (struct svc_xprt *); +#define RPC_MAX_ADDRBUFLEN (63U)/*
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -117,6 +117,7 @@ void svc_xprt_init(struct net *, struct svc_xprt_class *, struct svc_xprt *, struct svc_serv *);int svc_create_xprt (struct svc_serv *,const char *,struct net *,const int ,const unsigned short ,int ); +bool svc_xprt_has_something_to_do (struct svc_xprt *xprt);void svc_xprt_do_enqueue (struct svc_xprt *xprt);void svc_xprt_enqueue (struct svc_xprt *xprt);void svc_xprt_put (struct svc_xprt *xprt);
include/linux/sunrpc/svcsock.h +1 -0
@@ -53,6 +53,7 @@ static inline u32 svc_sock_final_rec(struct svc_sock *svsk)*/ void svc_close_net (struct svc_serv *,struct net *);int svc_recv (struct svc_rqst *,long ); +int svc_wq_recv (struct svc_rqst *);int svc_send (struct svc_rqst *);void svc_drop (struct svc_rqst *);void svc_sock_update_bufs (struct svc_serv *serv);
net/sunrpc/Makefile +1 -1
@@ -13,7 +13,7 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \ svc.o svcsock.o svcauth.o svcauth_unix.o \ addr.o rpcb_clnt.o timer.o xdr.o \ sunrpc_syms.o cache.o rpc_pipe.o \ - svc_xprt.o + svc_wq.o svc_xprt.o sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o sunrpc-$(CONFIG_PROC_FS) += stats.o
net/sunrpc/svc.c +13 -0
@@ -71,6 +71,8 @@ param_set_pool_mode(const char *val, struct kernel_param *kp) *ip =SVC_POOL_PERCPU ; else if (!strncmp(val, "pernode", 7)) *ip =SVC_POOL_PERNODE ; + else if (!strncmp(val, "workqueue", 9)) + *ip =SVC_POOL_WORKQUEUE ; else err = -EINVAL ;
@@ -94,6 +96,8 @@ param_get_pool_mode(char *buf, struct kernel_param *kp)return strlcpy (buf,"percpu" ,20 );case SVC_POOL_PERNODE :return strlcpy (buf,"pernode" ,20 ); +case SVC_POOL_WORKQUEUE : +return strlcpy (buf,"workqueue" ,20 );default :return sprintf (buf,"%d" , *ip); }
@@ -242,6 +246,10 @@ svc_pool_map_get(void)case SVC_POOL_PERNODE : npools =svc_pool_map_init_pernode (m);break ; +case SVC_POOL_WORKQUEUE : +/* workqueues get a pool per numa node, but don't need a map */ + npools = nr_node_ids; +break ; }if (npools <0 ) {
@@ -534,6 +542,11 @@ svc_destroy(struct svc_serv *serv)if (svc_serv_is_pooled (serv))svc_pool_map_put (); +if (serv->sv_wq) { +destroy_workqueue (serv->sv_wq); +module_put (serv->sv_ops->svo_module); + } +kfree (serv->sv_pools);kfree (serv); }
net/sunrpc/svc_wq.c new file +146 -0
146 lines not on this page · raw message
net/sunrpc/svc_xprt.c +46 -1
59 lines not on this page · raw message
Signature · 1 line
2.1.0
PATCH v2 11/16 nfsd: keep a reference to the fs_struct in svc_rqst
In reply to Jeff LaytonWhen we convert this code to use a workqueue, we won't want to allocate a new fs_struct to handle each RPC. Doing so might also be problematic since we'd be swapping out the ->fs value on a "public" workqueue kthread.
Change the code to allocate an fs struct when when allocating a svc_rqst and then switch to using that in the "nfsd" function. Once we add workqueue support, we'll have the work function switch to the new fs struct when doing the work and then switch it back.
--- fs/fs_struct.c | 59 +++++++++++++++++++++++++++++++++++++++------- fs/nfsd/nfssvc.c | 17 ++++++------- include/linux/fs_struct.h | 3 +++ include/linux/sunrpc/svc.h | 1 + net/sunrpc/svc.c | 8 +++++++ 5 files changed, 69 insertions(+), 19 deletions(-)
fs/fs_struct.c +50 -9
@@ -127,26 +127,67 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old) }return fs; } +EXPORT_SYMBOL_GPL (copy_fs_struct); -int unshare_fs_struct (void ) +/* Replace current fs struct with one given. Return a pointer to old one. */ +static struct fs_struct * +swap_fs_struct (struct fs_struct *new_fs) { -struct fs_struct *fs = current->fs; -struct fs_struct *new_fs =copy_fs_struct (fs); -int kill; - -if (!new_fs) -return -ENOMEM ; +struct fs_struct *old_fs;task_lock (current); + old_fs = current->fs; + current->fs = new_fs; +task_unlock (current); + +return old_fs; +} + +/* Put a reference to a fs_struct. */ +void put_fs_struct (struct fs_struct *fs) +{ +bool kill; +spin_lock (&fs->lock); kill = !--fs->users; - current->fs = new_fs;spin_unlock (&fs->lock); -task_unlock (current);if (kill)free_fs_struct (fs); +} +EXPORT_SYMBOL_GPL (put_fs_struct); + +/* Take an extra reference to a fs_struct. Caller must already hold one! */ +struct fs_struct * +get_fs_struct (struct fs_struct *fs) +{ +spin_lock (&fs->lock); + ++fs->users; +spin_unlock (&fs->lock); +return fs; +} +EXPORT_SYMBOL_GPL (get_fs_struct); + +/* +* Swap in a new fs_struct and drop the reference on the old one. +* Caller must have already taken the reference to the new one. +*/ +void replace_fs_struct (struct fs_struct *new_fs) +{ +struct fs_struct *old_fs =swap_fs_struct (new_fs); + +put_fs_struct (old_fs); +} +EXPORT_SYMBOL_GPL (replace_fs_struct); + +int unshare_fs_struct (void ) +{ +struct fs_struct *new_fs =copy_fs_struct (current->fs); + +if (!new_fs) +return -ENOMEM ; +replace_fs_struct (new_fs);return 0 ; }EXPORT_SYMBOL_GPL (unshare_fs_struct);
fs/nfsd/nfssvc.c +7 -10
@@ -582,15 +582,13 @@ nfsd(void *vrqstp)/* Lock module and set up kernel thread */ mutex_lock (&nfsd_mutex); -/* At this point, the thread shares current->fs -* with the init process. We need to create files with a -* umask of 0 instead of init's umask. */ -if (unshare_fs_struct () <0 ) { -printk ("Unable to start nfsd thread: out of memory\n" ); - goto out; - } - - current->fs->umask =0 ; +/* +* At this point, the thread shares current->fs with the init process. +* We need to create files with a umask of 0 instead of init's umask, +* so switch to the fs_struct associated with the rqstp. +*/ +get_fs_struct (rqstp->rq_fs); +replace_fs_struct (rqstp->rq_fs);/* * thread is spawned with all signals set to SIG_IGN, re-enable
@@ -632,7 +630,6 @@ nfsd(void *vrqstp)mutex_lock (&nfsd_mutex); nfsdstats.th_cnt --; -out: rqstp->rq_server =NULL ;/* Release the thread */
include/linux/fs_struct.h +3 -0
@@ -21,7 +21,10 @@ extern void set_fs_root(struct fs_struct *, const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *); +extern void replace_fs_struct (struct fs_struct *);extern int unshare_fs_struct (void ); +struct fs_struct *get_fs_struct (struct fs_struct *); +void put_fs_struct (struct fs_struct *);static inline void get_fs_root (struct fs_struct *fs,struct path *root) {
include/linux/sunrpc/svc.h +1 -0
@@ -298,6 +298,7 @@ struct svc_rqst {struct svc_cacherep * rq_cacherep;/* cache info */ struct task_struct *rq_task;/* service thread */ spinlock_t rq_lock;/* per-request lock */ +struct fs_struct *rq_fs;struct work_struct rq_work;/* per-request work */ };
net/sunrpc/svc.c +8 -0
@@ -20,6 +20,7 @@#include <linux/module.h> #include <linux/kthread.h> #include <linux/slab.h> +#include <linux/fs_struct.h> #include <linux/sunrpc/types.h> #include <linux/sunrpc/xdr.h>
@@ -622,6 +623,11 @@ svc_rqst_alloc(struct svc_serv *serv, struct svc_pool *pool, int node)if (!svc_init_buffer (rqstp, serv->sv_max_mesg, node)) goto out_enomem; + rqstp->rq_fs =copy_fs_struct (current->fs); +if (!rqstp->rq_fs) + goto out_enomem; + + rqstp->rq_fs->umask =0 ;return rqstp; out_enomem:svc_rqst_free (rqstp);
@@ -784,6 +790,8 @@ svc_rqst_free(struct svc_rqst *rqstp)kfree (rqstp->rq_resp);kfree (rqstp->rq_argp);kfree (rqstp->rq_auth_data); +if (rqstp->rq_fs) +put_fs_struct (rqstp->rq_fs);kfree_rcu (rqstp, rq_rcu_head); }EXPORT_SYMBOL_GPL (svc_rqst_free);
Signature · 1 line
2.1.0
PATCH v2 12/16 nfsd: add support for workqueue based service processing
In reply to Jeff LaytonAllow nfsd to create a workqueue based nfsd service if the sunrpc pool_mode is set to "workqueue".
The first iteration here is fairly simple. The xprt work simply allocates a svc_rqst object, and then queues that work.
--- fs/fs_struct.c | 3 ++- fs/nfsd/nfssvc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++-- include/linux/fs_struct.h | 1 + 3 files changed, 70 insertions(+), 3 deletions(-)
fs/fs_struct.c +2 -1
@@ -130,7 +130,7 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)EXPORT_SYMBOL_GPL (copy_fs_struct);/* Replace current fs struct with one given. Return a pointer to old one. */ -static struct fs_struct * +struct fs_struct *swap_fs_struct (struct fs_struct *new_fs) {struct fs_struct *old_fs;
@@ -142,6 +142,7 @@ swap_fs_struct(struct fs_struct *new_fs)return old_fs; } +EXPORT_SYMBOL_GPL (swap_fs_struct);/* Put a reference to a fs_struct. */ void put_fs_struct (struct fs_struct *fs)
fs/nfsd/nfssvc.c +67 -2
97 lines not on this page · raw message
include/linux/fs_struct.h +1 -0
@@ -20,6 +20,7 @@ extern void exit_fs(struct task_struct *);extern void set_fs_root (struct fs_struct *,const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *); +extern struct fs_struct *swap_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *);extern void replace_fs_struct (struct fs_struct *);extern int unshare_fs_struct (void );
Signature · 1 line
2.1.0
PATCH v2 13/16 sunrpc: keep a cache of svc_rqsts for each NUMA node
In reply to Jeff LaytonAllocating an entire svc_rqst (including all of the pages, etc...) for each workqueue request is pretty expensive. Keep a cache of allocated svc_rqst structures for each NUMA node that we keep in svc_pool.
When an xprt needs servicing we look for an existing svc_rqst if possible, attach the xprt to it and then queue it to do the work. If one isn't currently available, we queue the svc_xprt work to allocate one, add it to the cache and then queue the svc_rqst's work to handle the rest.
In order to keep the cache from growing without bound, we register a shrinker. Since the cache is already NUMA-aware, we can use a NUMA-aware shrinker as well.
--- fs/nfsd/nfssvc.c | 11 ++-- include/linux/sunrpc/svc.h | 11 ++++ net/sunrpc/svc.c | 1 + net/sunrpc/svc_wq.c | 158 ++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 172 insertions(+), 9 deletions(-)
fs/nfsd/nfssvc.c +5 -6
@@ -669,26 +669,25 @@ nfsd_rqst_work(struct work_struct *work) rqstp->rq_server->sv_maxconn = nn->max_connections;if (svc_wq_recv (rqstp) <0 ) { -svc_rqst_free (rqstp); +put_svc_rqst (rqstp);return ; } saved_fs =swap_fs_struct (rqstp->rq_fs);svc_process (rqstp); - saved_fs =swap_fs_struct (saved_fs); -svc_rqst_free (rqstp); +swap_fs_struct (saved_fs); +put_svc_rqst (rqstp); }/* work function for workqueue-based nfsd */ static void nfsd_xprt_work (struct work_struct *work) { -int node =numa_node_id (); struct svc_xprt *xprt = container_of(work, struct svc_xprt, xpt_work); -struct svc_rqst *rqstp;struct svc_serv *serv = xprt->xpt_server; +struct svc_rqst *rqstp; - rqstp =svc_rqst_alloc (serv, &serv->sv_pools[node], node); + rqstp =find_or_alloc_svc_rqst (serv);if (!rqstp) {/* Alloc failure. Give up for now, and requeue the work */ queue_work (serv->sv_wq, &xprt->xpt_work);
include/linux/sunrpc/svc.h +11 -0
@@ -108,6 +108,7 @@ struct svc_serv {struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ struct workqueue_struct *sv_wq;/* workqueue for wq-based services */ +struct shrinker sv_shrinker;/* for shrinking svc_rqst caches */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -277,6 +278,7 @@ struct svc_rqst {#define RQ_VICTIM (5)/* about to be shut down */ #define RQ_BUSY (6)/* request is busy */ unsigned long rq_flags;/* flags field */ +unsigned long rq_time;/* when rqstp was last put */ void * rq_argp;/* decoded arguments */ void * rq_resp;/* xdr'd results */
@@ -496,6 +498,15 @@ char * svc_print_addr(struct svc_rqst *, char *, size_t);int svc_wq_setup (struct svc_serv *,struct svc_pool *,int );void svc_wq_enqueue_xprt (struct svc_xprt *); +struct svc_rqst *find_or_alloc_svc_rqst (struct svc_serv *serv); +void exit_svc_rqst_cache (struct svc_serv *serv); + +static inline void +put_svc_rqst (struct svc_rqst *rqstp) +{ + rqstp->rq_time = jiffies; +clear_bit (RQ_BUSY , &rqstp->rq_flags); +}#define RPC_MAX_ADDRBUFLEN (63U)
net/sunrpc/svc.c +1 -0
@@ -545,6 +545,7 @@ svc_destroy(struct svc_serv *serv)if (serv->sv_wq) {destroy_workqueue (serv->sv_wq); +exit_svc_rqst_cache (serv);module_put (serv->sv_ops->svo_module); }
net/sunrpc/svc_wq.c +155 -3
203 lines not on this page · raw message
Signature · 1 line
2.1.0
PATCH v2 14/16 sunrpc: add more tracepoints around svc_xprt handling
In reply to Jeff LaytonTurn the svc_xprt_dequeue and svc_xprt_do_enqueue tracepoints into a event classes, and use them to add events for svc_xprt_enqueued, svc_xprt_active, svc_xprt_received, and svc_xprt_enqueue.
--- include/trace/events/sunrpc.h | 36 +++++++++++++++++++++++++++--------- net/sunrpc/svc_wq.c | 4 ++++ net/sunrpc/svc_xprt.c | 9 ++++++++- 3 files changed, 39 insertions(+), 10 deletions(-)
include/trace/events/sunrpc.h +27 -9
@@ -496,28 +496,40 @@ DEFINE_EVENT(svc_rqst_status, svc_send, { (1UL << XPT_CACHE_AUTH), "XPT_CACHE_AUTH"}, \ { (1UL << XPT_LOCAL), "XPT_LOCAL"}) -TRACE_EVENT (svc_xprt_do_enqueue, - TP_PROTO(struct svc_xprt *xprt, struct svc_rqst *rqst), +/* events associated with both a rqst and a xprt */ +DECLARE_EVENT_CLASS (svc_xprt_rqst_event, + TP_PROTO(struct svc_rqst *rqst, struct svc_xprt *xprt), -TP_ARGS (xprt, rqst), +TP_ARGS (rqst, xprt),TP_STRUCT__entry ( - __field(struct svc_xprt *, xprt) __field(struct svc_rqst *, rqst) + __field(struct svc_xprt *, xprt) ),TP_fast_assign ( - __entry->xprt = xprt; __entry->rqst = rqst; + __entry->xprt = xprt; ), -TP_printk ("xprt=0x%p addr=%pIScp pid=%d flags=%s" , __entry->xprt, - (struct sockaddr *)&__entry->xprt->xpt_remote, - __entry->rqst ? __entry->rqst->rq_task->pid :0 , +TP_printk ("rqst=0x%p xprt=0x%p addr=%pIScp flags=%s" , __entry->rqst, + __entry->xprt, (struct sockaddr *)&__entry->xprt->xpt_remote,show_svc_xprt_flags (__entry->xprt->xpt_flags)) ); -TRACE_EVENT(svc_xprt_dequeue, +DEFINE_EVENT (svc_xprt_rqst_event, svc_xprt_enqueued, + TP_PROTO(struct svc_rqst *rqst, struct svc_xprt *xprt), +TP_ARGS (rqst, xprt)); + +DEFINE_EVENT (svc_xprt_rqst_event, svc_xprt_active, + TP_PROTO(struct svc_rqst *rqst, struct svc_xprt *xprt), +TP_ARGS (rqst, xprt)); + +DEFINE_EVENT (svc_xprt_rqst_event, svc_xprt_received, + TP_PROTO(struct svc_rqst *rqst, struct svc_xprt *xprt), +TP_ARGS (rqst, xprt)); + +DECLARE_EVENT_CLASS(svc_xprt_event, TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt),
@@ -539,6 +551,12 @@ TRACE_EVENT(svc_xprt_dequeue, show_svc_xprt_flags(__entry->flags)) ); +DEFINE_EVENT (svc_xprt_event, svc_xprt_enqueue, + TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt)); + +DEFINE_EVENT (svc_xprt_event, svc_xprt_dequeue, + TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt)); + TRACE_EVENT(svc_wake_up, TP_PROTO(int pid),
net/sunrpc/svc_wq.c +4 -0
@@ -257,6 +257,8 @@ svc_wq_enqueue_xprt(struct svc_xprt *xprt)struct svc_serv *serv = xprt->xpt_server;struct svc_rqst *rqstp; +trace_svc_xprt_enqueue (xprt); +if (!svc_xprt_has_something_to_do (xprt))return ;
@@ -290,9 +292,11 @@ out: rqstp =find_svc_rqst (serv);if (!rqstp) {queue_work (serv->sv_wq, &xprt->xpt_work); +trace_svc_xprt_enqueued (NULL , xprt);return ; } rqstp->rq_xprt = xprt;queue_work (serv->sv_wq, &rqstp->rq_work); +trace_svc_xprt_enqueued (rqstp, xprt); }EXPORT_SYMBOL_GPL (svc_wq_enqueue_xprt);
net/sunrpc/svc_xprt.c +8 -1
@@ -242,6 +242,7 @@ void svc_add_new_perm_xprt(struct svc_serv *serv, struct svc_xprt *new)spin_lock_bh (&serv->sv_lock);list_add (&new->xpt_list, &serv->sv_permsocks);spin_unlock_bh (&serv->sv_lock); +trace_svc_xprt_received (NULL , new);svc_xprt_received (new); }
@@ -329,6 +330,8 @@ void svc_xprt_do_enqueue(struct svc_xprt *xprt)int cpu;bool queued = false; +trace_svc_xprt_enqueue (xprt); +if (!svc_xprt_has_something_to_do (xprt)) goto out;
@@ -402,7 +405,7 @@ redo_search: rqstp =NULL ;put_cpu (); out: -trace_svc_xprt_do_enqueue (xprt, rqstp); +trace_svc_xprt_enqueued (rqstp, xprt); }EXPORT_SYMBOL_GPL (svc_xprt_do_enqueue);
@@ -738,6 +741,7 @@ static void svc_add_new_temp_xprt(struct svc_serv *serv, struct svc_xprt *newxpt jiffies + svc_conn_age_period * HZ); }spin_unlock_bh (&serv->sv_lock); +trace_svc_xprt_received (NULL , newxpt);svc_xprt_received (newxpt); }
@@ -746,6 +750,8 @@ static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)struct svc_serv *serv = rqstp->rq_server;int len =0 ; +trace_svc_xprt_active (rqstp, xprt); +if (test_bit (XPT_CLOSE , &xprt->xpt_flags)) {dprintk ("svc_recv: found XPT_CLOSE\n" );svc_delete_xprt (xprt);
@@ -780,6 +786,7 @@ static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)atomic_add (rqstp->rq_reserved, &xprt->xpt_reserved); }/* clear XPT_BUSY: */ +trace_svc_xprt_received (rqstp, xprt);svc_xprt_received (xprt); out:trace_svc_handle_xprt (xprt, len);
Signature · 1 line
2.1.0
PATCH v2 15/16 sunrpc: print the svc_rqst pointer value in svc_process tracepoint
In reply to Jeff Layton--- include/trace/events/sunrpc.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
include/trace/events/sunrpc.h +4 -2
@@ -454,6 +454,7 @@ DECLARE_EVENT_CLASS(svc_rqst_status, TP_ARGS(rqst, status),TP_STRUCT__entry ( +__field (struct svc_rqst *,rqst ) __field(struct sockaddr *, addr)__field (__be32, xid) __field(int, dropme)
@@ -462,14 +463,15 @@ DECLARE_EVENT_CLASS(svc_rqst_status, ), TP_fast_assign( + __entry->rqst = rqst; __entry->addr = (struct sockaddr *)&rqst->rq_addr; __entry->xid = rqst->rq_xid; __entry->status = status; __entry->flags = rqst->rq_flags; ), -TP_printk ("addr=%pIScp rq_xid=0x%x status=%d flags=%s" , - __entry->addr,be32_to_cpu (__entry->xid), +TP_printk ("rqst=0x%p addr=%pIScp rq_xid=0x%x status=%d flags=%s" , + __entry->rqst, __entry->addr,be32_to_cpu (__entry->xid), __entry->status,show_rqstp_flags (__entry->flags)) );
Signature · 1 line
2.1.0
PATCH v2 16/16 sunrpc: add tracepoints around svc_sock handling
In reply to Jeff Layton--- include/trace/events/sunrpc.h | 46 +++++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svcsock.c | 6 ++++++ 2 files changed, 52 insertions(+)
include/trace/events/sunrpc.h +46 -0
@@ -9,6 +9,7 @@#include <linux/sunrpc/svc.h> #include <linux/sunrpc/xprtsock.h> #include <linux/sunrpc/svc_xprt.h> +#include <linux/sunrpc/svcsock.h> #include <net/tcp_states.h> #include <linux/net.h> #include <linux/tracepoint.h>
@@ -594,6 +595,51 @@ TRACE_EVENT(svc_handle_xprt, (struct sockaddr *)&__entry->xprt->xpt_remote, __entry->len, show_svc_xprt_flags(__entry->xprt->xpt_flags)) ); + +DECLARE_EVENT_CLASS (svc_socket_event, + TP_PROTO(struct sock *sk), + +TP_ARGS (sk), + +TP_STRUCT__entry ( + __field(struct sock *, sk) + __field(struct svc_sock *, svsk) + __field_struct(struct sockaddr_storage, ss) + __field(unsigned long, xpt_flags) + ), + +TP_fast_assign ( + __entry->sk = sk; + __entry->svsk = sk->sk_user_data; + if (__entry->svsk) { + struct svc_xprt *xprt = &__entry->svsk->sk_xprt; + + memcpy(&__entry->ss, &xprt->xpt_remote, + sizeof(__entry->ss)); + __entry->xpt_flags = xprt->xpt_flags; + } else { +memset (&__entry->ss,0 ,sizeof (__entry->ss)); + __entry->xpt_flags =0 ; + } + ), + +TP_printk ("sk=0x%p peer=%pIScp sk_state=%s xpt_flags=%s" , __entry->sk, + (struct sockaddr *)&__entry->ss, +rpc_show_sock_state (__entry->sk->sk_state), +show_svc_xprt_flags (__entry->xpt_flags)) +); + +DEFINE_EVENT (svc_socket_event, svc_tcp_listen_data_ready, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_state_change, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_data_ready, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_accept, + TP_PROTO(struct sock *sk), TP_ARGS(sk));#endif /* _TRACE_SUNRPC_H */ #include <trace/define_trace.h>
net/sunrpc/svcsock.c +6 -0
@@ -44,6 +44,7 @@#include <asm/uaccess.h> #include <asm/ioctls.h> #include <trace/events/skb.h> +#include <trace/events/sunrpc.h> #include <linux/sunrpc/types.h> #include <linux/sunrpc/clnt.h>
@@ -765,6 +766,7 @@ static void svc_tcp_listen_data_ready(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq; +trace_svc_tcp_listen_data_ready (sk);dprintk ("svc: socket %p TCP (listen) state change %d\n" , sk, sk->sk_state);
@@ -799,6 +801,7 @@ static void svc_tcp_state_change(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq =sk_sleep (sk); +trace_svc_tcp_state_change (sk);dprintk ("svc: socket %p TCP (connected) state change %d (svsk %p)\n" , sk, sk->sk_state, sk->sk_user_data);
@@ -817,6 +820,7 @@ static void svc_tcp_data_ready(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq =sk_sleep (sk); +trace_svc_tcp_data_ready (sk);dprintk ("svc: socket %p TCP data ready (svsk %p)\n" , sk, sk->sk_user_data);if (svsk) {
@@ -842,6 +846,8 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)int err, slen; RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); +trace_svc_tcp_accept (svsk->sk_sk); +dprintk ("svc: tcp_accept %p sock %p\n" , svsk, sock);if (!sock)return NULL ;
Signature · 1 line
2.1.0
Hi Jeff-
Quoted 136 lines · Jeff Layton
This is the second revision of the workqueue-based nfsd. Changes from the RFC version I posted earlier:
v2:
- found and fixed problem causing the delays between work. It was a bug in the design of the new code. I was queueing the svc_xprt's work to do everything and that necessarily serialized that work. This version adds awork_structto thesvc_rqstas well, and that's where the bulk of the work.- no longer tries to use the thread count as a
max_activesetting for the workqueue. The workqueuemax_activesettings are left to the default. This means that/proc/fs/nfsd/threadsis basically a binary switch that's either zero (not running) or non-zero (running). The actual value has no meaning.- the handling of the
fs_structhas been reworked. It's now allocated as part of the structsvc_rqst. Eachsvc_rqsthas its ownfs_structnow.-
CONFIG_SUNRPC_SVC_WORKQUEUEhas been removed. Since the code must be enabled at runtime anyway, we don't need a switch to disable this code at build time.- the new tracepoints have been reworked.
This new code is still a little slower (2-3%) than the older, thread based code in my testing, though my hope is that it may perform better than the older code on a NUMA machine. I don't have one at the moment to verify that, however.
For background, here's an excerpt from the original posting:
This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
Performance:
------------
As far as numbers go, I ran a modified version of the fio tiobench-example.fio test that just reduces the file size to 128M:threads:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13740KB, aggrb=228KB/s, minb=57KB/s, maxb=57KB/s,
mint=60034msec, maxt=60049msecRun status group 1 (all jobs):
WRITE: io=14652KB, aggrb=244KB/s, minb=60KB/s, maxb=61KB/s,
mint=60002msec, maxt=60028msecRun status group 2 (all jobs):
READ: io=524288KB, aggrb=49639KB/s, minb=12409KB/s, maxb=12433KB/s,
mint=10542msec, maxt=10562msecRun status group 3 (all jobs):
READ: io=524288KB, aggrb=50670KB/s, minb=12667KB/s, maxb=12687KB/s,
mint=10331msec, maxt=10347msecworkqueue:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13632KB, aggrb=226KB/s, minb=56KB/s, maxb=56KB/s,
mint=60010msec, maxt=60061msecRun status group 1 (all jobs):
WRITE: io=14328KB, aggrb=238KB/s, minb=59KB/s, maxb=59KB/s,
mint=60023msec, maxt=60033msecRun status group 2 (all jobs):
READ: io=524288KB, aggrb=47779KB/s, minb=11944KB/s, maxb=11955KB/s,
mint=10963msec, maxt=10973msecRun status group 3 (all jobs):
READ: io=524288KB, aggrb=48379KB/s, minb=12094KB/s, maxb=12114KB/s,
mint=10819msec, maxt=10837msec...a small performance decrease using workqueues (about 5% in the worst case). It varies a little between runs but the results over several runs are fairly consistent in showing a small perf decrease.
In an attempt to ascertain where that comes from, I added some tracepoints and wrote a perl script to scrape the results and figure out where the latency comes from. The numbers here are from the ones that produced the above results:
[jlayton@palma nfsd-wq]$ ./analyze.pl < threads.txt
-------------------------------------------------------------
rpcs=272437
queue=3.79952429455322 queued=10.0558330900775 receive=10.6196625266701 process=2325.8173229043 total=2350.2923428156[jlayton@palma nfsd-wq]$ ./analyze.pl < workqueue.txt
-------------------------------------------------------------
rpcs=272329
queue=4.41979737859012
queued=8.124893049967 receive=11.3421082590608 process=2310.15945786277 total=2334.04625655039Here's a legend, the numbers represent a delta between two tracepoints for a particular xprt or rqst. I can provide the script if it's helpful but it's definitely hacked together:
rpcs: total number of rpcs processed (just a count)
queue: time it took to queue the xprt.xi
trace_svc_xprt_enqueued-trace_svc_xprt_enqueue(in usecs)queued: time between being queued and going "active"
trace_svc_xprt_active-trace_svc_xprt_enqueued(in usecs)receive: time between going "active" and completing the receive
trace_svc_xprt_received-trace_svc_xprt_active(in usecs)process: time between completing the receive and finishing processing
trace_svc_process-trace_svc_xprt_recevied(in usecs)total: total time from enqueueing to finishing the processing
trace_svc_process-trace_svc_xprt_enqueued(in usecs)The interesting bit is that according to these numbers, the workqueue RPCs ran more quickly on average (though most of that is in
svc_process, for which I have zero explanation). So, why are we getting slower results?
Quoted 7 lines
One theory is that it's getting bitten by the fact that the workqueue queueing/dequeueing code uses spinlocks that disable bottom halves. Perhaps that adds up and causes more latency to softirq processing? I'm not sure how best to nail that down...
It's probably worth it to start considering this for v3.20, but I'd like to get some time on a larger scale test rig to see how it does first.
I think there needs to be a guarantee that this does not break badly with NFS/RDMA, with xfs / btrfs / ext4, or with 40 and 56GbE. (Yes, we are volunteering to help with some of that).
How does it scale in number of individual transport sockets?
Does the new scheduling model use more or less CPU per byte and per RPC?
Also, running these performance tests with a CPU soaker running on the NFS server might be interesting. Something like a large software build. That could better expose issues with workqueue scheduling.
That feels like more work than any of us can do before 3.20.
Quoted 38 lines
We'll also need Al's ACK on the
fs_structstuff.Thoughts or comments are welcome.
Jeff Layton (16):
sunrpc: add a newsvc_serv_opsstruct and movesv_shutdowninto it
sunrpc: movesv_functionintosv_ops
sunrpc: movesv_moduleparm intosv_ops
sunrpc: turn enqueueing asvc_xprtinto asvc_servoperation
sunrpc: abstract outsvc_set_num_threadstosv_ops
sunrpc: movepool_modedefinitions intosvc.h
sunrpc: factorsvc_rqstallocation and freeing fromsv_nrthreads
refcounting
sunrpc: set up workqueue function insvc_xprt
sunrpc: set upsvc_rqstwork if it's defined
sunrpc: add basic support for workqueue-based services
nfsd: keep a reference to thefs_structinsvc_rqst
nfsd: add support for workqueue based service processing
sunrpc: keep a cache ofsvc_rqstsfor each NUMA node
sunrpc: add more tracepoints aroundsvc_xprthandling
sunrpc: print thesvc_rqstpointer value insvc_processtracepoint
sunrpc: add tracepoints aroundsvc_sockhandlingfs/fs_struct.c | 60 ++++++-- fs/lockd/svc.c | 7 +- fs/nfs/callback.c | 6 +- fs/nfsd/nfssvc.c | 100 +++++++++++-- include/linux/fs_struct.h | 4 + include/linux/sunrpc/svc.h | 93 ++++++++++--- include/linux/sunrpc/svc_xprt.h | 3 + include/linux/sunrpc/svcsock.h | 1 + include/trace/events/sunrpc.h | 88 ++++++++++-- net/sunrpc/Makefile | 2 +- net/sunrpc/svc.c | 141 +++++++++++-------- net/sunrpc/svc_wq.c | 302 ++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svc_xprt.c | 68 ++++++++- net/sunrpc/svcsock.c | 6 + 14 files changed, 758 insertions(+), 123 deletions(-)
Quoted 9 lines
create mode 100644
net/sunrpc/svc_wq.cSignature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Quoted 145 lines · On Wed, 10 Dec 2014 14:31:55 -0800 Chuck Lever <[email protected]> wrote:
Hi Jeff-
Quoted 141 lines · Jeff Layton
This is the second revision of the workqueue-based nfsd. Changes from the RFC version I posted earlier:
v2:
- found and fixed problem causing the delays between work. It was a bug in the design of the new code. I was queueing the svc_xprt's work to do everything and that necessarily serialized that work. This version adds awork_structto thesvc_rqstas well, and that's where the bulk of the work.- no longer tries to use the thread count as a
max_activesetting for the workqueue. The workqueuemax_activesettings are left to the default. This means that/proc/fs/nfsd/threadsis basically a binary switch that's either zero (not running) or non-zero (running). The actual value has no meaning.- the handling of the
fs_structhas been reworked. It's now allocated as part of the structsvc_rqst. Eachsvc_rqsthas its ownfs_structnow.-
CONFIG_SUNRPC_SVC_WORKQUEUEhas been removed. Since the code must be enabled at runtime anyway, we don't need a switch to disable this code at build time.- the new tracepoints have been reworked.
This new code is still a little slower (2-3%) than the older, thread based code in my testing, though my hope is that it may perform better than the older code on a NUMA machine. I don't have one at the moment to verify that, however.
For background, here's an excerpt from the original posting:
This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
Performance:
------------
As far as numbers go, I ran a modified version of the fio tiobench-example.fio test that just reduces the file size to 128M:threads:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13740KB, aggrb=228KB/s, minb=57KB/s, maxb=57KB/s,
mint=60034msec, maxt=60049msecRun status group 1 (all jobs):
WRITE: io=14652KB, aggrb=244KB/s, minb=60KB/s, maxb=61KB/s,
mint=60002msec, maxt=60028msecRun status group 2 (all jobs):
READ: io=524288KB, aggrb=49639KB/s, minb=12409KB/s, maxb=12433KB/s,
mint=10542msec, maxt=10562msecRun status group 3 (all jobs):
READ: io=524288KB, aggrb=50670KB/s, minb=12667KB/s, maxb=12687KB/s,
mint=10331msec, maxt=10347msecworkqueue:
----------------------------------------------------------------------
Run status group 0 (all jobs):
WRITE: io=13632KB, aggrb=226KB/s, minb=56KB/s, maxb=56KB/s,
mint=60010msec, maxt=60061msecRun status group 1 (all jobs):
WRITE: io=14328KB, aggrb=238KB/s, minb=59KB/s, maxb=59KB/s,
mint=60023msec, maxt=60033msecRun status group 2 (all jobs):
READ: io=524288KB, aggrb=47779KB/s, minb=11944KB/s, maxb=11955KB/s,
mint=10963msec, maxt=10973msecRun status group 3 (all jobs):
READ: io=524288KB, aggrb=48379KB/s, minb=12094KB/s, maxb=12114KB/s,
mint=10819msec, maxt=10837msec...a small performance decrease using workqueues (about 5% in the worst case). It varies a little between runs but the results over several runs are fairly consistent in showing a small perf decrease.
In an attempt to ascertain where that comes from, I added some tracepoints and wrote a perl script to scrape the results and figure out where the latency comes from. The numbers here are from the ones that produced the above results:
[jlayton@palma nfsd-wq]$ ./analyze.pl < threads.txt
-------------------------------------------------------------
rpcs=272437
queue=3.79952429455322 queued=10.0558330900775 receive=10.6196625266701 process=2325.8173229043 total=2350.2923428156[jlayton@palma nfsd-wq]$ ./analyze.pl < workqueue.txt
-------------------------------------------------------------
rpcs=272329
queue=4.41979737859012
queued=8.124893049967 receive=11.3421082590608 process=2310.15945786277 total=2334.04625655039Here's a legend, the numbers represent a delta between two tracepoints for a particular xprt or rqst. I can provide the script if it's helpful but it's definitely hacked together:
rpcs: total number of rpcs processed (just a count)
queue: time it took to queue the xprt.xi
trace_svc_xprt_enqueued-trace_svc_xprt_enqueue(in usecs)queued: time between being queued and going "active"
trace_svc_xprt_active-trace_svc_xprt_enqueued(in usecs)receive: time between going "active" and completing the receive
trace_svc_xprt_received-trace_svc_xprt_active(in usecs)process: time between completing the receive and finishing processing
trace_svc_process-trace_svc_xprt_recevied(in usecs)total: total time from enqueueing to finishing the processing
trace_svc_process-trace_svc_xprt_enqueued(in usecs)The interesting bit is that according to these numbers, the workqueue RPCs ran more quickly on average (though most of that is in
svc_process, for which I have zero explanation). So, why are we getting slower results?One theory is that it's getting bitten by the fact that the workqueue queueing/dequeueing code uses spinlocks that disable bottom halves. Perhaps that adds up and causes more latency to softirq processing? I'm not sure how best to nail that down...
Quoted 7 lines
Quoted 2 lines
It's probably worth it to start considering this for v3.20, but I'd like to get some time on a larger scale test rig to see how it does first.
I think there needs to be a guarantee that this does not break badly with NFS/RDMA, with xfs / btrfs / ext4, or with 40 and 56GbE. (Yes, we are volunteering to help with some of that).
That would be helpful. I wouldn't expect any breakage with any of that given where I added the operations, but you never know.
Quoted 2 lines
How does it scale in number of individual transport sockets?
I'd expect that it'll do well given that we use RCU when searching for a svc_rqst, but I don't have a test rig that has a ton of clients so I can't guarantee that.
Quoted 3 lines
Does the new scheduling model use more or less CPU per byte and per RPC?
Good question. My expectation would be that it wouldn't make much difference, but again I don't know for sure.
I'd also like very much to account for the drop in throughput I'm seeing with this code, but I'm unclear on the best method to chase that down as of yet.
Quoted 7 lines
Also, running these performance tests with a CPU soaker running on the NFS server might be interesting. Something like a large software build. That could better expose issues with workqueue scheduling.
That feels like more work than any of us can do before 3.20.
Quite likely.
v3.20 isn't a firm goal. Heck I may drop this altogether if it turns out to be slower and we can't figure out any way to make it better.
A primary goal here is to avoid breaking the existing code (of course). One thing I'm really trying to do is avoid substantially altering the code in the case where you don't opt to use workqueues.
I'm adding some function pointers that will need to be chased, of course but I don't think that'll add much overhead in the context of all of the other stuff you have to do to handle an RPC.
Quoted 56 lines
Quoted 36 lines
We'll also need Al's ACK on the
fs_structstuff.Thoughts or comments are welcome.
Jeff Layton (16):
sunrpc: add a newsvc_serv_opsstruct and movesv_shutdowninto it
sunrpc: movesv_functionintosv_ops
sunrpc: movesv_moduleparm intosv_ops
sunrpc: turn enqueueing asvc_xprtinto asvc_servoperation
sunrpc: abstract outsvc_set_num_threadstosv_ops
sunrpc: movepool_modedefinitions intosvc.h
sunrpc: factorsvc_rqstallocation and freeing fromsv_nrthreads
refcounting
sunrpc: set up workqueue function insvc_xprt
sunrpc: set upsvc_rqstwork if it's defined
sunrpc: add basic support for workqueue-based services
nfsd: keep a reference to thefs_structinsvc_rqst
nfsd: add support for workqueue based service processing
sunrpc: keep a cache ofsvc_rqstsfor each NUMA node
sunrpc: add more tracepoints aroundsvc_xprthandling
sunrpc: print thesvc_rqstpointer value insvc_processtracepoint
sunrpc: add tracepoints aroundsvc_sockhandlingfs/fs_struct.c | 60 ++++++-- fs/lockd/svc.c | 7 +- fs/nfs/callback.c | 6 +- fs/nfsd/nfssvc.c | 100 +++++++++++-- include/linux/fs_struct.h | 4 + include/linux/sunrpc/svc.h | 93 ++++++++++--- include/linux/sunrpc/svc_xprt.h | 3 + include/linux/sunrpc/svcsock.h | 1 + include/trace/events/sunrpc.h | 88 ++++++++++-- net/sunrpc/Makefile | 2 +- net/sunrpc/svc.c | 141 +++++++++++-------- net/sunrpc/svc_wq.c | 302Quoted 13 lines
++++++++++++++++++++++++++++++++++++++++
net/sunrpc/svc_xprt.c| 68 ++++++++-net/sunrpc/svcsock.c| 6 + 14 files changed, 758
insertions(+), 123 deletions(-) create mode 100644net/sunrpc/svc_wq.cSignature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Signature · 1 line
Jeff Layton <[email protected]>
Al Viro
maintainerQuoted 1 line · Jeff Layton
We'll also need Al's ACK on the
fs_structstuff.
... and I'm not happy with it. First of all, ditch those EXPORT_SYMBOL_GPL(); if it's too low-level for general export (and many of those are), tacking _GPL on it doesn't make it any better. unshare_fs_struct() misbegotten export is a historical accident - somebody (agruen, perhaps?) slapped a _GPL export on its precursors and I had taken a lazy approach back then. Shouldn't have done that... Please, don't add more of that shit.
More to the point, though, it *is* far too low-level, and for no visible reason. AFAICS, what you want to achieve is zero umask in that fucker. TBH, I really wonder if any of that is needed at all. Why do we want kernel threads to get umask shared with init(8), anyway? It's very easy to change - all it takes is
* make init_fs.umask zero
* make kernel_init cloned without CLONE_FS and have it immediately
set its ->fs->umask to 0022
* make ____call_usermodehelper() (always called very early in the
life of a thread that had been cloned without CLONE_FS) do the same thing. Voila - all kernel threads have umask 0 and it's not affected by whatever init(8) might be pulling off. And that includes all worqueue workers, etc.
With that I'm not sure we need to have unshare_fs_struct() at all; at least not unless some thread wants to play with chdir() and chroot() and I don't see anything of that sort in nfsd and lustre (the only callers of unshare_fs_struct() in the tree). Note that set_fs_pwd() and set_fs_root() are *not* exported, and neither are sys_{chdir,fchdir,chroot}, so nfsd and lustre would have a hard time trying to do something of that sort anyway. There is open-coded crap trying to implement chdir in lustre_compat25.h, but it has no callers...
Linus, do you see any problems with the following patch (against the mainline)? If not, I'll put it into the next vfs.git pull request, along with removal of all mentionings of ->fs-> in lustre (aside of aforementioned dead code, there's open-coded current_umask() in one place and broken attempt to reimplement dentry_path() in another).
---
drivers/staging/lustre/include/linux/libcfs/libcfs.h +0 -1
@@ -136,7 +136,6 @@ void cfs_enter_debugger(void);/* * Defined by platform */ -int unshare_fs_struct (void );sigset_t cfs_get_blocked_sigs (void );sigset_t cfs_block_allsigs (void );sigset_t cfs_block_sigs (unsigned long sigs);
drivers/staging/lustre/lustre/obdclass/genops.c +0 -1
@@ -1713,7 +1713,6 @@ EXPORT_SYMBOL(obd_zombie_barrier);*/ static int obd_zombie_impexp_thread (void *unused) { -unshare_fs_struct ();complete (&obd_zombie_start); obd_zombie_pid =current_pid ();
drivers/staging/lustre/lustre/obdclass/llog.c +0 -2
@@ -411,8 +411,6 @@ static int llog_process_thread_daemonize(void *arg)struct lu_env env;int rc; -unshare_fs_struct (); -/* client env has no keys, tags is just 0 */ rc =lu_env_init (&env,LCT_LOCAL |LCT_MG_THREAD );if (rc)
drivers/staging/lustre/lustre/ptlrpc/import.c +0 -2
@@ -1290,8 +1290,6 @@ static int ptlrpc_invalidate_import_thread(void *data) {struct obd_import *imp = data; -unshare_fs_struct (); -CDEBUG (D_HA ,"thread invalidate import %s to %s@%s\n" , imp->imp_obd->obd_name,obd2cli_tgt (imp->imp_obd), imp->imp_connection->c_remote_uuid.uuid);
drivers/staging/lustre/lustre/ptlrpc/pinger.c +0 -2
@@ -586,8 +586,6 @@ static int ping_evictor_main(void *arg)struct l_wait_info lwi = {0 };time_t expire_time; -unshare_fs_struct (); -CDEBUG (D_HA ,"Starting Ping Evictor\n" ); pet_state =PET_READY ;while (1 ) {
drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c +0 -1
@@ -382,7 +382,6 @@ static int ptlrpcd(void *arg)struct lu_env env = { .le_ses =NULL };int rc, exit =0 ; -unshare_fs_struct ();#if defined(CONFIG_SMP )if (test_bit (LIOD_BIND , &pc->pc_flags)) {int index = pc->pc_index;
drivers/staging/lustre/lustre/ptlrpc/sec_gc.c +0 -2
@@ -164,8 +164,6 @@ static int sec_gc_main(void *arg)struct ptlrpc_thread *thread = (struct ptlrpc_thread *) arg;struct l_wait_info lwi; -unshare_fs_struct (); -/* Record that the thread is running */ thread_set_flags (thread,SVC_RUNNING );wake_up (&thread->t_ctl_waitq);
drivers/staging/lustre/lustre/ptlrpc/service.c +0 -2
@@ -2277,7 +2277,6 @@ static int ptlrpc_main(void *arg)int counter =0 , rc =0 ; thread->t_pid =current_pid (); -unshare_fs_struct ();/* NB: we will call cfs_cpt_bind() for all threads, because we * might want to run lustre server only on a subset of system CPUs,
@@ -2478,7 +2477,6 @@ static int ptlrpc_hr_main(void *arg)snprintf (threadname,sizeof (threadname),"ptlrpc_hr%02d_%03d" , hrp->hrp_cpt, hrt->hrt_id); -unshare_fs_struct (); rc =cfs_cpt_bind (ptlrpc_hr.hr_cpt_table, hrp->hrp_cpt);if (rc !=0 ) {
fs/fs_struct.c +1 -24
@@ -128,29 +128,6 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)return fs; } -int unshare_fs_struct (void ) -{ -struct fs_struct *fs = current->fs; -struct fs_struct *new_fs =copy_fs_struct (fs); -int kill; - -if (!new_fs) -return -ENOMEM ; - -task_lock (current); -spin_lock (&fs->lock); - kill = !--fs->users; - current->fs = new_fs; -spin_unlock (&fs->lock); -task_unlock (current); - -if (kill) -free_fs_struct (fs); - -return 0 ; -} -EXPORT_SYMBOL_GPL (unshare_fs_struct); -int current_umask (void ) {return current->fs->umask;
@@ -162,5 +139,5 @@ struct fs_struct init_fs = { .users = 1, .lock = __SPIN_LOCK_UNLOCKED(init_fs.lock), .seq = SEQCNT_ZERO(init_fs.seq), - .umask = 0022, + .umask = 0, };
fs/nfsd/nfssvc.c +0 -10
@@ -573,16 +573,6 @@ nfsd(void *vrqstp)/* Lock module and set up kernel thread */ mutex_lock (&nfsd_mutex); -/* At this point, the thread shares current->fs -* with the init process. We need to create files with a -* umask of 0 instead of init's umask. */ -if (unshare_fs_struct () <0 ) { -printk ("Unable to start nfsd thread: out of memory\n" ); - goto out; - } - - current->fs->umask =0 ; -/* * thread is spawned with all signals set to SIG_IGN, re-enable * the ones that will bring down the thread
include/linux/fs_struct.h +0 -1
@@ -21,7 +21,6 @@ extern void set_fs_root(struct fs_struct *, const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *); -extern int unshare_fs_struct (void );static inline void get_fs_root (struct fs_struct *fs,struct path *root) {
init/main.c +2 -1
@@ -399,7 +399,7 @@ static noinline void __init_refok rest_init(void)* the init task will end up wanting to create kthreads, which, if * we schedule it before we create kthreadd, will OOPS. */ -kernel_thread (kernel_init,NULL ,CLONE_FS ); +kernel_thread (kernel_init,NULL ,0 );numa_default_policy (); pid =kernel_thread (kthreadd,NULL ,CLONE_FS |CLONE_FILES );rcu_read_lock ();
@@ -924,6 +924,7 @@ static int __ref kernel_init(void *unused) {int ret; + current->fs->umask =0022 ;kernel_init_freeable ();/* need to finish all async __init code before freeing the memory */ async_synchronize_full ();
kernel/kmod.c +1 -0
@@ -219,6 +219,7 @@ static int ____call_usermodehelper(void *data)struct cred *new;int retval; + current->fs->umask =0022 ;spin_lock_irq (¤t->sighand->siglock);flush_signal_handlers (current,1 );spin_unlock_irq (¤t->sighand->siglock);
Quoted 2 lines · Al Viro
Linus, do you see any problems with the following patch (against the mainline)?
Not concpetually, but create_kthread() uses CLONE_FS, and I don't think it's just umask that things like nfsd want to avoid sharing. What about all the *other* fields?
Just as an example: even if all the threads actually end up all having the same global root, what about contention on 'fs->lock'?
I have *not* looked at the details, and maybe there's some reason I'm completely off, but it worries me.
Linus
Al Viro
maintainerQuoted 8 lines · Linus Torvalds
Quoted 2 lines · Al Viro
Linus, do you see any problems with the following patch (against the mainline)?
Not concpetually, but
create_kthread()usesCLONE_FS, and I don't think it's just umask that things like nfsd want to avoid sharing. What about all the *other* fields?
Quoted 5 lines
Just as an example: even if all the threads actually end up all having the same global root, what about contention on '
fs->lock'?I have *not* looked at the details, and maybe there's some reason I'm completely off, but it worries me.
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway. fh_to_dentry and friends doesn't care about root/cwd, etc.
I don't see anything that could cause that kind of issues.
Al Viro
maintainerQuoted 15 lines · Al Viro
Quoted 8 lines · Linus Torvalds
Quoted 2 lines · Al Viro
Linus, do you see any problems with the following patch (against the mainline)?
Not concpetually, but
create_kthread()usesCLONE_FS, and I don't think it's just umask that things like nfsd want to avoid sharing. What about all the *other* fields?Quoted 5 lines
Just as an example: even if all the threads actually end up all having the same global root, what about contention on '
fs->lock'?I have *not* looked at the details, and maybe there's some reason I'm completely off, but it worries me.
Quoted 7 lines
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway.
fh_to_dentryand friends doesn't care about root/cwd, etc.I don't see anything that could cause that kind of issues.
PS: I haven't checked if lustre is trying to avoid that kind of contention; it might be, but I would consider having those threads resolve _any_ kind of pathnames from root or cwd as serious bug - after all, we are not guaranteed that filesystem in question is reachable from the namespace PID 1 is running it.
Quoted 15 lines · On Fri, 12 Dec 2014 03:02:06 +0000 Al Viro <[email protected]> wrote:
Quoted 8 lines · Linus Torvalds
Quoted 2 lines · Al Viro
Linus, do you see any problems with the following patch (against the mainline)?
Not concpetually, but
create_kthread()usesCLONE_FS, and I don't think it's just umask that things like nfsd want to avoid sharing. What about all the *other* fields?Quoted 5 lines
Just as an example: even if all the threads actually end up all having the same global root, what about contention on '
fs->lock'?I have *not* looked at the details, and maybe there's some reason I'm completely off, but it worries me.
Quoted 7 lines
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway.
fh_to_dentryand friends doesn't care about root/cwd, etc.I don't see anything that could cause that kind of issues.
I like the change overall -- it would certainly make my patch series simpler, but what about pathwalking? We do take the fs->lock in unlazy_walk. Is it possible we'd end up with more contention there?
Signature · 1 line
Jeff Layton <[email protected]>
Al Viro
maintainerQuoted 11 lines · Jeff Layton
Quoted 7 lines
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway.
fh_to_dentryand friends doesn't care about root/cwd, etc.I don't see anything that could cause that kind of issues.
I like the change overall -- it would certainly make my patch series simpler, but what about pathwalking? We do take the
fs->lockinunlazy_walk. Is it possible we'd end up with more contention there?
That would take a pathname lookup in kernel thread side of nfsd that
* isn't single-component
* isn't LOOKUP_ROOT one (i.e. vfs_path_lookup() or file_open_root())
and I would really hope we don't have such things. Any such a beast would allow probing the tree layout outside of what we export, to start with...
AFAICS, we really don't have anything of that sort. Note that e.g. lookup_one_len() doesn't go anywhere near ->fs->lock...
Jeff Layton
Acked-byQuoted 14 lines · On Fri, 12 Dec 2014 16:59:52 +0000 Al Viro <[email protected]> wrote:
Quoted 11 lines · Jeff Layton
Quoted 7 lines
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway.
fh_to_dentryand friends doesn't care about root/cwd, etc.I don't see anything that could cause that kind of issues.
I like the change overall -- it would certainly make my patch series simpler, but what about pathwalking? We do take the
fs->lockinunlazy_walk. Is it possible we'd end up with more contention there?
Quoted 8 lines
That would take a pathname lookup in kernel thread side of nfsd that
* isn't single-component
* isn'tLOOKUP_ROOTone (i.e.vfs_path_lookup()orfile_open_root())
and I would really hope we don't have such things. Any such a beast would allow probing the tree layout outside of what we export, to start with...AFAICS, we really don't have anything of that sort. Note that e.g.
lookup_one_len()doesn't go anywhere near ->fs->lock...
Ahh right. Ok, then I don't see any issue with this so far. Maybe worth letting it stew in -next once -rc1 ships? Thanks!
Al Viro
maintainerQuoted 23 lines · Jeff Layton
Quoted 20 lines · On Fri, 12 Dec 2014 16:59:52 +0000 Al Viro <[email protected]> wrote:
Quoted 11 lines · Jeff Layton
Quoted 7 lines
Umm... I would be very surprised if it turned out to be a problem. nfsd really doesn't give a fuck about its cwd and root - not in the thread side of things. And (un)exporting is (a) not on a hot path and (b) not done from a kernel thread anyway.
fh_to_dentryand friends doesn't care about root/cwd, etc.I don't see anything that could cause that kind of issues.
I like the change overall -- it would certainly make my patch series simpler, but what about pathwalking? We do take the
fs->lockinunlazy_walk. Is it possible we'd end up with more contention there?That would take a pathname lookup in kernel thread side of nfsd that
* isn't single-component
* isn'tLOOKUP_ROOTone (i.e.vfs_path_lookup()orfile_open_root())
and I would really hope we don't have such things. Any such a beast would allow probing the tree layout outside of what we export, to start with...
Quoted 5 lines
Quoted 2 lines
AFAICS, we really don't have anything of that sort. Note that e.g.
lookup_one_len()doesn't go anywhere near ->fs->lock...Ahh right. Ok, then I don't see any issue with this so far. Maybe worth letting it stew in -next once -rc1 ships? Thanks!
FWIW, right now I think that out of those 3 commits #1 (separating PID 1 from init_fs + making all kernel threads get umask 0) and #3 (assorted crapectomy in lustre, getting rid of odd games with fs_struct there) are OK for mainline, with #2 (removal of unshare_fs_struct()) being -next fodder, to see if we get anything like unexpected contention, etc.
Al Viro
maintainerQuoted 33 lines · Al Viro
Quoted 1 line · Jeff Layton
We'll also need Al's ACK on the
fs_structstuff.... and I'm not happy with it. First of all, ditch those
EXPORT_SYMBOL_GPL(); if it's too low-level for general export (and many of those are), tacking_GPLon it doesn't make it any better.unshare_fs_struct()misbegotten export is a historical accident - somebody (agruen, perhaps?) slapped a_GPLexport on its precursors and I had taken a lazy approach back then. Shouldn't have done that... Please, don't add more of that shit.More to the point, though, it *is* far too low-level, and for no visible reason. AFAICS, what you want to achieve is zero umask in that fucker. TBH, I really wonder if any of that is needed at all. Why do we want kernel threads to get umask shared with init(8), anyway? It's very easy to change - all it takes is
* make init_fs.umask zero
* makekernel_initcloned withoutCLONE_FSand have it immediately
set its ->fs->umask to 0022
* make____call_usermodehelper()(always called very early in the
life of a thread that had been cloned withoutCLONE_FS) do the same thing. Voila - all kernel threads have umask 0 and it's not affected by whatever init(8) might be pulling off. And that includes all worqueue workers, etc.With that I'm not sure we need to have
unshare_fs_struct()at all; at least not unless some thread wants to play withchdir()andchroot()and I don't see anything of that sort in nfsd and lustre (the only callers ofunshare_fs_struct()in the tree). Note thatset_fs_pwd()andset_fs_root()are *not* exported, and neither are sys_{chdir,fchdir,chroot}, so nfsd and lustre would have a hard time trying to do something of that sort anyway. There is open-coded crap trying to implement chdir inlustre_compat25.h, but it has no callers...
Quoted 5 lines
Linus, do you see any problems with the following patch (against the mainline)? If not, I'll put it into the next vfs.git pull request, along with removal of all mentionings of ->fs-> in lustre (aside of aforementioned dead code, there's open-coded
current_umask()in one place and broken attempt to reimplementdentry_path()in another).
Grr... With includes of <linux/fs_struct.h> added in init/main.c and kernel/kmod.c. Sorry. That way it builds and, AFAICT, works... I think it ought to be 3 commits -
* giving PID 1 fs_struct of its own, making umask for all kernel
threads zero, while keeping the same value (0022) for PID 1 and all userland processes spawned by call_usermodehelper().
* removal of unshare_fs_struct() - it becomes unneeded after the
previous commit
* removal of assorted junk in lustre.
All three combined yield this:
.../staging/lustre/include/linux/libcfs/libcfs.h | 1 - .../lustre/lustre/include/linux/lustre_compat25.h | 24 --------------------- drivers/staging/lustre/lustre/llite/dir.c | 2 +- drivers/staging/lustre/lustre/llite/llite_lib.c | 17 +-------------- drivers/staging/lustre/lustre/obdclass/genops.c | 1 - drivers/staging/lustre/lustre/obdclass/llog.c | 2 -- drivers/staging/lustre/lustre/ptlrpc/import.c | 2 -- drivers/staging/lustre/lustre/ptlrpc/pinger.c | 2 -- drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c | 1 - drivers/staging/lustre/lustre/ptlrpc/sec_gc.c | 2 -- drivers/staging/lustre/lustre/ptlrpc/service.c | 2 -- fs/fs_struct.c | 25 +--------------------- fs/nfsd/nfssvc.c | 11 ---------- include/linux/fs_struct.h | 1 - init/main.c | 4 +++- kernel/kmod.c | 2 ++ 16 files changed, 8 insertions(+), 91 deletions(-)
drivers/staging/lustre/include/linux/libcfs/libcfs.h +0 -1
@@ -136,7 +136,6 @@ void cfs_enter_debugger(void);/* * Defined by platform */ -int unshare_fs_struct (void );sigset_t cfs_get_blocked_sigs (void );sigset_t cfs_block_allsigs (void );sigset_t cfs_block_sigs (unsigned long sigs);
drivers/staging/lustre/lustre/include/linux/lustre_compat25.h +0 -24
@@ -42,28 +42,6 @@#include "lustre_patchless_compat.h" -# define LOCK_FS_STRUCT (fs) spin_lock(&(fs)->lock) -# define UNLOCK_FS_STRUCT (fs) spin_unlock(&(fs)->lock) - -static inline void ll_set_fs_pwd (struct fs_struct *fs,struct vfsmount *mnt, -struct dentry *dentry) -{ -struct path path; -struct path old_pwd; - - path.mnt = mnt; - path.dentry = dentry; -LOCK_FS_STRUCT (fs); - old_pwd = fs->pwd; -path_get (&path); - fs->pwd = path; -UNLOCK_FS_STRUCT (fs); - -if (old_pwd.dentry) -path_put (&old_pwd); -} - -/* * set ATTR_BLOCKS to a high value to avoid any risk of collision with other * ATTR_* attributes (see bug 13828)
@@ -112,8 +90,6 @@ static inline void ll_set_fs_pwd(struct fs_struct *fs, struct vfsmount *mnt,#define cfs_bio_io_error (a, b) bio_io_error((a))#define cfs_bio_endio (a, b, c) bio_endio((a), (c)) -#define cfs_fs_pwd (fs) ((fs)->pwd.dentry) -#define cfs_fs_mnt (fs) ((fs)->pwd.mnt)#define cfs_path_put (nd) path_put(&(nd)->path)
drivers/staging/lustre/lustre/llite/dir.c +1 -1
@@ -661,7 +661,7 @@ int ll_dir_setdirstripe(struct inode *dir, struct lmv_user_md *lump,int mode;int err; - mode = (0755 & (S_IRWXUGO |S_ISVTX ) & ~current->fs->umask) |S_IFDIR ; + mode = (0755 & ~current_umask ()) |S_IFDIR ; op_data =ll_prep_md_op_data (NULL , dir,NULL , filename,strlen (filename), mode,LUSTRE_OPC_MKDIR , lump);
drivers/staging/lustre/lustre/llite/llite_lib.c +1 -16
@@ -2396,21 +2396,6 @@ char *ll_get_fsname(struct super_block *sb, char *buf, int buflen)return buf; } -static char *ll_d_path (struct dentry *dentry,char *buf,int bufsize) -{ -char *path =NULL ; - -struct path p; - - p.dentry = dentry; - p.mnt = current->fs->root.mnt; -path_get (&p); - path =d_path (&p, buf, bufsize); -path_put (&p); - -return path; -} -void ll_dirty_page_discard_warn (struct page *page,int ioret) {char *buf, *path =NULL ;
@@ -2422,7 +2407,7 @@ void ll_dirty_page_discard_warn(struct page *page, int ioret)if (buf !=NULL ) { dentry =d_find_alias (page->mapping->host);if (dentry !=NULL ) - path =ll_d_path (dentry, buf,PAGE_SIZE ); + path =dentry_path_raw (dentry, buf,PAGE_SIZE ); } CDEBUG(D_WARNING,
drivers/staging/lustre/lustre/obdclass/genops.c +0 -1
@@ -1713,7 +1713,6 @@ EXPORT_SYMBOL(obd_zombie_barrier);*/ static int obd_zombie_impexp_thread (void *unused) { -unshare_fs_struct ();complete (&obd_zombie_start); obd_zombie_pid =current_pid ();
drivers/staging/lustre/lustre/obdclass/llog.c +0 -2
@@ -411,8 +411,6 @@ static int llog_process_thread_daemonize(void *arg)struct lu_env env;int rc; -unshare_fs_struct (); -/* client env has no keys, tags is just 0 */ rc =lu_env_init (&env,LCT_LOCAL |LCT_MG_THREAD );if (rc)
drivers/staging/lustre/lustre/ptlrpc/import.c +0 -2
@@ -1290,8 +1290,6 @@ static int ptlrpc_invalidate_import_thread(void *data) {struct obd_import *imp = data; -unshare_fs_struct (); -CDEBUG (D_HA ,"thread invalidate import %s to %s@%s\n" , imp->imp_obd->obd_name,obd2cli_tgt (imp->imp_obd), imp->imp_connection->c_remote_uuid.uuid);
drivers/staging/lustre/lustre/ptlrpc/pinger.c +0 -2
@@ -586,8 +586,6 @@ static int ping_evictor_main(void *arg)struct l_wait_info lwi = {0 };time_t expire_time; -unshare_fs_struct (); -CDEBUG (D_HA ,"Starting Ping Evictor\n" ); pet_state =PET_READY ;while (1 ) {
drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c +0 -1
@@ -382,7 +382,6 @@ static int ptlrpcd(void *arg)struct lu_env env = { .le_ses =NULL };int rc, exit =0 ; -unshare_fs_struct ();#if defined(CONFIG_SMP )if (test_bit (LIOD_BIND , &pc->pc_flags)) {int index = pc->pc_index;
drivers/staging/lustre/lustre/ptlrpc/sec_gc.c +0 -2
@@ -164,8 +164,6 @@ static int sec_gc_main(void *arg)struct ptlrpc_thread *thread = (struct ptlrpc_thread *) arg;struct l_wait_info lwi; -unshare_fs_struct (); -/* Record that the thread is running */ thread_set_flags (thread,SVC_RUNNING );wake_up (&thread->t_ctl_waitq);
drivers/staging/lustre/lustre/ptlrpc/service.c +0 -2
@@ -2277,7 +2277,6 @@ static int ptlrpc_main(void *arg)int counter =0 , rc =0 ; thread->t_pid =current_pid (); -unshare_fs_struct ();/* NB: we will call cfs_cpt_bind() for all threads, because we * might want to run lustre server only on a subset of system CPUs,
@@ -2478,7 +2477,6 @@ static int ptlrpc_hr_main(void *arg)snprintf (threadname,sizeof (threadname),"ptlrpc_hr%02d_%03d" , hrp->hrp_cpt, hrt->hrt_id); -unshare_fs_struct (); rc =cfs_cpt_bind (ptlrpc_hr.hr_cpt_table, hrp->hrp_cpt);if (rc !=0 ) {
fs/fs_struct.c +1 -24
@@ -128,29 +128,6 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)return fs; } -int unshare_fs_struct (void ) -{ -struct fs_struct *fs = current->fs; -struct fs_struct *new_fs =copy_fs_struct (fs); -int kill; - -if (!new_fs) -return -ENOMEM ; - -task_lock (current); -spin_lock (&fs->lock); - kill = !--fs->users; - current->fs = new_fs; -spin_unlock (&fs->lock); -task_unlock (current); - -if (kill) -free_fs_struct (fs); - -return 0 ; -} -EXPORT_SYMBOL_GPL (unshare_fs_struct); -int current_umask (void ) {return current->fs->umask;
@@ -162,5 +139,5 @@ struct fs_struct init_fs = { .users = 1, .lock = __SPIN_LOCK_UNLOCKED(init_fs.lock), .seq = SEQCNT_ZERO(init_fs.seq), - .umask = 0022, + .umask = 0, };
fs/nfsd/nfssvc.c +0 -11
@@ -573,16 +573,6 @@ nfsd(void *vrqstp)/* Lock module and set up kernel thread */ mutex_lock (&nfsd_mutex); -/* At this point, the thread shares current->fs -* with the init process. We need to create files with a -* umask of 0 instead of init's umask. */ -if (unshare_fs_struct () <0 ) { -printk ("Unable to start nfsd thread: out of memory\n" ); - goto out; - } - - current->fs->umask =0 ; -/* * thread is spawned with all signals set to SIG_IGN, re-enable * the ones that will bring down the thread
@@ -623,7 +613,6 @@ nfsd(void *vrqstp)mutex_lock (&nfsd_mutex); nfsdstats.th_cnt --; -out: rqstp->rq_server =NULL ;/* Release the thread */
include/linux/fs_struct.h +0 -1
@@ -21,7 +21,6 @@ extern void set_fs_root(struct fs_struct *, const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *); -extern int unshare_fs_struct (void );static inline void get_fs_root (struct fs_struct *fs,struct path *root) {
init/main.c +3 -1
@@ -77,6 +77,7 @@#include <linux/context_tracking.h> #include <linux/random.h> #include <linux/list.h> +#include <linux/fs_struct.h> #include <asm/io.h> #include <asm/bugs.h>
@@ -399,7 +400,7 @@ static noinline void __init_refok rest_init(void)* the init task will end up wanting to create kthreads, which, if * we schedule it before we create kthreadd, will OOPS. */ -kernel_thread (kernel_init,NULL ,CLONE_FS ); +kernel_thread (kernel_init,NULL ,0 );numa_default_policy (); pid =kernel_thread (kthreadd,NULL ,CLONE_FS |CLONE_FILES );rcu_read_lock ();
@@ -924,6 +925,7 @@ static int __ref kernel_init(void *unused) {int ret; + current->fs->umask =0022 ;kernel_init_freeable ();/* need to finish all async __init code before freeing the memory */ async_synchronize_full ();
kernel/kmod.c +2 -0
@@ -39,6 +39,7 @@#include <linux/rwsem.h> #include <linux/ptrace.h> #include <linux/async.h> +#include <linux/fs_struct.h> #include <asm/uaccess.h> #include <trace/events/module.h>
@@ -219,6 +220,7 @@ static int ____call_usermodehelper(void *data)struct cred *new;int retval; + current->fs->umask =0022 ;spin_lock_irq (¤t->sighand->siglock);flush_signal_handlers (current,1 );spin_unlock_irq (¤t->sighand->siglock);