Re: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()

[Luis Henriques <luis.henriques@linux.dev>]

On Wed, Jul 24 2024, Jan Kara wrote:

> On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote:
>> Function jbd2_journal_shrink_checkpoint_list() assumes that '0' is not a
>> valid value for transaction IDs, which is incorrect.
>> 
>> Furthermore, the sbi->s_fc_ineligible_tid handling also makes the same
>> assumption by being initialised to '0'.  Fortunately, the sb flag
>> EXT4_MF_FC_INELIGIBLE can be used to check whether sbi->s_fc_ineligible_tid
>> has been previously set instead of comparing it with '0'.
>> 
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>
> Just one style nit below, otherwise looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> BTW, the ineligibility handling looks flaky to me, in particular the cases
> where we call ext4_fc_mark_ineligible() with NULL handle seem racy to me as
> fastcommit can happen *before* we mark the filesystem as ineligible.  But
> that's not really related to your changes, they just made me look at that
> code in detail and I couldn't resist complaining :)

Heh, fair enough.  Regarding this race, I may try to look into it but I'll
need to dig a bit more.  And yeah it's probably better to separate that
from this patch.

>
>> ---
>>  fs/ext4/fast_commit.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
>> index 3926a05eceee..3e0793cfea38 100644
>> --- a/fs/ext4/fast_commit.c
>> +++ b/fs/ext4/fast_commit.c
>> @@ -339,22 +339,29 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl
>>  {
>>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>>  	tid_t tid;
>> +	bool has_transaction = true;
>> +	bool is_ineligible;
>>  
>>  	if (ext4_fc_disabled(sb))
>>  		return;
>>  
>> -	ext4_set_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
>>  	if (handle && !IS_ERR(handle))
>>  		tid = handle->h_transaction->t_tid;
>>  	else {
>>  		read_lock(&sbi->s_journal->j_state_lock);
>> -		tid = sbi->s_journal->j_running_transaction ?
>> -				sbi->s_journal->j_running_transaction->t_tid : 0;
>> +		if (sbi->s_journal->j_running_transaction)
>> +			tid = sbi->s_journal->j_running_transaction->t_tid;
>> +		else
>> +			has_transaction = false;
>>  		read_unlock(&sbi->s_journal->j_state_lock);
>>  	}
>>  	spin_lock(&sbi->s_fc_lock);
>> -	if (tid_gt(tid, sbi->s_fc_ineligible_tid))
>> +	is_ineligible = ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
>> +	if (has_transaction &&
>> +	    ((!is_ineligible) ||
> 	     ^^ these extra braces look strange
>

They do, indeed.  I think my initial version had an explicit comparison
with 'false'.  v2 will have those removed.  And once again, thanks for
your review, Jan!

Cheers,
-- 
Luís


>> +	     (is_ineligible && tid_gt(tid, sbi->s_fc_ineligible_tid))))
>>  		sbi->s_fc_ineligible_tid = tid;
>> +	ext4_set_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
>
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR