From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:41146 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751398AbdEQUev (ORCPT ); Wed, 17 May 2017 16:34:51 -0400 Subject: Re: [PATCH v2 1/2] btrfs: Separate space_info create/update To: Noah Massey Cc: David Sterba , linux-btrfs , jeffm@suse.com References: <1495033663-15610-1-git-send-email-nborisov@suse.com> From: Nikolay Borisov Message-ID: Date: Wed, 17 May 2017 23:34:47 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 17.05.2017 21:57, Noah Massey wrote: > On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov 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 >> Reviewed-by: Jeff Mahoney >> --- >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >