btrfs: fix periodic reclaim condition with some cleanup

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, then resumes when space_info changes or new reclaimable groups appear.

CC Boris Burkov <[email protected]>

Fixes 813d4c6422516 ("btrfs: prevent pathological periodic reclaim loops")

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.c | 15 +++++++-------
 fs/btrfs/space-info.c  | 44 +++++++++++++++++++-----------------------
 fs/btrfs/space-info.h  | 28 ++++++++++++++++++---------
 3 files changed, 46 insertions(+), 41 deletions(-)

fs/btrfs/block-group.c +2 -0

index e417aba4c4c7..94a4068cd42a 100644
@@ -1871,6 +1871,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	while (!list_empty(&fs_info->reclaim_bgs)) {
 		u64 used;
 		u64 reserved;
+		u64 old_total;
 		int ret = 0;
 
 		bg = list_first_entry(&fs_info->reclaim_bgs,

@@ -1936,6 +1937,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 		}
 
 		spin_unlock(&bg->lock);
+		old_total = space_info->total_bytes;
 		spin_unlock(&space_info->lock);
 
 		/*

@@ -1988,14 +1990,14 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 			reserved = 0;
 			spin_lock(&space_info->lock);
 			space_info->reclaim_errors++;

fs/btrfs/block-group.c +0 -2

 		spin_unlock(&space_info->lock);
 
 next:

@@ -3730,8 +3732,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		space_info->bytes_reserved -= num_bytes;
 		space_info->bytes_used += num_bytes;
 		space_info->disk_used += num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, -num_bytes);
 		spin_unlock(&cache->lock);
 		spin_unlock(&space_info->lock);
 	} else {

fs/btrfs/block-group.c +0 -0

 		spin_unlock(&space_info->lock);
 
 		btrfs_set_extent_bit(&trans->transaction->pinned_extents, bytenr,

fs/btrfs/space-info.c +9 -22

index 7b7b7255f7d8..de8bde1081be 100644
@@ -2119,48 +2119,44 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 	 * really need a block group, do take a fresh one.
 	 */
 	if (try_again && urgent) {
-		try_again = false;
+		urgent = false;
 		goto again;
 	}
 
 	up_read(&space_info->groups_sem);
-}
-
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes)
-{
-	u64 chunk_sz = calc_effective_data_chunk_size(space_info->fs_info);
-
-	lockdep_assert_held(&space_info->lock);
-	space_info->reclaimable_bytes += bytes;
 
-	if (space_info->reclaimable_bytes >= chunk_sz)
-		btrfs_set_periodic_reclaim_ready(space_info, true);
-}
-
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready)
-{
-	lockdep_assert_held(&space_info->lock);
-	if (!READ_ONCE(space_info->periodic_reclaim))
-		return;
-	if (ready != space_info->periodic_reclaim_ready) {
-		space_info->periodic_reclaim_ready = ready;
-		if (!ready)
-			space_info->reclaimable_bytes = 0;
+	/*
+	 * Temporary pause periodic reclaim until reclaim make some progress.
+	 * This can prevent periodic reclaim keep happening but make no progress.
+	 */
+	if (!try_again) {
+		spin_lock(&space_info->lock);
+		btrfs_pause_periodic_reclaim(space_info);
+		spin_unlock(&space_info->lock);
 	}
 }
 
 static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 {

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

+	if (ret)
+		btrfs_resume_periodic_reclaim(space_info);
 	spin_unlock(&space_info->lock);
 
 	return ret;

fs/btrfs/space-info.h +19 -9

index 0703f24b23f7..a49a4c7b0a68 100644
@@ -214,14 +214,11 @@ struct btrfs_space_info {
 
 	/*
 	 * Periodic reclaim should be a no-op if a space_info hasn't
-	 * freed any space since the last time we tried.
+	 * freed any space since the last time we made no progress.
 	 */
-	bool periodic_reclaim_ready;
-
-	/*
-	 * Net bytes freed or allocated since the last reclaim pass.
-	 */
-	s64 reclaimable_bytes;
+	bool periodic_reclaim_paused;
+	int last_reclaim_threshold;
+	u64 last_reclaim_unused;
 };
 
 static inline bool btrfs_mixed_space_info(const struct btrfs_space_info *space_info)

@@ -301,9 +298,22 @@ void btrfs_dump_space_info_for_trans_abort(struct btrfs_fs_info *fs_info);
 void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info);
 u64 btrfs_account_ro_block_groups_free_space(struct btrfs_space_info *sinfo);
 
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes);
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready);
 int btrfs_calc_reclaim_threshold(const struct btrfs_space_info *space_info);
+static inline void btrfs_resume_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (space_info->periodic_reclaim_paused)
+		space_info->periodic_reclaim_paused = false;
+}
+static inline void btrfs_pause_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (!space_info->periodic_reclaim_paused) {
+		space_info->periodic_reclaim_paused = true;
+		space_info->last_reclaim_threshold = btrfs_calc_reclaim_threshold(space_info);
+		space_info->last_reclaim_unused = space_info->total_bytes - space_info->bytes_used;
+	}
+}
 void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info);
 void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len);
 

Signature · 1 line

2.51.2

On Sat, Jan 03, 2026 at 08:19:48PM +0800, Sun YangKai wrote:

First off, thank you very much for your attention to this and for your in depth fixes.

Quoted 3 lines

Problems with current implementation:
1. reclaimable_bytes is signed while chunk_sz is unsigned, causing negative reclaimable_bytes to trigger reclaim unexpectedly

Agreed, completely. IMO, the message here could use a bit more clarity that negative reclaimable_bytes is an intentionally designed condition so this is a fundamental bug, not a weird edge case.

Quoted 3 lines

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.

I think I understand what you are saying, but let me try to restate it myself and confirm:

Previously, due to bug 1, we were calling btrfs_set_periodic_reclaim_ready() too frequently (after an allocation that made the reclaimable_bytes negative). With bug 1 fixed, you really have to free a chunk_sz to call btrfs_set_periodic_reclaim_ready(). As a result, periodic_reclaim became way too conservative. Now the multi-sweep marking thing AND reclaim_ready mean we basically never reclaim, as the second sweep doesn't get triggered later.

Is that about right? I agree that this needs some re-thinking / fixing, if so.

Quoted 4 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, then resumes when space_info changes or new reclaimable groups appear.

I think you are making a much bigger change than merely fixing the bugs in the original design, so it requires deeper explanation of the new invariants you are introducing. Clearly, I am one to talk about it, as I messed up with my attempt, but I still think it is very valuable for future readers. Thanks for bearing with me :)

I think the simplest pure "fix" is to change the current "ready = false" setting from unconditional on every run of periodic reclaim to only when a sweep actually queues a block group for reclaim. I have compiled but not yet tested the code I included in this mail :) We can do that more carefully once we agree how we want to move forward.

If you still want to re-design things on top of a fix like this one, I am totally open to that.

Thanks again for all your work on this, and happy new year, Boris

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

index 6babbe333741..aad402485763 100644
@@ -2095,12 +2095,13 @@ 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 urgent;
+	bool ret = false;
 
 	spin_lock(&space_info->lock);
 	urgent = is_reclaim_urgent(space_info);

@@ -2122,8 +2123,10 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 		}
 		bg->reclaim_mark++;
 		spin_unlock(&bg->lock);
-		if (reclaim)
+		if (reclaim) {
 			btrfs_mark_bg_to_reclaim(bg);
+			ret = true;
+		}
 		btrfs_put_block_group(bg);
 	}
 

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

 void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes)

@@ -2149,7 +2153,8 @@ void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s6
 	lockdep_assert_held(&space_info->lock);
 	space_info->reclaimable_bytes += bytes;
 
-	if (space_info->reclaimable_bytes >= chunk_sz)
+	if (space_info->reclaimable_bytes > 0 &&
+	    space_info->reclaimable_bytes >= chunk_sz)
 		btrfs_set_periodic_reclaim_ready(space_info, true);
 }
 

@@ -2176,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;

Quoted 33 lines

CC Boris Burkov <[email protected]>

Fixes 813d4c6422516 ("btrfs: prevent pathological periodic reclaim loops")

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.c | 15 +++++++-------
 fs/btrfs/space-info.c  | 44 +++++++++++++++++++-----------------------
 fs/btrfs/space-info.h  | 28 ++++++++++++++++++---------
 3 files changed, 46 insertions(+), 41 deletions(-)

fs/btrfs/block-group.c +2 -0

