[PATCH 1/3] btrfs: Refactor __get_raid_index() to btrfs_bg_flags_to_raid_index()

[PATCH 1/3] btrfs: Refactor __get_raid_index() to btrfs_bg_flags_to_raid_index()

[Qu Wenruo]

Function __get_raid_index() is used to convert block group flags into
raid index, which can be used to get various info directly from
btrfs_raid_array[].

Refactor this function a little:

1) Rename to btrfs_bg_flags_to_raid_index()
   Double underline prefix is normally for internal functions, while the
   function is used by both extent-tree and volumes.

   Although the name is a little longer, but it should explain its usage
   quite well.

2) Move it to volumes.h and make it static inline
   Just several if-else branches, really no need to define it as a normal
   function.

   This also makes later code re-use between kernel and btrfs-progs
   easier.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 26 ++++----------------------
 fs/btrfs/volumes.c     |  2 +-
 fs/btrfs/volumes.h     | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 2f4328511ac8..e9c31b567a9c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7346,27 +7346,9 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
 	return ret;
 }
 
-int __get_raid_index(u64 flags)
-{
-	if (flags & BTRFS_BLOCK_GROUP_RAID10)
-		return BTRFS_RAID_RAID10;
-	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
-		return BTRFS_RAID_RAID1;
-	else if (flags & BTRFS_BLOCK_GROUP_DUP)
-		return BTRFS_RAID_DUP;
-	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
-		return BTRFS_RAID_RAID0;
-	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
-		return BTRFS_RAID_RAID5;
-	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
-		return BTRFS_RAID_RAID6;
-
-	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
-}
-
 int get_block_group_index(struct btrfs_block_group_cache *cache)
 {
-	return __get_raid_index(cache->flags);
+	return btrfs_bg_flags_to_raid_index(cache->flags);
 }
 
 static const char *btrfs_raid_type_names[BTRFS_NR_RAID_TYPES] = {
@@ -7483,7 +7465,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
 	u64 empty_cluster = 0;
 	struct btrfs_space_info *space_info;
 	int loop = 0;
-	int index = __get_raid_index(flags);
+	int index = btrfs_bg_flags_to_raid_index(flags);
 	bool failed_cluster_refill = false;
 	bool failed_alloc = false;
 	bool use_cluster = true;
@@ -7579,7 +7561,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
 	}
 search:
 	have_caching_bg = false;
