[PATCH v2 1/2] btrfs: Separate space_info create/update

[PATCH v2 1/2] btrfs: Separate space_info create/update

[Nikolay Borisov]

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 <nborisov@suse.com>
Reviewed-by: Jeff Mahoney <jeffm@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 */
 
 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

[PATCH v2 2/2] btrfs: Refactor update_space_info

[Nikolay Borisov]

Following the factoring out of the creation code udpate_space_info can only
be called for already-existing space_info structs. Remove superfulous error
handling and use the return value to return a pointer to the found space_info.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c | 68 ++++++++++++++++++--------------------------------
 1 file changed, 24 insertions(+), 44 deletions(-)

Change since v1
 - Incorporated Jeff Mahoney's feedback and added his reviewed-by tag

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 28848e45b018..3d5bf0b7f719 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3959,10 +3959,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags,
 	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)
+static struct btrfs_space_info *update_space_info(struct btrfs_fs_info *info,
+												 u64 flags, u64 total_bytes,
+												 u64 bytes_used,
+												 u64 bytes_readonly)
 {
 	struct btrfs_space_info *found;
 	int factor;
@@ -3974,21 +3974,21 @@ static int update_space_info(struct btrfs_fs_info *info, u64 flags,
 		factor = 1;
 
 	found = __find_space_info(info, flags);
-	if (found) {
-		spin_lock(&found->lock);
-		found->total_bytes += total_bytes;
-		found->disk_total += total_bytes * factor;
-		found->bytes_used += bytes_used;
-		found->disk_used += bytes_used * factor;
-		found->bytes_readonly += bytes_readonly;
-		if (total_bytes > 0)
-			found->full = 0;
-		space_info_add_new_bytes(info, found, total_bytes -
-					 bytes_used - bytes_readonly);
-		spin_unlock(&found->lock);
-		*space_info = found;
-		return 0;
-	}
+	BUG_ON(!found);
+
+	spin_lock(&found->lock);
+	found->total_bytes += total_bytes;
+	found->disk_total += total_bytes * factor;
+	found->bytes_used += bytes_used;
+	found->disk_used += bytes_used * factor;
+	found->bytes_readonly += bytes_readonly;
+	if (total_bytes > 0)
+		found->full = 0;
+	space_info_add_new_bytes(info, found, total_bytes -
+				 bytes_used - bytes_readonly);
+	spin_unlock(&found->lock);
+
+	return found;
 }
 
 static void set_avail_alloc_bits(struct btrfs_fs_info *fs_info, u64 flags)
@@ -10042,19 +10042,9 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 		}
 
 		trace_btrfs_add_block_group(info, cache, 0);
-		ret = update_space_info(info, cache->flags, found_key.offset,
-					btrfs_block_group_used(&cache->item),
-					cache->bytes_super, &space_info);
-		if (ret) {
-			btrfs_remove_free_space_cache(cache);
-			spin_lock(&info->block_group_cache_lock);
-			rb_erase(&cache->cache_node,
-				 &info->block_group_cache_tree);
-			RB_CLEAR_NODE(&cache->cache_node);
-			spin_unlock(&info->block_group_cache_lock);
-			btrfs_put_block_group(cache);
-			goto error;
-		}
+		space_info = update_space_info(info, cache->flags, found_key.offset,
+									  btrfs_block_group_used(&cache->item),
+									  cache->bytes_super);
 
 		cache->space_info = space_info;
 
@@ -10212,18 +10202,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
 	 * the rbtree, update the space info's counters.
 	 */
 	trace_btrfs_add_block_group(fs_info, cache, 1);
-	ret = update_space_info(fs_info, cache->flags, size, bytes_used,
-				cache->bytes_super, &cache->space_info);
-	if (ret) {
-		btrfs_remove_free_space_cache(cache);
-		spin_lock(&fs_info->block_group_cache_lock);
-		rb_erase(&cache->cache_node,
-			 &fs_info->block_group_cache_tree);
-		RB_CLEAR_NODE(&cache->cache_node);
-		spin_unlock(&fs_info->block_group_cache_lock);
-		btrfs_put_block_group(cache);
-		return ret;
-	}
+	cache->space_info = update_space_info(fs_info, cache->flags, size,
+										 bytes_used, cache->bytes_super);
 	update_global_block_rsv(fs_info);
 
 	__link_block_group(cache->space_info, cache);
-- 
2.7.4

Re: [PATCH v2 2/2] btrfs: Refactor update_space_info

[Noah Massey]

minor nitpicks in the comment

On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@suse.com> wrote:
> Following the factoring out of the creation code udpate_space_info can only

" code, update_space_info "

> be called for already-existing space_info structs.

", which always succeeds"?

> Remove superfulous error
> handling and use the return value to return a pointer to the found space_info.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reviewed-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 68 ++++++++++++++++++--------------------------------
>  1 file changed, 24 insertions(+), 44 deletions(-)
>
> Change since v1
>  - Incorporated Jeff Mahoney's feedback and added his reviewed-by tag
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 28848e45b018..3d5bf0b7f719 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3959,10 +3959,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags,
>         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)
> +static struct btrfs_space_info *update_space_info(struct btrfs_fs_info *info,
> +                                                                                                u64 flags, u64 total_bytes,
> +                                                                                                u64 bytes_used,
> +                                                                                                u64 bytes_readonly)
>  {
>         struct btrfs_space_info *found;
>         int factor;
> @@ -3974,21 +3974,21 @@ static int update_space_info(struct btrfs_fs_info *info, u64 flags,
>                 factor = 1;
>
>         found = __find_space_info(info, flags);
> -       if (found) {
> -               spin_lock(&found->lock);
> -               found->total_bytes += total_bytes;
> -               found->disk_total += total_bytes * factor;
> -               found->bytes_used += bytes_used;
> -               found->disk_used += bytes_used * factor;
> -               found->bytes_readonly += bytes_readonly;
> -               if (total_bytes > 0)
> -                       found->full = 0;
> -               space_info_add_new_bytes(info, found, total_bytes -
> -                                        bytes_used - bytes_readonly);
> -               spin_unlock(&found->lock);
> -               *space_info = found;
> -               return 0;
> -       }
> +       BUG_ON(!found);
> +
> +       spin_lock(&found->lock);
> +       found->total_bytes += total_bytes;
> +       found->disk_total += total_bytes * factor;
> +       found->bytes_used += bytes_used;
> +       found->disk_used += bytes_used * factor;
> +       found->bytes_readonly += bytes_readonly;
> +       if (total_bytes > 0)
> +               found->full = 0;
> +       space_info_add_new_bytes(info, found, total_bytes -
> +                                bytes_used - bytes_readonly);
> +       spin_unlock(&found->lock);
> +
> +       return found;
>  }
>
>  static void set_avail_alloc_bits(struct btrfs_fs_info *fs_info, u64 flags)
> @@ -10042,19 +10042,9 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>                 }
>
>                 trace_btrfs_add_block_group(info, cache, 0);
> -               ret = update_space_info(info, cache->flags, found_key.offset,
> -                                       btrfs_block_group_used(&cache->item),
> -                                       cache->bytes_super, &space_info);
> -               if (ret) {
> -                       btrfs_remove_free_space_cache(cache);
> -                       spin_lock(&info->block_group_cache_lock);
> -                       rb_erase(&cache->cache_node,
> -                                &info->block_group_cache_tree);
> -                       RB_CLEAR_NODE(&cache->cache_node);
> -                       spin_unlock(&info->block_group_cache_lock);
> -                       btrfs_put_block_group(cache);
> -                       goto error;
> -               }
> +               space_info = update_space_info(info, cache->flags, found_key.offset,
> +                                                                         btrfs_block_group_used(&cache->item),
> +                                                                         cache->bytes_super);
>
>                 cache->space_info = space_info;
>
> @@ -10212,18 +10202,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
>          * the rbtree, update the space info's counters.
>          */
>         trace_btrfs_add_block_group(fs_info, cache, 1);
> -       ret = update_space_info(fs_info, cache->flags, size, bytes_used,
> -                               cache->bytes_super, &cache->space_info);
> -       if (ret) {
> -               btrfs_remove_free_space_cache(cache);
> -               spin_lock(&fs_info->block_group_cache_lock);
> -               rb_erase(&cache->cache_node,
> -                        &fs_info->block_group_cache_tree);
> -               RB_CLEAR_NODE(&cache->cache_node);
> -               spin_unlock(&fs_info->block_group_cache_lock);
> -               btrfs_put_block_group(cache);
> -               return ret;
> -       }
> +       cache->space_info = update_space_info(fs_info, cache->flags, size,
> +                                                                                bytes_used, cache->bytes_super);
>         update_global_block_rsv(fs_info);
>
>         __link_block_group(cache->space_info, cache);
> --
> 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

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Noah Massey]