index e417aba4c4c7..94a4068cd42a 100644
@@ -1871,6 +1871,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	while (!list_empty(&fs_info->reclaim_bgs)) {
 		u64 used;
 		u64 reserved;
+		u64 old_total;
 		int ret = 0;
 
 		bg = list_first_entry(&fs_info->reclaim_bgs,

@@ -1936,6 +1937,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 		}
 
 		spin_unlock(&bg->lock);
+		old_total = space_info->total_bytes;
 		spin_unlock(&space_info->lock);
 
 		/*

@@ -1988,14 +1990,14 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 			reserved = 0;
 			spin_lock(&space_info->lock);
 			space_info->reclaim_errors++;

fs/btrfs/block-group.c void btrfs_reclaim_bgs_work(struct work_struct *work) · line 2000

Quoted 10 lines

fs/btrfs/block-group.c +2 -2

-			if (READ_ONCE(space_info->periodic_reclaim))
-				space_info->periodic_reclaim_ready = false;
 			spin_unlock(&space_info->lock);
 		}
 		spin_lock(&space_info->lock);
 		space_info->reclaim_count++;
 		space_info->reclaim_bytes += used;
 		space_info->reclaim_bytes += reserved;
+		if (space_info->total_bytes < old_total)
+			btrfs_resume_periodic_reclaim(space_info);

Why is this here? We've just completed a reclaim, which I would expect to be neutral to the space_info->total_bytes (just moving them around). So if (any) unrelated freeing happens to happen while we are reclaiming, we resume? Doesn't seem wrong, but also seems a little specific and random. I am probably missing some aspect of your new design.

Put a different way, what is special about frees that happen *while* we are reclaiming?

Quoted 13 lines

fs/btrfs/block-group.c +0 -2

 		spin_unlock(&space_info->lock);
 
 next:

@@ -3730,8 +3732,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		space_info->bytes_reserved -= num_bytes;
 		space_info->bytes_used += num_bytes;
 		space_info->disk_used += num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, -num_bytes);
 		spin_unlock(&cache->lock);
 		spin_unlock(&space_info->lock);
 	} else {

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3744

Quoted 8 lines

fs/btrfs/block-group.c +1 -4

@@ -3741,12 +3741,11 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		btrfs_space_info_update_bytes_pinned(space_info, num_bytes);
 		space_info->bytes_used -= num_bytes;
 		space_info->disk_used -= num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, num_bytes);
-		else
-			reclaim = should_reclaim_block_group(cache, num_bytes);
+		reclaim = should_reclaim_block_group(cache, num_bytes);

I think this is a bug with periodic_reclaim == 1

In that case, if should_reclaim_block_group() returns true (could be a fixed or dynamic threshold), we will put that block group directly on the reclaim list, which is a complete contradiction of the point of periodic_reclaim.

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3747

Quoted 4 lines

fs/btrfs/block-group.c +2 -0

 		spin_unlock(&cache->lock);
+		if (reclaim)
+			btrfs_resume_periodic_reclaim(space_info);

This also makes me wonder about the idea behind your change. If you want periodic reclaim to "pause" until a block group meets the condition and then we "resume", that's not exactly in the spirit of checking at a periodic cadence, rather than as block groups update.

Quoted 52 lines

fs/btrfs/block-group.c +0 -0

 		spin_unlock(&space_info->lock);
 
 		btrfs_set_extent_bit(&trans->transaction->pinned_extents, bytenr,

fs/btrfs/space-info.c +9 -22

index 7b7b7255f7d8..de8bde1081be 100644
@@ -2119,48 +2119,44 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 	 * really need a block group, do take a fresh one.
 	 */
 	if (try_again && urgent) {
-		try_again = false;
+		urgent = false;
 		goto again;
 	}
 
 	up_read(&space_info->groups_sem);
-}
-
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes)
-{
-	u64 chunk_sz = calc_effective_data_chunk_size(space_info->fs_info);
-
-	lockdep_assert_held(&space_info->lock);
-	space_info->reclaimable_bytes += bytes;
 
-	if (space_info->reclaimable_bytes >= chunk_sz)
-		btrfs_set_periodic_reclaim_ready(space_info, true);
-}
-
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready)
-{
-	lockdep_assert_held(&space_info->lock);
-	if (!READ_ONCE(space_info->periodic_reclaim))
-		return;
-	if (ready != space_info->periodic_reclaim_ready) {
-		space_info->periodic_reclaim_ready = ready;
-		if (!ready)
-			space_info->reclaimable_bytes = 0;
+	/*
+	 * Temporary pause periodic reclaim until reclaim make some progress.
+	 * This can prevent periodic reclaim keep happening but make no progress.
+	 */
+	if (!try_again) {
+		spin_lock(&space_info->lock);
+		btrfs_pause_periodic_reclaim(space_info);
+		spin_unlock(&space_info->lock);
 	}
 }
 
 static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 {

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2150

Quoted 10 lines

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

 	bool ret;
+	u64 chunk_sz;
+	u64 unused;
 
 	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		return false;
 	if (!READ_ONCE(space_info->periodic_reclaim))
 		return false;
+	if (!READ_ONCE(space_info->periodic_reclaim_paused))
+		return true;

This condition doesn't feel like a "pause". If the "pause" is set to false, then we proceed no matter what, otherwise we check conditions? It should be something like:

if (READ_ONCE(space_info->periodic_reclaim_force))

        return true;

as it is acting like a "force" not a "not paused".

Hope that makes sense, it's obviously a bit tied up in language semantics..

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2155

Quoted 7 lines

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

+
+	chunk_sz = calc_effective_data_chunk_size(space_info->fs_info);
 
 	spin_lock(&space_info->lock);
-	ret = space_info->periodic_reclaim_ready;
-	btrfs_set_periodic_reclaim_ready(space_info, false);
+	unused = space_info->total_bytes - space_info->bytes_used;

Probably makes sense to use a zoned aware wrapper for this, just in case we make this friendly with zones eventually.

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2157

Quoted 2 lines

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

+	ret = (unused >= space_info->last_reclaim_unused + chunk_sz ||
+	       btrfs_calc_reclaim_threshold(space_info) != space_info->last_reclaim_threshold);

This second condition is quite interesting to me, I hadn't thought of it. I think it makes some sense to care again if the threshold changed, but it is a new behavior, rather than a fix, per-se.

What made you want to add this?

Quoted 53 lines

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

+	if (ret)
+		btrfs_resume_periodic_reclaim(space_info);
 	spin_unlock(&space_info->lock);
 
 	return ret;

fs/btrfs/space-info.h +19 -9

index 0703f24b23f7..a49a4c7b0a68 100644
@@ -214,14 +214,11 @@ struct btrfs_space_info {
 
 	/*
 	 * Periodic reclaim should be a no-op if a space_info hasn't
-	 * freed any space since the last time we tried.
+	 * freed any space since the last time we made no progress.
 	 */
-	bool periodic_reclaim_ready;
-
-	/*
-	 * Net bytes freed or allocated since the last reclaim pass.
-	 */
-	s64 reclaimable_bytes;
+	bool periodic_reclaim_paused;
+	int last_reclaim_threshold;
+	u64 last_reclaim_unused;
 };
 
 static inline bool btrfs_mixed_space_info(const struct btrfs_space_info *space_info)

@@ -301,9 +298,22 @@ void btrfs_dump_space_info_for_trans_abort(struct btrfs_fs_info *fs_info);
 void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info);
 u64 btrfs_account_ro_block_groups_free_space(struct btrfs_space_info *sinfo);
 
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes);
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready);
 int btrfs_calc_reclaim_threshold(const struct btrfs_space_info *space_info);
+static inline void btrfs_resume_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (space_info->periodic_reclaim_paused)
+		space_info->periodic_reclaim_paused = false;
+}
+static inline void btrfs_pause_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (!space_info->periodic_reclaim_paused) {
+		space_info->periodic_reclaim_paused = true;
+		space_info->last_reclaim_threshold = btrfs_calc_reclaim_threshold(space_info);
+		space_info->last_reclaim_unused = space_info->total_bytes - space_info->bytes_used;
+	}
+}
 void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info);
 void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len);
 

Quoted 3 lines

Signature · 1 line

2.51.2

Quoted 4 lines · Boris Burkov

On Sat, Jan 03, 2026 at 08:19:48PM +0800, Sun YangKai wrote:

First off, thank you very much for your attention to this and for your in depth fixes.

Glad to hear from you and Happy New Year! :)

Quoted 23 lines

Quoted 3 lines

Problems with current implementation:
1. reclaimable_bytes is signed while chunk_sz is unsigned, causing negative reclaimable_bytes to trigger reclaim unexpectedly

Agreed, completely. IMO, the message here could use a bit more clarity that negative reclaimable_bytes is an intentionally designed condition so this is a fundamental bug, not a weird edge case.

Quoted 3 lines

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.

I think I understand what you are saying, but let me try to restate it myself and confirm:

