On Fri, 6 Jun 2008 14:16:12 -0400 "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: > On Fri, Jun 06, 2008 at 02:11:16PM -0400, Jeff Layton wrote: > > I think I've goofed this part, actually. I was thinking that we didn't > > need to bump the refcount here, and that the kernel would realize that > > nfsd() hadn't returned and would prevent unloading until it had. This > > doesn't seem to be the case. I'll need to go back and add refcounting > > back in. > > OK. If you decide it is needed here, could you double-check the lockd > conversion as well? Looks like some refcounting logic might have gotten > lost there too. > > --b. Full disclosure: I don't completely understand module refcounts and when we need to take a reference. So feel free to set me straight if my comments below are wrong :-) The change to lockd was deliberate and was suggested by Neil Brown, when I was working on an earlier version of the lockd-kthread patch: --------------[snip]------------------ > - module_put_and_exit(0); > + module_put(THIS_MODULE); > + return 0; This changes bothers me. Putting the last ref to a module in code inside that module is not safe, which is why module_put_and_exit exists. So this module_put is either unsafe or not needed. I think the latter. As you say in the comment, lockd_down now blocks until lockd actually exits. As every caller for lockd_down will own a reference to the lockd module, the lockd thread no longer needs to own a reference too. So I think it is safe to remove the module_put, and also remove the __module_get at the top of the lockd function. --------------[snip]------------------ So I followed his advice and everything seems to be OK. I don't see a way to yank out the lockd module while lockd is actually up, since the callers of lockd_up() have to have a reference to the lockd module, and if those modules go away, then lockd should be down anyway. This is what led me to think that we didn't need this for nfsd either, but that seems to be incorrect. I think nfsd is different because it's started directly from userspace. We don't have any persistent module references so we need to take them explicitly. -- 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
