[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dilger, Andreas]

On Dec 2, 2017, at 11:40, Aliaksei Karaliou <akaraliou.dev@gmail.com> wrote:
> 
> Lustre code lacks checking the result of register_shrinker()
> in several places. register_shrinker() was tagged __must_check
> recently so that sparse has started reporting it.

Thank you for your patch.  Some comments below.

> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> ---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     |  7 +++++--
> drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
> drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
> drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    | 13 ++++++++-----
> 4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> index da65d00a7811..7795ececa6d3 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> @@ -1086,8 +1086,11 @@ int ldlm_pools_init(void)
> 	int rc;
> 
> 	rc = ldlm_pools_thread_start();
> -	if (rc == 0)
> -		register_shrinker(&ldlm_pools_cli_shrinker);
> +	if (rc == 0) {
> +		rc = register_shrinker(&ldlm_pools_cli_shrinker);
> +		if (rc)
> +			ldlm_pools_thread_stop();
> +	}

Rather than nesting conditionals like this, kernel style prefers to use
"goto label" to do the cleanup in one place at the end of the function.
That keeps the indentation level reasonable, and avoids making the error
handling increasingly complex if more conditions are added in the future.
The preferred way to handle this would be:

	rc = ldlm_pools_thread_start();
	if (rc)
		goto out;

	rc = register_shrinker(&ldlm_pools_cli_shrinker);
	if (rc)
		goto out_pools;

	return 0;

out_pools:
	ldlm_pools_thread_stop();
out:
	goto out;
}

Then, if a new error condition is added, it just means an "out_shrinker:"
label is added and calling unregister_shrinker() at that point.

> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> index b938a3f9d50a..9e0256ca2079 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> @@ -1951,7 +1951,7 @@ int lu_global_init(void)
> 	 * inode, one for ea. Unfortunately setting this high value results in
> 	 * lu_object/inode cache consuming all the memory.
> 	 */
> -	register_shrinker(&lu_site_shrinker);
> +	result = register_shrinker(&lu_site_shrinker);
> 
> 	return result;