Previously, due to bug 1, we were calling btrfs_set_periodic_reclaim_ready() too frequently (after an allocation that made the reclaimable_bytes negative). With bug 1 fixed, you really have to free a chunk_sz to call btrfs_set_periodic_reclaim_ready(). As a result, periodic_reclaim became way too conservative. Now the multi-sweep marking thing AND reclaim_ready mean we basically never reclaim, as the second sweep doesn't get triggered later.

Quoted 2 lines

Is that about right? I agree that this needs some re-thinking / fixing, if so.

Yes that is the case. Your statement is way much better than mine. A sweep always reclaim blockgroups marked by the previous sweep and mark blockgroups for the next sweep. That explains why the assumption is not proper and we need a new assumption to work on.

Quoted 10 lines

Quoted 4 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, then resumes when space_info changes or new reclaimable groups appear.

I think you are making a much bigger change than merely fixing the bugs in the original design, so it requires deeper explanation of the new invariants you are introducing. Clearly, I am one to talk about it, as I messed up with my attempt, but I still think it is very valuable for future readers. Thanks for bearing with me :)

Thank you for bring us such a cool feature so I can try it out, learn details of the implementation and bring some my opinions. I'm quite happy with these experience. I started with a simple fix but the change was getting bigger and bigger to reduce regressions and finally it's more like a redesign rather than a fix.

Sorry for the previous short but unclear statement and let me try to make it clear in the new year :)

Let's start with the new assumptions:
1. Reclaim is for getting more unallocated blockgroups/spaces.
2. Periodic reclaim should happen periodically :)

I think both users and developers will agree with this but I want to explain more about 2. Currently, the bg->reclaim_mark related logic and dynamic periodic reclaim logic both depend on 2. Your previous statement explains the former one very well and I'll focus on the latter one here.

When we have less than 10GiB unallocated space, dynamic periodic reclaim will kick in. With the filesystem getting fuller, the bg_reclaim_threshold is getting larger to make periodic reclaim more aggressive to reclaim some unallocated space. However, if we paused periodic reclaim when the threshold is small, like 19, then we'll never resume it even if we want to reclaim more aggressively with a larger threshold later. And dynamic periodic reclaim will not work as expected. This may happen when we first try reclaiming with threshold 19, while having some quite large extents in 10% used blockgroups and having no enough continuous free space in other 70+% full blockgroups. We'll fail to get more unallocated space, pause periodic reclaim, and not try reclaiming when the threshold gets larger than 79 later. This is why I resume periodic reclaim when threshold changes.

So the core issue of this part logic about periodic reclaim is when to pause and when to continue.

If we cannot free more unallocated blockgroups in periodic reclaim, we should pause it because we don't want to just move things from a blockgroup to a newly allocated blockgroup and then remove the old one periodically.

And we should continue periodic reclaim when we may be able to free more blockgroups.

For pausing, it's hard to tell if we're freeing more unallocated directly without changing current code. But in btrfs_reclaim_bgs_work() we have

btrfs_relocate_chunk()
->btrfs_remove_chunk()

    ->btrfs_remove_block_group()

So the old blockgroup is removed and we can see that in space_info->total_bytes.

Let's start with all the blockgroups having the same size and reclaiming a single blockgroup. If we succeeded moving extents into existing blockgroups, we'll not allocate a new one and space_info->total_bytes is getting smaller because we've freed the old one. If we failed moving things into existing bgs and allocated a new one, then space_info->total_bytes will not change.

When reclaiming n blockgroups, as long as we're using less space, such as n-1 new blockgroups are allocated or n new smaller blockgroups, we're making some progress and able to detect that from space_info->total_bytes.

Reclaim may racing with some other work that free/allocate blockgroups, which will lead to either an unnecessary extra periodic reclaim or an unexpected pause. An extra periodic reclaim is not a big thing. But the unexpected pause matters. This is the only regression currently and I think that will not happen frequently.

For continuing, rather than tracing used_bytes, I think we should care more about unused_bytes. This is inspired by calc_dynamic_reclaim_threshold(), which will return 0 if there's no enough unused space. And also care about reclaim_threshold to make dynamic periodic reclaim work.

Quoted 43 lines

I think the simplest pure "fix" is to change the current "ready = false" setting from unconditional on every run of periodic reclaim to only when a sweep actually queues a block group for reclaim. I have compiled but not yet tested the code I included in this mail :) We can do that more carefully once we agree how we want to move forward.

If you still want to re-design things on top of a fix like this one, I am totally open to that.

Thanks again for all your work on this, and happy new year, Boris

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

index 6babbe333741..aad402485763 100644
@@ -2095,12 +2095,13 @@ 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 urgent;
+	bool ret = false;
 
 	spin_lock(&space_info->lock);
 	urgent = is_reclaim_urgent(space_info);

@@ -2122,8 +2123,10 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 		}
 		bg->reclaim_mark++;
 		spin_unlock(&bg->lock);
-		if (reclaim)
+		if (reclaim) {
 			btrfs_mark_bg_to_reclaim(bg);
+			ret = true;
+		}
 		btrfs_put_block_group(bg);
 	}
 

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2147

Quoted 5 lines

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

@@ -2140,6 +2143,7 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 	}
 
 	up_read(&space_info->groups_sem);
+	return ret;
 }

It's a good idea to return bool here! I hadn't thought of it.

Quoted 18 lines

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

 void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes)

@@ -2149,7 +2153,8 @@ void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s6
 	lockdep_assert_held(&space_info->lock);
 	space_info->reclaimable_bytes += bytes;
 
-	if (space_info->reclaimable_bytes >= chunk_sz)
+	if (space_info->reclaimable_bytes > 0 &&
+	    space_info->reclaimable_bytes >= chunk_sz)
 		btrfs_set_periodic_reclaim_ready(space_info, true);
 }
 

@@ -2176,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);
 

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

Quoted 11 lines

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

 	return ret;

@@ -2190,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) {
 		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);
+		}

Even if we have blockgroups to reclaim in this turn, we still mark some bg->reclaim_mark and expect the next periodic reclaim, which will not run, might reclaim them.

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

Quoted 3 lines

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

 	}
 }

So I'm afraid this patch will introduce obvious regression on periodic reclaim. Add the "changed threshold" condition can alleviate this when dynamic reclaim is also enabled but will not work for fixed threshold. So I failed to come up with a simple fix patch with no obvious regression :(

Quoted 50 lines

Quoted 33 lines

CC Boris Burkov <[email protected]>

Fixes 813d4c6422516 ("btrfs: prevent pathological periodic reclaim loops")

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.c | 15 +++++++-------
 fs/btrfs/space-info.c  | 44 +++++++++++++++++++-----------------------
 fs/btrfs/space-info.h  | 28 ++++++++++++++++++---------
 3 files changed, 46 insertions(+), 41 deletions(-)

fs/btrfs/block-group.c +2 -0