-	if (index == 0 || index == __get_raid_index(flags))
+	if (index == 0 || index == btrfs_bg_flags_to_raid_index(flags))
 		full_search = true;
 	down_read(&space_info->groups_sem);
 	list_for_each_entry(block_group, &space_info->block_groups[index],
@@ -9643,7 +9625,7 @@ int btrfs_can_relocate(struct btrfs_fs_info *fs_info, u64 bytenr)
 	 */
 	target = get_restripe_target(fs_info, block_group->flags);
 	if (target) {
-		index = __get_raid_index(extended_to_chunk(target));
+		index = btrfs_bg_flags_to_raid_index(extended_to_chunk(target));
 	} else {
 		/*
 		 * this is just a balance, so if we were marked as full
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a25684287501..d818b1f9c625 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4625,7 +4625,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	if (list_empty(&fs_devices->alloc_list))
 		return -ENOSPC;
 
-	index = __get_raid_index(type);
+	index = btrfs_bg_flags_to_raid_index(type);
 
 	sub_stripes = btrfs_raid_array[index].sub_stripes;
 	dev_stripes = btrfs_raid_array[index].dev_stripes;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index ff15208344a7..413e07e44b42 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -533,6 +533,24 @@ static inline void btrfs_dev_stat_reset(struct btrfs_device *dev,
 	btrfs_dev_stat_set(dev, index, 0);
 }
 
+static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
+{
+	if (flags & BTRFS_BLOCK_GROUP_RAID10)
+		return BTRFS_RAID_RAID10;
+	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
+		return BTRFS_RAID_RAID1;
+	else if (flags & BTRFS_BLOCK_GROUP_DUP)
+		return BTRFS_RAID_DUP;
+	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
+		return BTRFS_RAID_RAID0;
+	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
+		return BTRFS_RAID_RAID5;
+	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
+		return BTRFS_RAID_RAID6;
+
+	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
+}
+
 void btrfs_update_commit_device_size(struct btrfs_fs_info *fs_info);
 void btrfs_update_commit_device_bytes_used(struct btrfs_fs_info *fs_info,
 					struct btrfs_transaction *transaction);
-- 
2.16.1

[PATCH 2/3] btrfs: Unexport get_block_group_index()

[Qu Wenruo]

That function is only used inside extent-tree.c.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h       | 1 -
 fs/btrfs/extent-tree.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 13c260b525a1..27249240fa3e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2629,7 +2629,6 @@ struct btrfs_block_group_cache *btrfs_lookup_block_group(
 						 u64 bytenr);
 void btrfs_get_block_group(struct btrfs_block_group_cache *cache);
 void btrfs_put_block_group(struct btrfs_block_group_cache *cache);
-int get_block_group_index(struct btrfs_block_group_cache *cache);
 struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
 					     struct btrfs_root *root,
 					     u64 parent, u64 root_objectid,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e9c31b567a9c..6e1128aa29d6 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7346,7 +7346,8 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
 	return ret;
 }
 
-int get_block_group_index(struct btrfs_block_group_cache *cache)
+static enum btrfs_raid_types
+get_block_group_index(struct btrfs_block_group_cache *cache)
 {
 	return btrfs_bg_flags_to_raid_index(cache->flags);
 }
-- 
2.16.1

[PATCH 3/3] btrfs: Refactor parameter of BTRFS_MAX_DEVS() from root to fs_info

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d818b1f9c625..215e85e22c8e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4581,7 +4581,7 @@ static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type)
 	btrfs_set_fs_incompat(info, RAID56);
 }
 
-#define BTRFS_MAX_DEVS(r) ((BTRFS_MAX_ITEM_SIZE(r->fs_info)		\
+#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
 			- sizeof(struct btrfs_chunk))		\
 			/ sizeof(struct btrfs_stripe) + 1)
 
@@ -4638,7 +4638,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		max_stripe_size = SZ_1G;
 		max_chunk_size = 10 * max_stripe_size;
 		if (!devs_max)
-			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
+			devs_max = BTRFS_MAX_DEVS(info);
 	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
 		/* for larger filesystems, use larger metadata chunks */
 		if (fs_devices->total_rw_bytes > 50ULL * SZ_1G)
@@ -4647,7 +4647,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 			max_stripe_size = SZ_256M;
 		max_chunk_size = max_stripe_size;
 		if (!devs_max)
-			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
+			devs_max = BTRFS_MAX_DEVS(info);
 	} else if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
 		max_stripe_size = SZ_32M;
 		max_chunk_size = 2 * max_stripe_size;
-- 
2.16.1

Re: [PATCH 1/3] btrfs: Refactor __get_raid_index() to btrfs_bg_flags_to_raid_index()

[Anand Jain]

On 01/30/2018 03:40 PM, Qu Wenruo wrote:
> Function __get_raid_index() is used to convert block group flags into
> raid index, which can be used to get various info directly from
> btrfs_raid_array[].
> 
> Refactor this function a little:
> 
> 1) Rename to btrfs_bg_flags_to_raid_index()
>     Double underline prefix is normally for internal functions, while the
>     function is used by both extent-tree and volumes.
> 
>     Although the name is a little longer, but it should explain its usage
>     quite well.
> 
> 2) Move it to volumes.h and make it static inline
>     Just several if-else branches, really no need to define it as a normal
>     function.
> 
>     This also makes later code re-use between kernel and btrfs-progs
>     easier.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand


> ---
>   fs/btrfs/extent-tree.c | 26 ++++----------------------
>   fs/btrfs/volumes.c     |  2 +-
>   fs/btrfs/volumes.h     | 18 ++++++++++++++++++
>   3 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 2f4328511ac8..e9c31b567a9c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7346,27 +7346,9 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>   	return ret;
>   }
>   
> -int __get_raid_index(u64 flags)
> -{
> -	if (flags & BTRFS_BLOCK_GROUP_RAID10)
> -		return BTRFS_RAID_RAID10;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> -		return BTRFS_RAID_RAID1;
> -	else if (flags & BTRFS_BLOCK_GROUP_DUP)
> -		return BTRFS_RAID_DUP;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> -		return BTRFS_RAID_RAID0;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> -		return BTRFS_RAID_RAID5;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> -		return BTRFS_RAID_RAID6;
> -
> -	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> -}
> -
>   int get_block_group_index(struct btrfs_block_group_cache *cache)
>   {
> -	return __get_raid_index(cache->flags);
> +	return btrfs_bg_flags_to_raid_index(cache->flags);
>   }
>   
>   static const char *btrfs_raid_type_names[BTRFS_NR_RAID_TYPES] = {
> @@ -7483,7 +7465,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>   	u64 empty_cluster = 0;
>   	struct btrfs_space_info *space_info;
>   	int loop = 0;
> -	int index = __get_raid_index(flags);
> +	int index = btrfs_bg_flags_to_raid_index(flags);
>   	bool failed_cluster_refill = false;
>   	bool failed_alloc = false;
>   	bool use_cluster = true;
> @@ -7579,7 +7561,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>   	}
>   search:
>   	have_caching_bg = false;
> -	if (index == 0 || index == __get_raid_index(flags))
> +	if (index == 0 || index == btrfs_bg_flags_to_raid_index(flags))
>   		full_search = true;
>   	down_read(&space_info->groups_sem);
>   	list_for_each_entry(block_group, &space_info->block_groups[index],
> @@ -9643,7 +9625,7 @@ int btrfs_can_relocate(struct btrfs_fs_info *fs_info, u64 bytenr)
>   	 */
>   	target = get_restripe_target(fs_info, block_group->flags);
>   	if (target) {
> -		index = __get_raid_index(extended_to_chunk(target));
> +		index = btrfs_bg_flags_to_raid_index(extended_to_chunk(target));
>   	} else {
>   		/*
>   		 * this is just a balance, so if we were marked as full
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a25684287501..d818b1f9c625 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4625,7 +4625,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>   	if (list_empty(&fs_devices->alloc_list))
>   		return -ENOSPC;
>   
> -	index = __get_raid_index(type);
> +	index = btrfs_bg_flags_to_raid_index(type);
>   
>   	sub_stripes = btrfs_raid_array[index].sub_stripes;
>   	dev_stripes = btrfs_raid_array[index].dev_stripes;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index ff15208344a7..413e07e44b42 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -533,6 +533,24 @@ static inline void btrfs_dev_stat_reset(struct btrfs_device *dev,
>   	btrfs_dev_stat_set(dev, index, 0);
>   }
>   
> +static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
> +{
> +	if (flags & BTRFS_BLOCK_GROUP_RAID10)
> +		return BTRFS_RAID_RAID10;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> +		return BTRFS_RAID_RAID1;
> +	else if (flags & BTRFS_BLOCK_GROUP_DUP)
> +		return BTRFS_RAID_DUP;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> +		return BTRFS_RAID_RAID0;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> +		return BTRFS_RAID_RAID5;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> +		return BTRFS_RAID_RAID6;
> +
> +	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> +}
> +
>   void btrfs_update_commit_device_size(struct btrfs_fs_info *fs_info);
>   void btrfs_update_commit_device_bytes_used(struct btrfs_fs_info *fs_info,
>   					struct btrfs_transaction *transaction);
> 

Re: [PATCH 2/3] btrfs: Unexport get_block_group_index()

[Anand Jain]

> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index e9c31b567a9c..6e1128aa29d6 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7346,7 +7346,8 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>   	return ret;
>   }
>   
> -int get_block_group_index(struct btrfs_block_group_cache *cache)
> +static enum btrfs_raid_types
> +get_block_group_index(struct btrfs_block_group_cache *cache)
>   {
>   	return btrfs_bg_flags_to_raid_index(cache->flags);
>   }

  This changed should rather be part of 1/3.

Thanks, Anand

Re: [PATCH 1/3] btrfs: Refactor __get_raid_index() to btrfs_bg_flags_to_raid_index()

[Nikolay Borisov]

On 30.01.2018 09:40, Qu Wenruo wrote:
> Function __get_raid_index() is used to convert block group flags into
> raid index, which can be used to get various info directly from
> btrfs_raid_array[].
> 
> Refactor this function a little:
> 
> 1) Rename to btrfs_bg_flags_to_raid_index()
>    Double underline prefix is normally for internal functions, while the
>    function is used by both extent-tree and volumes.
> 
>    Although the name is a little longer, but it should explain its usage
>    quite well.
> 
> 2) Move it to volumes.h and make it static inline
>    Just several if-else branches, really no need to define it as a normal
>    function.
> 
>    This also makes later code re-use between kernel and btrfs-progs
>    easier.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Generally this looks good so you can add my :

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

However I wonder whether we should strive to add proper kernel docs to
function whenever we can (without going out of the way to do so). I.e
you are modifying the function now so might as well add them and
explicitly state it's used to convert blockgroup flags to raid levels.
The end goal is to make it easier for people who are looking for the
first time at the source to have easier time.

> ---
>  fs/btrfs/extent-tree.c | 26 ++++----------------------
>  fs/btrfs/volumes.c     |  2 +-
>  fs/btrfs/volumes.h     | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 2f4328511ac8..e9c31b567a9c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7346,27 +7346,9 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>  	return ret;
>  }
>  
> -int __get_raid_index(u64 flags)
> -{
> -	if (flags & BTRFS_BLOCK_GROUP_RAID10)
> -		return BTRFS_RAID_RAID10;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> -		return BTRFS_RAID_RAID1;
> -	else if (flags & BTRFS_BLOCK_GROUP_DUP)
> -		return BTRFS_RAID_DUP;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> -		return BTRFS_RAID_RAID0;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> -		return BTRFS_RAID_RAID5;
> -	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> -		return BTRFS_RAID_RAID6;
> -
> -	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> -}
> -
>  int get_block_group_index(struct btrfs_block_group_cache *cache)
>  {
> -	return __get_raid_index(cache->flags);
> +	return btrfs_bg_flags_to_raid_index(cache->flags);
>  }
>  
>  static const char *btrfs_raid_type_names[BTRFS_NR_RAID_TYPES] = {
> @@ -7483,7 +7465,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>  	u64 empty_cluster = 0;
>  	struct btrfs_space_info *space_info;
>  	int loop = 0;
> -	int index = __get_raid_index(flags);
> +	int index = btrfs_bg_flags_to_raid_index(flags);
>  	bool failed_cluster_refill = false;
>  	bool failed_alloc = false;
>  	bool use_cluster = true;
> @@ -7579,7 +7561,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>  	}
>  search:
>  	have_caching_bg = false;
> -	if (index == 0 || index == __get_raid_index(flags))
> +	if (index == 0 || index == btrfs_bg_flags_to_raid_index(flags))
>  		full_search = true;
>  	down_read(&space_info->groups_sem);
>  	list_for_each_entry(block_group, &space_info->block_groups[index],
> @@ -9643,7 +9625,7 @@ int btrfs_can_relocate(struct btrfs_fs_info *fs_info, u64 bytenr)
>  	 */
>  	target = get_restripe_target(fs_info, block_group->flags);
>  	if (target) {
> -		index = __get_raid_index(extended_to_chunk(target));
> +		index = btrfs_bg_flags_to_raid_index(extended_to_chunk(target));
>  	} else {
>  		/*
>  		 * this is just a balance, so if we were marked as full
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a25684287501..d818b1f9c625 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4625,7 +4625,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  	if (list_empty(&fs_devices->alloc_list))
>  		return -ENOSPC;
>  
> -	index = __get_raid_index(type);
> +	index = btrfs_bg_flags_to_raid_index(type);
>  
>  	sub_stripes = btrfs_raid_array[index].sub_stripes;
>  	dev_stripes = btrfs_raid_array[index].dev_stripes;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index ff15208344a7..413e07e44b42 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -533,6 +533,24 @@ static inline void btrfs_dev_stat_reset(struct btrfs_device *dev,
>  	btrfs_dev_stat_set(dev, index, 0);
>  }
>  
> +static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
> +{
> +	if (flags & BTRFS_BLOCK_GROUP_RAID10)
> +		return BTRFS_RAID_RAID10;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
> +		return BTRFS_RAID_RAID1;
> +	else if (flags & BTRFS_BLOCK_GROUP_DUP)
> +		return BTRFS_RAID_DUP;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
> +		return BTRFS_RAID_RAID0;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
> +		return BTRFS_RAID_RAID5;
> +	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
> +		return BTRFS_RAID_RAID6;
> +
> +	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
> +}
> +
>  void btrfs_update_commit_device_size(struct btrfs_fs_info *fs_info);
>  void btrfs_update_commit_device_bytes_used(struct btrfs_fs_info *fs_info,
>  					struct btrfs_transaction *transaction);
> 

Re: [PATCH 2/3] btrfs: Unexport get_block_group_index()

[Nikolay Borisov]

On 30.01.2018 09:40, Qu Wenruo wrote:
> That function is only used inside extent-tree.c.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ctree.h       | 1 -
>  fs/btrfs/extent-tree.c | 3 ++-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 13c260b525a1..27249240fa3e 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2629,7 +2629,6 @@ struct btrfs_block_group_cache *btrfs_lookup_block_group(
>  						 u64 bytenr);
>  void btrfs_get_block_group(struct btrfs_block_group_cache *cache);
>  void btrfs_put_block_group(struct btrfs_block_group_cache *cache);
> -int get_block_group_index(struct btrfs_block_group_cache *cache);
>  struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
>  					     struct btrfs_root *root,
>  					     u64 parent, u64 root_objectid,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index e9c31b567a9c..6e1128aa29d6 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7346,7 +7346,8 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>  	return ret;
>  }
>  
> -int get_block_group_index(struct btrfs_block_group_cache *cache)
> +static enum btrfs_raid_types
> +get_block_group_index(struct btrfs_block_group_cache *cache)
>  {
>  	return btrfs_bg_flags_to_raid_index(cache->flags);
>  }

I'd rather you removed this function and used bg_flags_to_raid_index
directly. Otherwise we have yet another level of indirection with a
rather wrong name - "block_group_index" ?

> 

Re: [PATCH 3/3] btrfs: Refactor parameter of BTRFS_MAX_DEVS() from root to fs_info

[Anand Jain]

On 01/30/2018 03:40 PM, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/volumes.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d818b1f9c625..215e85e22c8e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4581,7 +4581,7 @@ static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type)
>   	btrfs_set_fs_incompat(info, RAID56);
>   }
>   
> -#define BTRFS_MAX_DEVS(r) ((BTRFS_MAX_ITEM_SIZE(r->fs_info)		\
> +#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
>   			- sizeof(struct btrfs_chunk))		\
>   			/ sizeof(struct btrfs_stripe) + 1)
>   
> @@ -4638,7 +4638,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>   		max_stripe_size = SZ_1G;
>   		max_chunk_size = 10 * max_stripe_size;
>   		if (!devs_max)
> -			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
> +			devs_max = BTRFS_MAX_DEVS(info);
>   	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
>   		/* for larger filesystems, use larger metadata chunks */
>   		if (fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> @@ -4647,7 +4647,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>   			max_stripe_size = SZ_256M;
>   		max_chunk_size = max_stripe_size;
>   		if (!devs_max)
> -			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
> +			devs_max = BTRFS_MAX_DEVS(info);
>   	} else if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
>   		max_stripe_size = SZ_32M;
>   		max_chunk_size = 2 * max_stripe_size;
> 

Re: [PATCH 3/3] btrfs: Refactor parameter of BTRFS_MAX_DEVS() from root to fs_info

[Nikolay Borisov]

On 30.01.2018 09:40, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/volumes.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d818b1f9c625..215e85e22c8e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4581,7 +4581,7 @@ static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type)
>  	btrfs_set_fs_incompat(info, RAID56);
>  }
>  
> -#define BTRFS_MAX_DEVS(r) ((BTRFS_MAX_ITEM_SIZE(r->fs_info)		\
> +#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
>  			- sizeof(struct btrfs_chunk))		\
>  			/ sizeof(struct btrfs_stripe) + 1)
>  
> @@ -4638,7 +4638,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  		max_stripe_size = SZ_1G;
>  		max_chunk_size = 10 * max_stripe_size;
>  		if (!devs_max)
> -			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
> +			devs_max = BTRFS_MAX_DEVS(info);
>  	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
>  		/* for larger filesystems, use larger metadata chunks */
>  		if (fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> @@ -4647,7 +4647,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  			max_stripe_size = SZ_256M;
>  		max_chunk_size = max_stripe_size;
>  		if (!devs_max)
> -			devs_max = BTRFS_MAX_DEVS(info->chunk_root);
> +			devs_max = BTRFS_MAX_DEVS(info);
>  	} else if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
>  		max_stripe_size = SZ_32M;
>  		max_chunk_size = 2 * max_stripe_size;
> 

Re: [PATCH 2/3] btrfs: Unexport get_block_group_index()

[Qu Wenruo]

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



On 2018年01月30日 16:18, Nikolay Borisov wrote:
> 
> 
> On 30.01.2018 09:40, Qu Wenruo wrote:
>> That function is only used inside extent-tree.c.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/ctree.h       | 1 -
>>  fs/btrfs/extent-tree.c | 3 ++-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 13c260b525a1..27249240fa3e 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -2629,7 +2629,6 @@ struct btrfs_block_group_cache *btrfs_lookup_block_group(
>>  						 u64 bytenr);
>>  void btrfs_get_block_group(struct btrfs_block_group_cache *cache);
>>  void btrfs_put_block_group(struct btrfs_block_group_cache *cache);
>> -int get_block_group_index(struct btrfs_block_group_cache *cache);
>>  struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
>>  					     struct btrfs_root *root,
>>  					     u64 parent, u64 root_objectid,
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index e9c31b567a9c..6e1128aa29d6 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -7346,7 +7346,8 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>>  	return ret;
>>  }
>>  
>> -int get_block_group_index(struct btrfs_block_group_cache *cache)
>> +static enum btrfs_raid_types
>> +get_block_group_index(struct btrfs_block_group_cache *cache)
>>  {
>>  	return btrfs_bg_flags_to_raid_index(cache->flags);
>>  }
> 
> I'd rather you removed this function and used bg_flags_to_raid_index
> directly. Otherwise we have yet another level of indirection with a
> rather wrong name - "block_group_index" ?

