Neil Brown wrote:
>
> Fair question.
> The new nfsd_mutex primarily protects "nfsd_serv", both the pointer
> itself and various members of the structure that it sometimes points
> to.
> In particular, ->sv_nrthreads but also to some extent sv_temp_socks
> and sv_permsocks.
>
Those two fields should be guarded by svc_serv->sv_lock only. In fact
IIRC the only field of svc_serv guarded by the global mutex is
sv_nrthreads in it's role as pseudo-refcount.
> Having said all that, I think I see a race.
> When a new rqst is created at put on sp_all_threads, ->rq_task it not
> set and doesn't get set until the thread runs and gets the mutex. So
> there is a brief hole when ->rq_task isn't set. I don't know if that
> can cause a problem, but it feels wrong.
> When you switch to kthreads, you use the fact that kthread_create
> returns a task_struct, and assign that to ->rq_task in
> __svc_create_thread instead of nfsd, which will close that hole.
>
Yes, that aspect of Jeff's patch is a definite improvement.
> If you look in nfsctl.c, you will probably be able to find plenty of
> places where nfsd_serv is dereferenced without any locking. These are
> all wrong and need fixing.
>
Agreed!
--
Greg Banks, P.Engineer, SGI Australian Software Group.
The cake is *not* a lie.
I don't speak for SGI.