On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@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 <nborisov@suse.com>
> Reviewed-by: Jeff Mahoney <jeffm@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

>
>  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

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Nikolay Borisov]

On 17.05.2017 21:57, Noah Massey wrote:
> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@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 <nborisov@suse.com>
>> Reviewed-by: Jeff Mahoney <jeffm@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.

> 
>>
>>  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
> 

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Noah Massey]

On Wed, May 17, 2017 at 4:34 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 17.05.2017 21:57, Noah Massey wrote:
>> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@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 <nborisov@suse.com>
>>> Reviewed-by: Jeff Mahoney <jeffm@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

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 8432 bytes --]

On 5/17/17 4:52 PM, Noah Massey wrote:
> On Wed, May 17, 2017 at 4:34 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>
>> On 17.05.2017 21:57, Noah Massey wrote:
>>> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@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 <nborisov@suse.com>
>>>> Reviewed-by: Jeff Mahoney <jeffm@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.

The BUG_ON(!space_info) is older than the call to update_space_info().
It should've been removed when we started creating the space info there.

-Jeff

> 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?



-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Noah Massey]

On Wed, May 17, 2017 at 4:55 PM, Jeff Mahoney <jeffm@suse.com> wrote:
> On 5/17/17 4:52 PM, Noah Massey wrote:
>> On Wed, May 17, 2017 at 4:34 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>>
>>>
>>> On 17.05.2017 21:57, Noah Massey wrote:
>>>> On Wed, May 17, 2017 at 11:07 AM, Nikolay Borisov <nborisov@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 <nborisov@suse.com>
>>>>> Reviewed-by: Jeff Mahoney <jeffm@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.
>
> The BUG_ON(!space_info) is older than the call to update_space_info().
> It should've been removed when we started creating the space info there.
>
> -Jeff

Understood.
Thank you for your patience.

~ Noah

Re: [PATCH v2 1/2] btrfs: Separate space_info create/update

[Liu Bo]

On Wed, May 17, 2017 at 06:07:42PM +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. 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@suse.com>
> Reviewed-by: Jeff Mahoney <jeffm@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);

over 80.

> +	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 */

While we're here, can we drop the BUG_ON?  (a separate patch is fine)

>  	}
> -	BUG_ON(!space_info); /* Logic error */
>  
>  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),

whitespace?

>  			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);

over 80, please trim it.

Others look good.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo

> +		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

Re: [PATCH v2 2/2] btrfs: Refactor update_space_info

[Liu Bo]

On Wed, May 17, 2017 at 06:07:43PM +0300, Nikolay Borisov wrote:
> Following the factoring out of the creation code udpate_space_info can only
> be called for already-existing space_info structs. Remove superfulous error
> handling and use the return value to return a pointer to the found space_info.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reviewed-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 68 ++++++++++++++++++--------------------------------
>  1 file changed, 24 insertions(+), 44 deletions(-)
> 
> Change since v1
>  - Incorporated Jeff Mahoney's feedback and added his reviewed-by tag
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 28848e45b018..3d5bf0b7f719 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3959,10 +3959,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags,
>  	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)
> +static struct btrfs_space_info *update_space_info(struct btrfs_fs_info *info,
> +												 u64 flags, u64 total_bytes,
> +												 u64 bytes_used,
> +												 u64 bytes_readonly)