Right, also pointed out by Anand, I'll just remove it.

Thanks,
Qu

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


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

Re: [PATCH 1/3] btrfs: Refactor __get_raid_index() to btrfs_bg_flags_to_raid_index()

[Qu Wenruo]

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



On 2018年01月30日 16:16, Nikolay Borisov wrote:
> 
> 
> On 30.01.2018 09:40, Qu Wenruo wrote:
>> Function __get_raid_index() is used to convert block group flags into
>> raid index, which can be used to get various info directly from
>> btrfs_raid_array[].
>>
>> Refactor this function a little:
>>
>> 1) Rename to btrfs_bg_flags_to_raid_index()
>>    Double underline prefix is normally for internal functions, while the
>>    function is used by both extent-tree and volumes.
>>
>>    Although the name is a little longer, but it should explain its usage
>>    quite well.
>>
>> 2) Move it to volumes.h and make it static inline
>>    Just several if-else branches, really no need to define it as a normal
>>    function.
>>
>>    This also makes later code re-use between kernel and btrfs-progs
>>    easier.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Generally this looks good so you can add my :
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> However I wonder whether we should strive to add proper kernel docs to
> function whenever we can (without going out of the way to do so).

Kernel docs here seems a little over killed here.
IMHO kernel docs are used for things like API, state machine or design
overview.

