Skip to contentSkip to conversation

jbd2: bound j_list_lock hold time in the checkpoint shrinker

PATCH Applied Max Kellermann 6 participants 13 messages Jul 13 – Jul 23

Subsystems (2)
Stable branch Journalling layer for block devices (JBD2)
lore
Share & tools
lore

Max Kellermann

author
#0 lore raw

cover jbd2: bound j_list_lock hold time in the checkpoint shrinker

We hit recurring RCU stalls / soft lockups on a busy ext4 filesystem that serves as the CacheFiles backing store for a Ceph (fscache) client. The stall is always the journal commit thread spinning on journal->j_list_lock:

  rcu: INFO: rcu_sched self-detected stall on CPU
  rcu:   106-....: (2099 ticks this GP) ... (t=2100 jiffies)
  CPU: 106 Comm: jbd2/nvme1n1p1-
  RIP: 0010:queued_spin_lock_slowpath+0x20a/0x240
  Call Trace:
   jbd2_journal_write_metadata_buffer+0x1c0/0x310
   jbd2_journal_commit_transaction+0x5e2/0x16e0
   kjournald2+0xa1/0x220
   kthread+0xe4/0x1d0

kjournald2 has already passed the "wait for outstanding handles" barrier and is in the metadata write-out loop; it is simply unable to acquire j_list_lock for >21s. The lock holder is the jbd2 checkpoint shrinker.

Under memory pressure, the shrinker (jbd2_journal_shrink_scan -> jbd2_journal_shrink_checkpoint_list -> journal_shrink_one_cp_list) walks a transaction's checkpoint list under j_list_lock. On this workload, the lists are large and dominated by busy buffers (dirty / under writeback / attached to the running transaction), and journal_shrink_one_cp_list() can therefore hold j_list_lock for a time proportional to the whole list length, for two reasons:

1. The JBD2_SHRINK_BUSY_SKIP path uses "continue", which also skips the need_resched() check, so the scan does not yield even when a reschedule is pending.

2. The scan budget (nr_to_scan) is decremented only for buffers that are actually freed, so busy buffers do not consume it and the loop is not bounded by the shrinker's batch at all.

Both were introduced by commit b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()"), which folded journal_clean_one_cp_list() into journal_shrink_one_cp_list() and dropped the per-examined-buffer budget that the shrinker previously had.

The result is a self-reinforcing collapse: shrinker instances across many CPUs hold/contend j_list_lock, kjournald2 cannot commit, the journal fills, CacheFiles lookups block in ext4, fscache cookies get stuck in LOOKING_UP, and netfs I/O times out.

Max Kellermann (2):
jbd2: check need_resched() when skipping busy checkpoint buffers
jbd2: bound shrinker scans by examined checkpoint buffers

 fs/jbd2/checkpoint.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
Signature · 1 line
2.47.3
author
#1 lore raw

PATCH 1/2 jbd2: check need_resched() when skipping busy checkpoint buffers

journal_shrink_one_cp_list() skips busy checkpoint buffers when called with JBD2_SHRINK_BUSY_SKIP. The continue statement on this path also skips the need_resched() check at the end of the loop body.

Consequently, when a checkpoint list contains mostly busy buffers, the shrinker can walk the entire list while holding journal->j_list_lock, even when a reschedule has been requested. Large checkpoint lists under memory pressure can therefore cause long lock hold times and leave other CPUs spinning on j_list_lock, resulting in soft lockups or RCU stalls.

Route the busy-buffer path through the need_resched() check so that the shrinker can release j_list_lock and reschedule promptly, restoring parity with the clean-buffer path, which already checks need_resched(). This does not change which checkpoint buffers are eligible for removal.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>
---
 fs/jbd2/checkpoint.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/jbd2/checkpoint.c +2 -1
