在 2025/7/30 18:52, Jan Kara 写道:
On Wed 30-07-25 09:47:06, Dai Junbing wrote:
Set the TASK_FREEZABLE flag when the kjournald2 kernel thread sleeps
during journal commit operations. This prevents premature wakeups
during system suspend/resume cycles, avoiding unnecessary CPU wakeups
and power consumption.
in this case, the original code:
prepare_to_wait(&journal->j_wait_commit, &wait,
TASK_INTERRUPTIBLE);
if (journal->j_commit_sequence != journal->j_commit_request)
should_sleep = 0;
transaction = journal->j_running_transaction;
if (transaction && time_after_eq(jiffies, transaction->t_expires))
should_sleep = 0;
......
......
if (should_sleep) {
write_unlock(&journal->j_state_lock);
schedule();
write_lock(&journal->j_state_lock);
}
is functionally equivalent to the more concise:
write_unlock(&journal->j_state_lock);
wait_event_freezable_exclusive(&journal->j_wait_commit,
journal->j_commit_sequence == journal->j_commit_request ||
(journal->j_running_transaction &&
time_after_eq(jiffies, transaction->t_expires)) ||
(journal->j_flags & JBD2_UNMOUNT));
write_lock(&journal->j_state_lock);
This would be actually wrong because you cannot safely do some of the
dereferences without holding j_state_lock. Luckily you didn't modify the
existing code in the patch, just the changelog is bogus so please fix it.
Thank you for pointing this out. I'll make the corresponding changelog
updates.
Signed-off-by: Dai Junbing <daijunbing@xxxxxxxx>
---
fs/jbd2/journal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index d480b94117cd..9a1def9f730b 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -222,7 +222,7 @@ static int kjournald2(void *arg)
DEFINE_WAIT(wait);
prepare_to_wait(&journal->j_wait_commit, &wait,
- TASK_INTERRUPTIBLE);
+ TASK_INTERRUPTIBLE | TASK_FREEZABLE);
So this looks fine but I have one question. There's code like:
if (freezing(current)) {
/*
* The simpler the better. Flushing journal isn't a
* good idea, because that depends on threads that may
* be already stopped.
*/
jbd2_debug(1, "Now suspending kjournald2\n");
write_unlock(&journal->j_state_lock);
try_to_freeze();
write_lock(&journal->j_state_lock);
a few lines above. Is it still needed after your change? I guess that
probably yes (e.g. when the freeze attempt happens while kjournald still
performs some work then the later schedule in TASK_FREEZABLE state doesn't
necessarily freeze the kthread). But getting a confirmation would be nice.
I agree with your perspective.
While cleaner implementations may exist,I haven't made changes due to
uncertainty about the alternatives>
Honza
transaction = journal->j_running_transaction;
if (transaction == NULL ||
time_before(jiffies, transaction->t_expires)) {
--
2.25.1
