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.
> 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.
Honza
> transaction = journal->j_running_transaction;
> if (transaction == NULL ||
> time_before(jiffies, transaction->t_expires)) {
> --
> 2.25.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