The format shown here is not indented correctly.  Please check the tool you
generated the patch or the email client.

I remember that your previous patch set about converting struct inode to struct
btrfs_inode also breaks several indent in code.

>  {
>  	struct btrfs_space_info *found;
>  	int factor;
> @@ -3974,21 +3974,21 @@ static int update_space_info(struct btrfs_fs_info *info, u64 flags,
>  		factor = 1;
>  
>  	found = __find_space_info(info, flags);
> -	if (found) {
> -		spin_lock(&found->lock);
> -		found->total_bytes += total_bytes;
> -		found->disk_total += total_bytes * factor;
> -		found->bytes_used += bytes_used;
> -		found->disk_used += bytes_used * factor;
> -		found->bytes_readonly += bytes_readonly;
> -		if (total_bytes > 0)
> -			found->full = 0;
> -		space_info_add_new_bytes(info, found, total_bytes -
> -					 bytes_used - bytes_readonly);
> -		spin_unlock(&found->lock);
> -		*space_info = found;
> -		return 0;
> -	}
> +	BUG_ON(!found);

No more BUG_ON(), try ASSERT for debugging purpose.

> +
> +	spin_lock(&found->lock);
> +	found->total_bytes += total_bytes;
> +	found->disk_total += total_bytes * factor;
> +	found->bytes_used += bytes_used;
> +	found->disk_used += bytes_used * factor;
> +	found->bytes_readonly += bytes_readonly;
> +	if (total_bytes > 0)
> +		found->full = 0;
> +	space_info_add_new_bytes(info, found, total_bytes -
> +				 bytes_used - bytes_readonly);
> +	spin_unlock(&found->lock);
> +
> +	return found;
>  }
>  
>  static void set_avail_alloc_bits(struct btrfs_fs_info *fs_info, u64 flags)
> @@ -10042,19 +10042,9 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>  		}
>  
>  		trace_btrfs_add_block_group(info, cache, 0);
> -		ret = update_space_info(info, cache->flags, found_key.offset,
> -					btrfs_block_group_used(&cache->item),
> -					cache->bytes_super, &space_info);
> -		if (ret) {
> -			btrfs_remove_free_space_cache(cache);
> -			spin_lock(&info->block_group_cache_lock);
> -			rb_erase(&cache->cache_node,
> -				 &info->block_group_cache_tree);
> -			RB_CLEAR_NODE(&cache->cache_node);
> -			spin_unlock(&info->block_group_cache_lock);
> -			btrfs_put_block_group(cache);
> -			goto error;
> -		}
> +		space_info = update_space_info(info, cache->flags, found_key.offset,
> +									  btrfs_block_group_used(&cache->item),
> +									  cache->bytes_super);
>

Same indent issue.

>  		cache->space_info = space_info;
>  
> @@ -10212,18 +10202,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
>  	 * the rbtree, update the space info's counters.
>  	 */
>  	trace_btrfs_add_block_group(fs_info, cache, 1);
> -	ret = update_space_info(fs_info, cache->flags, size, bytes_used,
> -				cache->bytes_super, &cache->space_info);
> -	if (ret) {
> -		btrfs_remove_free_space_cache(cache);
> -		spin_lock(&fs_info->block_group_cache_lock);
> -		rb_erase(&cache->cache_node,
> -			 &fs_info->block_group_cache_tree);
> -		RB_CLEAR_NODE(&cache->cache_node);
> -		spin_unlock(&fs_info->block_group_cache_lock);
> -		btrfs_put_block_group(cache);
> -		return ret;
> -	}
> +	cache->space_info = update_space_info(fs_info, cache->flags, size,
> +										 bytes_used, cache->bytes_super);

Same indent issue.

Thanks,

-liubo
>  	update_global_block_rsv(fs_info);
>  
>  	__link_block_group(cache->space_info, cache);
> -- 
> 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