Skip to contentSkip to conversation

btrfs: allocate raid type kobjects dynamically

PATCH In review Jeff Mahoney 2 participants 5 messages May 27, 2014 Btrfs file system

lore
Share & tools
lore

Jeff Mahoney

author
#0 lore raw

PATCH v4 1/1 btrfs: allocate raid type kobjects dynamically

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt
# btrfs balance start -mconvert=single -dconvert=single /mnt -f
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Signed-off-by Jeff Mahoney <[email protected]>
---
 fs/btrfs/ctree.h       |    8 +++++++-
 fs/btrfs/extent-tree.c |   39 ++++++++++++++++++++++++++-------------
 fs/btrfs/sysfs.c       |    5 +++--
 3 files changed, 36 insertions(+), 16 deletions(-)
fs/btrfs/ctree.h +7 -1
@@ -1113,6 +1113,12 @@ struct btrfs_qgroup_limit_item {
 	__le64 rsv_excl;
 } __attribute__ ((__packed__));
 
+/* For raid type sysfs entries */
+struct raid_kobject {
+	int raid_type;
+	struct kobject kobj;
+};
+
 struct btrfs_space_info {
 	spinlock_t lock;
 

@@ -1163,7 +1169,7 @@ struct btrfs_space_info {
 	wait_queue_head_t wait;
 
 	struct kobject kobj;
-	struct kobject block_group_kobjs[BTRFS_NR_RAID_TYPES];
+	struct kobject *block_group_kobjs[BTRFS_NR_RAID_TYPES];
 };
 
 #define	BTRFS_BLOCK_RSV_GLOBAL		1
fs/btrfs/extent-tree.c +26 -13
@@ -3401,10 +3401,8 @@ static int update_space_info(struct btrf
 		return ret;
 	}
 
-	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
+	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
 		INIT_LIST_HEAD(&found->block_groups[i]);
-		kobject_init(&found->block_group_kobjs[i], &btrfs_raid_ktype);
-	}
 	init_rwsem(&found->groups_sem);
 	spin_lock_init(&found->lock);
 	found->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK;

