btrfs: Use btrfs_mark_bg_unused() to replace open code

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 <[email protected]>

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

fs/btrfs/ctree.h +1 -0

index bbb358143ded..4a34bc443fe4 100644
@@ -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,

fs/btrfs/extent-tree.c +17 -19

index ccf2690f7ca1..33f046103073 100644
@@ -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);
+}

fs/btrfs/scrub.c +1 -8

index a59005862010..40086b47a65f 100644
@@ -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);
 		}

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

Quoted 2 lines · Qu Wenruo

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · David Sterba

Quoted 2 lines · Qu Wenruo

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.