btrfs: fix periodic reclaim condition

Problems with current implementation:
1. reclaimable_bytes is signed while chunk_sz is unsigned, causing negative reclaimable_bytes to trigger reclaim unexpectedly
2. The "space must be freed between scans" assumption breaks the two-scan requirement: first scan marks block groups, second scan reclaims them. Without the second scan, no reclamation occurs.

Instead, track actual reclaim progress: pause reclaim when block groups will be reclaimed, and resume only when progress is made. This ensures reclaim continues until no further progress can be made. And resume perioidc reclaim when there's enough free space.

fs/btrfs/space-info.c +0 -0

index ebe97d6d67d3a..bc493243f777d 100644

fs/btrfs/space-info.c +2 -2

@@ -2098,11 +2098,11 @@ static bool is_reclaim_urgent(struct btrfs_space_info *space_info)
 	return unalloc < data_chunk_size;
 }
 
-static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
+static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 {
 	struct btrfs_block_group *bg;
 	int thresh_pct;
-	bool try_again = true;
+	bool will_reclaim = false;
 	bool urgent;
 
 	spin_lock(&space_info->lock);

fs/btrfs/space-info.c +3 -2

@@ -2137,12 +2137,13 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 	 * If we have any staler groups, we don't touch the fresher ones, but if we
 	 * really need a block group, do take a fresh one.
 	 */
-	if (try_again && urgent) {
-		try_again = false;
+	if (!will_reclaim && urgent) {
+		urgent = false;
 		goto again;
 	}
 
 	up_read(&space_info->groups_sem);
+	return will_reclaim;
 }

fs/btrfs/space-info.c +0 -1

@@ -2179,7 +2181,6 @@ static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 
 	spin_lock(&space_info->lock);
 	ret = space_info->periodic_reclaim_ready;
-	btrfs_set_periodic_reclaim_ready(space_info, false);
 	spin_unlock(&space_info->lock);
 
 	return ret;

@@ -2193,8 +2194,10 @@ void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info)
 	list_for_each_entry(space_info, &fs_info->space_info, list) {

Quoted 7 lines · Sun YangKai

Problems with current implementation:
1. reclaimable_bytes is signed while chunk_sz is unsigned, causing negative reclaimable_bytes to trigger reclaim unexpectedly
2. The "space must be freed between scans" assumption breaks the two-scan requirement: first scan marks block groups, second scan reclaims them. Without the second scan, no reclamation occurs.

Quoted 5 lines

Instead, track actual reclaim progress: pause reclaim when block groups will be reclaimed, and resume only when progress is made. This ensures reclaim continues until no further progress can be made. And resume perioidc reclaim when there's enough free space.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect:

commit 19eff93dc738e8afaa59cb374b44bb5a162e6c2d
Author: Sun YangKai <[email protected]>

btrfs: fix periodic reclaim condition

This commit fixes issues with periodic reclaim by tracking actual reclaim progress rather than relying on the assumption that space must be freed between scans. The patch addresses problems with signed/unsigned comparison and ensures reclaim continues until no further progress can be made.

Quoted 4 lines

fs/btrfs/space-info.c +0 -0

index ebe97d6d67d3a..bc493243f777d 100644

[ ... ]

Quoted 14 lines

fs/btrfs/space-info.c +2 -2

@@ -2098,11 +2098,11 @@ static bool is_reclaim_urgent(struct btrfs_space_info *space_info)
  	return unalloc < data_chunk_size;
  }
 
-static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
+static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
  {
  	struct btrfs_block_group *bg;
  	int thresh_pct;
-	bool try_again = true;
+	bool will_reclaim = false;
  	bool urgent;
 
  	spin_lock(&space_info->lock);

[ ... ]

Quoted 14 lines

fs/btrfs/space-info.c +3 -2

@@ -2137,12 +2137,13 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
  	 * If we have any staler groups, we don't touch the fresher ones, but if we
  	 * really need a block group, do take a fresh one.
  	 */
-	if (try_again && urgent) {
-		try_again = false;
+	if (!will_reclaim && urgent) {
+		urgent = false;
  		goto again;
  	}
 
  	up_read(&space_info->groups_sem);
+	return will_reclaim;
  }

[ ... ]

Quoted 10 lines

fs/btrfs/space-info.c +0 -1

@@ -2179,7 +2181,6 @@ static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 
  	spin_lock(&space_info->lock);
  	ret = space_info->periodic_reclaim_ready;
-	btrfs_set_periodic_reclaim_ready(space_info, false);
  	spin_unlock(&space_info->lock);
 
  	return ret;

@@ -2193,8 +2194,10 @@ void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info)
  	list_for_each_entry(space_info, &fs_info->space_info, list) {

fs/btrfs/space-info.c void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info) · line 2202

Quoted 10 lines

fs/btrfs/space-info.c +4 -2

  		if (!btrfs_should_periodic_reclaim(space_info))
  			continue;
-		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++)
-			do_reclaim_sweep(space_info, raid);
+		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++) {
+			if (do_reclaim_sweep(space_info, raid))
+				btrfs_set_periodic_reclaim_ready(space_info, false);
+		}
  	}
  }

Does btrfs_reclaim_sweep()->btrfs_set_periodic_reclaim_ready() violate locking requirements? The function btrfs_set_periodic_reclaim_ready() has lockdep_assert_held(&space_info->lock) at line 2163, but at the call site above (line 2199), no lock is held.

Looking at do_reclaim_sweep(), it releases space_info->lock via up_read(&space_info->groups_sem) at line 2145 before returning. Similarly, the old code had btrfs_should_periodic_reclaim() release the lock with spin_unlock(&space_info->lock) at line 2184 before returning.

In the original code, btrfs_set_periodic_reclaim_ready() was called from btrfs_should_periodic_reclaim() while holding space_info->lock (acquired at line 2182, call to btrfs_set_periodic_reclaim_ready, then release at line 2184). This patch moves the call to btrfs_reclaim_sweep() but doesn't acquire the lock at the new call site.

Should space_info->lock be acquired before calling btrfs_set_periodic_reclaim_ready() at line 2199?

Quoted 7 lines · Sun YangKai

Problems with current implementation:
1. reclaimable_bytes is signed while chunk_sz is unsigned, causing negative reclaimable_bytes to trigger reclaim unexpectedly
2. The "space must be freed between scans" assumption breaks the two-scan requirement: first scan marks block groups, second scan reclaims them. Without the second scan, no reclamation occurs.

Quoted 5 lines

Instead, track actual reclaim progress: pause reclaim when block groups will be reclaimed, and resume only when progress is made. This ensures reclaim continues until no further progress can be made. And resume perioidc reclaim when there's enough free space.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect:

commit 19eff93dc738e8afaa59cb374b44bb5a162e6c2d
Author: Sun YangKai <[email protected]>

btrfs: fix periodic reclaim condition

This commit fixes issues with periodic reclaim by tracking actual reclaim progress rather than relying on the assumption that space must be freed between scans. The patch addresses problems with signed/unsigned comparison and ensures reclaim continues until no further progress can be made.

Quoted 4 lines

fs/btrfs/space-info.c +0 -0

index ebe97d6d67d3a..bc493243f777d 100644

[ ... ]

Quoted 14 lines

fs/btrfs/space-info.c +2 -2

@@ -2098,11 +2098,11 @@ static bool is_reclaim_urgent(struct btrfs_space_info *space_info)
  	return unalloc < data_chunk_size;
  }
 
-static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
+static bool do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
  {
  	struct btrfs_block_group *bg;
  	int thresh_pct;
-	bool try_again = true;
+	bool will_reclaim = false;
  	bool urgent;
 
  	spin_lock(&space_info->lock);

[ ... ]

Quoted 14 lines

fs/btrfs/space-info.c +3 -2

@@ -2137,12 +2137,13 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
  	 * If we have any staler groups, we don't touch the fresher ones, but if we
  	 * really need a block group, do take a fresh one.
  	 */
-	if (try_again && urgent) {
-		try_again = false;
+	if (!will_reclaim && urgent) {
+		urgent = false;
  		goto again;
  	}
 
  	up_read(&space_info->groups_sem);
+	return will_reclaim;
  }

[ ... ]

Quoted 10 lines

fs/btrfs/space-info.c +0 -1

@@ -2179,7 +2181,6 @@ static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 
  	spin_lock(&space_info->lock);
  	ret = space_info->periodic_reclaim_ready;
-	btrfs_set_periodic_reclaim_ready(space_info, false);
  	spin_unlock(&space_info->lock);
 
  	return ret;

@@ -2193,8 +2194,10 @@ void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info)
  	list_for_each_entry(space_info, &fs_info->space_info, list) {

fs/btrfs/space-info.c void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info) · line 2202

Quoted 10 lines

fs/btrfs/space-info.c +4 -2

  		if (!btrfs_should_periodic_reclaim(space_info))
  			continue;
-		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++)
-			do_reclaim_sweep(space_info, raid);
+		for (raid = 0; raid < BTRFS_NR_RAID_TYPES; raid++) {
+			if (do_reclaim_sweep(space_info, raid))
+				btrfs_set_periodic_reclaim_ready(space_info, false);
+		}
  	}
  }

Does btrfs_reclaim_sweep()->btrfs_set_periodic_reclaim_ready() violate locking requirements? The function btrfs_set_periodic_reclaim_ready() has lockdep_assert_held(&space_info->lock) at line 2163, but at the call site above (line 2199), no lock is held.

Looking at do_reclaim_sweep(), it releases space_info->lock via up_read(&space_info->groups_sem) at line 2145 before returning. Similarly, the old code had btrfs_should_periodic_reclaim() release the lock with spin_unlock(&space_info->lock) at line 2184 before returning.

In the original code, btrfs_set_periodic_reclaim_ready() was called from btrfs_should_periodic_reclaim() while holding space_info->lock (acquired at line 2182, call to btrfs_set_periodic_reclaim_ready, then release at line 2184). This patch moves the call to btrfs_reclaim_sweep() but doesn't acquire the lock at the new call site.

Should space_info->lock be acquired before calling btrfs_set_periodic_reclaim_ready() at line 2199?

This series eliminates wasteful periodic reclaim operations that were occurring when already failed to reclaim any new space, and includes several preparatory cleanups.

Patch 1 fixes the core issue.
Patch 2 is a trival cleanup.

Changelog

v4:
- Remove setting periodic_reclaim_ready to false when failed to reclaim a blockgroup since it's unnecessary. Sugggested by Boris Burkov <[email protected]>

v3:
- Fix the core issue with minimal changes, suggested by Boris Burkov <[email protected]>
- Drop some cleanups which might conflict with some other recent patches in mail list. I'll send them in a seperate serie.

Sun YangKai (2):
btrfs: fix periodic reclaim condition
btrfs: consolidate reclaim readiness checks in btrfs_should_reclaim()

 fs/btrfs/block-group.c | 18 ++++++++++--------
 fs/btrfs/space-info.c  | 21 ++++++++++++---------
 2 files changed, 22 insertions(+), 17 deletions(-)

Signature · 1 line

2.52.0