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.
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?
Thank you,
~ Noah
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
