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);
Signed-off-by: Dai Junbing <daijunbing@vivo.com>
---
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);
transaction = journal->j_running_transaction;
if (transaction == NULL ||
time_before(jiffies, transaction->t_expires)) {
--
2.25.1On 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@vivo.com>
> ---
> 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@suse.com>
SUSE Labs, CR
在 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@vivo.com>
>> ---
>> 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
>>
© 2016 - 2025 Red Hat, Inc.