jbd2: make '0' an invalid transaction sequence

[RFC PATCH] jbd2: make '0' an invalid transaction sequence

Posted by Luis Henriques 1 year, 7 months ago

Since there's code (in fast-commit) that already handles a '0' tid as a
special case, it's better to ensure that jbd2 never sets it to that value
when journal->j_transaction_sequence increment wraps.

Suggested-by: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
 fs/jbd2/transaction.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 66513c18ca29..4dbdd37349c3 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal,
 	transaction->t_state = T_RUNNING;
 	transaction->t_start_time = ktime_get();
 	transaction->t_tid = journal->j_transaction_sequence++;
+	if (unlikely(transaction->t_tid == 0))
+		transaction->t_tid = journal->j_transaction_sequence++;
 	transaction->t_expires = jiffies + journal->j_commit_interval;
 	atomic_set(&transaction->t_updates, 0);
 	atomic_set(&transaction->t_outstanding_credits,

Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

Posted by Jan Kara 1 year, 6 months ago

On Fri 12-07-24 10:53:02, Luis Henriques wrote:
> Since there's code (in fast-commit) that already handles a '0' tid as a
> special case, it's better to ensure that jbd2 never sets it to that value
> when journal->j_transaction_sequence increment wraps.
> 
> Suggested-by: Andreas Dilger <adilger@dilger.ca>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

Well, sadly it isn't so simple. If nothing else, journal replay
(do_one_pass()) will get broken by the skipped tid as we do check:

                if (sequence != next_commit_ID) {
                        brelse(bh);
                        break;
                }

So we'd abort journal replay too early. Secondly, there's also code
handling journal replay in libext2fs which would need to be checked and
fixed up. Finally, I've found code in mballoc which alternates between two
lists based on tid & 1, so this logic would get broken by skipping 0 tid
as well.

Overall, I think we might be better off to go and fix places that assume
tid 0 is not valid. I can see those assumptions in:

ext4_fc_mark_ineligible()
ext4_wait_for_tail_page_commit()
__jbd2_log_wait_for_space()
jbd2_journal_shrink_checkpoint_list()

Now I don't see it as urgent to fix all these right now. Just for this
series let's not add another place making tid 0 special. Later we can fixup
the other places...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

Posted by Luis Henriques 1 year, 6 months ago

On Tue, Jul 16 2024, Jan Kara wrote:

> On Fri 12-07-24 10:53:02, Luis Henriques wrote:
>> Since there's code (in fast-commit) that already handles a '0' tid as a
>> special case, it's better to ensure that jbd2 never sets it to that value
>> when journal->j_transaction_sequence increment wraps.
>> 
>> Suggested-by: Andreas Dilger <adilger@dilger.ca>
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>
> Well, sadly it isn't so simple. If nothing else, journal replay
> (do_one_pass()) will get broken by the skipped tid as we do check:
>
>                 if (sequence != next_commit_ID) {
>                         brelse(bh);
>                         break;
>                 }
>
> So we'd abort journal replay too early. Secondly, there's also code
> handling journal replay in libext2fs which would need to be checked and
> fixed up. Finally, I've found code in mballoc which alternates between two
> lists based on tid & 1, so this logic would get broken by skipping 0 tid
> as well.
>
> Overall, I think we might be better off to go and fix places that assume
> tid 0 is not valid. I can see those assumptions in:
>
> ext4_fc_mark_ineligible()
> ext4_wait_for_tail_page_commit()
> __jbd2_log_wait_for_space()
> jbd2_journal_shrink_checkpoint_list()
>
> Now I don't see it as urgent to fix all these right now. Just for this
> series let's not add another place making tid 0 special. Later we can fixup
> the other places...

Yikes!  Looks like I haven't done my homework -- I should have caught at
least one of the three breakages you point out.  Obviously, because I've
seen this assumption in different places, I thought it would be OK.

Anyway, thanks a lot for point this out, Jan.  I'll add a new TODO to my
list to start looking at other places that need to be fixed.

Cheers,
-- 
Luís

Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

Posted by wangjianjian (C) 1 year, 7 months ago

On 2024/7/12 17:53, Luis Henriques wrote:
> Since there's code (in fast-commit) that already handles a '0' tid as a
> special case, it's better to ensure that jbd2 never sets it to that value
> when journal->j_transaction_sequence increment wraps.
> 
> Suggested-by: Andreas Dilger <adilger@dilger.ca>
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> ---
>   fs/jbd2/transaction.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 66513c18ca29..4dbdd37349c3 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal,
>   	transaction->t_state = T_RUNNING;
>   	transaction->t_start_time = ktime_get();
>   	transaction->t_tid = journal->j_transaction_sequence++;
> +	if (unlikely(transaction->t_tid == 0))
> +		transaction->t_tid = journal->j_transaction_sequence++;
Do we need add j_transaction_sequence again here? if tansanction->t_tid 
== 0, then journal->j_trnasaction_sequence must be 1

>   	transaction->t_expires = jiffies + journal->j_commit_interval;
>   	atomic_set(&transaction->t_updates, 0);
>   	atomic_set(&transaction->t_outstanding_credits,
-- 
Regards

Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence

Posted by wangjianjian (C) 1 year, 7 months ago

On 2024/7/12 18:04, wangjianjian (C) wrote:
> On 2024/7/12 17:53, Luis Henriques wrote:
>> Since there's code (in fast-commit) that already handles a '0' tid as a
>> special case, it's better to ensure that jbd2 never sets it to that value
>> when journal->j_transaction_sequence increment wraps.
>>
>> Suggested-by: Andreas Dilger <adilger@dilger.ca>
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>> ---
>>   fs/jbd2/transaction.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 66513c18ca29..4dbdd37349c3 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal,
>>       transaction->t_state = T_RUNNING;
>>       transaction->t_start_time = ktime_get();
>>       transaction->t_tid = journal->j_transaction_sequence++;
>> +    if (unlikely(transaction->t_tid == 0))
>> +        transaction->t_tid = journal->j_transaction_sequence++;
> Do we need add j_transaction_sequence again here? if tansanction->t_tid 
> == 0, then journal->j_trnasaction_sequence must be 1
still need add 1. Sorry for confuse.
> 
>>       transaction->t_expires = jiffies + journal->j_commit_interval;
>>       atomic_set(&transaction->t_updates, 0);
>>       atomic_set(&transaction->t_outstanding_credits,
-- 
Regards