On 17.05.2017 21:57, Noah Massey wrote:
> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@xxxxxxxx> 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 <nborisov@xxxxxxxx>
>> Reviewed-by: Jeff Mahoney <jeffm@xxxxxxxx>
>> ---
>> 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.
>
>>
>> again:
>> spin_lock(&space_info->lock);
>> @@ -5763,7 +5758,7 @@ int btrfs_orphan_reserve_metadata(struct btrfs_trans_handle *trans,
>> */
>> u64 num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
>>
>> - trace_btrfs_space_reservation(fs_info, "orphan", btrfs_ino(inode),
>> + trace_btrfs_space_reservation(fs_info, "orphan", btrfs_ino(inode),
>> num_bytes, 1);
>> return btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, 1);
>> }
>> @@ -10191,16 +10186,18 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
>> }
>> #endif
>> /*
>> - * Call to ensure the corresponding space_info object is created and
>> - * assigned to our block group, but don't update its counters just yet.
>> - * We want our bg to be added to the rbtree with its ->space_info set.
>> + * Ensure the corresponding space_info object is created and
>> + * assigned to our block group. We want our bg to be added to the rbtree
>> + * with its ->space_info set.
>> */
>> - ret = update_space_info(fs_info, cache->flags, 0, 0, 0,
>> - &cache->space_info);
>> - if (ret) {
>> - btrfs_remove_free_space_cache(cache);
>> - btrfs_put_block_group(cache);
>> - return ret;
>> + cache->space_info = __find_space_info(fs_info, cache->flags);
>> + if (!cache->space_info) {
>> + ret = create_space_info(fs_info, cache->flags, &cache->space_info);
>> + if (ret) {
>> + btrfs_remove_free_space_cache(cache);
>> + btrfs_put_block_group(cache);
>> + return ret;
>> + }
>> }
>>
>> ret = btrfs_add_block_group_cache(fs_info, cache);
>> @@ -10774,21 +10771,21 @@ int btrfs_init_space_info(struct btrfs_fs_info *fs_info)
>> mixed = 1;
>>
>> flags = BTRFS_BLOCK_GROUP_SYSTEM;
>> - ret = update_space_info(fs_info, flags, 0, 0, 0, &space_info);
>> + ret = create_space_info(fs_info, flags, &space_info);
>> if (ret)
>> goto out;
>>
>> if (mixed) {
>> flags = BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA;
>> - ret = update_space_info(fs_info, flags, 0, 0, 0, &space_info);
>> + ret = create_space_info(fs_info, flags, &space_info);
>> } else {
>> flags = BTRFS_BLOCK_GROUP_METADATA;
>> - ret = update_space_info(fs_info, flags, 0, 0, 0, &space_info);
>> + ret = create_space_info(fs_info, flags, &space_info);
>> if (ret)
>> goto out;
>>
>> flags = BTRFS_BLOCK_GROUP_DATA;
>> - ret = update_space_info(fs_info, flags, 0, 0, 0, &space_info);
>> + ret = create_space_info(fs_info, flags, &space_info);
>> }
>> out:
>> return ret;
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
