Re: [PATCH] btrfs: allocate raid type kobjects dynamically

[Jeff Mahoney <jeffm@suse.com>]

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

On 5/23/14, 3:04 PM, Jeff Mahoney wrote:
> 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.

Nack.

Works with switching raid groups but crashes on umount.

- -Jeff

> Signed-off-by: Jeff Mahoney <jeffm@suse.com> --- fs/btrfs/ctree.h
> |    8 +++++++- fs/btrfs/extent-tree.c |   33
> +++++++++++++++++++++------------ fs/btrfs/sysfs.c       |    5
> +++-- 3 files changed, 31 insertions(+), 15 deletions(-)
> 
> --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -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 --- a/fs/btrfs/extent-tree.c +++
> b/fs/btrfs/extent-tree.c @@ -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,7
> +8325,8 @@ 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]; +			kobj =
> space_info->block_group_kobjs[i]; +
> space_info->block_group_kobjs[i] = NULL; if (kobj->parent) { 
> 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_KERNEL); +		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 * @@ -8796,8 +8804,9 @@ 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]); +
> kobject_del(block_group->space_info->block_group_kobjs[index]); +
> kobject_put(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); ---
> a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -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 = {
> 
> 


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

iQIcBAEBAgAGBQJTf6RhAAoJEB57S2MheeWyMMsP/0qXabjwUETeCxa/VIKbAMP8
C/xzcOPrVIsxK/3EjD865o8dEoUI56FT0pWgxYxysafwdG8VVTCTJi1GXb8gl+yo
JQZOS/JzLSPM+KElzBDZ+2KkDxz3wtx/XVrWQruOCQ+ycx1KVSS5TQ5vNFPMucik
/MBYSx3rDmD9Np5iyv2McbZLqZDlqe57dsuCjZd0p9cBR1DguhZKuhgl1Bf4j7RR
+O7Gip15SAuwYbU+Qntwe6e4i7OAuVIqeYDXP/EJtN/PWzoopwnSatYIHUFhjAXj
Y+GIzxNAZP9rpCjc0akZM1zxgHi9Xrowg/2bWWGfu0GU+ChL02+CdQFt8ZUylsQQ
AndOwTW8QU+PMV9qOzkDj38yrSTWTWeh9SAE85JSzaP4bRaFKqNudnwGrlYUwRqe
TFbuR4JMubGCM4+vSzVgPznY7tiapNtRuoE7Ij5TYQW6arB4aXSUuk351qFGozI+
7N1G1S7KOSFvXGCPgIiz7QqREq9uahb9nEtEBOdkuSZWxMJBmaxfBI5o6n4E0MQA
UEt2IDsZV6p6FbEZGG49eXBVV6JYD0UGrW4I0HAgYFbIRx/0E+E+VuxMsTqjpGl1
NprRDOO6UdzbGM+hCYIc++wXJgq0PYJYBbOM7PKLaCLdTyRlgeZWmmUoIcXkm1oh
tODf44NEMvOVF/sKIMZz
=3iv4
-----END PGP SIGNATURE-----