index 1508e2f54462..5266017565ac 100644
@@ -389,7 +389,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			ret = jbd2_journal_try_remove_checkpoint(jh);
 			if (ret < 0) {
 				if (type == JBD2_SHRINK_BUSY_SKIP)
-					continue;
+					goto next;
 				break;
 			}
 		}

@@ -400,6 +400,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			break;
 		}
 
+next:
 		if (need_resched())
 			break;
 	} while (jh != last_jh);
Signature · 1 line
2.47.3
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] jbd2: check need_resched() when skipping busy checkpoint buffers

Quoted 10 lines · Max Kellermann

journal_shrink_one_cp_list() skips busy checkpoint buffers when called with JBD2_SHRINK_BUSY_SKIP. The continue statement on this path also skips the need_resched() check at the end of the loop body.

Consequently, when a checkpoint list contains mostly busy buffers, the shrinker can walk the entire list while holding journal->j_list_lock, even when a reschedule has been requested. Large checkpoint lists under memory pressure can therefore cause long lock hold times and leave other CPUs spinning on j_list_lock, resulting in soft lockups or RCU stalls.

Quoted 8 lines

Route the busy-buffer path through the need_resched() check so that the shrinker can release j_list_lock and reschedule promptly, restoring parity with the clean-buffer path, which already checks need_resched(). This does not change which checkpoint buffers are eligible for removal.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

Good catch! Thanks for fixing this. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 28 lines
---
 fs/jbd2/checkpoint.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/jbd2/checkpoint.c +2 -1
index 1508e2f54462..5266017565ac 100644
@@ -389,7 +389,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			ret = jbd2_journal_try_remove_checkpoint(jh);
 			if (ret < 0) {
 				if (type == JBD2_SHRINK_BUSY_SKIP)
-					continue;
+					goto next;
 				break;
 			}
 		}

@@ -400,6 +400,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			break;
 		}
 
+next:
 		if (need_resched())
 			break;
 	} while (jh != last_jh);
Signature · 1 line
2.47.3
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
Reviewed-by on v1 1/2
#3 lore raw

Re: [PATCH 1/2] jbd2: check need_resched() when skipping busy checkpoint buffers

In reply to Max Kellermann
Quoted 10 lines · Max Kellermann

journal_shrink_one_cp_list() skips busy checkpoint buffers when called with JBD2_SHRINK_BUSY_SKIP. The continue statement on this path also skips the need_resched() check at the end of the loop body.

Consequently, when a checkpoint list contains mostly busy buffers, the shrinker can walk the entire list while holding journal->j_list_lock, even when a reschedule has been requested. Large checkpoint lists under memory pressure can therefore cause long lock hold times and leave other CPUs spinning on j_list_lock, resulting in soft lockups or RCU stalls.

Quoted 8 lines

Route the busy-buffer path through the need_resched() check so that the shrinker can release j_list_lock and reschedule promptly, restoring parity with the clean-buffer path, which already checks need_resched(). This does not change which checkpoint buffers are eligible for removal.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

Thanks for the fix! This looks good to me.

Reviewed-by Zhang Yi <[email protected]>
Quoted 25 lines
---
  fs/jbd2/checkpoint.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
fs/jbd2/checkpoint.c +2 -1
index 1508e2f54462..5266017565ac 100644
@@ -389,7 +389,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
  			ret = jbd2_journal_try_remove_checkpoint(jh);
  			if (ret < 0) {
  				if (type == JBD2_SHRINK_BUSY_SKIP)
-					continue;
+					goto next;
  				break;
  			}
  		}

@@ -400,6 +400,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
  			break;
  		}
 
+next:
  		if (need_resched())
  			break;
  	} while (jh != last_jh);
#4 lore raw

Re: [PATCH 1/2] jbd2: check need_resched() when skipping busy checkpoint buffers

In reply to Max Kellermann
Quoted 11 lines · Max Kellermann

journal_shrink_one_cp_list() skips busy checkpoint buffers when called with JBD2_SHRINK_BUSY_SKIP. The continue statement on this path also skips the need_resched() check at the end of the loop body.

Consequently, when a checkpoint list contains mostly busy buffers, the shrinker can walk the entire list while holding journal->j_list_lock, even when a reschedule has been requested. Large checkpoint lists under memory pressure can therefore cause long lock hold times and leave other CPUs spinning on j_list_lock, resulting in soft lockups or RCU stalls.

Quoted 4 lines

Route the busy-buffer path through the need_resched() check so that the shrinker can release j_list_lock and reschedule promptly, restoring parity with the clean-buffer path, which already checks need_resched(). This does not change which checkpoint buffers are eligible for removal.

One question I had about this patch is whether the list cleanup could get into a livelock if it becomes too large? If the iterator is processing a large number of jh that cannot be freed, then need_resched() is hit, if it restarts from the beginning of the list each time without processing at least one jh it will not make forward progress.

Cheers, Andreas

Quoted 32 lines
Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

---
fs/jbd2/checkpoint.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)
fs/jbd2/checkpoint.c +2 -1
index 1508e2f54462..5266017565ac 100644
@@ -389,7 +389,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			ret = jbd2_journal_try_remove_checkpoint(jh);
 			if (ret < 0) {
 				if (type == JBD2_SHRINK_BUSY_SKIP)
-					continue;
+					goto next;
 				break;
 			}
 		}

@@ -400,6 +400,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 			break;
 		}
 
+next:
 		if (need_resched())
 			break;
 	} while (jh != last_jh);
Signature · 1 line
2.47.3

Cheers, Andreas

author
#5 lore raw

Re: [PATCH 1/2] jbd2: check need_resched() when skipping busy checkpoint buffers

Quoted 2 lines · Andreas Dilger

One question I had about this patch is whether the list cleanup could get into a livelock if it becomes too large?

It was a livelock *without* this patch, because all CPUs were busy inside this piece of shrinker code; one of them holding j_list_lock, all others spinning on that very spinlock (all CPUs busy), and actal I/O was stalled seemingly forever.

Quoted 4 lines

If the iterator is processing
a large number of jh that cannot be freed, then need_resched() is hit, if it restarts from the beginning of the list each time without processing at least one jh it will not make forward progress.

If no progress is made, then this shrinker call just wasn't able to free anything. That was the behavior before b98dba273a0e ("jbd2:
remove journal_clean_one_cp_list()") and my patch restores this behavior. It's not a problem. It's faily normal for shrinkers to not be able to free anything.

Signature · 8 lines
Max Kellermann
Principal Architect
Hosting Technology

cm4all | Im Mediapark 6a | 50670 Köln | Germany
General information about the company can be found here:
https://www.cm4all.com/impressum
A member of the IONOS Group
author
#6 lore raw

PATCH 2/2 jbd2: bound shrinker scans by examined checkpoint buffers

In reply to Max Kellermann

The jbd2 shrinker currently accounts only checkpoint buffers that it successfully releases against nr_to_scan. Busy buffers therefore do not consume the scan budget.

If a checkpoint transaction contains mostly busy buffers, the shrinker can scan its entire checkpoint list while holding journal->j_list_lock. Large checkpoint lists can result in excessive lock hold times and leave other CPUs spinning on j_list_lock, causing soft lockups or RCU stalls.

Pass nr_to_scan into journal_shrink_one_cp_list() and decrement it for every buffer examined, including busy buffers. Pass NULL from checkpoint cleanup paths so their existing full-list behavior is preserved.

This restores the scan-budget semantics that existed before journal_shrink_one_cp_list() was changed to always scan a complete checkpoint list.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>
---
 fs/jbd2/checkpoint.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