> }
> @@ -1961,7 +1961,8 @@ int lu_global_init(void)
>  */
> void lu_global_fini(void)
> {
> -	unregister_shrinker(&lu_site_shrinker);
> +	if (lu_site_shrinker.nr_deferred)
> +		unregister_shrinker(&lu_site_shrinker);
> 	lu_context_key_degister(&lu_global_key);

Initially, I didn't think this was needed, but it seems that
unregister_shrinker() is not safe to be called on a shrinker that was
not initialized properly.

While the above check makes this callsite safe, and I'm OK with landing
this part of the patch, it might be safer for the kernel as a whole if
unregister_shrinker() was made safe against this internally?  Something like:

 void unregister_shrinker(struct shrinker *shrinker)
 {
+	if (!shrinker->nr_deferred)
+		return;
+
        down_write(&shrinker_rwsem);
        list_del(&shrinker->list);
        up_write(&shrinker_rwsem);
        kfree(shrinker->nr_deferred);
+	shrinker->nr_deferred = NULL;
 }

That avoids the need for the caller to "know" about nr_deferred.

> 
> 	/*
> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
> index 53eda4c99142..45b1ebf33363 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
> @@ -2844,7 +2844,9 @@ static int __init osc_init(void)
> 	if (rc)
> 		goto out_kmem;
> 
> -	register_shrinker(&osc_cache_shrinker);
> +	rc = register_shrinker(&osc_cache_shrinker);
> +	if (rc)
> +		goto out_type;
> 
> 	/* This is obviously too much memory, only prevent overflow here */
> 	if (osc_reqpool_mem_max >= 1 << 12 || osc_reqpool_mem_max == 0) {
> diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> index 77a3721beaee..4eeff832fd4a 100644
> --- a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> +++ b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> @@ -396,6 +396,8 @@ static struct shrinker pools_shrinker = {
> 
> int sptlrpc_enc_pool_init(void)
> {
> +	int rc = -ENOMEM;
> +
> 	/*
> 	 * maximum capacity is 1/8 of total physical memory.
> 	 * is the 1/8 a good number?
> @@ -429,12 +431,13 @@ int sptlrpc_enc_pool_init(void)
> 	page_pools.epp_st_outofmem = 0;
> 
> 	enc_pools_alloc();
> -	if (!page_pools.epp_pools)
> -		return -ENOMEM;
> -
> -	register_shrinker(&pools_shrinker);
> +	if (page_pools.epp_pools) {
> +		rc = register_shrinker(&pools_shrinker);
> +		if (rc)
> +			enc_pools_free();
> +	}

Same comment here as above - please use labels to handle error cleanup.

> -	return 0;
> +	return rc;
> }
> 
> void sptlrpc_enc_pool_fini(void)
> -- 
> 2.11.0
> 

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dan Carpenter]

On Sun, Dec 03, 2017 at 12:47:03AM +0000, Dilger, Andreas wrote:
> The preferred way to handle this would be:
> 
> 	rc = ldlm_pools_thread_start();
> 	if (rc)
> 		goto out;
> 
> 	rc = register_shrinker(&ldlm_pools_cli_shrinker);
> 	if (rc)
> 		goto out_pools;
> 
> 	return 0;
> 
> out_pools:
> 	ldlm_pools_thread_stop();
> out:
> 	goto out;
> }

I so badly hate "out" as a label name...  But yours made me laugh.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dan Carpenter]

On Sun, Dec 03, 2017 at 07:59:07PM +0300, ak wrote:
> Thank you for your extensive comments.
> 
> I've also thought about adding more protection into unregister_shrinker(),
> but not sure how to properly organize the patch set, because there will be
> three patches:
>     * change in mm/vmscan that adds protection and sanitizer.
>     * fixed change for Lustre driver
>     * there also two explicit usages of shrinker->nr_deferred in drivers -
>        good idea to fix too.
> All patches have different lists of maintainers, and second and third depend
> on first one. And I don't like to send them separately.
> So, I'm going to at least prepend this patch with mm/vmscan one.

Fix the style for the Lustre patch and resend.  Then patch
unregister_shrinker().  Then remove the checks.

The unregister_shrinker() changes seem like a good idea, but I haven't
really looked at it.  It might be more involved than it seems.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dan Carpenter]

On Mon, Dec 04, 2017 at 09:42:12PM +0300, Aliaksei Karaliou wrote:
> On 12/04/2017 11:40 AM, Dan Carpenter wrote:
> > On Sun, Dec 03, 2017 at 07:59:07PM +0300, ak wrote:
> > > Thank you for your extensive comments.
> > > 
> > > I've also thought about adding more protection into unregister_shrinker(),
> > > but not sure how to properly organize the patch set, because there will be
> > > three patches:
> > >      * change in mm/vmscan that adds protection and sanitizer.
> > >      * fixed change for Lustre driver
> > >      * there also two explicit usages of shrinker->nr_deferred in drivers -
> > >         good idea to fix too.
> > > All patches have different lists of maintainers, and second and third depend
> > > on first one. And I don't like to send them separately.
> > > So, I'm going to at least prepend this patch with mm/vmscan one.
> > Fix the style for the Lustre patch and resend.  Then patch
> > unregister_shrinker().  Then remove the checks.
> > 
> > The unregister_shrinker() changes seem like a good idea, but I haven't
> > really looked at it.  It might be more involved than it seems.
> > 
> > regards,
> > dan carpenter
> Thanks for the comments too.
> I'll send patch with accumulated fixes.
> > >   }
> > > diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > index b938a3f9d50a..9e0256ca2079 100644
> > > --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > @@ -1951,7 +1951,7 @@ int lu_global_init(void)
> > >   	 * inode, one for ea. Unfortunately setting this high value results in
> > >   	 * lu_object/inode cache consuming all the memory.
> > >   	 */
> > > -	register_shrinker(&lu_site_shrinker);
> > > +	result = register_shrinker(&lu_site_shrinker);
> > There should be some error handling if the register fails.
> Yeah, I think so, but it seems that it is out of scope of this patch.
> The whole negative branch in the mainline kernel looks broken (IMHO).

No, it's not.  If you introduce new error paths, you're expected to make
them clean up instead of leaking resources.

regards,
dan carpenter

[lustre-devel] [PATCH v2 1/2] staging: lustre: check result of register_shrinker

[Greg KH]

On Mon, Dec 04, 2017 at 10:21:56PM +0300, Aliaksei Karaliou wrote:
> Lustre code lacks checking the result of register_shrinker()
> in several places. register_shrinker() was tagged __must_check
> recently so that sparse has started reporting it.
> 
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> ---
>  drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     | 12 +++++++++---
>  drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
>  drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
>  drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    |  8 +++++++-
>  4 files changed, 22 insertions(+), 7 deletions(-)
> 
> v2: Style fixes, as suggested by Cheers, Andreas and Dan Carpenter.
>     Added one more patch to address resource cleanup, suggested by Dan Carpenter.
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> index da65d00a7811..9fef2d52d6c2 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> @@ -1086,10 +1086,16 @@ int ldlm_pools_init(void)
>  	int rc;
>  
>  	rc = ldlm_pools_thread_start();
> -	if (rc == 0)
> -		register_shrinker(&ldlm_pools_cli_shrinker);
> +	if (rc)
> +		return rc;
>  
> -	return rc;
> +	rc = register_shrinker(&ldlm_pools_cli_shrinker);
> +	if (rc) {
> +		ldlm_pools_thread_stop();
> +		return rc;
> +	}
> +
> +	return 0;
>  }
>  
>  void ldlm_pools_fini(void)
> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> index b938a3f9d50a..9e0256ca2079 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> @@ -1951,7 +1951,7 @@ int lu_global_init(void)
>  	 * inode, one for ea. Unfortunately setting this high value results in
>  	 * lu_object/inode cache consuming all the memory.
>  	 */
> -	register_shrinker(&lu_site_shrinker);
> +	result = register_shrinker(&lu_site_shrinker);
>  
>  	return result;

	return register_shrinker()?

> -	register_shrinker(&pools_shrinker);
> +	rc = register_shrinker(&pools_shrinker);
> +	if (rc) {
> +		enc_pools_free();
> +		return rc;

Drop the return rc, and then just do:

> +	}
>  
>  	return 0;

	return rc;
there.  Makes the patch smaller.

thanks,

greg k-h

[lustre-devel] [PATCH v2 1/2] staging: lustre: check result of register_shrinker

[Greg KH]

On Wed, Dec 06, 2017 at 08:40:43PM +0300, Aliaksei Karaliou wrote:
> On 12/06/2017 11:51 AM, Greg KH wrote:
> 
> > On Mon, Dec 04, 2017 at 10:21:56PM +0300, Aliaksei Karaliou wrote:
> > > Lustre code lacks checking the result of register_shrinker()
> > > in several places. register_shrinker() was tagged __must_check
> > > recently so that sparse has started reporting it.
> > > 
> > > Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> > > ---
> > >   drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     | 12 +++++++++---
> > >   drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
> > >   drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
> > >   drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    |  8 +++++++-
> > >   4 files changed, 22 insertions(+), 7 deletions(-)
> > > 
> > > v2: Style fixes, as suggested by Cheers, Andreas and Dan Carpenter.
> > >      Added one more patch to address resource cleanup, suggested by Dan Carpenter.
> > > 
> > > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> > > index da65d00a7811..9fef2d52d6c2 100644
> > > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> > > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> > > @@ -1086,10 +1086,16 @@ int ldlm_pools_init(void)
> > >   	int rc;
> > >   	rc = ldlm_pools_thread_start();
> > > -	if (rc == 0)
> > > -		register_shrinker(&ldlm_pools_cli_shrinker);
> > > +	if (rc)
> > > +		return rc;
> > > -	return rc;
> > > +	rc = register_shrinker(&ldlm_pools_cli_shrinker);
> > > +	if (rc) {
> > > +		ldlm_pools_thread_stop();
> > > +		return rc;
> > > +	}
> > > +
> > > +	return 0;
> > >   }
> > >   void ldlm_pools_fini(void)
> > > diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > index b938a3f9d50a..9e0256ca2079 100644
> > > --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> > > @@ -1951,7 +1951,7 @@ int lu_global_init(void)
> > >   	 * inode, one for ea. Unfortunately setting this high value results in
> > >   	 * lu_object/inode cache consuming all the memory.
> > >   	 */
> > > -	register_shrinker(&lu_site_shrinker);
> > > +	result = register_shrinker(&lu_site_shrinker);
> > >   	return result;
> > 	return register_shrinker()?
> Yes, It looks easier, thank you for your comments.
> But still what to do with the fact that this changed place actually leads to
> resources being not freed (what I actually fix in second patch) ?
> Should I just merge both commits together ?

Don't write a patch that you know is broken, and then fix it up in a
follow-on patch.  That's not good at all.

Just fix one call-site at a time, that should be easier, right?

greg k-h

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dilger, Andreas]

On Dec 4, 2017, at 11:42, Aliaksei Karaliou <akaraliou.dev@gmail.com> wrote:
> 
> On 12/04/2017 11:40 AM, Dan Carpenter wrote:
>> On Sun, Dec 03, 2017 at 07:59:07PM +0300, ak wrote:
>>> Thank you for your extensive comments.
>>> 
>>> I've also thought about adding more protection into unregister_shrinker(),
>>> but not sure how to properly organize the patch set, because there will be
>>> three patches:
>>>     * change in mm/vmscan that adds protection and sanitizer.
>>>     * fixed change for Lustre driver
>>>     * there also two explicit usages of shrinker->nr_deferred in drivers -
>>>        good idea to fix too.
>>> All patches have different lists of maintainers, and second and third depend
>>> on first one. And I don't like to send them separately.
>>> So, I'm going to at least prepend this patch with mm/vmscan one.
>> Fix the style for the Lustre patch and resend.  Then patch
>> unregister_shrinker().  Then remove the checks.
>> 
>> The unregister_shrinker() changes seem like a good idea, but I haven't
>> really looked at it.  It might be more involved than it seems.
>> 
>> regards,
>> dan carpenter
> Thanks for the comments too.
> I'll send patch with accumulated fixes.
>>>  }
>>> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
>>> index b938a3f9d50a..9e0256ca2079 100644
>>> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
>>> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
>>> @@ -1951,7 +1951,7 @@ int lu_global_init(void)
>>>  	 * inode, one for ea. Unfortunately setting this high value results in
>>>  	 * lu_object/inode cache consuming all the memory.
>>>  	 */
>>> -	register_shrinker(&lu_site_shrinker);
>>> +	result = register_shrinker(&lu_site_shrinker);
>> There should be some error handling if the register fails. 
> Yeah, I think so, but it seems that it is out of scope of this patch.
> The whole negative branch in the mainline kernel looks broken (IMHO).
> In mainline Lustre's git there is a reworked version of upper function obdclass_init(),
> which at least calls lu_global_fini() before exiting `module_init` on further failure,
> but yeah, still lacks proper cleanup inside lu_global_initcall().
> I'll add one more patch in a patch-set so that maintainers may decide what to do with that.

