Re: [PATCH v2 4/8] ext4: extract ext4_check_nojournal_options() from __ext4_fill_super()

[Baokun Li <libaokun1@huawei.com>]

On 2025/1/21 19:04, Jan Kara wrote:
> On Tue 21-01-25 15:10:46, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Extract the ext4_check_nojournal_options() helper function to reduce code
>> duplication. No functional changes.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Looks good. Just a small naming suggestion:
>
>> +static const char *ext4_check_nojournal_options(struct super_block *sb)
> I'd call this ext4_has_journal_option() to make it somewhat more obvious
> what we are checking. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> 								Honza
Awesome, this function name is much better and makes more sense.
I'll use it in the next version. Naming is really hard! 😅

Thanks a lot for the review!

Cheers,
Baokun
>
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +
>> +	if (test_opt(sb, JOURNAL_ASYNC_COMMIT))
>> +		return "journal_async_commit";
>> +	if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM))
>> +		return "journal_checksum";
>> +	if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ)
>> +		return "commit=";
>> +	if (EXT4_MOUNT_DATA_FLAGS &
>> +	    (sbi->s_mount_opt ^ sbi->s_def_mount_opt))
>> +		return "data=";
>> +	if (test_opt(sb, DATA_ERR_ABORT))
>> +		return "data_err=abort";
>> +	return NULL;
>> +}
>> +
>>   static int ext4_load_super(struct super_block *sb, ext4_fsblk_t *lsb,
>>   			   int silent)
>>   {
>> @@ -5411,35 +5429,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>>   		       "suppressed and not mounted read-only");
>>   		goto failed_mount3a;
>>   	} else {
>> -		/* Nojournal mode, all journal mount options are illegal */
>> -		if (test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "journal_async_commit, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> +		const char *journal_option;
>>   
>> -		if (test_opt2(sb, EXPLICIT_JOURNAL_CHECKSUM)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "journal_checksum, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> -		if (sbi->s_commit_interval != JBD2_DEFAULT_MAX_COMMIT_AGE*HZ) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "commit=%lu, fs mounted w/o journal",
>> -				 sbi->s_commit_interval / HZ);
>> -			goto failed_mount3a;
>> -		}
>> -		if (EXT4_MOUNT_DATA_FLAGS &
>> -		    (sbi->s_mount_opt ^ sbi->s_def_mount_opt)) {
>> -			ext4_msg(sb, KERN_ERR, "can't mount with "
>> -				 "data=, fs mounted w/o journal");
>> -			goto failed_mount3a;
>> -		}
>> -		if (test_opt(sb, DATA_ERR_ABORT)) {
>> +		/* Nojournal mode, all journal mount options are illegal */
>> +		journal_option = ext4_check_nojournal_options(sb);
>> +		if (journal_option != NULL) {
>>   			ext4_msg(sb, KERN_ERR,
>> -				 "can't mount with data_err=abort, fs mounted w/o journal");
>> +				 "can't mount with %s, fs mounted w/o journal",
>> +				 journal_option);
>>   			goto failed_mount3a;
>>   		}
>> +
>>   		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>>   		clear_opt(sb, JOURNAL_CHECKSUM);
>>   		clear_opt(sb, DATA_FLAGS);
>> -- 
>> 2.39.2
>>