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

[Baokun Li]

On 2025/1/21 19:04, Jan Kara wrote:
> On Tue 21-01-25 15:10:46, libaokun@xxxxxxxxxxxxxxx wrote:
> > From: Baokun Li <libaokun1@xxxxxxxxxx>
> >
> > Extract the ext4_check_nojournal_options() helper function to reduce code
> > duplication. No functional changes.
> >
> > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > Reviewed-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 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@xxxxxxx>
>
> 								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