[lustre-devel] [PATCH v2 05/29] lustre: llite: don't use class_setup_tunables()

[James Simmons]

> On Mon, May 20 2019, James Simmons wrote:
> 
> > llite is very different from the other types of lustre devices.
> > Since this is the case llite should register independently. Doing
> > this allows us to cleanup the debugfs registering in the release
> > function of struct kobj_type.
> >
> > Signed-off-by: James Simmons <uja.ornl at yahoo.com>
> > WC-bug-id: https://jira.whamcloud.com/browse/LU-8066
> > Reviewed-on: https://review.whamcloud.com/34292
> > Reviewed-by: Andreas Dilger <adilger at whamcloud.com>
> > Reviewed-by: Ben Evans <bevans at cray.com>
> > Reviewed-by: Oleg Drokin <green at whamcloud.com>
> > Signed-off-by: James Simmons <jsimmons at infradead.org>
> > ---
> >  fs/lustre/llite/lproc_llite.c | 41 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/lustre/llite/lproc_llite.c b/fs/lustre/llite/lproc_llite.c
> > index 5de8462..91b0a50 100644
> > --- a/fs/lustre/llite/lproc_llite.c
> > +++ b/fs/lustre/llite/lproc_llite.c
> > @@ -42,18 +42,40 @@
> >  static struct kobject *llite_kobj;
> >  static struct dentry *llite_root;
> >  
> > +static void llite_kobj_release(struct kobject *kobj)
> > +{
> > +	if (!IS_ERR_OR_NULL(llite_root)) {
> > +		debugfs_remove(llite_root);
> > +		llite_root = NULL;
> > +	}
> > +
> > +	kfree(kobj);
> > +}
> > +
> > +static struct kobj_type llite_kobj_ktype = {
> > +	.release	= llite_kobj_release,
> > +	.sysfs_ops	= &lustre_sysfs_ops,
> > +};
> > +
> >  int llite_tunables_register(void)
> >  {
> > -	int rc = 0;
> > +	int rc;
> > +
> > +	llite_kobj = kzalloc(sizeof(*llite_kobj), GFP_KERNEL);
> > +	if (!llite_kobj)
> > +		return -ENOMEM;
> >  
> > -	llite_kobj = class_setup_tunables("llite");
> > -	if (IS_ERR(llite_kobj))
> > -		return PTR_ERR(llite_kobj);
> > +	llite_kobj->kset = lustre_kset;
> > +	rc = kobject_init_and_add(llite_kobj, &llite_kobj_ktype,
> > +				  &lustre_kset->kobj, "%s", "llite");
> > +	if (rc)
> > +		goto free_kobj;
> >  
> >  	llite_root = debugfs_create_dir("llite", debugfs_lustre_root);
> >  	if (IS_ERR_OR_NULL(llite_root)) {
> >  		rc = llite_root ? PTR_ERR(llite_root) : -ENOMEM;
> >  		llite_root = NULL;
> > +free_kobj:
> >  		kobject_put(llite_kobj);
> >  		llite_kobj = NULL;
> 
> eeek.  Goto into the inside of an if/then clause is .... not my
> favourite.
> 
> > +	rc = kobject_init_and_add(llite_kobj, &llite_kobj_ktype,
> > +				  &lustre_kset->kobj, "%s", "llite");
> > +	if (rc)
> > +		goto free_kobj;
> >  
> >  	llite_root = debugfs_create_dir("llite", debugfs_lustre_root);
> >  	if (IS_ERR_OR_NULL(llite_root)) {
> >  		rc = llite_root ? PTR_ERR(llite_root) : -ENOMEM;
> >  		llite_root = NULL;
>                 goto free_kobj;
>         }
>         return 0;
> > +free_kobj:
> > 	kobject_put(llite_kobj);
> > 	llite_kobj = NULL;
>         return rc;
> 
> Isn't that much cleaner?
> 
> Otherwise, I like the patch.

Sure that is fine. Just did it that way since Greg would grip about
how having more than one return in a function is not proper kernel
coding style. So I tend to write code this that way. 

> Thanks,
> NeilBrown
> 
> 
> >  	}
> > @@ -65,9 +87,6 @@ void llite_tunables_unregister(void)
> >  {
> >  	kobject_put(llite_kobj);
> >  	llite_kobj = NULL;
> > -
> > -	debugfs_remove(llite_root);
> > -	llite_root = NULL;
> >  }
> >  
> >  /* debugfs llite mount point registration */
> > @@ -1216,17 +1235,17 @@ static ssize_t ll_nosquash_nids_seq_write(struct file *file,
> >  	NULL,
> >  };
> >  
> > -static void llite_kobj_release(struct kobject *kobj)
> > +static void sbi_kobj_release(struct kobject *kobj)
> >  {
> >  	struct ll_sb_info *sbi = container_of(kobj, struct ll_sb_info,
> >  					      ll_kset.kobj);
> >  	complete(&sbi->ll_kobj_unregister);
> >  }
> >  
> > -static struct kobj_type llite_ktype = {
> > +static struct kobj_type sbi_ktype = {
> >  	.default_attrs	= llite_attrs,
> >  	.sysfs_ops	= &lustre_sysfs_ops,
> > -	.release	= llite_kobj_release,
> > +	.release	= sbi_kobj_release,
> >  };
> >  
> >  static const struct llite_file_opcode {
> > @@ -1384,7 +1403,7 @@ int ll_debugfs_register_super(struct super_block *sb, const char *name)
> >  out_ll_kset:
> >  	/* Yes we also register sysfs mount kset here as well */
> >  	sbi->ll_kset.kobj.parent = llite_kobj;
> > -	sbi->ll_kset.kobj.ktype = &llite_ktype;
> > +	sbi->ll_kset.kobj.ktype = &sbi_ktype;
> >  	init_completion(&sbi->ll_kobj_unregister);
> >  	err = kobject_set_name(&sbi->ll_kset.kobj, "%s", name);
> >  	if (err)
> > -- 
> > 1.8.3.1
>