fs/jbd2/checkpoint.c +13 -12
index 5266017565ac..513273712010 100644
@@ -358,15 +358,16 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
 /*
  * journal_shrink_one_cp_list
  *
- * Find all the written-back checkpoint buffers in the given list
- * and try to release them. If the whole transaction is released, set
- * the 'released' parameter. Return the number of released checkpointed
- * buffers.
+ * Find written-back checkpoint buffers in the given list and try to release
+ * them. If 'nr_to_scan' is set, scan at most that many buffers. If the whole
+ * transaction is released, set the 'released' parameter. Return the number of
+ * released checkpointed buffers.
  *
  * Called with j_list_lock held.
  */
 static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 						enum jbd2_shrink_type type,
+						unsigned long *nr_to_scan,
 						bool *released)
 {
 	struct journal_head *last_jh;

@@ -375,13 +376,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 	int ret;
 
 	*released = false;
-	if (!jh)
+	if (!jh || (nr_to_scan && !*nr_to_scan))
 		return 0;
 
 	last_jh = jh->b_cpprev;
 	do {
 		jh = next_jh;
 		next_jh = jh->b_cpnext;
+		if (nr_to_scan)
+			(*nr_to_scan)--;
 
 		if (type == JBD2_SHRINK_DESTROY) {
 			ret = __jbd2_journal_remove_checkpoint(jh);

@@ -403,7 +406,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 next:
 		if (need_resched())
 			break;
-	} while (jh != last_jh);
+	} while (jh != last_jh && (!nr_to_scan || *nr_to_scan));
 
 	return nr_freed;
 }

@@ -425,7 +428,6 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
 	tid_t first_tid = 0, last_tid = 0, next_tid = 0;
 	tid_t tid = 0;
 	unsigned long nr_freed = 0;
-	unsigned long freed;
 	bool first_set = false;
 
 again:

@@ -458,10 +460,9 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
 		next_transaction = transaction->t_cpnext;
 		tid = transaction->t_tid;
 
-		freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-						   JBD2_SHRINK_BUSY_SKIP, &released);
-		nr_freed += freed;
-		(*nr_to_scan) -= min(*nr_to_scan, freed);
+		nr_freed += journal_shrink_one_cp_list(transaction->t_checkpoint_list,
+						       JBD2_SHRINK_BUSY_SKIP,
+						       nr_to_scan, &released);
 		if (*nr_to_scan == 0)
 			break;
 		if (need_resched() || spin_needbreak(&journal->j_list_lock))

@@ -517,7 +518,7 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
 		transaction = next_transaction;
 		next_transaction = transaction->t_cpnext;
 		journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-					   type, &released);
+					   type, NULL, &released);
 		/*
 		 * This function only frees up some memory if possible so we
 		 * dont have an obligation to finish processing. Bail out if
Signature · 1 line
2.47.3
Reviewed-by on v1 2/2
#7 lore raw

Re: [PATCH 2/2] jbd2: bound shrinker scans by examined checkpoint buffers

Quoted 13 lines · Max Kellermann

The jbd2 shrinker currently accounts only checkpoint buffers that it successfully releases against nr_to_scan. Busy buffers therefore do not consume the scan budget.

If a checkpoint transaction contains mostly busy buffers, the shrinker can scan its entire checkpoint list while holding journal->j_list_lock. Large checkpoint lists can result in excessive lock hold times and leave other CPUs spinning on j_list_lock, causing soft lockups or RCU stalls.

Pass nr_to_scan into journal_shrink_one_cp_list() and decrement it for every buffer examined, including busy buffers. Pass NULL from checkpoint cleanup paths so their existing full-list behavior is preserved.

Quoted 7 lines

This restores the scan-budget semantics that existed before journal_shrink_one_cp_list() was changed to always scan a complete checkpoint list.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

Looks good. Feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 89 lines
---
 fs/jbd2/checkpoint.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
fs/jbd2/checkpoint.c +13 -12
index 5266017565ac..513273712010 100644
@@ -358,15 +358,16 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
 /*
  * journal_shrink_one_cp_list
  *
- * Find all the written-back checkpoint buffers in the given list
- * and try to release them. If the whole transaction is released, set
- * the 'released' parameter. Return the number of released checkpointed
- * buffers.
+ * Find written-back checkpoint buffers in the given list and try to release
+ * them. If 'nr_to_scan' is set, scan at most that many buffers. If the whole
+ * transaction is released, set the 'released' parameter. Return the number of
+ * released checkpointed buffers.
  *
  * Called with j_list_lock held.
  */
 static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 						enum jbd2_shrink_type type,
+						unsigned long *nr_to_scan,
 						bool *released)
 {
 	struct journal_head *last_jh;

@@ -375,13 +376,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 	int ret;
 
 	*released = false;
-	if (!jh)
+	if (!jh || (nr_to_scan && !*nr_to_scan))
 		return 0;
 
 	last_jh = jh->b_cpprev;
 	do {
 		jh = next_jh;
 		next_jh = jh->b_cpnext;
+		if (nr_to_scan)
+			(*nr_to_scan)--;
 
 		if (type == JBD2_SHRINK_DESTROY) {
 			ret = __jbd2_journal_remove_checkpoint(jh);

@@ -403,7 +406,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
 next:
 		if (need_resched())
 			break;
-	} while (jh != last_jh);
+	} while (jh != last_jh && (!nr_to_scan || *nr_to_scan));
 
 	return nr_freed;
 }

@@ -425,7 +428,6 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
 	tid_t first_tid = 0, last_tid = 0, next_tid = 0;
 	tid_t tid = 0;
 	unsigned long nr_freed = 0;
-	unsigned long freed;
 	bool first_set = false;
 
 again:

@@ -458,10 +460,9 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
 		next_transaction = transaction->t_cpnext;
 		tid = transaction->t_tid;
 
-		freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-						   JBD2_SHRINK_BUSY_SKIP, &released);
-		nr_freed += freed;
-		(*nr_to_scan) -= min(*nr_to_scan, freed);
+		nr_freed += journal_shrink_one_cp_list(transaction->t_checkpoint_list,
+						       JBD2_SHRINK_BUSY_SKIP,
+						       nr_to_scan, &released);
 		if (*nr_to_scan == 0)
 			break;
 		if (need_resched() || spin_needbreak(&journal->j_list_lock))

@@ -517,7 +518,7 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
 		transaction = next_transaction;
 		next_transaction = transaction->t_cpnext;
 		journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-					   type, &released);
+					   type, NULL, &released);
 		/*
 		 * This function only frees up some memory if possible so we
 		 * dont have an obligation to finish processing. Bail out if
Signature · 1 line
2.47.3
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
Reviewed-by on v1 2/2
#8 lore raw

Re: [PATCH 2/2] jbd2: bound shrinker scans by examined checkpoint buffers

In reply to Max Kellermann
Quoted 13 lines · Max Kellermann

The jbd2 shrinker currently accounts only checkpoint buffers that it successfully releases against nr_to_scan. Busy buffers therefore do not consume the scan budget.

If a checkpoint transaction contains mostly busy buffers, the shrinker can scan its entire checkpoint list while holding journal->j_list_lock. Large checkpoint lists can result in excessive lock hold times and leave other CPUs spinning on j_list_lock, causing soft lockups or RCU stalls.

Pass nr_to_scan into journal_shrink_one_cp_list() and decrement it for every buffer examined, including busy buffers. Pass NULL from checkpoint cleanup paths so their existing full-list behavior is preserved.

Quoted 7 lines

This restores the scan-budget semantics that existed before journal_shrink_one_cp_list() was changed to always scan a complete checkpoint list.

Fixes b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()")
Cc [email protected]
Signed-off-by Max Kellermann <[email protected]>

This makes the semantic of nr_to_scan more clear.

Reviewed-by Zhang Yi <[email protected]>
Quoted 86 lines
---
  fs/jbd2/checkpoint.c | 25 +++++++++++++------------
  1 file changed, 13 insertions(+), 12 deletions(-)
fs/jbd2/checkpoint.c +13 -12
index 5266017565ac..513273712010 100644
@@ -358,15 +358,16 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
  /*
   * journal_shrink_one_cp_list
   *
- * Find all the written-back checkpoint buffers in the given list
- * and try to release them. If the whole transaction is released, set
- * the 'released' parameter. Return the number of released checkpointed
- * buffers.
+ * Find written-back checkpoint buffers in the given list and try to release
+ * them. If 'nr_to_scan' is set, scan at most that many buffers. If the whole
+ * transaction is released, set the 'released' parameter. Return the number of
+ * released checkpointed buffers.
   *
   * Called with j_list_lock held.
   */
  static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
  						enum jbd2_shrink_type type,
