Patchwork [2/2] ext4: use kobj_completion in ext4_sb_info

login
register
mail settings
Submitter Jeff Mahoney
Date Sept. 10, 2013, 5:44 p.m.
Message ID <522F5A6D.50100@suse.com>
Download mbox | patch
Permalink /patch/273944/
State New
Headers show

Comments

Jeff Mahoney - Sept. 10, 2013, 5:44 p.m.
Now that we have the kobj_completion structure available, let's use it
in ext4.

Note that we aggregate the del/put/wait_for_completion in one
kobj_completion_del_and_wait call. Since the kobject isn't used for
anything while the fs is shutting down and we wait until it's released
before continuing anyway, this is safe.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/ext4/ext4.h  |    4 ++--
 fs/ext4/super.c |   41 ++++++++++++-----------------------------
 2 files changed, 14 insertions(+), 31 deletions(-)
Theodore Ts'o - Sept. 12, 2013, 1:48 a.m.
On Tue, Sep 10, 2013 at 01:44:13PM -0400, Jeff Mahoney wrote:
> Now that we have the kobj_completion structure available, let's use it
> in ext4.
> 
> Note that we aggregate the del/put/wait_for_completion in one
> kobj_completion_del_and_wait call. Since the kobject isn't used for
> anything while the fs is shutting down and we wait until it's released
> before continuing anyway, this is safe.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

I don't have a huge objection to this, but given that it's not fixing
a bug, and it's not saving _that_ many lines of code, if you want to
run the first patch through Greg's tree (which is fine with me), I
might wait to merge this patch until 3.13, so it's not obvious that
it's worth dealing with the inter-tree dependency, since it
complicates testing on my end.

Does that sound reasonable?

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Mahoney - Sept. 12, 2013, 1:52 a.m.
On 9/11/13 9:48 PM, Theodore Ts'o wrote:
> On Tue, Sep 10, 2013 at 01:44:13PM -0400, Jeff Mahoney wrote:
>> Now that we have the kobj_completion structure available, let's use it
>> in ext4.
>>
>> Note that we aggregate the del/put/wait_for_completion in one
>> kobj_completion_del_and_wait call. Since the kobject isn't used for
>> anything while the fs is shutting down and we wait until it's released
>> before continuing anyway, this is safe.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> 
> I don't have a huge objection to this, but given that it's not fixing
> a bug, and it's not saving _that_ many lines of code, if you want to
> run the first patch through Greg's tree (which is fine with me), I
> might wait to merge this patch until 3.13, so it's not obvious that
> it's worth dealing with the inter-tree dependency, since it
> complicates testing on my end.
> 
> Does that sound reasonable?

Works for me. The ext4 case isn't something I'm in a huge rush to get in
-- it was just the most obvious (to me) existing users of the
kobj/completion pair.

-Jeff

Patch

--- a/fs/ext4/ext4.h	2013-09-03 11:23:20.054928553 -0400
+++ b/fs/ext4/ext4.h	2013-09-10 13:03:42.794548845 -0400
@@ -30,6 +30,7 @@ 
 #include <linux/blockgroup_lock.h>
 #include <linux/percpu_counter.h>
 #include <crypto/hash.h>
+#include <linux/kobj_completion.h>
 #ifdef __KERNEL__
 #include <linux/compat.h>
 #endif