@@ -8327,8 +8325,9 @@ int btrfs_free_block_groups(struct btrfs
 		list_del(&space_info->list);
 		for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
 			struct kobject *kobj;
-			kobj = &space_info->block_group_kobjs[i];
-			if (kobj->parent) {
+			kobj = space_info->block_group_kobjs[i];
+			space_info->block_group_kobjs[i] = NULL;
+			if (kobj) {
 				kobject_del(kobj);
 				kobject_put(kobj);
 			}

@@ -8352,17 +8351,26 @@ static void __link_block_group(struct bt
 	up_write(&space_info->groups_sem);
 
 	if (first) {
-		struct kobject *kobj = &space_info->block_group_kobjs[index];
+		struct raid_kobject *rkobj;
 		int ret;
 
-		kobject_get(&space_info->kobj); /* put in release */
-		ret = kobject_add(kobj, &space_info->kobj, "%s",
-				  get_raid_name(index));
+		rkobj = kzalloc(sizeof(*rkobj), GFP_NOFS);
+		if (!rkobj)
+			goto out_err;
+		rkobj->raid_type = index;
+		kobject_init(&rkobj->kobj, &btrfs_raid_ktype);
+		ret = kobject_add(&rkobj->kobj, &space_info->kobj,
+				  "%s", get_raid_name(index));
 		if (ret) {
-			pr_warn("BTRFS: failed to add kobject for block cache. ignoring.\n");
-			kobject_put(&space_info->kobj);
+			kobject_put(&rkobj->kobj);
+			goto out_err;
 		}
+		space_info->block_group_kobjs[index] = &rkobj->kobj;
 	}
+
+	return;
+out_err:
+	pr_warn("BTRFS: failed to add kobject for block cache. ignoring.\n");
 }
 
 static struct btrfs_block_group_cache *

@@ -8697,6 +8705,7 @@ int btrfs_remove_block_group(struct btrf
 	struct btrfs_root *tree_root = root->fs_info->tree_root;
 	struct btrfs_key key;
 	struct inode *inode;
+	struct kobject *kobj = NULL;
 	int ret;
 	int index;
 	int factor;

@@ -8796,11 +8805,15 @@ int btrfs_remove_block_group(struct btrf
 	 */
 	list_del_init(&block_group->list);
 	if (list_empty(&block_group->space_info->block_groups[index])) {
-		kobject_del(&block_group->space_info->block_group_kobjs[index]);
-		kobject_put(&block_group->space_info->block_group_kobjs[index]);
+		kobj = block_group->space_info->block_group_kobjs[index];
+		block_group->space_info->block_group_kobjs[index] = NULL;
 		clear_avail_alloc_bits(root->fs_info, block_group->flags);
 	}
 	up_write(&block_group->space_info->groups_sem);
+	if (kobj) {
+		kobject_del(kobj);
+		kobject_put(kobj);
+	}
 
 	if (block_group->cached == BTRFS_CACHE_STARTED)
 		wait_block_group_cache_done(block_group);
fs/btrfs/sysfs.c +3 -2
@@ -254,6 +254,7 @@ static ssize_t global_rsv_reserved_show(
 BTRFS_ATTR(global_rsv_reserved, 0444, global_rsv_reserved_show);
 
 #define to_space_info(_kobj) container_of(_kobj, struct btrfs_space_info, kobj)
+#define to_raid_kobj(_kobj) container_of(_kobj, struct raid_kobject, kobj)
 
 static ssize_t raid_bytes_show(struct kobject *kobj,
 			       struct kobj_attribute *attr, char *buf);

@@ -266,7 +267,7 @@ static ssize_t raid_bytes_show(struct ko
 {
 	struct btrfs_space_info *sinfo = to_space_info(kobj->parent);
 	struct btrfs_block_group_cache *block_group;
-	int index = kobj - sinfo->block_group_kobjs;
+	int index = to_raid_kobj(kobj)->raid_type;
 	u64 val = 0;
 
 	down_read(&sinfo->groups_sem);

@@ -288,7 +289,7 @@ static struct attribute *raid_attributes
 
 static void release_raid_kobj(struct kobject *kobj)
 {
-	kobject_put(kobj->parent);
+	kfree(to_raid_kobj(kobj));
 }
 
 struct kobj_type btrfs_raid_ktype = {
Signature · 2 lines
Jeff Mahoney
SUSE Labs
#1 lore raw
Quoted 18 lines · Jeff Mahoney

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt
# btrfs balance start -mconvert=single -dconvert=single /mnt -f
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

Quoted 5 lines

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Signed-off-by Jeff Mahoney <[email protected]>

Lets see how smart patchwork is:

Reported-by David Sterba <[email protected]>

-chris

#2 lore raw
Quoted 23 lines · Chris Mason
Quoted 22 lines · Jeff Mahoney

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt
# btrfs balance start -mconvert=single -dconvert=single /mnt -f
# btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Quoted 5 lines
Quoted 1 line
Signed-off-by Jeff Mahoney <[email protected]>

Lets see how smart patchwork is:

Reported-by David Sterba <[email protected]>

For future reference, not that smart.

-chris

author
#3 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 27 lines · Chris Mason
Quoted 24 lines · Chris Mason
Quoted 23 lines · Jeff Mahoney

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt #
btrfs balance start -mconvert=single -dconvert=single /mnt -f #
btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Jeff Mahoney <[email protected]>

Lets see how smart patchwork is:

Reported-by David Sterba <[email protected]>

For future reference, not that smart.

Have you already added it or should I just post it again with the rb?

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)

iQIcBAEBAgAGBQJThMl4AAoJEB57S2MheeWywE0QAI07WMgArtqHqIxpQcZIOS6V 1tIIflZB3FSLOxtrfTHbGAAMcfUueuz+wJvkaj/qKpcZ4ONVn3yeBpmYvH8FleAd V7SjGvp+ZiUKZVk0PR2bPUdKABcqaVntHs7fs4YZ0ivyBz7XsOtLv5WmuF+CFZb5 QFq5oGAwnMPmxY42O65gn6dK+c+IbkfkLlVNFqg0hKFfhraNF8eCnW2wf3jt2/gl 92ktasmfiD/6f87dijo51o2AkVWBv7UsiFnK95HJ3u5CSXsoQ6SdCQCY4vfUuvBS S0WiKM2qp3l3ZMZ63EVxdNQ0Nv95DTJhQalES0wYPZDy65O11MqFRSV+08Nw0KG5 9ZaPM4EZzbQmyowzzb+ck91GHJ25WNt50RQsUMrFuMXlMEQR0o9gN4F1asAB6VJL G/67B50NXgUBXAqHg9did70t9ipYZIWwsF/UBAtmeachIwq9AUnD4FCA9vu3Qe2q y/C86hT3GgTcBvehuOZO3GZZeIC1YuW1jTWwtNan0RfPlvtcs0djIF3h2cXiYyCF DqdV41aT7+vwT2QSSEDflPfI5uhJUbFezlwANFKX7ecC6ZkfCCLUzZj8YiqD5tN2 kQtSR9uPNPpWhB8xOwchlTQH8CZAUOM/+LpguQsh6DbFgb3Yi2A2lO7sfcMpFjcA H11fIBAYlsUy6ZSd+S0B
=8PGQ
-----END PGP SIGNATURE-----

#4 lore raw
Quoted 31 lines · Jeff Mahoney

- gpg control packet

Quoted 27 lines · Chris Mason
Quoted 26 lines · Chris Mason
Quoted 24 lines · Jeff Mahoney

We are currently allocating space_info objects in an array when we allocate space_info. When a user does something like:

# btrfs balance start -mconvert=raid1 -dconvert=raid1 /mnt #
btrfs balance start -mconvert=single -dconvert=single /mnt -f #
btrfs balance start -mconvert=raid1 -dconvert=raid1 /

We can end up with memory corruption since the kobject hasn't been reinitialized properly and the name pointer was left set.

The rationale behind allocating them statically was to avoid creating a separate kobject container that just contained the raid type. It used the index in the array to determine the index.

Ultimately, though, this wastes more memory than it saves in all but the most complex scenarios and introduces kobject lifetime questions.

This patch allocates the kobjects dynamically instead. Note that we also remove the kobject_get/put of the parent kobject since kobject_add and kobject_del do that internally.

Signed-off-by Jeff Mahoney <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Lets see how smart patchwork is:

Reported-by David Sterba <[email protected]>

For future reference, not that smart.

Have you already added it or should I just post it again with the rb?

Thanks, I did it here. I was just curious if patchwork would save me the pain of editing the mbox file ;)

-chris