Re: [PATCH] staging: lustre: check result of register_shrinker

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Dec 4, 2017, at 11:42, Aliaksei Karaliou <akaraliou.dev@xxxxxxxxx> 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







_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux