[PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[Qu Wenruo]

Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
locks and add a block group to unused_bgs list.

No functional modification, and only 3 callers are involved.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
This patch should provide the basis for later block group auto-removal
to get more info (mostly transid) to determine should one block group
being removed in current trans.

changelog:
v2:
  Add ASSERT() for btrfs_read_block_groups(), as in that call site a
  block group should not be added to other list (new_bgs or
  deleted_bgs).
  Rename the function to btrfs_mark_bg_unused().
  Both suggested by Nikolay.
---
 fs/btrfs/ctree.h       |  1 +
 fs/btrfs/extent-tree.c | 36 +++++++++++++++++-------------------
 fs/btrfs/scrub.c       |  9 +--------
 3 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index bbb358143ded..4a34bc443fe4 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2827,6 +2827,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info, const u64 type);
 u64 add_new_free_space(struct btrfs_block_group_cache *block_group,
 		       u64 start, u64 end);
+void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg);
 
 /* ctree.c */
 int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ccf2690f7ca1..33f046103073 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6312,16 +6312,8 @@ static int update_block_group(struct btrfs_trans_handle *trans,
 		 * dirty list to avoid races between cleaner kthread and space
 		 * cache writeout.
 		 */
-		if (!alloc && old_val == 0) {
-			spin_lock(&info->unused_bgs_lock);
-			if (list_empty(&cache->bg_list)) {
-				btrfs_get_block_group(cache);
-				trace_btrfs_add_unused_block_group(cache);
-				list_add_tail(&cache->bg_list,
-					      &info->unused_bgs);
-			}
-			spin_unlock(&info->unused_bgs_lock);
-		}
+		if (!alloc && old_val == 0)
+			btrfs_mark_bg_unused(cache);
 
 		btrfs_put_block_group(cache);
 		total -= num_bytes;
@@ -10144,15 +10136,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 		if (btrfs_chunk_readonly(info, cache->key.objectid)) {
 			inc_block_group_ro(cache, 1);
 		} else if (btrfs_block_group_used(&cache->item) == 0) {
-			spin_lock(&info->unused_bgs_lock);
-			/* Should always be true but just in case. */
-			if (list_empty(&cache->bg_list)) {
-				btrfs_get_block_group(cache);
-				trace_btrfs_add_unused_block_group(cache);
-				list_add_tail(&cache->bg_list,
-					      &info->unused_bgs);
-			}
-			spin_unlock(&info->unused_bgs_lock);
+			ASSERT(list_empty(&cache->bg_list));
+			btrfs_mark_bg_unused(cache);
 		}
 	}
 
@@ -11071,3 +11056,16 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root)
 			       !atomic_read(&root->will_be_snapshotted));
 	}
 }
+
+void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg)
+{
+	struct btrfs_fs_info *fs_info = bg->fs_info;
+
+	spin_lock(&fs_info->unused_bgs_lock);
+	if (list_empty(&bg->bg_list)) {
+		btrfs_get_block_group(bg);
+		trace_btrfs_add_unused_block_group(bg);
+		list_add_tail(&bg->bg_list, &fs_info->unused_bgs);
+	}
+	spin_unlock(&fs_info->unused_bgs_lock);
+}
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index a59005862010..40086b47a65f 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 		if (!cache->removed && !cache->ro && cache->reserved == 0 &&
 		    btrfs_block_group_used(&cache->item) == 0) {
 			spin_unlock(&cache->lock);
-			spin_lock(&fs_info->unused_bgs_lock);
-			if (list_empty(&cache->bg_list)) {
-				btrfs_get_block_group(cache);
-				trace_btrfs_add_unused_block_group(cache);
-				list_add_tail(&cache->bg_list,
-					      &fs_info->unused_bgs);
-			}
-			spin_unlock(&fs_info->unused_bgs_lock);
+			btrfs_mark_bg_unused(cache);
 		} else {
 			spin_unlock(&cache->lock);
 		}
-- 
2.17.0

Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[Nikolay Borisov]

On 22.05.2018 11:43, Qu Wenruo wrote:
> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> locks and add a block group to unused_bgs list.
> 
> No functional modification, and only 3 callers are involved.>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

> ---
> This patch should provide the basis for later block group auto-removal
> to get more info (mostly transid) to determine should one block group
> being removed in current trans.
> 
> changelog:
> v2:
>   Add ASSERT() for btrfs_read_block_groups(), as in that call site a
>   block group should not be added to other list (new_bgs or
>   deleted_bgs).
>   Rename the function to btrfs_mark_bg_unused().
>   Both suggested by Nikolay.
> ---
>  fs/btrfs/ctree.h       |  1 +
>  fs/btrfs/extent-tree.c | 36 +++++++++++++++++-------------------
>  fs/btrfs/scrub.c       |  9 +--------
>  3 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index bbb358143ded..4a34bc443fe4 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2827,6 +2827,7 @@ void check_system_chunk(struct btrfs_trans_handle *trans,
>  			struct btrfs_fs_info *fs_info, const u64 type);
>  u64 add_new_free_space(struct btrfs_block_group_cache *block_group,
>  		       u64 start, u64 end);
> +void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg);
>  
>  /* ctree.c */
>  int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index ccf2690f7ca1..33f046103073 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6312,16 +6312,8 @@ static int update_block_group(struct btrfs_trans_handle *trans,
>  		 * dirty list to avoid races between cleaner kthread and space
>  		 * cache writeout.
>  		 */
> -		if (!alloc && old_val == 0) {
> -			spin_lock(&info->unused_bgs_lock);
> -			if (list_empty(&cache->bg_list)) {
> -				btrfs_get_block_group(cache);
> -				trace_btrfs_add_unused_block_group(cache);
> -				list_add_tail(&cache->bg_list,
> -					      &info->unused_bgs);
> -			}
> -			spin_unlock(&info->unused_bgs_lock);
> -		}
> +		if (!alloc && old_val == 0)
> +			btrfs_mark_bg_unused(cache);
>  
>  		btrfs_put_block_group(cache);
>  		total -= num_bytes;
> @@ -10144,15 +10136,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>  		if (btrfs_chunk_readonly(info, cache->key.objectid)) {
>  			inc_block_group_ro(cache, 1);
>  		} else if (btrfs_block_group_used(&cache->item) == 0) {
> -			spin_lock(&info->unused_bgs_lock);
> -			/* Should always be true but just in case. */
> -			if (list_empty(&cache->bg_list)) {
> -				btrfs_get_block_group(cache);
> -				trace_btrfs_add_unused_block_group(cache);
> -				list_add_tail(&cache->bg_list,
> -					      &info->unused_bgs);
> -			}
> -			spin_unlock(&info->unused_bgs_lock);
> +			ASSERT(list_empty(&cache->bg_list));
> +			btrfs_mark_bg_unused(cache);
>  		}
>  	}
>  
> @@ -11071,3 +11056,16 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root)
>  			       !atomic_read(&root->will_be_snapshotted));
>  	}
>  }
> +
> +void btrfs_mark_bg_unused(struct btrfs_block_group_cache *bg)
> +{
> +	struct btrfs_fs_info *fs_info = bg->fs_info;
> +
> +	spin_lock(&fs_info->unused_bgs_lock);
> +	if (list_empty(&bg->bg_list)) {
> +		btrfs_get_block_group(bg);
> +		trace_btrfs_add_unused_block_group(bg);
> +		list_add_tail(&bg->bg_list, &fs_info->unused_bgs);
> +	}
> +	spin_unlock(&fs_info->unused_bgs_lock);
> +}
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index a59005862010..40086b47a65f 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3981,14 +3981,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>  		if (!cache->removed && !cache->ro && cache->reserved == 0 &&
>  		    btrfs_block_group_used(&cache->item) == 0) {
>  			spin_unlock(&cache->lock);
> -			spin_lock(&fs_info->unused_bgs_lock);
> -			if (list_empty(&cache->bg_list)) {
> -				btrfs_get_block_group(cache);
> -				trace_btrfs_add_unused_block_group(cache);
> -				list_add_tail(&cache->bg_list,
> -					      &fs_info->unused_bgs);
> -			}
> -			spin_unlock(&fs_info->unused_bgs_lock);
> +			btrfs_mark_bg_unused(cache);
>  		} else {
>  			spin_unlock(&cache->lock);
>  		}
> 

Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[David Sterba]

On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> locks and add a block group to unused_bgs list.

The helper is nice but hides that there's a reference taken on the 'bg'.
This would be good to add at least to the function comment or somehow
squeeze it into the function name itself, like
btrfs_get_and_mark_bg_unused.

Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[Qu Wenruo]

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



On 2018年05月22日 20:14, David Sterba wrote:
> On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
>> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
>> locks and add a block group to unused_bgs list.
> 
> The helper is nice but hides that there's a reference taken on the 'bg'.
> This would be good to add at least to the function comment or somehow
> squeeze it into the function name itself, like
> btrfs_get_and_mark_bg_unused.

That btrfs_get_block_group() call is only for later removal.
The reference will be removed in btrfs_delete_unused_bgs().

So I don't think the function name needs the "_get" part.

Although I could add more comment about the btrfs_get_block_group() call.

How do you think about this?

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: 488 bytes --]

Re: [PATCH v2] btrfs: Use btrfs_mark_bg_unused() to replace open code

[David Sterba]

On Sun, May 27, 2018 at 09:25:25AM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月22日 20:14, David Sterba wrote:
> > On Tue, May 22, 2018 at 04:43:47PM +0800, Qu Wenruo wrote:
> >> Introduce a small helper, btrfs_mark_bg_unused(), to accquire needed
> >> locks and add a block group to unused_bgs list.
> > 
> > The helper is nice but hides that there's a reference taken on the 'bg'.
> > This would be good to add at least to the function comment or somehow
> > squeeze it into the function name itself, like
> > btrfs_get_and_mark_bg_unused.
> 
> That btrfs_get_block_group() call is only for later removal.
> The reference will be removed in btrfs_delete_unused_bgs().
> 
> So I don't think the function name needs the "_get" part.

Agreed, it's inside the helpers. Patch added to misc-next, thanks.