+						unsigned long *nr_to_scan,
  						bool *released)
  {
  	struct journal_head *last_jh;

@@ -375,13 +376,15 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
  	int ret;
 
  	*released = false;
-	if (!jh)
+	if (!jh || (nr_to_scan && !*nr_to_scan))
  		return 0;
 
  	last_jh = jh->b_cpprev;
  	do {
  		jh = next_jh;
  		next_jh = jh->b_cpnext;
+		if (nr_to_scan)
+			(*nr_to_scan)--;
 
  		if (type == JBD2_SHRINK_DESTROY) {
  			ret = __jbd2_journal_remove_checkpoint(jh);

@@ -403,7 +406,7 @@ static unsigned long journal_shrink_one_cp_list(struct journal_head *jh,
  next:
  		if (need_resched())
  			break;
-	} while (jh != last_jh);
+	} while (jh != last_jh && (!nr_to_scan || *nr_to_scan));
 
  	return nr_freed;
  }

@@ -425,7 +428,6 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
  	tid_t first_tid = 0, last_tid = 0, next_tid = 0;
  	tid_t tid = 0;
  	unsigned long nr_freed = 0;
-	unsigned long freed;
  	bool first_set = false;
 
  again:

@@ -458,10 +460,9 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
  		next_transaction = transaction->t_cpnext;
  		tid = transaction->t_tid;
 
-		freed = journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-						   JBD2_SHRINK_BUSY_SKIP, &released);
-		nr_freed += freed;
-		(*nr_to_scan) -= min(*nr_to_scan, freed);
+		nr_freed += journal_shrink_one_cp_list(transaction->t_checkpoint_list,
+						       JBD2_SHRINK_BUSY_SKIP,
+						       nr_to_scan, &released);
  		if (*nr_to_scan == 0)
  			break;
  		if (need_resched() || spin_needbreak(&journal->j_list_lock))

