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
