[02/37] lustre: llite: don't use class_setup_tunables()
diff mbox series

Message ID 155053494490.24125.1514109985903155907.stgit@noble.brown
State New
Headers show
Series
  • More lustre patches from obdclass
Related show

Commit Message

NeilBrown Feb. 19, 2019, 12:09 a.m. UTC
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(-)

Comments

James Simmons Feb. 24, 2019, 4:35 p.m. UTC | #1
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.
James Simmons Feb. 24, 2019, 4:52 p.m. UTC | #2
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 :-(
NeilBrown Feb. 25, 2019, 10:27 p.m. UTC | #3
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.
NeilBrown Feb. 25, 2019, 10:38 p.m. UTC | #4
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 :-(
Simmons, James A. Feb. 26, 2019, 8:41 p.m. UTC | #5
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.
James Simmons Feb. 26, 2019, 10:18 p.m. UTC | #6
> >> 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. 
>

Patch
diff mbox series

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);