I was looking at the lack of error handling as well, but then I wondered if the module_init() call returns an error, is module_exit() still called, or does the module not load at all in the error case?

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dan Carpenter]

On Tue, Dec 05, 2017 at 12:50:25AM +0000, Dilger, Andreas wrote:
> On Dec 4, 2017, at 11:42, Aliaksei Karaliou <akaraliou.dev@gmail.com> wrote:
> > 
> > On 12/04/2017 11:40 AM, Dan Carpenter wrote:
> >> On Sun, Dec 03, 2017 at 07:59:07PM +0300, ak wrote:
> >>> Thank you for your extensive comments.
> >>> 
> >>> I've also thought about adding more protection into unregister_shrinker(),
> >>> but not sure how to properly organize the patch set, because there will be
> >>> three patches:
> >>>     * change in mm/vmscan that adds protection and sanitizer.
> >>>     * fixed change for Lustre driver
> >>>     * there also two explicit usages of shrinker->nr_deferred in drivers -
> >>>        good idea to fix too.
> >>> All patches have different lists of maintainers, and second and third depend
> >>> on first one. And I don't like to send them separately.
> >>> So, I'm going to at least prepend this patch with mm/vmscan one.
> >> Fix the style for the Lustre patch and resend.  Then patch
> >> unregister_shrinker().  Then remove the checks.
> >> 
> >> The unregister_shrinker() changes seem like a good idea, but I haven't
> >> really looked at it.  It might be more involved than it seems.
> >> 
> >> regards,
> >> dan carpenter
> > Thanks for the comments too.
> > I'll send patch with accumulated fixes.
> >>>  }
> >>> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> >>> index b938a3f9d50a..9e0256ca2079 100644
> >>> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> >>> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> >>> @@ -1951,7 +1951,7 @@ int lu_global_init(void)
> >>>  	 * inode, one for ea. Unfortunately setting this high value results in
> >>>  	 * lu_object/inode cache consuming all the memory.
> >>>  	 */
> >>> -	register_shrinker(&lu_site_shrinker);
> >>> +	result = register_shrinker(&lu_site_shrinker);
> >> There should be some error handling if the register fails. 
> > Yeah, I think so, but it seems that it is out of scope of this patch.
> > The whole negative branch in the mainline kernel looks broken (IMHO).
> > In mainline Lustre's git there is a reworked version of upper function obdclass_init(),
> > which at least calls lu_global_fini() before exiting `module_init` on further failure,
> > but yeah, still lacks proper cleanup inside lu_global_initcall().
> > I'll add one more patch in a patch-set so that maintainers may decide what to do with that.
> 
> I was looking at the lack of error handling as well, but then I wondered if the module_init() call returns an error, is module_exit() still called, or does the module not load at all in the error case?
>

