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

[Jeff Mahoney <jeffm@suse.com>]

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

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

It was due to if (kobj->parent) not being switched over to if (kobj)
in the cleanup-for-umount code.

I've posted a fixed version.

- -Jeff

> -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)

iQIcBAEBAgAGBQJTf6pVAAoJEB57S2MheeWylvwP/jaG8w4Pnu/0+FvxW10xMcFN
HPJyS98JfekVDvnGgVfOA4UrI1ioK3Luwp8jAbk4tnbmZS4pryeJFhTDMpXYLzgY
w5LuXDxMxwQK8U6P47IuYnnRNL0+k4yiBAVT+ULOc2Ly0y8MItl27a1UxWMofzbm
KtNRKwpJpA1QQWDz3aTeLgCGEHa+fGm9BBu5l7RE4wLAFYzYdWYXf0//kv/P0tZo
rruAHIPggsd4tSJwnQBbXTlfJMTf4vFBm1wceeLeXSKTgcZjO9oHOCKlo019nrkw
nR8FwBiWvmFox4m1Ub4zHs9AtIEdbnkUly9/6+L7Bc+L41ro5iMuG1tOrZo+5KJy
vjfcYJTRtn9T3ThoKojsP3Uu8HxFSB5ggi8znmZuzLU5yZMB4dwbGojoq6l4hAvj
XnDXYZapp3FQeBIMGUnEaValmiaYfCV2HjSMf07NvxTY40/Ce3XM6b35J5ESr0ms
ilzFdXPTg2GifWFzSXprunXsvxRRviQrys740U8nW1MOnyTIEipb75fMNmcfT49J
ABzkuvFnI6z82bVK02q1XifKgVsQ4T35aDW2O/AzHWfmIPistdstDFUrRntWLZCS
km29dtdSQC8Ql5uJrw7pRby75O9TRlMtKrD6pic/BfD3aM77DCudnWEhkvm1WEEf
toOjMaX0w5Nk5/VYSCHc
=JmMQ
-----END PGP SIGNATURE-----