@@ -1198,8 +1199,7 @@  struct ext4_sb_info {
 	struct percpu_counter s_dirtyclusters_counter;
 	struct blockgroup_lock *s_blockgroup_lock;
 	struct proc_dir_entry *s_proc;
-	struct kobject s_kobj;
-	struct completion s_kobj_unregister;
+	struct kobj_completion s_kobjc;
 	struct super_block *s_sb;

 	/* Journaling */
--- a/fs/ext4/super.c	2013-09-03 11:23:20.066928375 -0400
+++ b/fs/ext4/super.c	2013-09-10 13:04:16.702156431 -0400
@@ -793,7 +793,8 @@  static void ext4_put_super(struct super_
 		remove_proc_entry("options", sbi->s_proc);
 		remove_proc_entry(sb->s_id, ext4_proc_root);
 	}
-	kobject_del(&sbi->s_kobj);
+
+	kobj_completion_del_and_wait(&sbi->s_kobjc);

 	for (i = 0; i < sbi->s_gdb_count; i++)
 		brelse(sbi->s_group_desc[i]);
@@ -832,12 +833,6 @@  static void ext4_put_super(struct super_
 	if (sbi->s_mmp_tsk)
 		kthread_stop(sbi->s_mmp_tsk);
 	sb->s_fs_info = NULL;
-	/*
-	 * 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);
@@ -2622,11 +2617,12 @@  static struct attribute *ext4_feat_attrs
 	NULL,
 };

+#define kobj_to_sb_info(kobj) \
+	container_of(kobj, struct ext4_sb_info, s_kobjc.kc_kobj)
 static ssize_t ext4_attr_show(struct kobject *kobj,
 			      struct attribute *attr, char *buf)
 {
-	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
-						s_kobj);
+	struct ext4_sb_info *sbi = kobj_to_sb_info(kobj);
 	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);

 	return a->show ? a->show(a, sbi, buf) : 0;
@@ -2636,19 +2632,12 @@  static ssize_t ext4_attr_store(struct ko
 			       struct attribute *attr,
 			       const char *buf, size_t len)
 {
-	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
-						s_kobj);
+	struct ext4_sb_info *sbi = kobj_to_sb_info(kobj);
 	struct ext4_attr *a = container_of(attr, struct ext4_attr, attr);

 	return a->store ? a->store(a, sbi, buf, len) : 0;
 }

-static void ext4_sb_release(struct kobject *kobj)
-{
-	struct ext4_sb_info *sbi = container_of(kobj, struct ext4_sb_info,
-						s_kobj);
-	complete(&sbi->s_kobj_unregister);
-}

 static const struct sysfs_ops ext4_attr_ops = {
 	.show	= ext4_attr_show,
@@ -2658,18 +2647,13 @@  static const struct sysfs_ops ext4_attr_
 static struct kobj_type ext4_ktype = {
 	.default_attrs	= ext4_attrs,
 	.sysfs_ops	= &ext4_attr_ops,
-	.release	= ext4_sb_release,
+	.release	= kobj_completion_release,
 };

-static void ext4_feat_release(struct kobject *kobj)
-{
-	complete(&ext4_feat->f_kobj_unregister);
-}
-
 static struct kobj_type ext4_feat_ktype = {
 	.default_attrs	= ext4_feat_attrs,
 	.sysfs_ops	= &ext4_attr_ops,
-	.release	= ext4_feat_release,
+	.release	= kobj_completion_release,
 };

 /*
@@ -4066,10 +4050,9 @@  no_journal:
 	if (err)
 		goto failed_mount6;

-	sbi->s_kobj.kset = ext4_kset;
-	init_completion(&sbi->s_kobj_unregister);
-	err = kobject_init_and_add(&sbi->s_kobj, &ext4_ktype, NULL,
-				   "%s", sb->s_id);
+	sbi->s_kobjc.kc_kobj.kset = ext4_kset;
+	kobj_completion_init(&sbi->s_kobjc, &ext4_ktype);
+	err = kobject_add(&sbi->s_kobjc.kc_kobj, NULL, "%s", sb->s_id);
 	if (err)
 		goto failed_mount7;

@@ -4125,7 +4108,7 @@  cantfind_ext4:

 #ifdef CONFIG_QUOTA
 failed_mount8:
-	kobject_del(&sbi->s_kobj);
+	kobj_completion_del_and_wait(&sbi->s_kobjc);
 #endif
 failed_mount7:
 	ext4_unregister_li_request(sb);