module_init() is supposed to clean up after itself.  module_exit() is
only called if init succeeds.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker

[Dan Carpenter]

On Sat, Dec 02, 2017 at 09:40:46PM +0300, Aliaksei Karaliou wrote:
> Lustre code lacks checking the result of register_shrinker()
> in several places. register_shrinker() was tagged __must_check
> recently so that sparse has started reporting it.
> 
> Signed-off-by: Aliaksei Karaliou <akaraliou.dev@gmail.com>
> ---
>  drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     |  7 +++++--
>  drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
>  drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
>  drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    | 13 ++++++++-----
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> index da65d00a7811..7795ececa6d3 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> @@ -1086,8 +1086,11 @@ int ldlm_pools_init(void)
>  	int rc;
>  
>  	rc = ldlm_pools_thread_start();
> -	if (rc == 0)
> -		register_shrinker(&ldlm_pools_cli_shrinker);
> +	if (rc == 0) {
> +		rc = register_shrinker(&ldlm_pools_cli_shrinker);
> +		if (rc)
> +			ldlm_pools_thread_stop();
> +	}
>  
>  	return rc;


Originally, the code had two anti-patterns 1) Making the last check in
the function different from the others, 2)  Success handling instead
of failure handling.  Success handling leads to spaghetti code.  Write
it like this instead.

	rc = ldlm_pools_thread_start();
	if (rc)
		return rc;

	rc = register_shrinker(&ldlm_pools_cli_shrinker);
	if (rc) {
		ldlm_pools_thread_stop();
		return rc;
	}

	return 0;