Here the raid type index is mostly internal used as a quicker and
unified way to access btrfs_raid_array[].

Extra comment (maybe 1 or 2 lines) will be added in next version about
the usage, and hopes it should work.

> I.e
> you are modifying the function now so might as well add them and
> explicitly state it's used to convert blockgroup flags to raid levels.
> The end goal is to make it easier for people who are looking for the
> first time at the source to have easier time.

Yep, making the source easier to read is always good.

Thanks,
Qu

> 
>> ---
>>  fs/btrfs/extent-tree.c | 26 ++++----------------------
>>  fs/btrfs/volumes.c     |  2 +-
>>  fs/btrfs/volumes.h     | 18 ++++++++++++++++++
>>  3 files changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 2f4328511ac8..e9c31b567a9c 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -7346,27 +7346,9 @@ wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
>>  	return ret;
>>  }
>>  
>> -int __get_raid_index(u64 flags)
>> -{
>> -	if (flags & BTRFS_BLOCK_GROUP_RAID10)
>> -		return BTRFS_RAID_RAID10;
>> -	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
>> -		return BTRFS_RAID_RAID1;
>> -	else if (flags & BTRFS_BLOCK_GROUP_DUP)
>> -		return BTRFS_RAID_DUP;
>> -	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
>> -		return BTRFS_RAID_RAID0;
>> -	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
>> -		return BTRFS_RAID_RAID5;
>> -	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
>> -		return BTRFS_RAID_RAID6;
>> -
>> -	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
>> -}
>> -
>>  int get_block_group_index(struct btrfs_block_group_cache *cache)
>>  {
>> -	return __get_raid_index(cache->flags);
>> +	return btrfs_bg_flags_to_raid_index(cache->flags);
>>  }
>>  
>>  static const char *btrfs_raid_type_names[BTRFS_NR_RAID_TYPES] = {
>> @@ -7483,7 +7465,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>>  	u64 empty_cluster = 0;
>>  	struct btrfs_space_info *space_info;
>>  	int loop = 0;
>> -	int index = __get_raid_index(flags);
>> +	int index = btrfs_bg_flags_to_raid_index(flags);
>>  	bool failed_cluster_refill = false;
>>  	bool failed_alloc = false;
>>  	bool use_cluster = true;
>> @@ -7579,7 +7561,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>>  	}
>>  search:
>>  	have_caching_bg = false;
>> -	if (index == 0 || index == __get_raid_index(flags))
>> +	if (index == 0 || index == btrfs_bg_flags_to_raid_index(flags))
>>  		full_search = true;
>>  	down_read(&space_info->groups_sem);
>>  	list_for_each_entry(block_group, &space_info->block_groups[index],
>> @@ -9643,7 +9625,7 @@ int btrfs_can_relocate(struct btrfs_fs_info *fs_info, u64 bytenr)
>>  	 */
>>  	target = get_restripe_target(fs_info, block_group->flags);
>>  	if (target) {
>> -		index = __get_raid_index(extended_to_chunk(target));
>> +		index = btrfs_bg_flags_to_raid_index(extended_to_chunk(target));
>>  	} else {
>>  		/*
>>  		 * this is just a balance, so if we were marked as full
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index a25684287501..d818b1f9c625 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4625,7 +4625,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  	if (list_empty(&fs_devices->alloc_list))
>>  		return -ENOSPC;
>>  
>> -	index = __get_raid_index(type);
>> +	index = btrfs_bg_flags_to_raid_index(type);
>>  
>>  	sub_stripes = btrfs_raid_array[index].sub_stripes;
>>  	dev_stripes = btrfs_raid_array[index].dev_stripes;
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index ff15208344a7..413e07e44b42 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -533,6 +533,24 @@ static inline void btrfs_dev_stat_reset(struct btrfs_device *dev,
>>  	btrfs_dev_stat_set(dev, index, 0);
>>  }
>>  
>> +static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
>> +{
>> +	if (flags & BTRFS_BLOCK_GROUP_RAID10)
>> +		return BTRFS_RAID_RAID10;
>> +	else if (flags & BTRFS_BLOCK_GROUP_RAID1)
>> +		return BTRFS_RAID_RAID1;
>> +	else if (flags & BTRFS_BLOCK_GROUP_DUP)
>> +		return BTRFS_RAID_DUP;
>> +	else if (flags & BTRFS_BLOCK_GROUP_RAID0)
>> +		return BTRFS_RAID_RAID0;
>> +	else if (flags & BTRFS_BLOCK_GROUP_RAID5)
>> +		return BTRFS_RAID_RAID5;
>> +	else if (flags & BTRFS_BLOCK_GROUP_RAID6)
>> +		return BTRFS_RAID_RAID6;
>> +
>> +	return BTRFS_RAID_SINGLE; /* BTRFS_BLOCK_GROUP_SINGLE */
>> +}
>> +
>>  void btrfs_update_commit_device_size(struct btrfs_fs_info *fs_info);
>>  void btrfs_update_commit_device_bytes_used(struct btrfs_fs_info *fs_info,
>>  					struct btrfs_transaction *transaction);
>>
> --
> 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
> 


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