| Message ID | 155053494490.24125.1514109985903155907.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More lustre patches from obdclass | expand |
y > llite_kobj does not benefit from being in the > lustre_kset kset (though it does need lustre_kset > as a parent) Nak. Yes llite does benefit from being in the kset. On large clusters you can end up with 1000s of clients so keeping the sysfs files setting in sync needs to be done in mass. The way Lustre does this is by running on the MGS server 'lctl conf_param $FSNAME.llite.lazystats=1' or with the newer 'lctl set_param -P llite.$FSNAME-*.lazystats=1' as an example. When this is done on the MGS that new value is first persistently stored and then transmitted to the target nodes. The magic behind this is with class_process_config(). In the code path LCFG_PARAM, which handles the conf_param case' the main function is class_modify_config() which is used by both llite and the obd devices. This function first attempts to find the sysfs attribute from the kobject and call lustre_attr_store() directly to set it. If it can't find the attribute, which means its a debugfs file most likely, then a uevent is created and sent off. The udev rule then exexcutes some application to handle the debugfs settings. For uevents to work the kobject (llite) must belong to a kset. With LCFG_SET_PARAM, which handles the 'set_param -P' case the master function is process_param2_config(). For this case we always send attribute changes with uevents. Besides the requirement of the kobject belonging to the kset this function uses kset_find_object() directly which if llite is not in the kset will never be found. > It also does not need the class_ktype type, as dynamic_kobj_ktype > is sufficient. > > So use the simple kobject_create_and_add() to initialize it. > > This provides flexibility for making changes to > class_setup_tunables(). > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > drivers/staging/lustre/lustre/llite/lproc_llite.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c > index 8215296dc15d..78ec0fa65c58 100644 > --- a/drivers/staging/lustre/lustre/llite/lproc_llite.c > +++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c > @@ -46,7 +46,7 @@ int llite_tunables_register(void) > { > int rc = 0; > > - llite_kobj = class_setup_tunables("llite"); > + llite_kobj = kobject_create_and_add("llite", &lustre_kset->kobj); > if (IS_ERR(llite_kobj)) > return PTR_ERR(llite_kobj); > llite_kobj->kset = kset_get(&lustre_kset); > In llite_tunables_unregister() the call for kobject_put() should really be replaced with kobject_del(). This is a bug in my original work.
For some reason this patch didn't land in my mailbox but I can see it on https://patchwork.kernel.org/patch/10819037. This patch is mostly good since llite now uses dynamic_kobj_ktype with its own ktype. Thus class_sysfs_release() will never be called with llite. What does need fixing is -------------------------------------------------------- type->typ_kobj.kset = lustre_kset; changed to: type->typ_kobj.kset = kset_get(&lustre_kset); -------------------------------------------------------- Next change needed it change all the kobject_put(&type->typ_kobj); -> kobject_del(&type->typ_kobj); So we properly handle the kset referencing. That is currently broken by me :-(
On Sun, Feb 24 2019, James Simmons wrote: > y >> llite_kobj does not benefit from being in the >> lustre_kset kset (though it does need lustre_kset >> as a parent) > > Nak. > > Yes llite does benefit from being in the kset. On large clusters you > can end up with 1000s of clients so keeping the sysfs files setting > in sync needs to be done in mass. The way Lustre does this is by > running on the MGS server 'lctl conf_param $FSNAME.llite.lazystats=1' > or with the newer 'lctl set_param -P llite.$FSNAME-*.lazystats=1' as > an example. When this is done on the MGS that new value is first > persistently stored and then transmitted to the target nodes. > > The magic behind this is with class_process_config(). In the code > path LCFG_PARAM, which handles the conf_param case' the main function > is class_modify_config() which is used by both llite and the obd > devices. This function first attempts to find the sysfs attribute > from the kobject and call lustre_attr_store() directly to set it. > If it can't find the attribute, which means its a debugfs file > most likely, then a uevent is created and sent off. The udev rule > then exexcutes some application to handle the debugfs settings. > For uevents to work the kobject (llite) must belong to a kset. > > With LCFG_SET_PARAM, which handles the 'set_param -P' case the > master function is process_param2_config(). For this case we > always send attribute changes with uevents. Besides the requirement > of the kobject belonging to the kset this function uses > kset_find_object() directly which if llite is not in the kset > will never be found. Thanks a lot for the explanation - there are definitely things I missed there. I'll drop this patch, and keep class_setup_tunables() in the later patch that removed it. Thanks, NeilBrown > >> It also does not need the class_ktype type, as dynamic_kobj_ktype >> is sufficient. >> >> So use the simple kobject_create_and_add() to initialize it. >> >> This provides flexibility for making changes to >> class_setup_tunables(). >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> drivers/staging/lustre/lustre/llite/lproc_llite.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c >> index 8215296dc15d..78ec0fa65c58 100644 >> --- a/drivers/staging/lustre/lustre/llite/lproc_llite.c >> +++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c >> @@ -46,7 +46,7 @@ int llite_tunables_register(void) >> { >> int rc = 0; >> >> - llite_kobj = class_setup_tunables("llite"); >> + llite_kobj = kobject_create_and_add("llite", &lustre_kset->kobj); >> if (IS_ERR(llite_kobj)) >> return PTR_ERR(llite_kobj); >> > > llite_kobj->kset = kset_get(&lustre_kset); >> > > In llite_tunables_unregister() the call for kobject_put() > should really be replaced with kobject_del(). This is a bug > in my original work.
On Sun, Feb 24 2019, James Simmons wrote: > For some reason this patch didn't land in my mailbox but I can > see it on https://patchwork.kernel.org/patch/10819037. This patch > is mostly good since llite now uses dynamic_kobj_ktype with its own > ktype. Thus class_sysfs_release() will never be called with llite. > > What does need fixing is > > -------------------------------------------------------- > type->typ_kobj.kset = lustre_kset; > > changed to: > > type->typ_kobj.kset = kset_get(&lustre_kset); Why? Where is the kset_put() what will match this? > -------------------------------------------------------- > > Next change needed it change all the > > kobject_put(&type->typ_kobj); -> kobject_del(&type->typ_kobj); > Why? kobject_del() removed from sysfs. kobject_put() calls kobject_release() on the last put. This calls kobject_cleanup() which calls kobject_del() if needed. So why do we need to call kobject_del()? Thanks, NeilBrown > So we properly handle the kset referencing. That is currently > broken by me :-(
On Sun, Feb 24 2019, James Simmons wrote: >> For some reason this patch didn't land in my mailbox but I can see it >> on https://patchwork.kernel.org/patch/10819037. This patch is mostly >> good since llite now uses dynamic_kobj_ktype with its own ktype. Thus >> class_sysfs_release() will never be called with llite. >> >> What does need fixing is >> >> -------------------------------------------------------- >> type->typ_kobj.kset = lustre_kset; >> >> changed to: >> >> type->typ_kobj.kset = kset_get(&lustre_kset); > >Why? Where is the kset_put() what will match this? Just an off the cuff review. I'm work on a full fledge patch. Just testing on OpenSFS branch since the server side has some unique needs. Basically in the old days the lov and osc layer were present on servers so you have a /sys/fs/lustre/osc tree on the MDS server for example. Now a new layer osp has replaced it but we need to keep the old osc tree structure around. The function part is some people test with everything on one node which can create unique conditions to handle. I'm trying to sort it out. >> -------------------------------------------------------- >> >> Next change needed it change all the >> >> kobject_put(&type->typ_kobj); -> kobject_del(&type->typ_kobj); >> > >Why? >kobject_del() removed from sysfs. kobject_put() calls kobject_release() >on the last put. This calls kobject_cleanup() which calls kobject_del() >if needed. >So why do we need to call kobject_del()? You are right. I missed that kobject_cleanup() calls kobject_del(). I noticed It latter when you pointed out.
> >> llite_kobj does not benefit from being in the > >> lustre_kset kset (though it does need lustre_kset > >> as a parent) > > > > Nak. > > > > Yes llite does benefit from being in the kset. On large clusters you > > can end up with 1000s of clients so keeping the sysfs files setting > > in sync needs to be done in mass. The way Lustre does this is by > > running on the MGS server 'lctl conf_param $FSNAME.llite.lazystats=1' > > or with the newer 'lctl set_param -P llite.$FSNAME-*.lazystats=1' as > > an example. When this is done on the MGS that new value is first > > persistently stored and then transmitted to the target nodes. > > > > The magic behind this is with class_process_config(). In the code > > path LCFG_PARAM, which handles the conf_param case' the main function > > is class_modify_config() which is used by both llite and the obd > > devices. This function first attempts to find the sysfs attribute > > from the kobject and call lustre_attr_store() directly to set it. > > If it can't find the attribute, which means its a debugfs file > > most likely, then a uevent is created and sent off. The udev rule > > then exexcutes some application to handle the debugfs settings. > > For uevents to work the kobject (llite) must belong to a kset. > > > > With LCFG_SET_PARAM, which handles the 'set_param -P' case the > > master function is process_param2_config(). For this case we > > always send attribute changes with uevents. Besides the requirement > > of the kobject belonging to the kset this function uses > > kset_find_object() directly which if llite is not in the kset > > will never be found. > > Thanks a lot for the explanation - there are definitely things I missed > there. > I'll drop this patch, and keep class_setup_tunables() in the later patch > that removed it. I have a patch to replace this one. In fact I love the using the release for cleanups for ktype that I moved debugfs handling in llite into a new llite_kobj_release(). I'm looking at the next patch but that one is tricker since it impacts server code as well as I pointed out in another email. I think we can do a reasonable cleanup here. > >> It also does not need the class_ktype type, as dynamic_kobj_ktype > >> is sufficient. > >> > >> So use the simple kobject_create_and_add() to initialize it. > >> > >> This provides flexibility for making changes to > >> class_setup_tunables(). > >> > >> Signed-off-by: NeilBrown <neilb@suse.com> > >> --- > >> drivers/staging/lustre/lustre/llite/lproc_llite.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c > >> index 8215296dc15d..78ec0fa65c58 100644 > >> --- a/drivers/staging/lustre/lustre/llite/lproc_llite.c > >> +++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c > >> @@ -46,7 +46,7 @@ int llite_tunables_register(void) > >> { > >> int rc = 0; > >> > >> - llite_kobj = class_setup_tunables("llite"); > >> + llite_kobj = kobject_create_and_add("llite", &lustre_kset->kobj); > >> if (IS_ERR(llite_kobj)) > >> return PTR_ERR(llite_kobj); > >> > > > > llite_kobj->kset = kset_get(&lustre_kset); > >> > > > > In llite_tunables_unregister() the call for kobject_put() > > should really be replaced with kobject_del(). This is a bug > > in my original work. >
diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c index 8215296dc15d..78ec0fa65c58 100644 --- a/drivers/staging/lustre/lustre/llite/lproc_llite.c +++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c @@ -46,7 +46,7 @@ int llite_tunables_register(void) { int rc = 0; - llite_kobj = class_setup_tunables("llite"); + llite_kobj = kobject_create_and_add("llite", &lustre_kset->kobj); if (IS_ERR(llite_kobj)) return PTR_ERR(llite_kobj);
llite_kobj does not benefit from being in the lustre_kset kset (though it does need lustre_kset as a parent) It also does not need the class_ktype type, as dynamic_kobj_ktype is sufficient. So use the simple kobject_create_and_add() to initialize it. This provides flexibility for making changes to class_setup_tunables(). Signed-off-by: NeilBrown <neilb@suse.com> --- drivers/staging/lustre/lustre/llite/lproc_llite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)