index e417aba4c4c7..94a4068cd42a 100644
@@ -1871,6 +1871,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	while (!list_empty(&fs_info->reclaim_bgs)) {
 		u64 used;
 		u64 reserved;
+		u64 old_total;
 		int ret = 0;
 
 		bg = list_first_entry(&fs_info->reclaim_bgs,

@@ -1936,6 +1937,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 		}
 
 		spin_unlock(&bg->lock);
+		old_total = space_info->total_bytes;
 		spin_unlock(&space_info->lock);
 
 		/*

@@ -1988,14 +1990,14 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 			reserved = 0;
 			spin_lock(&space_info->lock);
 			space_info->reclaim_errors++;

fs/btrfs/block-group.c void btrfs_reclaim_bgs_work(struct work_struct *work) · line 2000

Quoted 10 lines

fs/btrfs/block-group.c +2 -2

-			if (READ_ONCE(space_info->periodic_reclaim))
-				space_info->periodic_reclaim_ready = false;
 			spin_unlock(&space_info->lock);
 		}
 		spin_lock(&space_info->lock);
 		space_info->reclaim_count++;
 		space_info->reclaim_bytes += used;
 		space_info->reclaim_bytes += reserved;
+		if (space_info->total_bytes < old_total)
+			btrfs_resume_periodic_reclaim(space_info);

Why is this here? We've just completed a reclaim, which I would expect to be neutral to the space_info->total_bytes (just moving them around). So if (any) unrelated freeing happens to happen while we are reclaiming, we resume? Doesn't seem wrong, but also seems a little specific and random. I am probably missing some aspect of your new design.

Quoted 2 lines

Put a different way, what is special about frees that happen *while* we are reclaiming?

As explained, I want to use this to detect if the reclaim freed some unallocated space.

Quoted 24 lines

Quoted 13 lines

fs/btrfs/block-group.c +0 -2

 		spin_unlock(&space_info->lock);
 
 next:

@@ -3730,8 +3732,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		space_info->bytes_reserved -= num_bytes;
 		space_info->bytes_used += num_bytes;
 		space_info->disk_used += num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, -num_bytes);
 		spin_unlock(&cache->lock);
 		spin_unlock(&space_info->lock);
 	} else {

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3744

Quoted 8 lines

fs/btrfs/block-group.c +1 -4

@@ -3741,12 +3741,11 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		btrfs_space_info_update_bytes_pinned(space_info, num_bytes);
 		space_info->bytes_used -= num_bytes;
 		space_info->disk_used -= num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, num_bytes);
-		else
-			reclaim = should_reclaim_block_group(cache, num_bytes);
+		reclaim = should_reclaim_block_group(cache, num_bytes);

I think this is a bug with periodic_reclaim == 1

Quoted 4 lines

In that case, if should_reclaim_block_group() returns true (could be a fixed or dynamic threshold), we will put that block group directly on the reclaim list, which is a complete contradiction of the point of periodic_reclaim.

I guess you mean just resume periodic reclaim is enough and put it on reclaim list is not necessary. I agree.

Quoted 9 lines

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3747

Quoted 4 lines

fs/btrfs/block-group.c +2 -0

 		spin_unlock(&cache->lock);
+		if (reclaim)
+			btrfs_resume_periodic_reclaim(space_info);

This also makes me wonder about the idea behind your change. If you want periodic reclaim to "pause" until a block group meets the condition and then we "resume", that's not exactly in the spirit of checking at a periodic cadence, rather than as block groups update.

Because IMO we need periodic reclaim to reclaim this blockgroup. So if it's paused, resume here so the next periodic reclaim will handle this blockgroup.

Quoted 67 lines

Quoted 52 lines

fs/btrfs/block-group.c +0 -0

 		spin_unlock(&space_info->lock);
 
 		btrfs_set_extent_bit(&trans->transaction->pinned_extents, bytenr,

fs/btrfs/space-info.c +9 -22

index 7b7b7255f7d8..de8bde1081be 100644
@@ -2119,48 +2119,44 @@ static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid)
 	 * really need a block group, do take a fresh one.
 	 */
 	if (try_again && urgent) {
-		try_again = false;
+		urgent = false;
 		goto again;
 	}
 
 	up_read(&space_info->groups_sem);
-}
-
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes)
-{
-	u64 chunk_sz = calc_effective_data_chunk_size(space_info->fs_info);
-
-	lockdep_assert_held(&space_info->lock);
-	space_info->reclaimable_bytes += bytes;
 
-	if (space_info->reclaimable_bytes >= chunk_sz)
-		btrfs_set_periodic_reclaim_ready(space_info, true);
-}
-
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready)
-{
-	lockdep_assert_held(&space_info->lock);
-	if (!READ_ONCE(space_info->periodic_reclaim))
-		return;
-	if (ready != space_info->periodic_reclaim_ready) {
-		space_info->periodic_reclaim_ready = ready;
-		if (!ready)
-			space_info->reclaimable_bytes = 0;
+	/*
+	 * Temporary pause periodic reclaim until reclaim make some progress.
+	 * This can prevent periodic reclaim keep happening but make no progress.
+	 */
+	if (!try_again) {
+		spin_lock(&space_info->lock);
+		btrfs_pause_periodic_reclaim(space_info);
+		spin_unlock(&space_info->lock);
 	}
 }
 
 static bool btrfs_should_periodic_reclaim(struct btrfs_space_info *space_info)
 {

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2150

Quoted 10 lines

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

 	bool ret;
+	u64 chunk_sz;
+	u64 unused;
 
 	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		return false;
 	if (!READ_ONCE(space_info->periodic_reclaim))
 		return false;
+	if (!READ_ONCE(space_info->periodic_reclaim_paused))
+		return true;

This condition doesn't feel like a "pause". If the "pause" is set to false, then we proceed no matter what, otherwise we check conditions? It should be something like:

Quoted 6 lines

if (READ_ONCE(space_info->periodic_reclaim_force))

        return true;

as it is acting like a "force" not a "not paused".

Hope that makes sense, it's obviously a bit tied up in language semantics..

Because assumption 2. if it's not paused, periodic reclaim should happen periodically. I use "pause" here because I expect periodic reclaim happen periodically, and setting "pause" to true express that the periodic reclaim should not keep ticking.

Quoted 11 lines

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2155

Quoted 7 lines

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

+
+	chunk_sz = calc_effective_data_chunk_size(space_info->fs_info);
 
 	spin_lock(&space_info->lock);
-	ret = space_info->periodic_reclaim_ready;
-	btrfs_set_periodic_reclaim_ready(space_info, false);
+	unused = space_info->total_bytes - space_info->bytes_used;

Probably makes sense to use a zoned aware wrapper for this, just in case we make this friendly with zones eventually.

Quoted 8 lines

fs/btrfs/space-info.c static void do_reclaim_sweep(struct btrfs_space_info *space_info, int raid) · line 2157

Quoted 2 lines

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

+	ret = (unused >= space_info->last_reclaim_unused + chunk_sz ||
+	       btrfs_calc_reclaim_threshold(space_info) != space_info->last_reclaim_threshold);

This second condition is quite interesting to me, I hadn't thought of it. I think it makes some sense to care again if the threshold changed, but it is a new behavior, rather than a fix, per-se.

What made you want to add this?

To make dynamic periodic reclaim work but it will also help for fixed threshold when it's changed :)

Quoted 53 lines

Quoted 53 lines

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

+	if (ret)
+		btrfs_resume_periodic_reclaim(space_info);
 	spin_unlock(&space_info->lock);
 
 	return ret;

fs/btrfs/space-info.h +19 -9

index 0703f24b23f7..a49a4c7b0a68 100644
@@ -214,14 +214,11 @@ struct btrfs_space_info {
 
 	/*
 	 * Periodic reclaim should be a no-op if a space_info hasn't
-	 * freed any space since the last time we tried.
+	 * freed any space since the last time we made no progress.
 	 */
-	bool periodic_reclaim_ready;
-
-	/*
-	 * Net bytes freed or allocated since the last reclaim pass.
-	 */
-	s64 reclaimable_bytes;
+	bool periodic_reclaim_paused;
+	int last_reclaim_threshold;
+	u64 last_reclaim_unused;
 };
 
 static inline bool btrfs_mixed_space_info(const struct btrfs_space_info *space_info)

@@ -301,9 +298,22 @@ void btrfs_dump_space_info_for_trans_abort(struct btrfs_fs_info *fs_info);
 void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info);
 u64 btrfs_account_ro_block_groups_free_space(struct btrfs_space_info *sinfo);
 
-void btrfs_space_info_update_reclaimable(struct btrfs_space_info *space_info, s64 bytes);
-void btrfs_set_periodic_reclaim_ready(struct btrfs_space_info *space_info, bool ready);
 int btrfs_calc_reclaim_threshold(const struct btrfs_space_info *space_info);
+static inline void btrfs_resume_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (space_info->periodic_reclaim_paused)
+		space_info->periodic_reclaim_paused = false;
+}
+static inline void btrfs_pause_periodic_reclaim(struct btrfs_space_info *space_info)
+{
+	lockdep_assert_held(&space_info->lock);
+	if (!space_info->periodic_reclaim_paused) {
+		space_info->periodic_reclaim_paused = true;
+		space_info->last_reclaim_threshold = btrfs_calc_reclaim_threshold(space_info);
+		space_info->last_reclaim_unused = space_info->total_bytes - space_info->bytes_used;
+	}
+}
 void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info);
 void btrfs_return_free_space(struct btrfs_space_info *space_info, u64 len);
 

Quoted 3 lines

Quoted 3 lines

Signature · 1 line

2.51.2

Thanks,
Sun Yangkai

Quoted 3 lines

And we should continue periodic reclaim when we may be able to free more blockgroups.

This assumption I would push back on. This was my initial assumption as well, but it is worth challenging. At Meta, we used to use the automatic threshold based reclaim that reclaims every bg that goes over threshold (25% for us) then back under. This actually creates a huge amount of reclaim and was the reason for even coming up with dynamic thresholds. In practice, switching to the buggy dynamic+periodic drastically reduced this extra reclaim.

Suppose we allocate some block extents / block groups then free half the space and fragment them. You might think it's best to rapidly compact everything. But as long as we have unallocated and the holes are big enough for allocations to come through and re-fill them, it's mostly OK.

We much more want to do a "minimal" amount of reclaim until we truly need it.

With all that said, your points above about the threshold going up but reclaim staying asleep make a lot of sense to me. That is a signal that the "need" is increasing.

