Re: [PATCH v1 5/5] jbd2: Add TASK_FREEZABLE to kjournald2 thread

[daijunbing <daijunbing@vivo.com>]

在 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
>>