Re: [PATCH 1/3] NFSD: Added fault injection

["J. Bruce Fields" <bfields@fieldses.org>]

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@netapp.com 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.

> 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.

--b.