On 5/17/17 4:52 PM, Noah Massey wrote: > On Wed, May 17, 2017 at 4:34 PM, Nikolay Borisov <nbori...@suse.com> wrote: >> >> >> On 17.05.2017 21:57, Noah Massey wrote: >>> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nbori...@suse.com> wrote: >>>> Currently the struct space_info creation code is intermixed in the >>>> udpate_space_info function. There are well-defined points at which the we >>> >>> ^^^ update_space_info >>> >>>> actually want to create brand-new space_info structs (e.g. during mount of >>>> the filesystem as well as sometimes when adding/initialising new chunks). >>>> In >>>> such cases udpate_space_info is called with 0 as the bytes parameter. All >>>> of >>>> this makes for spaghetti code. >>>> >>>> Fix it by factoring out the creation code in a separate create_space_info >>>> structure. This also allows to simplify the internals. Furthermore it will >>>> make the update_space_info function not fail, allowing to remove error >>>> handling in callers. This will come in a follow up patch. >>>> >>>> This bears no functional changes >>>> >>>> Signed-off-by: Nikolay Borisov <nbori...@suse.com> >>>> Reviewed-by: Jeff Mahoney <je...@suse.com> >>>> --- >>>> fs/btrfs/extent-tree.c | 127 >>>> ++++++++++++++++++++++++------------------------- >>>> 1 file changed, 62 insertions(+), 65 deletions(-) >>>> >>>> Change since v1: >>>> >>>> Incorporated Jeff Mahoney's feedback and added his reviewed-by >>>> >>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>>> index be5477676cc8..28848e45b018 100644 >>>> --- a/fs/btrfs/extent-tree.c >>>> +++ b/fs/btrfs/extent-tree.c >>>> @@ -3914,15 +3914,58 @@ static const char *alloc_name(u64 flags) >>>> }; >>>> } >>>> >>>> +static int create_space_info(struct btrfs_fs_info *info, u64 flags, >>>> + struct btrfs_space_info **new) { >>>> + >>>> + struct btrfs_space_info *space_info; >>>> + int i; >>>> + int ret; >>>> + >>>> + space_info = kzalloc(sizeof(*space_info), GFP_NOFS); >>>> + if (!space_info) >>>> + return -ENOMEM; >>>> + >>>> + ret = percpu_counter_init(&space_info->total_bytes_pinned, 0, >>>> GFP_KERNEL); >>>> + if (ret) { >>>> + kfree(space_info); >>>> + return ret; >>>> + } >>>> + >>>> + for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) >>>> + INIT_LIST_HEAD(&space_info->block_groups[i]); >>>> + init_rwsem(&space_info->groups_sem); >>>> + spin_lock_init(&space_info->lock); >>>> + space_info->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK; >>>> + space_info->force_alloc = CHUNK_ALLOC_NO_FORCE; >>>> + init_waitqueue_head(&space_info->wait); >>>> + INIT_LIST_HEAD(&space_info->ro_bgs); >>>> + INIT_LIST_HEAD(&space_info->tickets); >>>> + INIT_LIST_HEAD(&space_info->priority_tickets); >>>> + >>>> + ret = kobject_init_and_add(&space_info->kobj, &space_info_ktype, >>>> + info->space_info_kobj, "%s", >>>> + alloc_name(space_info->flags)); >>>> + if (ret) { >>>> + percpu_counter_destroy(&space_info->total_bytes_pinned); >>>> + kfree(space_info); >>>> + return ret; >>>> + } >>>> + >>>> + *new = space_info; >>>> + list_add_rcu(&space_info->list, &info->space_info); >>>> + if (flags & BTRFS_BLOCK_GROUP_DATA) >>>> + info->data_sinfo = space_info; >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int update_space_info(struct btrfs_fs_info *info, u64 flags, >>>> u64 total_bytes, u64 bytes_used, >>>> u64 bytes_readonly, >>>> struct btrfs_space_info **space_info) >>>> { >>>> struct btrfs_space_info *found; >>>> - int i; >>>> int factor; >>>> - int ret; >>>> >>>> if (flags & (BTRFS_BLOCK_GROUP_DUP | BTRFS_BLOCK_GROUP_RAID1 | >>>> BTRFS_BLOCK_GROUP_RAID10)) >>>> @@ -3946,53 +3989,6 @@ static int update_space_info(struct btrfs_fs_info >>>> *info, u64 flags, >>>> *space_info = found; >>>> return 0; >>>> } >>>> - found = kzalloc(sizeof(*found), GFP_NOFS); >>>> - if (!found) >>>> - return -ENOMEM; >>>> - >>>> - ret = percpu_counter_init(&found->total_bytes_pinned, 0, >>>> GFP_KERNEL); >>>> - if (ret) { >>>> - kfree(found); >>>> - return ret; >>>> - } >>>> - >>>> - for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) >>>> - INIT_LIST_HEAD(&found->block_groups[i]); >>>> - init_rwsem(&found->groups_sem); >>>> - spin_lock_init(&found->lock); >>>> - found->flags = flags & BTRFS_BLOCK_GROUP_TYPE_MASK; >>>> - found->total_bytes = total_bytes; >>>> - found->disk_total = total_bytes * factor; >>>> - found->bytes_used = bytes_used; >>>> - found->disk_used = bytes_used * factor; >>>> - found->bytes_pinned = 0; >>>> - found->bytes_reserved = 0; >>>> - found->bytes_readonly = bytes_readonly; >>>> - found->bytes_may_use = 0; >>>> - found->full = 0; >>>> - found->max_extent_size = 0; >>>> - found->force_alloc = CHUNK_ALLOC_NO_FORCE; >>>> - found->chunk_alloc = 0; >>>> - found->flush = 0; >>>> - init_waitqueue_head(&found->wait); >>>> - INIT_LIST_HEAD(&found->ro_bgs); >>>> - INIT_LIST_HEAD(&found->tickets); >>>> - INIT_LIST_HEAD(&found->priority_tickets); >>>> - >>>> - ret = kobject_init_and_add(&found->kobj, &space_info_ktype, >>>> - info->space_info_kobj, "%s", >>>> - alloc_name(found->flags)); >>>> - if (ret) { >>>> - kfree(found); >>>> - return ret; >>>> - } >>>> - >>>> - *space_info = found; >>>> - list_add_rcu(&found->list, &info->space_info); >>>> - if (flags & BTRFS_BLOCK_GROUP_DATA) >>>> - info->data_sinfo = found; >>>> - >>>> - return ret; >>>> } >>>> >>>> static void set_avail_alloc_bits(struct btrfs_fs_info *fs_info, u64 flags) >>>> @@ -4495,10 +4491,9 @@ static int do_chunk_alloc(struct btrfs_trans_handle >>>> *trans, >>>> >>>> space_info = __find_space_info(fs_info, flags); >>>> if (!space_info) { >>>> - ret = update_space_info(fs_info, flags, 0, 0, 0, >>>> &space_info); >>>> + ret = create_space_info(fs_info, flags, &space_info); >>>> BUG_ON(ret); /* -ENOMEM */ >>>> } >>>> - BUG_ON(!space_info); /* Logic error */ >>> >>> Isn't removing this BUG_ON a functional change? >>> I understand that it shouldn't happen in the current incarnation of >>> create_space_info, but that was true before the patch as well >> >> No, because this bug_on would have triggere only if !space_info, and >> this condition would have, in turn, triggered the if statement, which >> has a BUG_ON(ret). E.g. in case ret is 0 then space_info will definitely >> be set. Hence BUG_ON(!space_info) is redundant. >> > > But the BUG_ON(ret) value is independent of BUG_ON(!space_info) > The BUG_ON(!space_info) seems to have been there to catch a potential > logic error if update_space_info is modified / regressed to return 0 > and still not set space_info pointer, so removing the BUG_ON seems to > be a change separate from the switch to create_space_info.
The BUG_ON(!space_info) is older than the call to update_space_info(). It should've been removed when we started creating the space info there. -Jeff > I'm not saying it can't be removed: the current code looks good, and I > agree the BUG_ON won't trigger. It looks like the current incarnation > of create_space_info will always set space_info or return an error, > but so did the previous call to update_space_info, so I was not sure > it should be dropped as part of the switch. > > Is the thinking that since we are switching the called method and in > the new current code the BUG_ON won't trigger, there will be no > behavior change by removing it so we can drop it in the update? -- Jeff Mahoney SUSE Labs
signature.asc
Description: OpenPGP digital signature