On Tue 01 Nov 2011 10:17:46 AM EDT, J. Bruce Fields wrote: > On Tue, Nov 01, 2011 at 10:08:37AM -0400, Bryan Schumaker wrote: >> On Fri 28 Oct 2011 05:24:50 PM EDT, J. Bruce Fields wrote: >>> On Mon, Oct 24, 2011 at 07:20:57AM -0400, bjschuma@xxxxxxxxxx wrote: >>> ... >>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >>>> index db34a58..e67f30c 100644 >>>> --- a/fs/nfsd/nfsctl.c >>>> +++ b/fs/nfsd/nfsctl.c >>> ... >>>> @@ -1130,6 +1131,9 @@ static int __init init_nfsd(void) >>>> retval = nfs4_state_init(); /* nfs4 locking state */ >>>> if (retval) >>>> return retval; >>>> + retval = nfsd_fault_inject_init(); /* nfsd fault injection controls */ >>>> + if (retval) >>>> + goto out_cleanup_fault_injection; >>>> nfsd_stat_init(); /* Statistics */ >>>> retval = nfsd_reply_cache_init(); >>>> if (retval) >>>> @@ -1161,6 +1165,8 @@ out_free_cache: >>>> out_free_stat: >>>> nfsd_stat_shutdown(); >>>> nfsd4_free_slabs(); >>>> +out_cleanup_fault_injection: >>>> + nfsd_fault_inject_cleanup(); >>> >>> Check the cleanup here again.... The slabs are allocated in state_init, >>> so you need to do that on nfsd_fault_inject_init, but you *don't* need >>> the fault_inject_cleanup in that case, since fault_inject_init cleanup >>> cleans up after itself--that's needed only on later failures. >> >> Would it be better to change the fault_inject_init() function so it >> doesn't clean up after itself? This would keep it consistent with the >> other nfsd init functions. > > I don't think so. The pattern here is pretty common throughout the > kernel; it's approximately: > > do_thing1(); > err = do_thing2(); > if (err) > goto undo_thing1; > err = do_thing3(); > if (err) > goto undo_thing2; > ... > > Functions are no-ops on failure, and the caller's just responsible for > cleaning up previous stuff. Ok, I won't change that. > >> I didn't realize that slabs were initialized in the state_init() >> function, I'll fix that. > > I'll admit that's confusing. Any suggestion to make it clearer welcome. Sure. The comment after the call to nfs4_state_init() is "nfs4 locking state". Maybe this could be changed to "nfs4 locking state and slabs"? I can make this change and send it as a separate patch if you would like. > > --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 -- 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
