[1/3] btrfs: introduce btrfs_return_free_space()

Commit Message

Naohiro Aota Nov. 11, 2024, 7:46 a.m. UTC

This commit factors out a part of unpin_extent_range() into a function for
the next commit. Also, move the "len" variable into the loop to clarify we
don't need to carry it beyond an iteration.

Signed-off-by: Naohiro Aota <[e-mail]>
---
 fs/btrfs/extent-tree.c | 25 ++++---------------------
 fs/btrfs/space-info.c  | 24 ++++++++++++++++++++++++
 fs/btrfs/space-info.h  |  1 +
 3 files changed, 29 insertions(+), 21 deletions(-)

Comments

Johannes Thumshirn Nov. 11, 2024, 8:28 a.m. UTC | #1

On 11.11.24 08:50, Naohiro Aota wrote:
> +void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len)
> +{
> +	struct btrfs_fs_info *fs_info = space_info->fs_info;
> +	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
> +
> +	lockdep_assert_held(&space_info->lock);
> +
> +	if (global_rsv->space_info == space_info) {
> +		guard(spinlock)(&global_rsv->lock);
> +		if (!global_rsv->full) {
> +			u64 to_add = min(len, global_rsv->size - global_rsv->reserved);
> +
> +			global_rsv->reserved += to_add;
> +			btrfs_space_info_update_bytes_may_use(fs_info, space_info, to_add);
> +			if (global_rsv->reserved >= global_rsv->size)
> +				global_rsv->full = 1;
> +			len -= to_add;
> +		}
> +	}
> +	/* Add to any tickets we may have */
> +	if (len)
> +		btrfs_try_granting_tickets(fs_info, space_info);
> +}

Stylistic nitpick here,

If you reverse the 1st if condition we can save one level of indentation 
and therefore not go over 80 chars:

if (global_rsv->space_info != space_info)
	goto out;

guard(spinlock)(&global_rsv->lock);
if (!global_rsv->full) {
	/* ... */
}
out:
if (len)
	btrfs_try_granting_tickets(fs_info, space_info);

Possibly even the 'if (!global_rsv->full)' can have a reserved polarity.

Another thing is the use of the guard() macro. I find it a bit hard to 
see, what is actually guarded by it (the block inside 'if 
(global_rsv->space_info == space_info) {'), so not sure if it really 
improves anything here, but I'd prefer to leave that up to David to decide.

Code wise I think the change is legitim, so
Reviewed-by: Johannes Thumshirn <[e-mail]>
once the indentation is reduced.

Patch

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 412e318e4a22..ce7c963dd0a6 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2724,15 +2724,15 @@  static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_block_group *cache = NULL;
 	struct btrfs_space_info *space_info;
-	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
 	struct btrfs_free_cluster *cluster = NULL;
-	u64 len;
 	u64 total_unpinned = 0;
 	u64 empty_cluster = 0;
 	bool readonly;
 	int ret = 0;
 
 	while (start <= end) {
+		u64 len;
+
 		readonly = false;
 		if (!cache ||
 		    start >= cache->start + cache->length) {
@@ -2790,25 +2790,8 @@  static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 			readonly = true;
 		}
 		spin_unlock(&cache->lock);
-		if (!readonly && return_free_space &&
-		    global_rsv->space_info == space_info) {
-			spin_lock(&global_rsv->lock);
-			if (!global_rsv->full) {
-				u64 to_add = min(len, global_rsv->size -
-						      global_rsv->reserved);
-
-				global_rsv->reserved += to_add;
-				btrfs_space_info_update_bytes_may_use(fs_info,
-						space_info, to_add);
-				if (global_rsv->reserved >= global_rsv->size)
-					global_rsv->full = 1;
-				len -= to_add;
-			}
-			spin_unlock(&global_rsv->lock);
-		}
-		/* Add to any tickets we may have */
-		if (!readonly && return_free_space && len)
-			btrfs_try_granting_tickets(fs_info, space_info);
+		if (!readonly && return_free_space)
+			btrfs_return_free_space(space_info, len);
 		spin_unlock(&space_info->lock);
 	}
 
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 255e85f78313..bcdf0fdfa2d3 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -2082,3 +2082,27 @@  void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info)
 			do_reclaim_sweep(space_info, raid);
 	}
 }
+
+void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len)
+{
+	struct btrfs_fs_info *fs_info = space_info->fs_info;
+	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
+
+	lockdep_assert_held(&space_info->lock);
+
+	if (global_rsv->space_info == space_info) {
+		guard(spinlock)(&global_rsv->lock);
+		if (!global_rsv->full) {
+			u64 to_add = min(len, global_rsv->size - global_rsv->reserved);
+
+			global_rsv->reserved += to_add;
+			btrfs_space_info_update_bytes_may_use(fs_info, space_info, to_add);
+			if (global_rsv->reserved >= global_rsv->size)
+				global_rsv->full = 1;
+			len -= to_add;
+		}
+	}
+	/* Add to any tickets we may have */
+	if (len)
+		btrfs_try_granting_tickets(fs_info, space_info);
+}
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index efbecc0c5258..4c9e8aabee51 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -295,5 +295,6 @@  void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool
 bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info);
 int btrfs_calc_reclaim_threshold(const struct btrfs_space_info *space_info);
 void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info);
+void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len);
 
 #endif /* BTRFS_SPACE_INFO_H */