>  }
> diff --git a/drivers/staging/lustre/lustre/obdclass/lu_object.c b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> index b938a3f9d50a..9e0256ca2079 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c
> @@ -1951,7 +1951,7 @@ int lu_global_init(void)
>  	 * inode, one for ea. Unfortunately setting this high value results in
>  	 * lu_object/inode cache consuming all the memory.
>  	 */
> -	register_shrinker(&lu_site_shrinker);
> +	result = register_shrinker(&lu_site_shrinker);

There should be some error handling if the register fails.

>  
>  	return result;
>  }
> @@ -1961,7 +1961,8 @@ int lu_global_init(void)
>   */
>  void lu_global_fini(void)
>  {
> -	unregister_shrinker(&lu_site_shrinker);
> +	if (lu_site_shrinker.nr_deferred)
> +		unregister_shrinker(&lu_site_shrinker);
>  	lu_context_key_degister(&lu_global_key);
>  
>  	/*
> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
> index 53eda4c99142..45b1ebf33363 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
> @@ -2844,7 +2844,9 @@ static int __init osc_init(void)
>  	if (rc)
>  		goto out_kmem;
>  
> -	register_shrinker(&osc_cache_shrinker);
> +	rc = register_shrinker(&osc_cache_shrinker);
> +	if (rc)
> +		goto out_type;
>  
>  	/* This is obviously too much memory, only prevent overflow here */
>  	if (osc_reqpool_mem_max >= 1 << 12 || osc_reqpool_mem_max == 0) {
> diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> index 77a3721beaee..4eeff832fd4a 100644
> --- a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> +++ b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
> @@ -396,6 +396,8 @@ static struct shrinker pools_shrinker = {
>  
>  int sptlrpc_enc_pool_init(void)
>  {
> +	int rc = -ENOMEM;
> +
>  	/*
>  	 * maximum capacity is 1/8 of total physical memory.
>  	 * is the 1/8 a good number?
> @@ -429,12 +431,13 @@ int sptlrpc_enc_pool_init(void)
>  	page_pools.epp_st_outofmem = 0;
>  
>  	enc_pools_alloc();
> -	if (!page_pools.epp_pools)
> -		return -ENOMEM;
> -
> -	register_shrinker(&pools_shrinker);
> +	if (page_pools.epp_pools) {
> +		rc = register_shrinker(&pools_shrinker);
> +		if (rc)
> +			enc_pools_free();
> +	}
>  
> -	return 0;
> +	return rc;

This new code is *really* hard to read.  The original was more readable.
Do it like this:

	if (!page_pools.epp_pools)
		return -ENOMEM;

	rc = register_shrinker(&pools_shrinker);
	if (rc) {
		enc_pools_free();
		return rc;
	}

	return 0;


regards,
dan carpenter