On Fri, Jun 06, 2008 at 03:05:37PM -0400, Jeff Layton wrote: > 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. Yes, thanks for the reminder--that makes sense. > 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. Right. --b. -- 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
