Only minor nits On Fri, May 19, 2017 at 10:21:06AM +0300, 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. Also remove BUG_ON from > do_alloc_chunk since the callers handle errors. Furthermore it will > make the update_space_info function not fail, allowing us to remove error > handling in callers. This will come in a follow up patch. > > Reviewed-by: Jeff Mahoney <je...@suse.com> > Reviewed-by: Liu Bo <bo.li....@oracle.com> Missing signed-off-by > --- > fs/btrfs/extent-tree.c | 129 > ++++++++++++++++++++++++------------------------- > 1 file changed, 64 insertions(+), 65 deletions(-) > > > Worked on feedback from Liu Bo and discovered I didn't have vim's linuxtsy > plugin installed, hence the b0rked formatting in some of my patches. > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index be5477676cc8..55b6836f5a20 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) { function openning { on a new line > + > + 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) { > + 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,10 @@ 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); > - BUG_ON(ret); /* -ENOMEM */ > + ret = create_space_info(fs_info, flags, &space_info); > + if (ret) > + return -ENOMEM; create_space_info can possibly return other codes than just ENOMEM, 'return ret' is fine here. Otherwise looks good. -- 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