[PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[Nikolay Borisov]

The user facing function used to allocate new chunks is
btrfs_chunk_alloc, unfortunately there is yet another similar sounding
function - btrfs_alloc_chunk. This creates confusion, especially since
the latter function can be considered "private" in the sense that it
implements the first stage of chunk creation and as such is called by
btrfs_chunk_alloc.

To avoid the awkwardness that comes with having similarly named but
distinctly different in their purpose function rename btrfs_alloc_chunk
to btrfs_create_chunk, given that the main purpose of this function is
to orchestrate the whole process of allocating a chunk - reserving space
into devices, deciding on characteristics of the stripe size and
creating the in-memory structures.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V2:
 * Fix changlog to reflect reality

 fs/btrfs/block-group.c | 6 +++---
 fs/btrfs/volumes.c     | 8 ++++----
 fs/btrfs/volumes.h     | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 5c2361168363..500a85e4320f 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -3358,7 +3358,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
 	 */
 	check_system_chunk(trans, flags);

-	bg = btrfs_alloc_chunk(trans, flags);
+	bg = btrfs_create_chunk(trans, flags);
 	if (IS_ERR(bg)) {
 		ret = PTR_ERR(bg);
 		goto out;
@@ -3419,7 +3419,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
 		const u64 sys_flags = btrfs_system_alloc_profile(trans->fs_info);
 		struct btrfs_block_group *sys_bg;

-		sys_bg = btrfs_alloc_chunk(trans, sys_flags);
+		sys_bg = btrfs_create_chunk(trans, sys_flags);
 		if (IS_ERR(sys_bg)) {
 			ret = PTR_ERR(sys_bg);
 			btrfs_abort_transaction(trans, ret);
@@ -3712,7 +3712,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
 		 * could deadlock on an extent buffer since our caller may be
 		 * COWing an extent buffer from the chunk btree.
 		 */
-		bg = btrfs_alloc_chunk(trans, flags);
+		bg = btrfs_create_chunk(trans, flags);
 		if (IS_ERR(bg)) {
 			ret = PTR_ERR(bg);
 		} else if (!(type & BTRFS_BLOCK_GROUP_SYSTEM)) {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f820c32f4a0d..4f84b5d871dd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3086,7 +3086,7 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 		const u64 sys_flags = btrfs_system_alloc_profile(fs_info);
 		struct btrfs_block_group *sys_bg;

-		sys_bg = btrfs_alloc_chunk(trans, sys_flags);
+		sys_bg = btrfs_create_chunk(trans, sys_flags);
 		if (IS_ERR(sys_bg)) {
 			ret = PTR_ERR(sys_bg);
 			btrfs_abort_transaction(trans, ret);
@@ -5363,7 +5363,7 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
 	return block_group;
 }

-struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
+struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 					    u64 type)
 {
 	struct btrfs_fs_info *info = trans->fs_info;
@@ -5564,12 +5564,12 @@ static noinline int init_first_rw_device(struct btrfs_trans_handle *trans)
 	 */

 	alloc_profile = btrfs_metadata_alloc_profile(fs_info);
-	meta_bg = btrfs_alloc_chunk(trans, alloc_profile);
+	meta_bg = btrfs_create_chunk(trans, alloc_profile);
 	if (IS_ERR(meta_bg))
 		return PTR_ERR(meta_bg);

 	alloc_profile = btrfs_system_alloc_profile(fs_info);
-	sys_bg = btrfs_alloc_chunk(trans, alloc_profile);
+	sys_bg = btrfs_create_chunk(trans, alloc_profile);
 	if (IS_ERR(sys_bg))
 		return PTR_ERR(sys_bg);

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 70c749eee3ad..f9e13e8ca703 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -450,7 +450,7 @@ int btrfs_get_io_geometry(struct btrfs_fs_info *fs_info, struct extent_map *map,
 			  struct btrfs_io_geometry *io_geom);
 int btrfs_read_sys_array(struct btrfs_fs_info *fs_info);
 int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info);
-struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
+struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
 					    u64 type);
 void btrfs_mapping_tree_free(struct extent_map_tree *tree);
 blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
--
2.25.1

Re: [PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[Filipe Manana]

On Mon, Jul 5, 2021 at 10:19 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> The user facing function used to allocate new chunks is
> btrfs_chunk_alloc, unfortunately there is yet another similar sounding
> function - btrfs_alloc_chunk. This creates confusion, especially since
> the latter function can be considered "private" in the sense that it
> implements the first stage of chunk creation and as such is called by
> btrfs_chunk_alloc.
>
> To avoid the awkwardness that comes with having similarly named but
> distinctly different in their purpose function rename btrfs_alloc_chunk
> to btrfs_create_chunk, given that the main purpose of this function is
> to orchestrate the whole process of allocating a chunk - reserving space
> into devices, deciding on characteristics of the stripe size and
> creating the in-memory structures.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Looks good now, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
> V2:
>  * Fix changlog to reflect reality
>
>  fs/btrfs/block-group.c | 6 +++---
>  fs/btrfs/volumes.c     | 8 ++++----
>  fs/btrfs/volumes.h     | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 5c2361168363..500a85e4320f 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -3358,7 +3358,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
>          */
>         check_system_chunk(trans, flags);
>
> -       bg = btrfs_alloc_chunk(trans, flags);
> +       bg = btrfs_create_chunk(trans, flags);
>         if (IS_ERR(bg)) {
>                 ret = PTR_ERR(bg);
>                 goto out;
> @@ -3419,7 +3419,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
>                 const u64 sys_flags = btrfs_system_alloc_profile(trans->fs_info);
>                 struct btrfs_block_group *sys_bg;
>
> -               sys_bg = btrfs_alloc_chunk(trans, sys_flags);
> +               sys_bg = btrfs_create_chunk(trans, sys_flags);
>                 if (IS_ERR(sys_bg)) {
>                         ret = PTR_ERR(sys_bg);
>                         btrfs_abort_transaction(trans, ret);
> @@ -3712,7 +3712,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
>                  * could deadlock on an extent buffer since our caller may be
>                  * COWing an extent buffer from the chunk btree.
>                  */
> -               bg = btrfs_alloc_chunk(trans, flags);
> +               bg = btrfs_create_chunk(trans, flags);
>                 if (IS_ERR(bg)) {
>                         ret = PTR_ERR(bg);
>                 } else if (!(type & BTRFS_BLOCK_GROUP_SYSTEM)) {
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index f820c32f4a0d..4f84b5d871dd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3086,7 +3086,7 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
>                 const u64 sys_flags = btrfs_system_alloc_profile(fs_info);
>                 struct btrfs_block_group *sys_bg;
>
> -               sys_bg = btrfs_alloc_chunk(trans, sys_flags);
> +               sys_bg = btrfs_create_chunk(trans, sys_flags);
>                 if (IS_ERR(sys_bg)) {
>                         ret = PTR_ERR(sys_bg);
>                         btrfs_abort_transaction(trans, ret);
> @@ -5363,7 +5363,7 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>         return block_group;
>  }
>
> -struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
> +struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>                                             u64 type)
>  {
>         struct btrfs_fs_info *info = trans->fs_info;
> @@ -5564,12 +5564,12 @@ static noinline int init_first_rw_device(struct btrfs_trans_handle *trans)
>          */
>
>         alloc_profile = btrfs_metadata_alloc_profile(fs_info);
> -       meta_bg = btrfs_alloc_chunk(trans, alloc_profile);
> +       meta_bg = btrfs_create_chunk(trans, alloc_profile);
>         if (IS_ERR(meta_bg))
>                 return PTR_ERR(meta_bg);
>
>         alloc_profile = btrfs_system_alloc_profile(fs_info);
> -       sys_bg = btrfs_alloc_chunk(trans, alloc_profile);
> +       sys_bg = btrfs_create_chunk(trans, alloc_profile);
>         if (IS_ERR(sys_bg))
>                 return PTR_ERR(sys_bg);
>
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 70c749eee3ad..f9e13e8ca703 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -450,7 +450,7 @@ int btrfs_get_io_geometry(struct btrfs_fs_info *fs_info, struct extent_map *map,
>                           struct btrfs_io_geometry *io_geom);
>  int btrfs_read_sys_array(struct btrfs_fs_info *fs_info);
>  int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info);
> -struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
> +struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>                                             u64 type);
>  void btrfs_mapping_tree_free(struct extent_map_tree *tree);
>  blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> --
> 2.25.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[Anand Jain]

On 05/07/2021 17:16, Nikolay Borisov wrote:
> The user facing function used to allocate new chunks is
> btrfs_chunk_alloc, unfortunately there is yet another similar sounding
> function - btrfs_alloc_chunk. This creates confusion, especially since
> the latter function can be considered "private" in the sense that it
> implements the first stage of chunk creation and as such is called by
> btrfs_chunk_alloc.
> 
> To avoid the awkwardness that comes with having similarly named but
> distinctly different in their purpose function rename btrfs_alloc_chunk
> to btrfs_create_chunk, given that the main purpose of this function is
> to orchestrate the whole process of allocating a chunk - reserving space
> into devices, deciding on characteristics of the stripe size and
> creating the in-memory structures.
> 

Nits:

This is a kind of fixes:
  commit 11c67b1a40b0 (btrfs: Rename __btrfs_alloc_chunk to 
btrfs_alloc_chunk)


Can you also update the now stale reference to __btrfs_alloc_chunk under 
comments? as below.

volumes.c:
    5005  * Structure used internally for __btrfs_alloc_chunk() function.

zoned.c:
    588          * __btrfs_alloc_chunk(). Since we want stripe_len == 
zone_size,

With that,

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

Thanks, Anand


> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> V2:
>   * Fix changlog to reflect reality
> 
>   fs/btrfs/block-group.c | 6 +++---
>   fs/btrfs/volumes.c     | 8 ++++----
>   fs/btrfs/volumes.h     | 2 +-
>   3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 5c2361168363..500a85e4320f 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -3358,7 +3358,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
>   	 */
>   	check_system_chunk(trans, flags);
> 
> -	bg = btrfs_alloc_chunk(trans, flags);
> +	bg = btrfs_create_chunk(trans, flags);
>   	if (IS_ERR(bg)) {
>   		ret = PTR_ERR(bg);
>   		goto out;
> @@ -3419,7 +3419,7 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
>   		const u64 sys_flags = btrfs_system_alloc_profile(trans->fs_info);
>   		struct btrfs_block_group *sys_bg;
> 
> -		sys_bg = btrfs_alloc_chunk(trans, sys_flags);
> +		sys_bg = btrfs_create_chunk(trans, sys_flags);
>   		if (IS_ERR(sys_bg)) {
>   			ret = PTR_ERR(sys_bg);
>   			btrfs_abort_transaction(trans, ret);
> @@ -3712,7 +3712,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
>   		 * could deadlock on an extent buffer since our caller may be
>   		 * COWing an extent buffer from the chunk btree.
>   		 */
> -		bg = btrfs_alloc_chunk(trans, flags);
> +		bg = btrfs_create_chunk(trans, flags);
>   		if (IS_ERR(bg)) {
>   			ret = PTR_ERR(bg);
>   		} else if (!(type & BTRFS_BLOCK_GROUP_SYSTEM)) {
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index f820c32f4a0d..4f84b5d871dd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3086,7 +3086,7 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
>   		const u64 sys_flags = btrfs_system_alloc_profile(fs_info);
>   		struct btrfs_block_group *sys_bg;
> 
> -		sys_bg = btrfs_alloc_chunk(trans, sys_flags);
> +		sys_bg = btrfs_create_chunk(trans, sys_flags);
>   		if (IS_ERR(sys_bg)) {
>   			ret = PTR_ERR(sys_bg);
>   			btrfs_abort_transaction(trans, ret);
> @@ -5363,7 +5363,7 @@ static struct btrfs_block_group *create_chunk(struct btrfs_trans_handle *trans,
>   	return block_group;
>   }
> 
> -struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
> +struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>   					    u64 type)
>   {
>   	struct btrfs_fs_info *info = trans->fs_info;
> @@ -5564,12 +5564,12 @@ static noinline int init_first_rw_device(struct btrfs_trans_handle *trans)
>   	 */
> 
>   	alloc_profile = btrfs_metadata_alloc_profile(fs_info);
> -	meta_bg = btrfs_alloc_chunk(trans, alloc_profile);
> +	meta_bg = btrfs_create_chunk(trans, alloc_profile);
>   	if (IS_ERR(meta_bg))
>   		return PTR_ERR(meta_bg);
> 
>   	alloc_profile = btrfs_system_alloc_profile(fs_info);
> -	sys_bg = btrfs_alloc_chunk(trans, alloc_profile);
> +	sys_bg = btrfs_create_chunk(trans, alloc_profile);
>   	if (IS_ERR(sys_bg))
>   		return PTR_ERR(sys_bg);
> 
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 70c749eee3ad..f9e13e8ca703 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -450,7 +450,7 @@ int btrfs_get_io_geometry(struct btrfs_fs_info *fs_info, struct extent_map *map,
>   			  struct btrfs_io_geometry *io_geom);
>   int btrfs_read_sys_array(struct btrfs_fs_info *fs_info);
>   int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info);
> -struct btrfs_block_group *btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
> +struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans,
>   					    u64 type);
>   void btrfs_mapping_tree_free(struct extent_map_tree *tree);
>   blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> --
> 2.25.1
> 

Re: [PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[Nikolay Borisov]

On 5.07.21 г. 12:16, Nikolay Borisov wrote:
> The user facing function used to allocate new chunks is
> btrfs_chunk_alloc, unfortunately there is yet another similar sounding
> function - btrfs_alloc_chunk. This creates confusion, especially since
> the latter function can be considered "private" in the sense that it
> implements the first stage of chunk creation and as such is called by
> btrfs_chunk_alloc.
> 
> To avoid the awkwardness that comes with having similarly named but
> distinctly different in their purpose function rename btrfs_alloc_chunk
> to btrfs_create_chunk, given that the main purpose of this function is
> to orchestrate the whole process of allocating a chunk - reserving space
> into devices, deciding on characteristics of the stripe size and
> creating the in-memory structures.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Ping

Re: [PATCH v2] btrfs: rename btrfs_alloc_chunk to btrfs_create_chunk

[David Sterba]

On Wed, Aug 18, 2021 at 09:26:54AM +0300, Nikolay Borisov wrote:
> 
> 
> On 5.07.21 г. 12:16, Nikolay Borisov wrote:
> > The user facing function used to allocate new chunks is
> > btrfs_chunk_alloc, unfortunately there is yet another similar sounding
> > function - btrfs_alloc_chunk. This creates confusion, especially since
> > the latter function can be considered "private" in the sense that it
> > implements the first stage of chunk creation and as such is called by
> > btrfs_chunk_alloc.
> > 
> > To avoid the awkwardness that comes with having similarly named but
> > distinctly different in their purpose function rename btrfs_alloc_chunk
> > to btrfs_create_chunk, given that the main purpose of this function is
> > to orchestrate the whole process of allocating a chunk - reserving space
> > into devices, deciding on characteristics of the stripe size and
> > creating the in-memory structures.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Ping

Please refresh the patch and resend, thanks.