[PATCH] staging: lustre: check result of register_shrinker

ak akaraliou.dev at gmail.com
Sun Dec 3 16:59:07 UTC 2017


On 12/03/2017 03:47 AM, Dilger, Andreas wrote:

> On Dec 2, 2017, at 11:40, Aliaksei Karaliou <akaraliou.dev at 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 at 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
>
>
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.



More information about the devel mailing list