[lustre-devel] [PATCH] staging: lustre: check result of register_shrinker
andreas.dilger at intel.com
Mon Dec 4 16:50:25 PST 2017
On Dec 4, 2017, at 11:42, Aliaksei Karaliou <akaraliou.dev at 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.
>> 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?
Lustre Principal Architect
More information about the lustre-devel