| Message ID | 20230405130747.66006-1-frank.li@vivo.com |
|---|---|
| State | New |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubifs: remove unnecessary kobject_del() | expand |
Hi, Yangtao > kobject_put() actually covers kobject removal automatically, which is > single stage removal. So it is safe to kill kobject_del() directly. > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/ubifs/sysfs.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/ubifs/sysfs.c b/fs/ubifs/sysfs.c > index 1c958148bb87..1ffdc3c9b340 100644 > --- a/fs/ubifs/sysfs.c > +++ b/fs/ubifs/sysfs.c > @@ -130,7 +130,6 @@ int ubifs_sysfs_register(struct ubifs_info *c) > > void ubifs_sysfs_unregister(struct ubifs_info *c) > { > - kobject_del(&c->kobj); > kobject_put(&c->kobj); > wait_for_completion(&c->kobj_unregister); > > This patch looks harmless at the view of ubifs, kobject_cleanup() finally invokes __kobject_del and releases parent just like kobject_del() does. A difference is that the releasing sequence of kobj's parent is put after releasing kobj. About the use case of kobject_del(), we may refer to other filesystems' implementations, eg. ext4_put_super(). Firstly ext4_unregister_sysfs() removes all sysfs interfaces, then jbd2_journal_destroy() destroys sbi->s_journal, finally sbi->s_kobj is released. Here kobject_del() stops user accessing sbi->s_journal by sysfs interface journal_taskļ¼because sbi->s_journal will be released soon before kobject_put(). I think we should still reserve the 'redundant' kobject_del(), removing it won't bring any performance improvement. BTW, in ext4_put_super(), flush_stashed_error_work(which could access sbi->s_kobj) is flushed after kobject removed. is it okay to replace kobject_del(&sbi->s_kobj) with kobject_put(&sbi->s_kobj)? diff --git a/fs/ext4/super.c b/fs/ext4/super.c index a22417d113ca..9e3744099d1e 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1296,8 +1296,6 @@ static void ext4_put_super(struct super_block *sb) * Now that we are completely done shutting down the * superblock, we need to actually destroy the kobject. */ - kobject_put(&sbi->s_kobj); - wait_for_completion(&sbi->s_kobj_unregister); if (sbi->s_chksum_driver) crypto_free_shash(sbi->s_chksum_driver); kfree(sbi->s_blockgroup_lock); diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c index 12d6252e3e22..be92a09bb8a0 100644 --- a/fs/ext4/sysfs.c +++ b/fs/ext4/sysfs.c @@ -562,7 +562,8 @@ void ext4_unregister_sysfs(struct super_block *sb) if (sbi->s_proc) remove_proc_subtree(sb->s_id, ext4_proc_root); - kobject_del(&sbi->s_kobj); + kobject_put(&sbi->s_kobj); + wait_for_completion(&sbi->s_kobj_unregister); } int __init ext4_init_sysfs(void)
> I think we should still reserve the 'redundant' kobject_del(), > removing it won't bring any performance improvement. Since it's redundant, why not to remove it. Thx, Yangtao
Hi, >> I think we should still reserve the 'redundant' kobject_del(), >> removing it won't bring any performance improvement. > > Since it's redundant, why not to remove it. > In my personal view, 'redundant' means removing kobject_del() is okay, it won't bring any bugs. But removing it won't make code more readability or gain any performance improvement, so it could be reserved. Whether to remove kobject_del() depends on UBIFS maintainer, I just help to check if the modification could affect the normal logic.
diff --git a/fs/ubifs/sysfs.c b/fs/ubifs/sysfs.c index 1c958148bb87..1ffdc3c9b340 100644 --- a/fs/ubifs/sysfs.c +++ b/fs/ubifs/sysfs.c @@ -130,7 +130,6 @@ int ubifs_sysfs_register(struct ubifs_info *c) void ubifs_sysfs_unregister(struct ubifs_info *c) { - kobject_del(&c->kobj); kobject_put(&c->kobj); wait_for_completion(&c->kobj_unregister);
kobject_put() actually covers kobject removal automatically, which is single stage removal. So it is safe to kill kobject_del() directly. Signed-off-by: Yangtao Li <frank.li@vivo.com> --- fs/ubifs/sysfs.c | 1 - 1 file changed, 1 deletion(-)