@@ -517,7 +518,7 @@ void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
  		transaction = next_transaction;
  		next_transaction = transaction->t_cpnext;
  		journal_shrink_one_cp_list(transaction->t_checkpoint_list,
-					   type, &released);
+					   type, NULL, &released);
  		/*
  		 * This function only frees up some memory if possible so we
  		 * dont have an obligation to finish processing. Bail out if
#9 lore raw
In reply to Max Kellermann
Quoted 5 lines · Max Kellermann

We hit recurring RCU stalls / soft lockups on a busy ext4 filesystem that serves as the CacheFiles backing store for a Ceph (fscache) client. The stall is always the journal commit thread spinning on journal->j_list_lock:

Quoted 9 lines
  rcu: INFO: rcu_sched self-detected stall on CPU
  rcu:   106-....: (2099 ticks this GP) ... (t=2100 jiffies)
  CPU: 106 Comm: jbd2/nvme1n1p1-
  RIP: 0010:queued_spin_lock_slowpath+0x20a/0x240
  Call Trace:
   jbd2_journal_write_metadata_buffer+0x1c0/0x310
   jbd2_journal_commit_transaction+0x5e2/0x16e0
   kjournald2+0xa1/0x220
   kthread+0xe4/0x1d0

Hi, Max!

Thanks for the report!

I'm a bit curious about which kernel version and CONFIG you encountered this issue on. IIRC, after commits [1] and [2], the spin_needbreak() in jbd2_journal_shrink_checkpoint_list() should always be effective (unless the hardware does not support it, i.e., ARCH_NO_PREEMPT). Moreover, the number of buffer_heads in the checkpoint list of a single transaction should not be large enough to trigger such soft lockups. So I suspect this issue should be unlikely to occur on the latest kernel releases.

[1] 7dadeaa6e851 ("sched: Further restrict the preemption modes")
[2] 7c70cb94d29c ("sched: Add Lazy preemption model")

Thanks,
Yi.

Quoted 40 lines

kjournald2 has already passed the "wait for outstanding handles" barrier and is in the metadata write-out loop; it is simply unable to acquire j_list_lock for >21s. The lock holder is the jbd2 checkpoint shrinker.

Under memory pressure, the shrinker (jbd2_journal_shrink_scan -> jbd2_journal_shrink_checkpoint_list -> journal_shrink_one_cp_list) walks a transaction's checkpoint list under j_list_lock. On this workload, the lists are large and dominated by busy buffers (dirty / under writeback / attached to the running transaction), and journal_shrink_one_cp_list() can therefore hold j_list_lock for a time proportional to the whole list length, for two reasons:

1. The JBD2_SHRINK_BUSY_SKIP path uses "continue", which also skips the need_resched() check, so the scan does not yield even when a reschedule is pending.

2. The scan budget (nr_to_scan) is decremented only for buffers that are actually freed, so busy buffers do not consume it and the loop is not bounded by the shrinker's batch at all.

Both were introduced by commit b98dba273a0e ("jbd2: remove journal_clean_one_cp_list()"), which folded journal_clean_one_cp_list() into journal_shrink_one_cp_list() and dropped the per-examined-buffer budget that the shrinker previously had.

The result is a self-reinforcing collapse: shrinker instances across many CPUs hold/contend j_list_lock, kjournald2 cannot commit, the journal fills, CacheFiles lookups block in ext4, fscache cookies get stuck in LOOKING_UP, and netfs I/O times out.

Max Kellermann (2):
jbd2: check need_resched() when skipping busy checkpoint buffers
jbd2: bound shrinker scans by examined checkpoint buffers

 fs/jbd2/checkpoint.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
author
#10 lore raw
Quoted 5 lines · Zhang Yi

I'm a bit curious about which kernel version and CONFIG you encountered this issue on. IIRC, after commits [1] and [2], the spin_needbreak() in jbd2_journal_shrink_checkpoint_list() should always be effective (unless the hardware does not support it, i.e., ARCH_NO_PREEMPT).

We run 6.18.38, and preemption is disabled. Therefore, spin_needbreak() is a no-op.
(We used to always run the latest stable, but got burned by way too many critical Ceph/netfs regressions and settled on the LTS.)

But even with preemption enabled, this would likely be a problem, though a much smaller one.

Quoted 4 lines

Moreover, the number of buffer_heads in the
checkpoint list of a single transaction should not be large enough to trigger such soft lockups. So I suspect this issue should be unlikely to occur on the latest kernel releases.

Unfortunately, I don't have any numbers for you, because this problem occurs only every other week, but we have been haunted by this for a long time.

The setup is a 7 TB ext4 partition dedicated to fscache/Ceph. It's always full, and culling is running quite often.

For culling, we don't use cachefilesd but our own reimplementation
(https://github.com/CM4all/cash) because cachefilesd scales quadratically and never makes any progress while burning CPU cycles and I/O forever; ours is massively parallel thanks to io_uring. That puts a lot of pressure on the ext4 filesystem, and a cgroup memory.high setting means it's always under memory pressure (or else the kernel will accumulate many gigabytes of RAM usage very quickly during the filesystem scan that is necessary for culling). Which means the shrinker is running all the time. Plus the usual kernel-initiated I/O on that partition.
Every time this problem occurred, fscache culling was active.

Signature · 8 lines
Max Kellermann
Principal Architect
Hosting Technology

cm4all | Im Mediapark 6a | 50670 Köln | Germany
General information about the company can be found here:
https://www.cm4all.com/impressum
A member of the IONOS Group
#11 lore raw
Quoted 27 lines · Max Kellermann
Quoted 5 lines · Zhang Yi

I'm a bit curious about which kernel version and CONFIG you encountered this issue on. IIRC, after commits [1] and [2], the spin_needbreak() in jbd2_journal_shrink_checkpoint_list() should always be effective (unless the hardware does not support it, i.e., ARCH_NO_PREEMPT).

We run 6.18.38, and preemption is disabled. Therefore, spin_needbreak() is a no-op.
(We used to always run the latest stable, but got burned by way too many critical Ceph/netfs regressions and settled on the LTS.)

But even with preemption enabled, this would likely be a problem, though a much smaller one.

Quoted 4 lines

Moreover, the number of buffer_heads in the
checkpoint list of a single transaction should not be large enough to trigger such soft lockups. So I suspect this issue should be unlikely to occur on the latest kernel releases.

Unfortunately, I don't have any numbers for you, because this problem occurs only every other week, but we have been haunted by this for a long time.

The setup is a 7 TB ext4 partition dedicated to fscache/Ceph. It's always full, and culling is running quite often.

Quoted 12 lines

For culling, we don't use cachefilesd but our own reimplementation
(https://github.com/CM4all/cash) because cachefilesd scales quadratically and never makes any progress while burning CPU cycles and I/O forever; ours is massively parallel thanks to io_uring. That puts a lot of pressure on the ext4 filesystem, and a cgroup memory.high setting means it's always under memory pressure (or else the kernel will accumulate many gigabytes of RAM usage very quickly during the filesystem scan that is necessary for culling). Which means the shrinker is running all the time. Plus the usual kernel-initiated I/O on that partition.
Every time this problem occurred, fscache culling was active.

Thank you for the details!

We recently encountered the same issue on older kernels (up to v6.6) during extreme memory pressure stress testing. Interestingly, we were unable to reproduce it on the latest v7.1 kernel.

After investigation, we identified that commit 7dadeaa6e851 ("sched:
Further restrict the preemption modes") effectively prevents disabling preemption entirely, which makes spin_needbreak() always work in practice. So I was previously curious whether you discovered this issue in the latest kernel.

However, let's come back to the point. I agree this is a real issue that deserves a proper fix, independent of the preemption model changes.

Thanks,
Yi.

maintainer Applied
#12 lore raw
In reply to Max Kellermann
Quoted 15 lines · Max Kellermann

We hit recurring RCU stalls / soft lockups on a busy ext4 filesystem that serves as the CacheFiles backing store for a Ceph (fscache) client. The stall is always the journal commit thread spinning on journal->j_list_lock:

  rcu: INFO: rcu_sched self-detected stall on CPU
  rcu:   106-....: (2099 ticks this GP) ... (t=2100 jiffies)
  CPU: 106 Comm: jbd2/nvme1n1p1-
  RIP: 0010:queued_spin_lock_slowpath+0x20a/0x240
  Call Trace:
   jbd2_journal_write_metadata_buffer+0x1c0/0x310
   jbd2_journal_commit_transaction+0x5e2/0x16e0
   kjournald2+0xa1/0x220
   kthread+0xe4/0x1d0
Quoted 1 line

[...]

Applied, thanks!

[1/2] jbd2: check need_resched() when skipping busy checkpoint buffers

      commit: f213e12ff5c9590b1034ae8da0e6d09665c772d0
[2/2] jbd2: bound shrinker scans by examined checkpoint buffers
      commit: 15cb16496446b94e67f7abcb049b8e2c75cd3d02

Best regards,

Signature · 1 line
Theodore Ts'o <[email protected]>