Quoted 7 lines

For pausing, it's hard to tell if we're freeing more unallocated directly without changing current code. But in btrfs_reclaim_bgs_work() we have

btrfs_relocate_chunk()
->btrfs_remove_chunk()

    ->btrfs_remove_block_group()

Quoted 7 lines

So the old blockgroup is removed and we can see that in space_info->total_bytes.

Let's start with all the blockgroups having the same size and reclaiming a single blockgroup. If we succeeded moving extents into existing blockgroups, we'll not allocate a new one and space_info->total_bytes is getting smaller because we've freed the old one. If we failed moving things into existing bgs and allocated a new one, then space_info->total_bytes will not change.

Some of my comments on this point were silly, I was thinking about used_bytes which would not change, not total_bytes. Thanks for the detailed description to knock me out of that mistake.

Also, I'm not sure the logic about total_bytes is 100% ironclad, as I haven't carefully checked what block group allocation/freeing is possible concurrently over the span where you track the sizes.

Quoted 10 lines

When reclaiming n blockgroups, as long as we're using less space, such as n-1 new blockgroups are allocated or n new smaller blockgroups, we're making some progress and able to detect that from space_info->total_bytes.

Reclaim may racing with some other work that free/allocate blockgroups, which will lead to either an unnecessary extra periodic reclaim or an unexpected pause. An extra periodic reclaim is not a big thing. But the unexpected pause matters. This is the only regression currently and I think that will not happen frequently.

OK, we're on the same page with the possibility of some racing :) Your explanation makes sense and I don't think it's a huge deal or anything.

Quoted 7 lines

For continuing, rather than tracing used_bytes, I think we should care more about unused_bytes. This is inspired by calc_dynamic_reclaim_threshold(), which will return 0 if there's no enough unused space. And also care about reclaim_threshold to make dynamic periodic reclaim work.

First, I think let's agree what we want and don't want to pause:
I don't think we care about running the reclaim sweep and checking thresholds, updating marks, etc. That is relatively cheap. So if we change the pausing logic to still allow that work more often, it's OK. We really want to prevent triggering reclaims in tight fail loops.

With that said, let's consider all the cases again.

1. We scan all the bgs and don't attempt any reclaims.
2. We attempt a reclaim of BG X at threshold T and it completely fails to allocate a new home for extent E of X.
3. We attempt a reclaim of BG X at threshold T and it "succeeds" by allocating a new BG Y and moving all the extents from X to Y.
4. We attempt a reclaim of BG X at threshold T and it makes proper progress and puts some the extents of X into the existing bgs.

After case 1, I think we agree pausing isn't needed, and is in fact bad.

After cases 2 and 3, we want to avoid triggering more reclaim until we have evidence that it is likely a reclaim could possibly succeed. If unused gets worse since then, and the threshold goes up, it is *possible* a reclaim will succeed (we might pick a more full block group that happens to be full of smaller easy to allocate extents) but it is much more likely it won't. As far as I can tell, the best signal for unpausing reclaim for the "we have seen a reclaim fail" case is still "relative freeing" of extents.

After case 4, we don't *need* to pause, as it is going to increase unallocated and reduce the "pressure" on the reclaim system, but it doesn't fundamentally hurt either. We made some progress and we want to be conservative and not be too greedy. Let allocation breathe and do some work to fill the gaps up. However, this case is quite different from 2 and 3 in that a subsequent relocation seems likely enough to also succeed.

My original (designed) logic was "pause on 2,3,4; unpause on the conditions that unstick 2,3" which I think you have correctly argued is suboptimal. 4 can get stuck never reclaiming even as things get worse, even if we might succeed (we've never failed..)

So I think the best (short term) solution is:
"detect failure modes 2 and 3 and unpause on release"

I believe your patches already enact "detect failure modes 2 and 3" and we just disagree on the unpausing mechanism.

Quoted 20 lines

Quoted 20 lines

I think the simplest pure "fix" is to change the current "ready = false" setting from unconditional on every run of periodic reclaim to only when a sweep actually queues a block group for reclaim. I have compiled but not yet tested the code I included in this mail :) We can do that more carefully once we agree how we want to move forward.

If you still want to re-design things on top of a fix like this one, I am totally open to that.

Thanks again for all your work on this, and happy new year, Boris

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

@@ -2176,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);
 

Quoted 15 lines

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

Quoted 11 lines

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

 	return ret;

@@ -2190,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) {
 		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);
+		}

Even if we have blockgroups to reclaim in this turn, we still mark some bg->reclaim_mark and expect the next periodic reclaim, which will not run, might reclaim them.

Why won't it run? We only pause if we truly called btrfs_mark_bg_to_reclaim() (the word mark is unfortunately overloaded here... that function really queues the bg for a reclaim attempt)

Quoted 4 lines

Review void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info) · line 2202

Quoted 2 lines

(quoted diff) +0 -0

 	}
 }

So I'm afraid this patch will introduce obvious regression on periodic reclaim.

Quoted 4 lines

Add the "changed threshold" condition can alleviate this when dynamic reclaim is also enabled but will not work for fixed threshold. So I failed to come up with a simple fix patch with no obvious regression :(

Can you share a description of any workloads you've been testing against? I think that would also help frame the discussion to avoid me talking too much :D

The regression I can foresee here is that the successful passes will pause for too long (until the next chunk_sz of freeing)

We can also relax that condition to something more like an extent size (1M or BTRFS_MAX_EXTENT_SIZE perhaps) to make it a little gentler of a pause.

We can also unpause on "urgent". I think that would likely be sufficient. I'll do some actual experiments.. :)

Thanks,
Boris

Quoted 50 lines

Quoted 50 lines

Quoted 33 lines

CC Boris Burkov <[email protected]>

Fixes 813d4c6422516 ("btrfs: prevent pathological periodic reclaim loops")

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.c | 15 +++++++-------
 fs/btrfs/space-info.c  | 44 +++++++++++++++++++-----------------------
 fs/btrfs/space-info.h  | 28 ++++++++++++++++++---------
 3 files changed, 46 insertions(+), 41 deletions(-)

fs/btrfs/block-group.c +2 -0

index e417aba4c4c7..94a4068cd42a 100644
@@ -1871,6 +1871,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	while (!list_empty(&fs_info->reclaim_bgs)) {
 		u64 used;
 		u64 reserved;
+		u64 old_total;
 		int ret = 0;
 
 		bg = list_first_entry(&fs_info->reclaim_bgs,

@@ -1936,6 +1937,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 		}
 
 		spin_unlock(&bg->lock);
+		old_total = space_info->total_bytes;
 		spin_unlock(&space_info->lock);
 
 		/*

@@ -1988,14 +1990,14 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 			reserved = 0;
 			spin_lock(&space_info->lock);
 			space_info->reclaim_errors++;

fs/btrfs/block-group.c void btrfs_reclaim_bgs_work(struct work_struct *work) · line 2000

Quoted 10 lines

fs/btrfs/block-group.c +2 -2

-			if (READ_ONCE(space_info->periodic_reclaim))
-				space_info->periodic_reclaim_ready = false;
 			spin_unlock(&space_info->lock);
 		}
 		spin_lock(&space_info->lock);
 		space_info->reclaim_count++;
 		space_info->reclaim_bytes += used;
 		space_info->reclaim_bytes += reserved;
+		if (space_info->total_bytes < old_total)
+			btrfs_resume_periodic_reclaim(space_info);

Why is this here? We've just completed a reclaim, which I would expect to be neutral to the space_info->total_bytes (just moving them around). So if (any) unrelated freeing happens to happen while we are reclaiming, we resume? Doesn't seem wrong, but also seems a little specific and random. I am probably missing some aspect of your new design.

Quoted 6 lines

Quoted 2 lines

Put a different way, what is special about frees that happen *while* we are reclaiming?

As explained, I want to use this to detect if the reclaim freed some unallocated space.

Makes sense now.

Quoted 24 lines

Quoted 24 lines

Quoted 13 lines

fs/btrfs/block-group.c +0 -2

 		spin_unlock(&space_info->lock);
 
 next:

@@ -3730,8 +3732,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		space_info->bytes_reserved -= num_bytes;
 		space_info->bytes_used += num_bytes;
 		space_info->disk_used += num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, -num_bytes);
 		spin_unlock(&cache->lock);
 		spin_unlock(&space_info->lock);
 	} else {

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3744

Quoted 8 lines

fs/btrfs/block-group.c +1 -4

@@ -3741,12 +3741,11 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		btrfs_space_info_update_bytes_pinned(space_info, num_bytes);
 		space_info->bytes_used -= num_bytes;
 		space_info->disk_used -= num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, num_bytes);
-		else
-			reclaim = should_reclaim_block_group(cache, num_bytes);
+		reclaim = should_reclaim_block_group(cache, num_bytes);

I think this is a bug with periodic_reclaim == 1

Quoted 8 lines

Quoted 4 lines

In that case, if should_reclaim_block_group() returns true (could be a fixed or dynamic threshold), we will put that block group directly on the reclaim list, which is a complete contradiction of the point of periodic_reclaim.

I guess you mean just resume periodic reclaim is enough and put it on reclaim list is not necessary. I agree.

+1

Quoted 10 lines

Quoted 9 lines

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3747

Quoted 4 lines

fs/btrfs/block-group.c +2 -0

 		spin_unlock(&cache->lock);
+		if (reclaim)
+			btrfs_resume_periodic_reclaim(space_info);

This also makes me wonder about the idea behind your change. If you want periodic reclaim to "pause" until a block group meets the condition and then we "resume", that's not exactly in the spirit of checking at a periodic cadence, rather than as block groups update.

Quoted 3 lines

Because IMO we need periodic reclaim to reclaim this blockgroup. So if it's paused, resume here so the next periodic reclaim will handle this blockgroup.

As argued above, we want to resume when there is space in the "target" block groups, not an exciting "source". Otherwise the failed stuck case can keep hammering hopelessly.

Quoted 11 lines · Boris Burkov

Quoted 3 lines

And we should continue periodic reclaim when we may be able to free more blockgroups.

This assumption I would push back on. This was my initial assumption as well, but it is worth challenging. At Meta, we used to use the automatic threshold based reclaim that reclaims every bg that goes over threshold (25% for us) then back under. This actually creates a huge amount of reclaim and was the reason for even coming up with dynamic thresholds. In practice, switching to the buggy dynamic+periodic drastically reduced this extra reclaim.

It seems hard to make periodic reclaim work practically with fixed threshold, and maybe we can merge them into "auto reclaim" or something in future.

Quoted 4 lines

Suppose we allocate some block extents / block groups then free half the space and fragment them. You might think it's best to rapidly compact everything. But as long as we have unallocated and the holes are big enough for allocations to come through and re-fill them, it's mostly OK.

I agree. When we have enough unallocated space, the fragment of free space is not that bad and it may disappear automatically with future allocation.

Quoted 2 lines

We much more want to do a "minimal" amount of reclaim until we truly need it.

And I guess we also don't want a "cliff", or we'll only try once with a high threshold when "urgent" instead of using a dynamic threshold :)

Quoted 66 lines

With all that said, your points above about the threshold going up but reclaim staying asleep make a lot of sense to me. That is a signal that the "need" is increasing.

Quoted 7 lines

For pausing, it's hard to tell if we're freeing more unallocated directly without changing current code. But in btrfs_reclaim_bgs_work() we have

btrfs_relocate_chunk()
->btrfs_remove_chunk()

    ->btrfs_remove_block_group()

Quoted 7 lines

So the old blockgroup is removed and we can see that in space_info->total_bytes.

Let's start with all the blockgroups having the same size and reclaiming a single blockgroup. If we succeeded moving extents into existing blockgroups, we'll not allocate a new one and space_info->total_bytes is getting smaller because we've freed the old one. If we failed moving things into existing bgs and allocated a new one, then space_info->total_bytes will not change.

Some of my comments on this point were silly, I was thinking about used_bytes which would not change, not total_bytes. Thanks for the detailed description to knock me out of that mistake.

Also, I'm not sure the logic about total_bytes is 100% ironclad, as I haven't carefully checked what block group allocation/freeing is possible concurrently over the span where you track the sizes.

Quoted 10 lines

When reclaiming n blockgroups, as long as we're using less space, such as n-1 new blockgroups are allocated or n new smaller blockgroups, we're making some progress and able to detect that from space_info->total_bytes.

Reclaim may racing with some other work that free/allocate blockgroups, which will lead to either an unnecessary extra periodic reclaim or an unexpected pause. An extra periodic reclaim is not a big thing. But the unexpected pause matters. This is the only regression currently and I think that will not happen frequently.

OK, we're on the same page with the possibility of some racing :) Your explanation makes sense and I don't think it's a huge deal or anything.

Quoted 7 lines

For continuing, rather than tracing used_bytes, I think we should care more about unused_bytes. This is inspired by calc_dynamic_reclaim_threshold(), which will return 0 if there's no enough unused space. And also care about reclaim_threshold to make dynamic periodic reclaim work.

First, I think let's agree what we want and don't want to pause:
I don't think we care about running the reclaim sweep and checking thresholds, updating marks, etc. That is relatively cheap. So if we change the pausing logic to still allow that work more often, it's OK. We really want to prevent triggering reclaims in tight fail loops.

With that said, let's consider all the cases again.

1. We scan all the bgs and don't attempt any reclaims.
2. We attempt a reclaim of BG X at threshold T and it completely fails to allocate a new home for extent E of X.
3. We attempt a reclaim of BG X at threshold T and it "succeeds" by allocating a new BG Y and moving all the extents from X to Y.
4. We attempt a reclaim of BG X at threshold T and it makes proper progress and puts some the extents of X into the existing bgs.

Quoted 1 line

After case 1, I think we agree pausing isn't needed, and is in fact bad.

Yes!

Quoted 9 lines

After cases 2 and 3, we want to avoid triggering more reclaim until we have evidence that it is likely a reclaim could possibly succeed. If unused gets worse since then, and the threshold goes up, it is *possible* a reclaim will succeed (we might pick a more full block group that happens to be full of smaller easy to allocate extents) but it is much more likely it won't. As far as I can tell, the best signal for unpausing reclaim for the "we have seen a reclaim fail" case is still "relative freeing" of extents.

Quoted 7 lines

After case 4, we don't *need* to pause, as it is going to increase unallocated and reduce the "pressure" on the reclaim system, but it doesn't fundamentally hurt either. We made some progress and we want to be conservative and not be too greedy. Let allocation breathe and do some work to fill the gaps up. However, this case is quite different from 2 and 3 in that a subsequent relocation seems likely enough to also succeed.

Yes. I started as a user of this function, monitoring the threshold, blockgroups' used rates, and filesystem's unallocated space. So I got confused when there were still some blockgroups could be reclaimed but periodic reclaim stopped working.

While you are the author of the function and focus on resolving the problem behind (running out of unallocated space) so just reclaim some blockgroups is ok so we don't need to try that hard.

Quoted 10 lines

My original (designed) logic was "pause on 2,3,4; unpause on the conditions that unstick 2,3" which I think you have correctly argued is suboptimal. 4 can get stuck never reclaiming even as things get worse, even if we might succeed (we've never failed..)

So I think the best (short term) solution is:
"detect failure modes 2 and 3 and unpause on release"

I believe your patches already enact "detect failure modes 2 and 3" and we just disagree on the unpausing mechanism.

For unpausing, I think we both agree on

1. enough space freed
2. threshold changed to a larger value because the increase of "need"
3. do not pause or unpause on case 4

I agree other unpausing conditions are not necessary.

Quoted 36 lines

Quoted 20 lines

Quoted 20 lines

I think the simplest pure "fix" is to change the current "ready = false" setting from unconditional on every run of periodic reclaim to only when a sweep actually queues a block group for reclaim. I have compiled but not yet tested the code I included in this mail :) We can do that more carefully once we agree how we want to move forward.

If you still want to re-design things on top of a fix like this one, I am totally open to that.

Thanks again for all your work on this, and happy new year, Boris

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

@@ -2176,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);
 

Quoted 15 lines

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

Quoted 11 lines

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

 	return ret;

@@ -2190,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) {
 		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);
+		}

Even if we have blockgroups to reclaim in this turn, we still mark some bg->reclaim_mark and expect the next periodic reclaim, which will not run, might reclaim them.

Quoted 3 lines

Why won't it run? We only pause if we truly called btrfs_mark_bg_to_reclaim() (the word mark is unfortunately overloaded here... that function really queues the bg for a reclaim attempt)

After userspace stopped writing, we still need 2 periodic reclaims to finish our work and each of them will reclaim some of the bgs in my experience. But with this patch, after the first one reclaimed some bgs, the second one will not run. We might reclaim less and the behavior is a little different. I called this a regression. But if we focus on "just don't run out of unallocated space", this patch seems fine.

Quoted 4 lines

Quoted 4 lines

Review void btrfs_reclaim_sweep(const struct btrfs_fs_info *fs_info) · line 2202

Quoted 2 lines

(quoted diff) +0 -0

 	}
 }

So I'm afraid this patch will introduce obvious regression on periodic reclaim.

I called it regression here :)

Quoted 8 lines

Quoted 4 lines

Add the "changed threshold" condition can alleviate this when dynamic reclaim is also enabled but will not work for fixed threshold. So I failed to come up with a simple fix patch with no obvious regression :(

Can you share a description of any workloads you've been testing against? I think that would also help frame the discussion to avoid me talking too much :D

I test this feature on by BT machine. Download, fill the fs to nearly full and delete some files and repeat...

Quoted 8 lines

The regression I can foresee here is that the successful passes will pause for too long (until the next chunk_sz of freeing)

We can also relax that condition to something more like an extent size (1M or BTRFS_MAX_EXTENT_SIZE perhaps) to make it a little gentler of a pause.

We can also unpause on "urgent". I think that would likely be sufficient. I'll do some actual experiments.. :)

I love these two ideas and looking forward to your experiment results :)

Thanks,
Sun YangKai

Quoted 107 lines

Thanks,
Boris

Quoted 50 lines

Quoted 50 lines

Quoted 33 lines

CC Boris Burkov <[email protected]>

Fixes 813d4c6422516 ("btrfs: prevent pathological periodic reclaim loops")

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.c | 15 +++++++-------
 fs/btrfs/space-info.c  | 44 +++++++++++++++++++-----------------------
 fs/btrfs/space-info.h  | 28 ++++++++++++++++++---------
 3 files changed, 46 insertions(+), 41 deletions(-)

fs/btrfs/block-group.c +2 -0

index e417aba4c4c7..94a4068cd42a 100644
@@ -1871,6 +1871,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	while (!list_empty(&fs_info->reclaim_bgs)) {
 		u64 used;
 		u64 reserved;
+		u64 old_total;
 		int ret = 0;
 
 		bg = list_first_entry(&fs_info->reclaim_bgs,

@@ -1936,6 +1937,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 		}
 
 		spin_unlock(&bg->lock);
+		old_total = space_info->total_bytes;
 		spin_unlock(&space_info->lock);
 
 		/*

@@ -1988,14 +1990,14 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 			reserved = 0;
 			spin_lock(&space_info->lock);
 			space_info->reclaim_errors++;

fs/btrfs/block-group.c void btrfs_reclaim_bgs_work(struct work_struct *work) · line 2000

Quoted 10 lines

fs/btrfs/block-group.c +2 -2

-			if (READ_ONCE(space_info->periodic_reclaim))
-				space_info->periodic_reclaim_ready = false;
 			spin_unlock(&space_info->lock);
 		}
 		spin_lock(&space_info->lock);
 		space_info->reclaim_count++;
 		space_info->reclaim_bytes += used;
 		space_info->reclaim_bytes += reserved;
+		if (space_info->total_bytes < old_total)
+			btrfs_resume_periodic_reclaim(space_info);

Why is this here? We've just completed a reclaim, which I would expect to be neutral to the space_info->total_bytes (just moving them around). So if (any) unrelated freeing happens to happen while we are reclaiming, we resume? Doesn't seem wrong, but also seems a little specific and random. I am probably missing some aspect of your new design.

Quoted 6 lines

Quoted 2 lines

Put a different way, what is special about frees that happen *while* we are reclaiming?

As explained, I want to use this to detect if the reclaim freed some unallocated space.

Makes sense now.

Quoted 24 lines

Quoted 24 lines

Quoted 13 lines

fs/btrfs/block-group.c +0 -2

 		spin_unlock(&space_info->lock);
 
 next:

@@ -3730,8 +3732,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		space_info->bytes_reserved -= num_bytes;
 		space_info->bytes_used += num_bytes;
 		space_info->disk_used += num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, -num_bytes);
 		spin_unlock(&cache->lock);
 		spin_unlock(&space_info->lock);
 	} else {

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3744

Quoted 8 lines

fs/btrfs/block-group.c +1 -4

@@ -3741,12 +3741,11 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 		btrfs_space_info_update_bytes_pinned(space_info, num_bytes);
 		space_info->bytes_used -= num_bytes;
 		space_info->disk_used -= num_bytes * factor;
-		if (READ_ONCE(space_info->periodic_reclaim))
-			btrfs_space_info_update_reclaimable(space_info, num_bytes);
-		else
-			reclaim = should_reclaim_block_group(cache, num_bytes);
+		reclaim = should_reclaim_block_group(cache, num_bytes);

I think this is a bug with periodic_reclaim == 1

Quoted 8 lines

Quoted 4 lines

In that case, if should_reclaim_block_group() returns true (could be a fixed or dynamic threshold), we will put that block group directly on the reclaim list, which is a complete contradiction of the point of periodic_reclaim.

I guess you mean just resume periodic reclaim is enough and put it on reclaim list is not necessary. I agree.

+1

Quoted 10 lines

Quoted 9 lines

fs/btrfs/block-group.c int btrfs_update_block_group(struct btrfs_trans_handle *trans, · line 3747

Quoted 4 lines

fs/btrfs/block-group.c +2 -0

 		spin_unlock(&cache->lock);
+		if (reclaim)
+			btrfs_resume_periodic_reclaim(space_info);

This also makes me wonder about the idea behind your change. If you want periodic reclaim to "pause" until a block group meets the condition and then we "resume", that's not exactly in the spirit of checking at a periodic cadence, rather than as block groups update.

Quoted 7 lines

Quoted 3 lines

Because IMO we need periodic reclaim to reclaim this blockgroup. So if it's paused, resume here so the next periodic reclaim will handle this blockgroup.

As argued above, we want to resume when there is space in the "target" block groups, not an exciting "source". Otherwise the failed stuck case can keep hammering hopelessly.

Make sense.

Given this and your other comments below about my proposed patch, would you like to send a v3 which mixes our two approaches and does something like:

- no new "pause" concept, keep the reclaim_ready (like my patch)
- only set ready = false in cases 2 and 3 (like your patch)

In that case we will *not* stop on pure success, and we will stop on both failures modes.

Going forward on top of that with deeper testing, there can be more changes like "special pause on 4 which gets unpaused on threshold change" or something. Or "always unpause on threshold change" and of course more radical ideas.

I'd just like to keep moving towards at least fixing the obvious horrible bug.

Thanks,
Boris

Reorder struct btrfs_block_group fields to improve packing and reduce memory footprint from 624 to 600 bytes (24 bytes saved per instance).

Here's pahole output after this change:

struct btrfs_block_group {

        struct btrfs_fs_info *     fs_info;              /*     0     8 */
        struct btrfs_inode *       inode;                /*     8     8 */
        u64                        start;                /*    16     8 */
        u64                        length;               /*    24     8 */
        u64                        pinned;               /*    32     8 */
        u64                        reserved;             /*    40     8 */
        u64                        used;                 /*    48     8 */
        u64                        delalloc_bytes;       /*    56     8 */

/* --- cacheline 1 boundary (64 bytes) --- */
u64 bytes_super; /* 64 8 */
u64 flags; /* 72 8 */
u64 cache_generation; /* 80 8 */
u64 global_root_id; /* 88 8 */
u64 commit_used; /* 96 8 */
u32 bitmap_high_thresh; /* 104 4 */
u32 bitmap_low_thresh; /* 108 4 */
struct rw_semaphore data_rwsem; /* 112 40 */
/* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */
unsigned long full_stripe_len; /* 152 8 */
unsigned long runtime_flags; /* 160 8 */
spinlock_t lock; /* 168 4 */
unsigned int ro; /* 172 4 */
enum btrfs_disk_cache_state disk_cache_state; /* 176 4 */
enum btrfs_caching_type cached; /* 180 4 */
struct btrfs_caching_control * caching_ctl; /* 184 8 */
/* --- cacheline 3 boundary (192 bytes) --- */
struct btrfs_space_info * space_info; /* 192 8 */
struct btrfs_free_space_ctl * free_space_ctl; /* 200 8 */
struct rb_node cache_node; /* 208 24 */
struct list_head list; /* 232 16 */
struct list_head cluster_list; /* 248 16 */
/* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
struct list_head bg_list; /* 264 16 */
struct list_head ro_list; /* 280 16 */
refcount_t refs; /* 296 4 */
atomic_t frozen; /* 300 4 */
struct list_head discard_list; /* 304 16 */
/* --- cacheline 5 boundary (320 bytes) --- */
enum btrfs_discard_state discard_state; /* 320 4 */
int discard_index; /* 324 4 */
u64 discard_eligible_time; /* 328 8 */
u64 discard_cursor; /* 336 8 */
struct list_head dirty_list; /* 344 16 */
struct list_head io_list; /* 360 16 */
struct btrfs_io_ctl io_ctl; /* 376 72 */
/* --- cacheline 7 boundary (448 bytes) --- */
atomic_t reservations; /* 448 4 */
atomic_t nocow_writers; /* 452 4 */
struct mutex free_space_lock; /* 456 32 */
bool using_free_space_bitmaps; /* 488 1 */
bool using_free_space_bitmaps_cached; /* 489 1 */
bool reclaim_mark; /* 490 1 */

/* XXX 1 byte hole, try to pack */

int swap_extents; /* 492 4 */
u64 alloc_offset; /* 496 8 */
u64 zone_unusable; /* 504 8 */
/* --- cacheline 8 boundary (512 bytes) --- */
u64 zone_capacity; /* 512 8 */
u64 meta_write_pointer; /* 520 8 */
struct btrfs_chunk_map * physical_map; /* 528 8 */
struct list_head active_bg_list; /* 536 16 */
struct work_struct zone_finish_work; /* 552 32 */
/* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */
struct extent_buffer * last_eb; /* 584 8 */
enum btrfs_block_group_size_class size_class; /* 592 4 */

/* size: 600, cachelines: 10, members: 56 */ /* sum members: 595, holes: 1, sum holes: 1 */ /* padding: 4 */
/* last cacheline: 24 bytes */
};

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

fs/btrfs/block-group.h +5 -5

index 3b3c61b3af2c..88c2e3a0a5a0 100644
@@ -118,7 +118,6 @@ struct btrfs_caching_control {
 struct btrfs_block_group {
        struct btrfs_fs_info *fs_info;
        struct btrfs_inode *inode;
-       spinlock_t lock;
        u64 start;
        u64 length;
        u64 pinned;

@@ -159,6 +158,8 @@ struct btrfs_block_group {
        unsigned long full_stripe_len;
        unsigned long runtime_flags;
 
+       spinlock_t lock;
+
        unsigned int ro;
 
        int disk_cache_state;

@@ -178,8 +179,6 @@ struct btrfs_block_group {
        /* For block groups in the same raid type */
        struct list_head list;
 
-       refcount_t refs;
-
        /*
         * List of struct btrfs_free_clusters for this block group.
         * Today it will only have one thing on it, but that may change

@@ -199,6 +198,8 @@ struct btrfs_block_group {
        /* For read-only block groups */
        struct list_head ro_list;
 
+       refcount_t refs;
+
        /*
         * When non-zero it means the block group's logical address and its
         * device extents can not be reused for future block group allocations

@@ -211,10 +212,10 @@ struct btrfs_block_group {
 
        /* For discard operations */
        struct list_head discard_list;
+       enum btrfs_discard_state discard_state;
        int discard_index;
        u64 discard_eligible_time;
        u64 discard_cursor;
-       enum btrfs_discard_state discard_state;
 
        /* For dirty block groups */
        struct list_head dirty_list;
--

2.51.2

Quoted 3 lines · Sun YangKai

Reorder struct btrfs_block_group fields to improve packing and reduce memory footprint from 624 to 600 bytes (24 bytes saved per instance).

The memory footprint is not reduced.

The structure's size is reduced, yes, but we are allocating block groups using kzalloc, so we end up still using the kmalloc-1k slab. Unless we could reduce the structure's size to 512 bytes, and therefore use the kmalloc-512 slab, we are not saving any memory.

The number of cache lines also remains the same, so no improvements there. Changing the order of the fields could also have an impact in the caching (either for the best or the worst), but I don't think it will be significantly visible for any realistic workload.

Thanks.

Quoted 138 lines

Here's pahole output after this change:

struct btrfs_block_group {

        struct btrfs_fs_info *     fs_info;              /*     0     8 */
        struct btrfs_inode *       inode;                /*     8     8 */
        u64                        start;                /*    16     8 */
        u64                        length;               /*    24     8 */
        u64                        pinned;               /*    32     8 */
        u64                        reserved;             /*    40     8 */
        u64                        used;                 /*    48     8 */
        u64                        delalloc_bytes;       /*    56     8 */

/* --- cacheline 1 boundary (64 bytes) --- */
u64 bytes_super; /* 64 8 */
u64 flags; /* 72 8 */
u64 cache_generation; /* 80 8 */
u64 global_root_id; /* 88 8 */
u64 commit_used; /* 96 8 */
u32 bitmap_high_thresh; /* 104 4 */
u32 bitmap_low_thresh; /* 108 4 */
struct rw_semaphore data_rwsem; /* 112 40 */
/* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */
unsigned long full_stripe_len; /* 152 8 */
unsigned long runtime_flags; /* 160 8 */
spinlock_t lock; /* 168 4 */
unsigned int ro; /* 172 4 */
enum btrfs_disk_cache_state disk_cache_state; /* 176 4 */
enum btrfs_caching_type cached; /* 180 4 */
struct btrfs_caching_control * caching_ctl; /* 184 8 */
/* --- cacheline 3 boundary (192 bytes) --- */
struct btrfs_space_info * space_info; /* 192 8 */
struct btrfs_free_space_ctl * free_space_ctl; /* 200 8 */
struct rb_node cache_node; /* 208 24 */
struct list_head list; /* 232 16 */
struct list_head cluster_list; /* 248 16 */
/* --- cacheline 4 boundary (256 bytes) was 8 bytes ago --- */
struct list_head bg_list; /* 264 16 */
struct list_head ro_list; /* 280 16 */
refcount_t refs; /* 296 4 */
atomic_t frozen; /* 300 4 */
struct list_head discard_list; /* 304 16 */
/* --- cacheline 5 boundary (320 bytes) --- */
enum btrfs_discard_state discard_state; /* 320 4 */
int discard_index; /* 324 4 */
u64 discard_eligible_time; /* 328 8 */
u64 discard_cursor; /* 336 8 */
struct list_head dirty_list; /* 344 16 */
struct list_head io_list; /* 360 16 */
struct btrfs_io_ctl io_ctl; /* 376 72 */
/* --- cacheline 7 boundary (448 bytes) --- */
atomic_t reservations; /* 448 4 */
atomic_t nocow_writers; /* 452 4 */
struct mutex free_space_lock; /* 456 32 */
bool using_free_space_bitmaps; /* 488 1 */
bool using_free_space_bitmaps_cached; /* 489 1 */
bool reclaim_mark; /* 490 1 */

/* XXX 1 byte hole, try to pack */

int swap_extents; /* 492 4 */
u64 alloc_offset; /* 496 8 */
u64 zone_unusable; /* 504 8 */
/* --- cacheline 8 boundary (512 bytes) --- */
u64 zone_capacity; /* 512 8 */
u64 meta_write_pointer; /* 520 8 */
struct btrfs_chunk_map * physical_map; /* 528 8 */
struct list_head active_bg_list; /* 536 16 */
struct work_struct zone_finish_work; /* 552 32 */
/* --- cacheline 9 boundary (576 bytes) was 8 bytes ago --- */
struct extent_buffer * last_eb; /* 584 8 */
enum btrfs_block_group_size_class size_class; /* 592 4 */

/* size: 600, cachelines: 10, members: 56 */ /* sum members: 595, holes: 1, sum holes: 1 */ /* padding: 4 */
/* last cacheline: 24 bytes */
};

Signed-off-by Sun YangKai <[email protected]>

---
 fs/btrfs/block-group.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

fs/btrfs/block-group.h +5 -5

index 3b3c61b3af2c..88c2e3a0a5a0 100644
@@ -118,7 +118,6 @@ struct btrfs_caching_control {
 struct btrfs_block_group {
        struct btrfs_fs_info *fs_info;
        struct btrfs_inode *inode;
-       spinlock_t lock;
        u64 start;
        u64 length;
        u64 pinned;

@@ -159,6 +158,8 @@ struct btrfs_block_group {
        unsigned long full_stripe_len;
        unsigned long runtime_flags;
 
+       spinlock_t lock;
+
        unsigned int ro;
 
        int disk_cache_state;

@@ -178,8 +179,6 @@ struct btrfs_block_group {
        /* For block groups in the same raid type */
        struct list_head list;
 
-       refcount_t refs;
-
        /*
         * List of struct btrfs_free_clusters for this block group.
         * Today it will only have one thing on it, but that may change

@@ -199,6 +198,8 @@ struct btrfs_block_group {
        /* For read-only block groups */
        struct list_head ro_list;
 
+       refcount_t refs;
+
        /*
         * When non-zero it means the block group's logical address and its
         * device extents can not be reused for future block group allocations

@@ -211,10 +212,10 @@ struct btrfs_block_group {
 
        /* For discard operations */
        struct list_head discard_list;
+       enum btrfs_discard_state discard_state;
        int discard_index;
        u64 discard_eligible_time;
        u64 discard_cursor;
-       enum btrfs_discard_state discard_state;
 
        /* For dirty block groups */
        struct list_head dirty_list;
--

2.51.2