On Mon, 19 May 2008 16:07:12 +1000 Neil Brown <neilb@xxxxxxx> wrote: > On Saturday May 17, jlayton@xxxxxxxxxx wrote: > > knfsd is the last NFS-related kernel thread that does not use the > > kthread API. This patchset represents a first pass at converting it. It > > seems to work, but changes the shutdown interface. knfsd currently > > allows signals to tell it when to come down. > > > > My main question is...how tied to this shutdown method are we? We can > > also take down nfsd by having people run: > > > > # rpc.nfsd 0 > > > > ...which basically does: > > > > # echo 0 > /proc/fs/nfsd/threads > > > > ...so we don't think we *have* to use signals here. Is signaling > > something we can reasonably eliminate? > > Good question. I suspect lots of distros still use "killall" or some > equivalent to stop nfsd - so at a minimum, some education would be > required. > > Most kernel threads are there to provide a service for some other > entity, such as a filesystem or a device etc. So it doesn't make > sense to kill them except when the device/filesystem/whatever goes > away. > > With nfsd, the thread *is* the central entity. So killing it does > make sense. > This doesn't argue strongly in favour of a killable nfsd, but does > explain why it might be different from all other kernel threads. > > > In addition to making the code a > > bit simpler and cleaner, I think it will also eliminate this race: > > > > http://lkml.org/lkml/2007/8/2/462 > > I never followed up on this did I... > The core problem seems to be the principle of > "The last one out turns off the lights" > but once you've turned off the lights, you can't see if someone else > snuck back in so you aren't the last one. You really have to have > only one door and stand in the doorway while switching off the > lights.... > > If we replace the BKL usage with a simple global semaphore, that > problem might just go away. We should only need to protect > svc_destroy, svc_exit_thread, and svc_set_num_threads from each other. > > It's long past time to discard the BLK here anyway. > > > > > If this isn't feasible, then I can add the signaling back in, but am not > > sure whether we can eliminate the race without adding more locking. > > I think I prefer keeping the signalling and fixing the locking. > > > > > If we can do this, we may need to provide an alternate way to specify > > that we want to take down all nfsd's but not flush the export table. > > Currently that's done with a SIGHUP, but the value of this facility is > > not clear to me since the kernel can just do another upcall. > > I added this functionality well before the kernel could do upcalls to > refill the export table. > > I wanted to be able to change the number of active threads without a > complete shutdown and restart, so I make the /proc/fs/nfs/threads > file accessed by "nfsd NN". > > Then I wanted > nfsd 0 > nfsd 8 > to just change the number of threads, not flush the exports table as > well (because at the time, flushing the exports table was more > serious). > So I arranged that "nfsd 0" just reduced the number of threads to 0 > and changed no other (externally visible) state. > > As you say, it isn't really needed now. I wouldn't have a problem > with removing the SIGHUP feature, but I think keeping SIGKILL and > SIGINT with their old meaning is important. > Ok, I'll plan to take a second crack at this when I get some time and put the signaling back in. I think removing the SIGHUP feature will make the code a little simpler, so I'll plan to take it out. I'll also have a look at putting better locking around the svc_* functions you mention above... Thanks for the review, -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
