On 5/9/17 4:34 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 > 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> > --- > fs/btrfs/extent-tree.c | 127 > ++++++++++++++++++++++++------------------------- > 1 file changed, 62 insertions(+), 65 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index be5477676cc8..e71921d8c610 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -3914,15 +3914,57 @@ 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) {
Not your bug (it's mine from 6ab0a2029c), but we need percpu_counter_destroy here. I just sent a patch to fix the original case so we can pull it into stable as well. > + 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 +3988,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 +4490,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 */ > > again: > spin_lock(&space_info->lock); > @@ -5763,7 +5757,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); > } > @@ -10153,6 +10147,7 @@ int btrfs_make_block_group(struct btrfs_trans_handle > *trans, > struct btrfs_block_group_cache *cache; > int ret; > > + > btrfs_set_log_full_commit(fs_info, trans); > > cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size); Drop this chunk. > @@ -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; > Reviewed-by: Jeff Mahoney <je...@suse.com> -Jeff -- Jeff Mahoney SUSE Labs
signature.asc
Description: OpenPGP digital signature