[PATCH 00/13] some refactor of __ext4_fill_super()

[PATCH 00/13] some refactor of __ext4_fill_super()

[Jason Yan]

This function is maybe the longest function I have seen in the kernel.
It has more than one thousand lines. This makes us not easy to read and
understand the code. So I made some refactors. The first two patches did
some preparation to the goto labels so that we can factor out some
functions easily.

After this refactor this function is about 500 lines shorter. I did not
go further because I'm not sure if people like this kind of change. If
there is any bad side effects, please let me know. If you strongly
dislike it, I am ok to stop this refactor.

Thansk.

Jason Yan (13):
  ext4: goto right label 'failed_mount3a'
  ext4: remove cantfind_ext4 error handler
  ext4: factor out ext4_set_def_opts()
  ext4: factor out ext4_handle_clustersize()
  ext4: factor out ext4_fast_commit_init()
  ext4: factor out ext4_inode_info_init()
  ext4: factor out ext4_encoding_init()
  ext4: factor out ext4_handle_csum()
  ext4: factor out ext4_compat_feature_check()
  ext4: factor out ext4_geometry_check()
  ext4: factor out ext4_group_desc_init() and ext4_group_desc_free()
  ext4: factor out ext4_load_and_init_journal()
  ext4: factor out ext4_journal_data_check()

 fs/ext4/super.c | 1070 ++++++++++++++++++++++++++---------------------
 1 file changed, 599 insertions(+), 471 deletions(-)

-- 
2.31.1

[PATCH 01/13] ext4: goto right label 'failed_mount3a'

[Jason Yan]

Before these two branches neither loaded the journal nor created the
xattr cache. So the right label to goto is 'failed_mount3a'. Although
this did not cause any issues because the error handler validated if the
pointer is null. However this still made me confused when reading
the code. So it's still worth to modify to goto the right label.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9a66abcca1a8..6126da867b26 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5079,30 +5079,30 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		   ext4_has_feature_journal_needs_recovery(sb)) {
 		ext4_msg(sb, KERN_ERR, "required journal recovery "
 		       "suppressed and not mounted read-only");
-		goto failed_mount_wq;
+		goto failed_mount3a;
 	} else {
 		/* Nojournal mode, all journal mount options are illegal */
 		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_mount_wq;
+			goto failed_mount3a;
 		}
 		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_mount_wq;
+			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_mount_wq;
+			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_mount_wq;
+			goto failed_mount3a;
 		}
 		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
 		clear_opt(sb, JOURNAL_CHECKSUM);
-- 
2.31.1

Re: [PATCH 01/13] ext4: goto right label 'failed_mount3a'

[Jan Kara]

On Tue 30-08-22 20:03:59, Jason Yan wrote:
> Before these two branches neither loaded the journal nor created the
> xattr cache. So the right label to goto is 'failed_mount3a'. Although
> this did not cause any issues because the error handler validated if the
> pointer is null. However this still made me confused when reading
> the code. So it's still worth to modify to goto the right label.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9a66abcca1a8..6126da867b26 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5079,30 +5079,30 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		   ext4_has_feature_journal_needs_recovery(sb)) {
>  		ext4_msg(sb, KERN_ERR, "required journal recovery "
>  		       "suppressed and not mounted read-only");
> -		goto failed_mount_wq;
> +		goto failed_mount3a;
>  	} else {
>  		/* Nojournal mode, all journal mount options are illegal */
>  		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_mount_wq;
> +			goto failed_mount3a;
>  		}
>  		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_mount_wq;
> +			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_mount_wq;
> +			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_mount_wq;
> +			goto failed_mount3a;
>  		}
>  		sbi->s_def_mount_opt &= ~EXT4_MOUNT_JOURNAL_CHECKSUM;
>  		clear_opt(sb, JOURNAL_CHECKSUM);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 02/13] ext4: remove cantfind_ext4 error handler

[Jason Yan]

The 'cantfind_ext4' error handler is just a error msg print and then
goto failed_mount. This two level goto makes the code complex and not
easy to read. The only benefit is that is saves a little bit code.
However some branches can merge and some branches dot not even need it.
So do some refactor and remove it.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6126da867b26..7341340e3c98 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4373,8 +4373,12 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	es = (struct ext4_super_block *) (bh->b_data + offset);
 	sbi->s_es = es;
 	sb->s_magic = le16_to_cpu(es->s_magic);
-	if (sb->s_magic != EXT4_SUPER_MAGIC)
-		goto cantfind_ext4;
+	if (sb->s_magic != EXT4_SUPER_MAGIC) {
+		if (!silent)
+			ext4_msg(sb, KERN_ERR, "VFS: Can't find ext4 filesystem");
+		goto failed_mount;
+	}
+
 	sbi->s_kbytes_written = le64_to_cpu(es->s_kbytes_written);
 
 	/* Warn if metadata_csum and gdt_csum are both set. */
@@ -4387,8 +4391,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	if (!ext4_verify_csum_type(sb, es)) {
 		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
 			 "unknown checksum algorithm.");
-		silent = 1;
-		goto cantfind_ext4;
+		goto failed_mount;
 	}
 	ext4_setup_csum_trigger(sb, EXT4_JTR_ORPHAN_FILE,
 				ext4_orphan_file_block_trigger);
@@ -4406,9 +4409,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	if (!ext4_superblock_csum_verify(sb, es)) {
 		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
 			 "invalid superblock checksum.  Run e2fsck?");
-		silent = 1;
 		ret = -EFSBADCRC;
-		goto cantfind_ext4;
+		goto failed_mount;
 	}
 
 	/* Precompute checksum seed for all metadata */
@@ -4798,8 +4800,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
 
 	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
-	if (sbi->s_inodes_per_block == 0)
-		goto cantfind_ext4;
+	if (sbi->s_inodes_per_block == 0 || (EXT4_BLOCKS_PER_GROUP(sb) == 0)) {
+		if (!silent)
+			ext4_msg(sb, KERN_ERR, "VFS: Can't find ext4 filesystem");
+		goto failed_mount;
+	}
 	if (sbi->s_inodes_per_group < sbi->s_inodes_per_block ||
 	    sbi->s_inodes_per_group > blocksize * 8) {
 		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
@@ -4896,9 +4901,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		goto failed_mount;
 	}
 
-	if (EXT4_BLOCKS_PER_GROUP(sb) == 0)
-		goto cantfind_ext4;
-
 	/* check blocks count against device size */
 	blocks_count = sb_bdev_nr_blocks(sb);
 	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
@@ -5408,11 +5410,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	return 0;
 
-cantfind_ext4:
-	if (!silent)
-		ext4_msg(sb, KERN_ERR, "VFS: Can't find ext4 filesystem");
-	goto failed_mount;
-
 failed_mount9:
 	ext4_release_orphan_info(sb);
 failed_mount8:
-- 
2.31.1

Re: [PATCH 02/13] ext4: remove cantfind_ext4 error handler

[Jan Kara]

On Tue 30-08-22 20:04:00, Jason Yan wrote:
> The 'cantfind_ext4' error handler is just a error msg print and then
> goto failed_mount. This two level goto makes the code complex and not
> easy to read. The only benefit is that is saves a little bit code.
> However some branches can merge and some branches dot not even need it.
> So do some refactor and remove it.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Yeah, probably makes sense. Just small style nits below. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> @@ -4798,8 +4800,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
>  
>  	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
> -	if (sbi->s_inodes_per_block == 0)
> -		goto cantfind_ext4;
> +	if (sbi->s_inodes_per_block == 0 || (EXT4_BLOCKS_PER_GROUP(sb) == 0)) {

I'd write this as:

	if (sbi->s_inodes_per_block == 0 || sbi->s_blocks_per_group == 0) {

to avoid superfluous braces and make the code a bit more natural.

									Honza

> +		if (!silent)
> +			ext4_msg(sb, KERN_ERR, "VFS: Can't find ext4 filesystem");
> +		goto failed_mount;
> +	}
>  	if (sbi->s_inodes_per_group < sbi->s_inodes_per_block ||
>  	    sbi->s_inodes_per_group > blocksize * 8) {
>  		ext4_msg(sb, KERN_ERR, "invalid inodes per group: %lu\n",
> @@ -4896,9 +4901,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		goto failed_mount;
>  	}
>  
> -	if (EXT4_BLOCKS_PER_GROUP(sb) == 0)
> -		goto cantfind_ext4;
> -
>  	/* check blocks count against device size */
>  	blocks_count = sb_bdev_nr_blocks(sb);
>  	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
> @@ -5408,11 +5410,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	return 0;
>  
> -cantfind_ext4:
> -	if (!silent)
> -		ext4_msg(sb, KERN_ERR, "VFS: Can't find ext4 filesystem");
> -	goto failed_mount;
> -
>  failed_mount9:
>  	ext4_release_orphan_info(sb);
>  failed_mount8:
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 02/13] ext4: remove cantfind_ext4 error handler

[Jason Yan]

On 2022/8/31 19:41, Jan Kara wrote:
> On Tue 30-08-22 20:04:00, Jason Yan wrote:
>> The 'cantfind_ext4' error handler is just a error msg print and then
>> goto failed_mount. This two level goto makes the code complex and not
>> easy to read. The only benefit is that is saves a little bit code.
>> However some branches can merge and some branches dot not even need it.
>> So do some refactor and remove it.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> 
> Yeah, probably makes sense. Just small style nits below. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
>> @@ -4798,8 +4800,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>>   	sbi->s_inodes_per_group = le32_to_cpu(es->s_inodes_per_group);
>>   
>>   	sbi->s_inodes_per_block = blocksize / EXT4_INODE_SIZE(sb);
>> -	if (sbi->s_inodes_per_block == 0)
>> -		goto cantfind_ext4;
>> +	if (sbi->s_inodes_per_block == 0 || (EXT4_BLOCKS_PER_GROUP(sb) == 0)) {
> 
> I'd write this as:
> 
> 	if (sbi->s_inodes_per_block == 0 || sbi->s_blocks_per_group == 0) {
> 
> to avoid superfluous braces and make the code a bit more natural.
> 
> 									Honza


Good suggestion. Will update.

Thanks.

[PATCH 03/13] ext4: factor out ext4_set_def_opts()

[Jason Yan]

Factor out ext4_set_def_opts(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 105 ++++++++++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 49 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7341340e3c98..66a128e5a9c8 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4311,6 +4311,61 @@ static struct ext4_sb_info *ext4_alloc_sbi(struct super_block *sb)
 	return NULL;
 }
 
+static void ext4_set_def_opts(struct super_block *sb,
+			      struct ext4_super_block *es)
+{
+	unsigned long def_mount_opts;
+
+	/* Set defaults before we parse the mount options */
+	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
+	set_opt(sb, INIT_INODE_TABLE);
+	if (def_mount_opts & EXT4_DEFM_DEBUG)
+		set_opt(sb, DEBUG);
+	if (def_mount_opts & EXT4_DEFM_BSDGROUPS)
+		set_opt(sb, GRPID);
+	if (def_mount_opts & EXT4_DEFM_UID16)
+		set_opt(sb, NO_UID32);
+	/* xattr user namespace & acls are now defaulted on */
+	set_opt(sb, XATTR_USER);
+#ifdef CONFIG_EXT4_FS_POSIX_ACL
+	set_opt(sb, POSIX_ACL);
+#endif
+	if (ext4_has_feature_fast_commit(sb))
+		set_opt2(sb, JOURNAL_FAST_COMMIT);
+	/* don't forget to enable journal_csum when metadata_csum is enabled. */
+	if (ext4_has_metadata_csum(sb))
+		set_opt(sb, JOURNAL_CHECKSUM);
+
+	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
+		set_opt(sb, JOURNAL_DATA);
+	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_ORDERED)
+		set_opt(sb, ORDERED_DATA);
+	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_WBACK)
+		set_opt(sb, WRITEBACK_DATA);
+
+	if (le16_to_cpu(es->s_errors) == EXT4_ERRORS_PANIC)
+		set_opt(sb, ERRORS_PANIC);
+	else if (le16_to_cpu(es->s_errors) == EXT4_ERRORS_CONTINUE)
+		set_opt(sb, ERRORS_CONT);
+	else
+		set_opt(sb, ERRORS_RO);
+	/* block_validity enabled by default; disable with noblock_validity */
+	set_opt(sb, BLOCK_VALIDITY);
+	if (def_mount_opts & EXT4_DEFM_DISCARD)
+		set_opt(sb, DISCARD);
+
+	if ((def_mount_opts & EXT4_DEFM_NOBARRIER) == 0)
+		set_opt(sb, BARRIER);
+
+	/*
+	 * enable delayed allocation by default
+	 * Use -o nodelalloc to turn it off
+	 */
+	if (!IS_EXT3_SB(sb) && !IS_EXT2_SB(sb) &&
+	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
+		set_opt(sb, DELALLOC);
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4320,7 +4375,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	ext4_fsblk_t block;
 	ext4_fsblk_t logical_sb_block;
 	unsigned long offset = 0;
-	unsigned long def_mount_opts;
 	struct inode *root;
 	int ret = -ENOMEM;
 	int blocksize, clustersize;
@@ -4420,43 +4474,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
 					       sizeof(es->s_uuid));
 
-	/* Set defaults before we parse the mount options */
-	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
-	set_opt(sb, INIT_INODE_TABLE);
-	if (def_mount_opts & EXT4_DEFM_DEBUG)
-		set_opt(sb, DEBUG);
-	if (def_mount_opts & EXT4_DEFM_BSDGROUPS)
-		set_opt(sb, GRPID);
-	if (def_mount_opts & EXT4_DEFM_UID16)
-		set_opt(sb, NO_UID32);
-	/* xattr user namespace & acls are now defaulted on */
-	set_opt(sb, XATTR_USER);
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-	set_opt(sb, POSIX_ACL);
-#endif
-	if (ext4_has_feature_fast_commit(sb))
-		set_opt2(sb, JOURNAL_FAST_COMMIT);
-	/* don't forget to enable journal_csum when metadata_csum is enabled. */
-	if (ext4_has_metadata_csum(sb))
-		set_opt(sb, JOURNAL_CHECKSUM);
-
-	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
-		set_opt(sb, JOURNAL_DATA);
-	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_ORDERED)
-		set_opt(sb, ORDERED_DATA);
-	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_WBACK)
-		set_opt(sb, WRITEBACK_DATA);
-
-	if (le16_to_cpu(sbi->s_es->s_errors) == EXT4_ERRORS_PANIC)
-		set_opt(sb, ERRORS_PANIC);
-	else if (le16_to_cpu(sbi->s_es->s_errors) == EXT4_ERRORS_CONTINUE)
-		set_opt(sb, ERRORS_CONT);
-	else
-		set_opt(sb, ERRORS_RO);
-	/* block_validity enabled by default; disable with noblock_validity */
-	set_opt(sb, BLOCK_VALIDITY);
-	if (def_mount_opts & EXT4_DEFM_DISCARD)
-		set_opt(sb, DISCARD);
+	ext4_set_def_opts(sb, es);
 
 	sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid));
 	sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid));
@@ -4464,17 +4482,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	sbi->s_min_batch_time = EXT4_DEF_MIN_BATCH_TIME;
 	sbi->s_max_batch_time = EXT4_DEF_MAX_BATCH_TIME;
 
-	if ((def_mount_opts & EXT4_DEFM_NOBARRIER) == 0)
-		set_opt(sb, BARRIER);
-
-	/*
-	 * enable delayed allocation by default
-	 * Use -o nodelalloc to turn it off
-	 */
-	if (!IS_EXT3_SB(sb) && !IS_EXT2_SB(sb) &&
-	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
-		set_opt(sb, DELALLOC);
-
 	/*
 	 * set default s_li_wait_mult for lazyinit, for the case there is
 	 * no mount option specified.
-- 
2.31.1

Re: [PATCH 03/13] ext4: factor out ext4_set_def_opts()

[Jan Kara]

On Tue 30-08-22 20:04:01, Jason Yan wrote:
> Factor out ext4_set_def_opts(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Yeah, nice factorization. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 105 ++++++++++++++++++++++++++----------------------
>  1 file changed, 56 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7341340e3c98..66a128e5a9c8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4311,6 +4311,61 @@ static struct ext4_sb_info *ext4_alloc_sbi(struct super_block *sb)
>  	return NULL;
>  }
>  
> +static void ext4_set_def_opts(struct super_block *sb,
> +			      struct ext4_super_block *es)
> +{
> +	unsigned long def_mount_opts;
> +
> +	/* Set defaults before we parse the mount options */
> +	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
> +	set_opt(sb, INIT_INODE_TABLE);
> +	if (def_mount_opts & EXT4_DEFM_DEBUG)
> +		set_opt(sb, DEBUG);
> +	if (def_mount_opts & EXT4_DEFM_BSDGROUPS)
> +		set_opt(sb, GRPID);
> +	if (def_mount_opts & EXT4_DEFM_UID16)
> +		set_opt(sb, NO_UID32);
> +	/* xattr user namespace & acls are now defaulted on */
> +	set_opt(sb, XATTR_USER);
> +#ifdef CONFIG_EXT4_FS_POSIX_ACL
> +	set_opt(sb, POSIX_ACL);
> +#endif
> +	if (ext4_has_feature_fast_commit(sb))
> +		set_opt2(sb, JOURNAL_FAST_COMMIT);
> +	/* don't forget to enable journal_csum when metadata_csum is enabled. */
> +	if (ext4_has_metadata_csum(sb))
> +		set_opt(sb, JOURNAL_CHECKSUM);
> +
> +	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
> +		set_opt(sb, JOURNAL_DATA);
> +	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_ORDERED)
> +		set_opt(sb, ORDERED_DATA);
> +	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_WBACK)
> +		set_opt(sb, WRITEBACK_DATA);
> +
> +	if (le16_to_cpu(es->s_errors) == EXT4_ERRORS_PANIC)
> +		set_opt(sb, ERRORS_PANIC);
> +	else if (le16_to_cpu(es->s_errors) == EXT4_ERRORS_CONTINUE)
> +		set_opt(sb, ERRORS_CONT);
> +	else
> +		set_opt(sb, ERRORS_RO);
> +	/* block_validity enabled by default; disable with noblock_validity */
> +	set_opt(sb, BLOCK_VALIDITY);
> +	if (def_mount_opts & EXT4_DEFM_DISCARD)
> +		set_opt(sb, DISCARD);
> +
> +	if ((def_mount_opts & EXT4_DEFM_NOBARRIER) == 0)
> +		set_opt(sb, BARRIER);
> +
> +	/*
> +	 * enable delayed allocation by default
> +	 * Use -o nodelalloc to turn it off
> +	 */
> +	if (!IS_EXT3_SB(sb) && !IS_EXT2_SB(sb) &&
> +	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
> +		set_opt(sb, DELALLOC);
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4320,7 +4375,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	ext4_fsblk_t block;
>  	ext4_fsblk_t logical_sb_block;
>  	unsigned long offset = 0;
> -	unsigned long def_mount_opts;
>  	struct inode *root;
>  	int ret = -ENOMEM;
>  	int blocksize, clustersize;
> @@ -4420,43 +4474,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
>  					       sizeof(es->s_uuid));
>  
> -	/* Set defaults before we parse the mount options */
> -	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
> -	set_opt(sb, INIT_INODE_TABLE);
> -	if (def_mount_opts & EXT4_DEFM_DEBUG)
> -		set_opt(sb, DEBUG);
> -	if (def_mount_opts & EXT4_DEFM_BSDGROUPS)
> -		set_opt(sb, GRPID);
> -	if (def_mount_opts & EXT4_DEFM_UID16)
> -		set_opt(sb, NO_UID32);
> -	/* xattr user namespace & acls are now defaulted on */
> -	set_opt(sb, XATTR_USER);
> -#ifdef CONFIG_EXT4_FS_POSIX_ACL
> -	set_opt(sb, POSIX_ACL);
> -#endif
> -	if (ext4_has_feature_fast_commit(sb))
> -		set_opt2(sb, JOURNAL_FAST_COMMIT);
> -	/* don't forget to enable journal_csum when metadata_csum is enabled. */
> -	if (ext4_has_metadata_csum(sb))
> -		set_opt(sb, JOURNAL_CHECKSUM);
> -
> -	if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_DATA)
> -		set_opt(sb, JOURNAL_DATA);
> -	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_ORDERED)
> -		set_opt(sb, ORDERED_DATA);
> -	else if ((def_mount_opts & EXT4_DEFM_JMODE) == EXT4_DEFM_JMODE_WBACK)
> -		set_opt(sb, WRITEBACK_DATA);
> -
> -	if (le16_to_cpu(sbi->s_es->s_errors) == EXT4_ERRORS_PANIC)
> -		set_opt(sb, ERRORS_PANIC);
> -	else if (le16_to_cpu(sbi->s_es->s_errors) == EXT4_ERRORS_CONTINUE)
> -		set_opt(sb, ERRORS_CONT);
> -	else
> -		set_opt(sb, ERRORS_RO);
> -	/* block_validity enabled by default; disable with noblock_validity */
> -	set_opt(sb, BLOCK_VALIDITY);
> -	if (def_mount_opts & EXT4_DEFM_DISCARD)
> -		set_opt(sb, DISCARD);
> +	ext4_set_def_opts(sb, es);
>  
>  	sbi->s_resuid = make_kuid(&init_user_ns, le16_to_cpu(es->s_def_resuid));
>  	sbi->s_resgid = make_kgid(&init_user_ns, le16_to_cpu(es->s_def_resgid));
> @@ -4464,17 +4482,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	sbi->s_min_batch_time = EXT4_DEF_MIN_BATCH_TIME;
>  	sbi->s_max_batch_time = EXT4_DEF_MAX_BATCH_TIME;
>  
> -	if ((def_mount_opts & EXT4_DEFM_NOBARRIER) == 0)
> -		set_opt(sb, BARRIER);
> -
> -	/*
> -	 * enable delayed allocation by default
> -	 * Use -o nodelalloc to turn it off
> -	 */
> -	if (!IS_EXT3_SB(sb) && !IS_EXT2_SB(sb) &&
> -	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
> -		set_opt(sb, DELALLOC);
> -
>  	/*
>  	 * set default s_li_wait_mult for lazyinit, for the case there is
>  	 * no mount option specified.
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 04/13] ext4: factor out ext4_handle_clustersize()

[Jason Yan]

Factor out ext4_handle_clustersize(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 110 +++++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 49 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 66a128e5a9c8..1855559be4f2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4366,6 +4366,64 @@ static void ext4_set_def_opts(struct super_block *sb,
 		set_opt(sb, DELALLOC);
 }
 
+static int ext4_handle_clustersize(struct super_block *sb, int blocksize)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+	int clustersize;
+
+	/* Handle clustersize */
+	clustersize = BLOCK_SIZE << le32_to_cpu(es->s_log_cluster_size);
+	if (ext4_has_feature_bigalloc(sb)) {
+		if (clustersize < blocksize) {
+			ext4_msg(sb, KERN_ERR,
+				 "cluster size (%d) smaller than "
+				 "block size (%d)", clustersize, blocksize);
+			return -EINVAL;
+		}
+		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
+			le32_to_cpu(es->s_log_block_size);
+		sbi->s_clusters_per_group =
+			le32_to_cpu(es->s_clusters_per_group);
+		if (sbi->s_clusters_per_group > blocksize * 8) {
+			ext4_msg(sb, KERN_ERR,
+				 "#clusters per group too big: %lu",
+				 sbi->s_clusters_per_group);
+			return -EINVAL;
+		}
+		if (sbi->s_blocks_per_group !=
+		    (sbi->s_clusters_per_group * (clustersize / blocksize))) {
+			ext4_msg(sb, KERN_ERR, "blocks per group (%lu) and "
+				 "clusters per group (%lu) inconsistent",
+				 sbi->s_blocks_per_group,
+				 sbi->s_clusters_per_group);
+			return -EINVAL;
+		}
+	} else {
+		if (clustersize != blocksize) {
+			ext4_msg(sb, KERN_ERR,
+				 "fragment/cluster size (%d) != "
+				 "block size (%d)", clustersize, blocksize);
+			return -EINVAL;
+		}
+		if (sbi->s_blocks_per_group > blocksize * 8) {
+			ext4_msg(sb, KERN_ERR,
+				 "#blocks per group too big: %lu",
+				 sbi->s_blocks_per_group);
+			return -EINVAL;
+		}
+		sbi->s_clusters_per_group = sbi->s_blocks_per_group;
+		sbi->s_cluster_bits = 0;
+	}
+	sbi->s_cluster_ratio = clustersize / blocksize;
+
+	/* Do we have standard group size of clustersize * 8 blocks ? */
+	if (sbi->s_blocks_per_group == clustersize << 3)
+		set_opt2(sb, STD_GROUP_SIZE);
+
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4377,7 +4435,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	unsigned long offset = 0;
 	struct inode *root;
 	int ret = -ENOMEM;
-	int blocksize, clustersize;
+	int blocksize;
 	unsigned int db_count;
 	unsigned int i;
 	int needs_recovery, has_huge_files;
@@ -4847,54 +4905,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		}
 	}
 
-	/* Handle clustersize */
-	clustersize = BLOCK_SIZE << le32_to_cpu(es->s_log_cluster_size);
-	if (ext4_has_feature_bigalloc(sb)) {
-		if (clustersize < blocksize) {
-			ext4_msg(sb, KERN_ERR,
-				 "cluster size (%d) smaller than "
-				 "block size (%d)", clustersize, blocksize);
-			goto failed_mount;
-		}
-		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
-			le32_to_cpu(es->s_log_block_size);
-		sbi->s_clusters_per_group =
-			le32_to_cpu(es->s_clusters_per_group);
-		if (sbi->s_clusters_per_group > blocksize * 8) {
-			ext4_msg(sb, KERN_ERR,
-				 "#clusters per group too big: %lu",
-				 sbi->s_clusters_per_group);
-			goto failed_mount;
-		}
-		if (sbi->s_blocks_per_group !=
-		    (sbi->s_clusters_per_group * (clustersize / blocksize))) {
-			ext4_msg(sb, KERN_ERR, "blocks per group (%lu) and "
-				 "clusters per group (%lu) inconsistent",
-				 sbi->s_blocks_per_group,
-				 sbi->s_clusters_per_group);
-			goto failed_mount;
-		}
-	} else {
-		if (clustersize != blocksize) {
-			ext4_msg(sb, KERN_ERR,
-				 "fragment/cluster size (%d) != "
-				 "block size (%d)", clustersize, blocksize);
-			goto failed_mount;
-		}
-		if (sbi->s_blocks_per_group > blocksize * 8) {
-			ext4_msg(sb, KERN_ERR,
-				 "#blocks per group too big: %lu",
-				 sbi->s_blocks_per_group);
-			goto failed_mount;
-		}
-		sbi->s_clusters_per_group = sbi->s_blocks_per_group;
-		sbi->s_cluster_bits = 0;
-	}
-	sbi->s_cluster_ratio = clustersize / blocksize;
-
-	/* Do we have standard group size of clustersize * 8 blocks ? */
-	if (sbi->s_blocks_per_group == clustersize << 3)
-		set_opt2(sb, STD_GROUP_SIZE);
+	if (ext4_handle_clustersize(sb, blocksize))
+		goto failed_mount;
 
 	/*
 	 * Test whether we have more sectors than will fit in sector_t,
-- 
2.31.1

Re: [PATCH 04/13] ext4: factor out ext4_handle_clustersize()

[Jan Kara]

On Tue 30-08-22 20:04:02, Jason Yan wrote:
> Factor out ext4_handle_clustersize(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 66a128e5a9c8..1855559be4f2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4366,6 +4366,64 @@ static void ext4_set_def_opts(struct super_block *sb,
>  		set_opt(sb, DELALLOC);
>  }
>  
> +static int ext4_handle_clustersize(struct super_block *sb, int blocksize)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_super_block *es = sbi->s_es;
> +	int clustersize;
> +
> +	/* Handle clustersize */
> +	clustersize = BLOCK_SIZE << le32_to_cpu(es->s_log_cluster_size);
> +	if (ext4_has_feature_bigalloc(sb)) {
> +		if (clustersize < blocksize) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "cluster size (%d) smaller than "
> +				 "block size (%d)", clustersize, blocksize);
> +			return -EINVAL;
> +		}
> +		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
> +			le32_to_cpu(es->s_log_block_size);
> +		sbi->s_clusters_per_group =
> +			le32_to_cpu(es->s_clusters_per_group);
> +		if (sbi->s_clusters_per_group > blocksize * 8) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "#clusters per group too big: %lu",
> +				 sbi->s_clusters_per_group);
> +			return -EINVAL;
> +		}
> +		if (sbi->s_blocks_per_group !=
> +		    (sbi->s_clusters_per_group * (clustersize / blocksize))) {
> +			ext4_msg(sb, KERN_ERR, "blocks per group (%lu) and "
> +				 "clusters per group (%lu) inconsistent",
> +				 sbi->s_blocks_per_group,
> +				 sbi->s_clusters_per_group);
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (clustersize != blocksize) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "fragment/cluster size (%d) != "
> +				 "block size (%d)", clustersize, blocksize);
> +			return -EINVAL;
> +		}
> +		if (sbi->s_blocks_per_group > blocksize * 8) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "#blocks per group too big: %lu",
> +				 sbi->s_blocks_per_group);
> +			return -EINVAL;
> +		}
> +		sbi->s_clusters_per_group = sbi->s_blocks_per_group;
> +		sbi->s_cluster_bits = 0;
> +	}
> +	sbi->s_cluster_ratio = clustersize / blocksize;
> +
> +	/* Do we have standard group size of clustersize * 8 blocks ? */
> +	if (sbi->s_blocks_per_group == clustersize << 3)
> +		set_opt2(sb, STD_GROUP_SIZE);
> +
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4377,7 +4435,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	unsigned long offset = 0;
>  	struct inode *root;
>  	int ret = -ENOMEM;
> -	int blocksize, clustersize;
> +	int blocksize;
>  	unsigned int db_count;
>  	unsigned int i;
>  	int needs_recovery, has_huge_files;
> @@ -4847,54 +4905,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		}
>  	}
>  
> -	/* Handle clustersize */
> -	clustersize = BLOCK_SIZE << le32_to_cpu(es->s_log_cluster_size);
> -	if (ext4_has_feature_bigalloc(sb)) {
> -		if (clustersize < blocksize) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "cluster size (%d) smaller than "
> -				 "block size (%d)", clustersize, blocksize);
> -			goto failed_mount;
> -		}
> -		sbi->s_cluster_bits = le32_to_cpu(es->s_log_cluster_size) -
> -			le32_to_cpu(es->s_log_block_size);
> -		sbi->s_clusters_per_group =
> -			le32_to_cpu(es->s_clusters_per_group);
> -		if (sbi->s_clusters_per_group > blocksize * 8) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "#clusters per group too big: %lu",
> -				 sbi->s_clusters_per_group);
> -			goto failed_mount;
> -		}
> -		if (sbi->s_blocks_per_group !=
> -		    (sbi->s_clusters_per_group * (clustersize / blocksize))) {
> -			ext4_msg(sb, KERN_ERR, "blocks per group (%lu) and "
> -				 "clusters per group (%lu) inconsistent",
> -				 sbi->s_blocks_per_group,
> -				 sbi->s_clusters_per_group);
> -			goto failed_mount;
> -		}
> -	} else {
> -		if (clustersize != blocksize) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "fragment/cluster size (%d) != "
> -				 "block size (%d)", clustersize, blocksize);
> -			goto failed_mount;
> -		}
> -		if (sbi->s_blocks_per_group > blocksize * 8) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "#blocks per group too big: %lu",
> -				 sbi->s_blocks_per_group);
> -			goto failed_mount;
> -		}
> -		sbi->s_clusters_per_group = sbi->s_blocks_per_group;
> -		sbi->s_cluster_bits = 0;
> -	}
> -	sbi->s_cluster_ratio = clustersize / blocksize;
> -
> -	/* Do we have standard group size of clustersize * 8 blocks ? */
> -	if (sbi->s_blocks_per_group == clustersize << 3)
> -		set_opt2(sb, STD_GROUP_SIZE);
> +	if (ext4_handle_clustersize(sb, blocksize))
> +		goto failed_mount;
>  
>  	/*
>  	 * Test whether we have more sectors than will fit in sector_t,
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 05/13] ext4: factor out ext4_fast_commit_init()

[Jason Yan]

Factor out ext4_fast_commit_init(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 1855559be4f2..d355eda2f184 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4424,6 +4424,30 @@ static int ext4_handle_clustersize(struct super_block *sb, int blocksize)
 	return 0;
 }
 
+static void ext4_fast_commit_init(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	/* Initialize fast commit stuff */
+	atomic_set(&sbi->s_fc_subtid, 0);
+	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_MAIN]);
+	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_STAGING]);
+	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_MAIN]);
+	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_STAGING]);
+	sbi->s_fc_bytes = 0;
+	ext4_clear_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
+	sbi->s_fc_ineligible_tid = 0;
+	spin_lock_init(&sbi->s_fc_lock);
+	memset(&sbi->s_fc_stats, 0, sizeof(sbi->s_fc_stats));
+	sbi->s_fc_replay_state.fc_regions = NULL;
+	sbi->s_fc_replay_state.fc_regions_size = 0;
+	sbi->s_fc_replay_state.fc_regions_used = 0;
+	sbi->s_fc_replay_state.fc_regions_valid = 0;
+	sbi->s_fc_replay_state.fc_modified_inodes = NULL;
+	sbi->s_fc_replay_state.fc_modified_inodes_size = 0;
+	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -5059,24 +5083,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
 	mutex_init(&sbi->s_orphan_lock);
 
-	/* Initialize fast commit stuff */
-	atomic_set(&sbi->s_fc_subtid, 0);
-	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_MAIN]);
-	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_STAGING]);
-	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_MAIN]);
-	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_STAGING]);
-	sbi->s_fc_bytes = 0;
-	ext4_clear_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
-	sbi->s_fc_ineligible_tid = 0;
-	spin_lock_init(&sbi->s_fc_lock);
-	memset(&sbi->s_fc_stats, 0, sizeof(sbi->s_fc_stats));
-	sbi->s_fc_replay_state.fc_regions = NULL;
-	sbi->s_fc_replay_state.fc_regions_size = 0;
-	sbi->s_fc_replay_state.fc_regions_used = 0;
-	sbi->s_fc_replay_state.fc_regions_valid = 0;
-	sbi->s_fc_replay_state.fc_modified_inodes = NULL;
-	sbi->s_fc_replay_state.fc_modified_inodes_size = 0;
-	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
+	ext4_fast_commit_init(sb);
 
 	sb->s_root = NULL;
 
-- 
2.31.1

Re: [PATCH 05/13] ext4: factor out ext4_fast_commit_init()

[Jan Kara]

On Tue 30-08-22 20:04:03, Jason Yan wrote:
> Factor out ext4_fast_commit_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Why not :). Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 1855559be4f2..d355eda2f184 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4424,6 +4424,30 @@ static int ext4_handle_clustersize(struct super_block *sb, int blocksize)
>  	return 0;
>  }
>  
> +static void ext4_fast_commit_init(struct super_block *sb)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	/* Initialize fast commit stuff */
> +	atomic_set(&sbi->s_fc_subtid, 0);
> +	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_MAIN]);
> +	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_STAGING]);
> +	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_MAIN]);
> +	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_STAGING]);
> +	sbi->s_fc_bytes = 0;
> +	ext4_clear_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
> +	sbi->s_fc_ineligible_tid = 0;
> +	spin_lock_init(&sbi->s_fc_lock);
> +	memset(&sbi->s_fc_stats, 0, sizeof(sbi->s_fc_stats));
> +	sbi->s_fc_replay_state.fc_regions = NULL;
> +	sbi->s_fc_replay_state.fc_regions_size = 0;
> +	sbi->s_fc_replay_state.fc_regions_used = 0;
> +	sbi->s_fc_replay_state.fc_regions_valid = 0;
> +	sbi->s_fc_replay_state.fc_modified_inodes = NULL;
> +	sbi->s_fc_replay_state.fc_modified_inodes_size = 0;
> +	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -5059,24 +5083,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
>  	mutex_init(&sbi->s_orphan_lock);
>  
> -	/* Initialize fast commit stuff */
> -	atomic_set(&sbi->s_fc_subtid, 0);
> -	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_MAIN]);
> -	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_STAGING]);
> -	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_MAIN]);
> -	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_STAGING]);
> -	sbi->s_fc_bytes = 0;
> -	ext4_clear_mount_flag(sb, EXT4_MF_FC_INELIGIBLE);
> -	sbi->s_fc_ineligible_tid = 0;
> -	spin_lock_init(&sbi->s_fc_lock);
> -	memset(&sbi->s_fc_stats, 0, sizeof(sbi->s_fc_stats));
> -	sbi->s_fc_replay_state.fc_regions = NULL;
> -	sbi->s_fc_replay_state.fc_regions_size = 0;
> -	sbi->s_fc_replay_state.fc_regions_used = 0;
> -	sbi->s_fc_replay_state.fc_regions_valid = 0;
> -	sbi->s_fc_replay_state.fc_modified_inodes = NULL;
> -	sbi->s_fc_replay_state.fc_modified_inodes_size = 0;
> -	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
> +	ext4_fast_commit_init(sb);
>  
>  	sb->s_root = NULL;
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 06/13] ext4: factor out ext4_inode_info_init()

[Jason Yan]

Factor out ext4_inode_info_init(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 137 ++++++++++++++++++++++++++----------------------
 1 file changed, 75 insertions(+), 62 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d355eda2f184..458d146149b2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4448,6 +4448,79 @@ static void ext4_fast_commit_init(struct super_block *sb)
 	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
 }
 
+static int ext4_inode_info_init(struct super_block *sb,
+				struct ext4_super_block *es,
+				int blocksize)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
+		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
+		sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;
+	} else {
+		sbi->s_inode_size = le16_to_cpu(es->s_inode_size);
+		sbi->s_first_ino = le32_to_cpu(es->s_first_ino);
+		if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {
+			ext4_msg(sb, KERN_ERR, "invalid first ino: %u",
+				 sbi->s_first_ino);
+			return -EINVAL;
+		}
+		if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||
+		    (!is_power_of_2(sbi->s_inode_size)) ||
+		    (sbi->s_inode_size > blocksize)) {
+			ext4_msg(sb, KERN_ERR,
+			       "unsupported inode size: %d",
+			       sbi->s_inode_size);
+			ext4_msg(sb, KERN_ERR, "blocksize: %d", blocksize);
+			return -EINVAL;
+		}
+		/*
+		 * i_atime_extra is the last extra field available for
+		 * [acm]times in struct ext4_inode. Checking for that
+		 * field should suffice to ensure we have extra space
+		 * for all three.
+		 */
+		if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +
+			sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
+			sb->s_time_gran = 1;
+			sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
+		} else {
+			sb->s_time_gran = NSEC_PER_SEC;
+			sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
+		}
+		sb->s_time_min = EXT4_TIMESTAMP_MIN;
+	}
+
+	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {
+		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
+			EXT4_GOOD_OLD_INODE_SIZE;
+		if (ext4_has_feature_extra_isize(sb)) {
+			unsigned v, max = (sbi->s_inode_size -
+					   EXT4_GOOD_OLD_INODE_SIZE);
+
+			v = le16_to_cpu(es->s_want_extra_isize);
+			if (v > max) {
+				ext4_msg(sb, KERN_ERR,
+					 "bad s_want_extra_isize: %d", v);
+				return -EINVAL;
+			}
+			if (sbi->s_want_extra_isize < v)
+				sbi->s_want_extra_isize = v;
+
+			v = le16_to_cpu(es->s_min_extra_isize);
+			if (v > max) {
+				ext4_msg(sb, KERN_ERR,
+					 "bad s_min_extra_isize: %d", v);
+				return -EINVAL;
+			}
+			if (sbi->s_want_extra_isize < v)
+				sbi->s_want_extra_isize = v;
+		}
+	}
+
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4590,68 +4663,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	if (blocksize == PAGE_SIZE)
 		set_opt(sb, DIOREAD_NOLOCK);
 
-	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
-		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
-		sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;
-	} else {
-		sbi->s_inode_size = le16_to_cpu(es->s_inode_size);
-		sbi->s_first_ino = le32_to_cpu(es->s_first_ino);
-		if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {
-			ext4_msg(sb, KERN_ERR, "invalid first ino: %u",
-				 sbi->s_first_ino);
-			goto failed_mount;
-		}
-		if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||
-		    (!is_power_of_2(sbi->s_inode_size)) ||
-		    (sbi->s_inode_size > blocksize)) {
-			ext4_msg(sb, KERN_ERR,
-			       "unsupported inode size: %d",
-			       sbi->s_inode_size);
-			ext4_msg(sb, KERN_ERR, "blocksize: %d", blocksize);
-			goto failed_mount;
-		}
-		/*
-		 * i_atime_extra is the last extra field available for
-		 * [acm]times in struct ext4_inode. Checking for that
-		 * field should suffice to ensure we have extra space
-		 * for all three.
-		 */
-		if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +
-			sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
-			sb->s_time_gran = 1;
-			sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
-		} else {
-			sb->s_time_gran = NSEC_PER_SEC;
-			sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
-		}
-		sb->s_time_min = EXT4_TIMESTAMP_MIN;
-	}
-	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {
-		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
-			EXT4_GOOD_OLD_INODE_SIZE;
-		if (ext4_has_feature_extra_isize(sb)) {
-			unsigned v, max = (sbi->s_inode_size -
-					   EXT4_GOOD_OLD_INODE_SIZE);
-
-			v = le16_to_cpu(es->s_want_extra_isize);
-			if (v > max) {
-				ext4_msg(sb, KERN_ERR,
-					 "bad s_want_extra_isize: %d", v);
-				goto failed_mount;
-			}
-			if (sbi->s_want_extra_isize < v)
-				sbi->s_want_extra_isize = v;
-
-			v = le16_to_cpu(es->s_min_extra_isize);
-			if (v > max) {
-				ext4_msg(sb, KERN_ERR,
-					 "bad s_min_extra_isize: %d", v);
-				goto failed_mount;
-			}
-			if (sbi->s_want_extra_isize < v)
-				sbi->s_want_extra_isize = v;
-		}
-	}
+	if (ext4_inode_info_init(sb, es, blocksize))
+		goto failed_mount;
 
 	err = parse_apply_sb_mount_options(sb, ctx);
 	if (err < 0)
-- 
2.31.1

Re: [PATCH 06/13] ext4: factor out ext4_inode_info_init()

[Jan Kara]

On Tue 30-08-22 20:04:04, Jason Yan wrote:
> Factor out ext4_inode_info_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 137 ++++++++++++++++++++++++++----------------------
>  1 file changed, 75 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d355eda2f184..458d146149b2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4448,6 +4448,79 @@ static void ext4_fast_commit_init(struct super_block *sb)
>  	sbi->s_fc_replay_state.fc_modified_inodes_used = 0;
>  }
>  
> +static int ext4_inode_info_init(struct super_block *sb,
> +				struct ext4_super_block *es,
> +				int blocksize)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
> +		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
> +		sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;
> +	} else {
> +		sbi->s_inode_size = le16_to_cpu(es->s_inode_size);
> +		sbi->s_first_ino = le32_to_cpu(es->s_first_ino);
> +		if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {
> +			ext4_msg(sb, KERN_ERR, "invalid first ino: %u",
> +				 sbi->s_first_ino);
> +			return -EINVAL;
> +		}
> +		if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||
> +		    (!is_power_of_2(sbi->s_inode_size)) ||
> +		    (sbi->s_inode_size > blocksize)) {
> +			ext4_msg(sb, KERN_ERR,
> +			       "unsupported inode size: %d",
> +			       sbi->s_inode_size);
> +			ext4_msg(sb, KERN_ERR, "blocksize: %d", blocksize);
> +			return -EINVAL;
> +		}
> +		/*
> +		 * i_atime_extra is the last extra field available for
> +		 * [acm]times in struct ext4_inode. Checking for that
> +		 * field should suffice to ensure we have extra space
> +		 * for all three.
> +		 */
> +		if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +
> +			sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
> +			sb->s_time_gran = 1;
> +			sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
> +		} else {
> +			sb->s_time_gran = NSEC_PER_SEC;
> +			sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
> +		}
> +		sb->s_time_min = EXT4_TIMESTAMP_MIN;
> +	}
> +
> +	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {
> +		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> +			EXT4_GOOD_OLD_INODE_SIZE;
> +		if (ext4_has_feature_extra_isize(sb)) {
> +			unsigned v, max = (sbi->s_inode_size -
> +					   EXT4_GOOD_OLD_INODE_SIZE);
> +
> +			v = le16_to_cpu(es->s_want_extra_isize);
> +			if (v > max) {
> +				ext4_msg(sb, KERN_ERR,
> +					 "bad s_want_extra_isize: %d", v);
> +				return -EINVAL;
> +			}
> +			if (sbi->s_want_extra_isize < v)
> +				sbi->s_want_extra_isize = v;
> +
> +			v = le16_to_cpu(es->s_min_extra_isize);
> +			if (v > max) {
> +				ext4_msg(sb, KERN_ERR,
> +					 "bad s_min_extra_isize: %d", v);
> +				return -EINVAL;
> +			}
> +			if (sbi->s_want_extra_isize < v)
> +				sbi->s_want_extra_isize = v;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4590,68 +4663,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	if (blocksize == PAGE_SIZE)
>  		set_opt(sb, DIOREAD_NOLOCK);
>  
> -	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
> -		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
> -		sbi->s_first_ino = EXT4_GOOD_OLD_FIRST_INO;
> -	} else {
> -		sbi->s_inode_size = le16_to_cpu(es->s_inode_size);
> -		sbi->s_first_ino = le32_to_cpu(es->s_first_ino);
> -		if (sbi->s_first_ino < EXT4_GOOD_OLD_FIRST_INO) {
> -			ext4_msg(sb, KERN_ERR, "invalid first ino: %u",
> -				 sbi->s_first_ino);
> -			goto failed_mount;
> -		}
> -		if ((sbi->s_inode_size < EXT4_GOOD_OLD_INODE_SIZE) ||
> -		    (!is_power_of_2(sbi->s_inode_size)) ||
> -		    (sbi->s_inode_size > blocksize)) {
> -			ext4_msg(sb, KERN_ERR,
> -			       "unsupported inode size: %d",
> -			       sbi->s_inode_size);
> -			ext4_msg(sb, KERN_ERR, "blocksize: %d", blocksize);
> -			goto failed_mount;
> -		}
> -		/*
> -		 * i_atime_extra is the last extra field available for
> -		 * [acm]times in struct ext4_inode. Checking for that
> -		 * field should suffice to ensure we have extra space
> -		 * for all three.
> -		 */
> -		if (sbi->s_inode_size >= offsetof(struct ext4_inode, i_atime_extra) +
> -			sizeof(((struct ext4_inode *)0)->i_atime_extra)) {
> -			sb->s_time_gran = 1;
> -			sb->s_time_max = EXT4_EXTRA_TIMESTAMP_MAX;
> -		} else {
> -			sb->s_time_gran = NSEC_PER_SEC;
> -			sb->s_time_max = EXT4_NON_EXTRA_TIMESTAMP_MAX;
> -		}
> -		sb->s_time_min = EXT4_TIMESTAMP_MIN;
> -	}
> -	if (sbi->s_inode_size > EXT4_GOOD_OLD_INODE_SIZE) {
> -		sbi->s_want_extra_isize = sizeof(struct ext4_inode) -
> -			EXT4_GOOD_OLD_INODE_SIZE;
> -		if (ext4_has_feature_extra_isize(sb)) {
> -			unsigned v, max = (sbi->s_inode_size -
> -					   EXT4_GOOD_OLD_INODE_SIZE);
> -
> -			v = le16_to_cpu(es->s_want_extra_isize);
> -			if (v > max) {
> -				ext4_msg(sb, KERN_ERR,
> -					 "bad s_want_extra_isize: %d", v);
> -				goto failed_mount;
> -			}
> -			if (sbi->s_want_extra_isize < v)
> -				sbi->s_want_extra_isize = v;
> -
> -			v = le16_to_cpu(es->s_min_extra_isize);
> -			if (v > max) {
> -				ext4_msg(sb, KERN_ERR,
> -					 "bad s_min_extra_isize: %d", v);
> -				goto failed_mount;
> -			}
> -			if (sbi->s_want_extra_isize < v)
> -				sbi->s_want_extra_isize = v;
> -		}
> -	}
> +	if (ext4_inode_info_init(sb, es, blocksize))
> +		goto failed_mount;
>  
>  	err = parse_apply_sb_mount_options(sb, ctx);
>  	if (err < 0)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 07/13] ext4: factor out ext4_encoding_init()

[Jason Yan]

Factor out ext4_encoding_init(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 80 +++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 458d146149b2..9ee27472b802 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4521,6 +4521,48 @@ static int ext4_inode_info_init(struct super_block *sb,
 	return 0;
 }
 
+static int ext4_encoding_init(struct super_block *sb, struct ext4_super_block *es)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	const struct ext4_sb_encodings *encoding_info;
+	struct unicode_map *encoding;
+	__u16 encoding_flags = le16_to_cpu(es->s_encoding_flags);
+
+	if (!ext4_has_feature_casefold(sb) || sb->s_encoding)
+		return 0;
+
+	encoding_info = ext4_sb_read_encoding(es);
+	if (!encoding_info) {
+		ext4_msg(sb, KERN_ERR,
+			"Encoding requested by superblock is unknown");
+		return -EINVAL;
+	}
+
+	encoding = utf8_load(encoding_info->version);
+	if (IS_ERR(encoding)) {
+		ext4_msg(sb, KERN_ERR,
+			"can't mount with superblock charset: %s-%u.%u.%u "
+			"not supported by the kernel. flags: 0x%x.",
+			encoding_info->name,
+			unicode_major(encoding_info->version),
+			unicode_minor(encoding_info->version),
+			unicode_rev(encoding_info->version),
+			encoding_flags);
+		return -EINVAL;
+	}
+	ext4_msg(sb, KERN_INFO,"Using encoding defined by superblock: "
+		"%s-%u.%u.%u with flags 0x%hx", encoding_info->name,
+		unicode_major(encoding_info->version),
+		unicode_minor(encoding_info->version),
+		unicode_rev(encoding_info->version),
+		encoding_flags);
+
+	sb->s_encoding = encoding;
+	sb->s_encoding_flags = encoding_flags;
+#endif
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4678,42 +4720,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	ext4_apply_options(fc, sb);
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
-		const struct ext4_sb_encodings *encoding_info;
-		struct unicode_map *encoding;
-		__u16 encoding_flags = le16_to_cpu(es->s_encoding_flags);
-
-		encoding_info = ext4_sb_read_encoding(es);
-		if (!encoding_info) {
-			ext4_msg(sb, KERN_ERR,
-				 "Encoding requested by superblock is unknown");
-			goto failed_mount;
-		}
-
-		encoding = utf8_load(encoding_info->version);
-		if (IS_ERR(encoding)) {
-			ext4_msg(sb, KERN_ERR,
-				 "can't mount with superblock charset: %s-%u.%u.%u "
-				 "not supported by the kernel. flags: 0x%x.",
-				 encoding_info->name,
-				 unicode_major(encoding_info->version),
-				 unicode_minor(encoding_info->version),
-				 unicode_rev(encoding_info->version),
-				 encoding_flags);
-			goto failed_mount;
-		}
-		ext4_msg(sb, KERN_INFO,"Using encoding defined by superblock: "
-			 "%s-%u.%u.%u with flags 0x%hx", encoding_info->name,
-			 unicode_major(encoding_info->version),
-			 unicode_minor(encoding_info->version),
-			 unicode_rev(encoding_info->version),
-			 encoding_flags);
-
-		sb->s_encoding = encoding;
-		sb->s_encoding_flags = encoding_flags;
-	}
-#endif
+	if (ext4_encoding_init(sb, es))
+		goto failed_mount;
 
 	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
 		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with data=journal disables delayed allocation, dioread_nolock, O_DIRECT and fast_commit support!\n");
-- 
2.31.1

Re: [PATCH 07/13] ext4: factor out ext4_encoding_init()

[Jan Kara]

On Tue 30-08-22 20:04:05, Jason Yan wrote:
> Factor out ext4_encoding_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 80 +++++++++++++++++++++++++++----------------------
>  1 file changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 458d146149b2..9ee27472b802 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4521,6 +4521,48 @@ static int ext4_inode_info_init(struct super_block *sb,
>  	return 0;
>  }
>  
> +static int ext4_encoding_init(struct super_block *sb, struct ext4_super_block *es)
> +{
> +#if IS_ENABLED(CONFIG_UNICODE)
> +	const struct ext4_sb_encodings *encoding_info;
> +	struct unicode_map *encoding;
> +	__u16 encoding_flags = le16_to_cpu(es->s_encoding_flags);
> +
> +	if (!ext4_has_feature_casefold(sb) || sb->s_encoding)
> +		return 0;
> +
> +	encoding_info = ext4_sb_read_encoding(es);
> +	if (!encoding_info) {
> +		ext4_msg(sb, KERN_ERR,
> +			"Encoding requested by superblock is unknown");
> +		return -EINVAL;
> +	}
> +
> +	encoding = utf8_load(encoding_info->version);
> +	if (IS_ERR(encoding)) {
> +		ext4_msg(sb, KERN_ERR,
> +			"can't mount with superblock charset: %s-%u.%u.%u "
> +			"not supported by the kernel. flags: 0x%x.",
> +			encoding_info->name,
> +			unicode_major(encoding_info->version),
> +			unicode_minor(encoding_info->version),
> +			unicode_rev(encoding_info->version),
> +			encoding_flags);
> +		return -EINVAL;
> +	}
> +	ext4_msg(sb, KERN_INFO,"Using encoding defined by superblock: "
> +		"%s-%u.%u.%u with flags 0x%hx", encoding_info->name,
> +		unicode_major(encoding_info->version),
> +		unicode_minor(encoding_info->version),
> +		unicode_rev(encoding_info->version),
> +		encoding_flags);
> +
> +	sb->s_encoding = encoding;
> +	sb->s_encoding_flags = encoding_flags;
> +#endif
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4678,42 +4720,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	ext4_apply_options(fc, sb);
>  
> -#if IS_ENABLED(CONFIG_UNICODE)
> -	if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
> -		const struct ext4_sb_encodings *encoding_info;
> -		struct unicode_map *encoding;
> -		__u16 encoding_flags = le16_to_cpu(es->s_encoding_flags);
> -
> -		encoding_info = ext4_sb_read_encoding(es);
> -		if (!encoding_info) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "Encoding requested by superblock is unknown");
> -			goto failed_mount;
> -		}
> -
> -		encoding = utf8_load(encoding_info->version);
> -		if (IS_ERR(encoding)) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "can't mount with superblock charset: %s-%u.%u.%u "
> -				 "not supported by the kernel. flags: 0x%x.",
> -				 encoding_info->name,
> -				 unicode_major(encoding_info->version),
> -				 unicode_minor(encoding_info->version),
> -				 unicode_rev(encoding_info->version),
> -				 encoding_flags);
> -			goto failed_mount;
> -		}
> -		ext4_msg(sb, KERN_INFO,"Using encoding defined by superblock: "
> -			 "%s-%u.%u.%u with flags 0x%hx", encoding_info->name,
> -			 unicode_major(encoding_info->version),
> -			 unicode_minor(encoding_info->version),
> -			 unicode_rev(encoding_info->version),
> -			 encoding_flags);
> -
> -		sb->s_encoding = encoding;
> -		sb->s_encoding_flags = encoding_flags;
> -	}
> -#endif
> +	if (ext4_encoding_init(sb, es))
> +		goto failed_mount;
>  
>  	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
>  		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with data=journal disables delayed allocation, dioread_nolock, O_DIRECT and fast_commit support!\n");
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 08/13] ext4: factor out ext4_handle_csum()

[Jason Yan]

Factor out ext4_handle_csum(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 83 +++++++++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9ee27472b802..96cf23787bba 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4563,6 +4563,50 @@ static int ext4_encoding_init(struct super_block *sb, struct ext4_super_block *e
 	return 0;
 }
 
+static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	/* Warn if metadata_csum and gdt_csum are both set. */
+	if (ext4_has_feature_metadata_csum(sb) &&
+	    ext4_has_feature_gdt_csum(sb))
+		ext4_warning(sb, "metadata_csum and uninit_bg are "
+			     "redundant flags; please run fsck.");
+
+	/* Check for a known checksum algorithm */
+	if (!ext4_verify_csum_type(sb, es)) {
+		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
+			 "unknown checksum algorithm.");
+		return -EINVAL;
+	}
+	ext4_setup_csum_trigger(sb, EXT4_JTR_ORPHAN_FILE,
+				ext4_orphan_file_block_trigger);
+
+	/* Load the checksum driver */
+	sbi->s_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
+	if (IS_ERR(sbi->s_chksum_driver)) {
+		int ret = PTR_ERR(sbi->s_chksum_driver);
+		ext4_msg(sb, KERN_ERR, "Cannot load crc32c driver.");
+		sbi->s_chksum_driver = NULL;
+		return ret;
+	}
+
+	/* Check superblock checksum */
+	if (!ext4_superblock_csum_verify(sb, es)) {
+		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
+			 "invalid superblock checksum.  Run e2fsck?");
+		return -EFSBADCRC;
+	}
+
+	/* Precompute checksum seed for all metadata */
+	if (ext4_has_feature_csum_seed(sb))
+		sbi->s_csum_seed = le32_to_cpu(es->s_checksum_seed);
+	else if (ext4_has_metadata_csum(sb) || ext4_has_feature_ea_inode(sb))
+		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
+					       sizeof(es->s_uuid));
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4632,44 +4676,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 
 	sbi->s_kbytes_written = le64_to_cpu(es->s_kbytes_written);
 
-	/* Warn if metadata_csum and gdt_csum are both set. */
-	if (ext4_has_feature_metadata_csum(sb) &&
-	    ext4_has_feature_gdt_csum(sb))
-		ext4_warning(sb, "metadata_csum and uninit_bg are "
-			     "redundant flags; please run fsck.");
-
-	/* Check for a known checksum algorithm */
-	if (!ext4_verify_csum_type(sb, es)) {
-		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
-			 "unknown checksum algorithm.");
-		goto failed_mount;
-	}
-	ext4_setup_csum_trigger(sb, EXT4_JTR_ORPHAN_FILE,
-				ext4_orphan_file_block_trigger);
-
-	/* Load the checksum driver */
-	sbi->s_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
-	if (IS_ERR(sbi->s_chksum_driver)) {
-		ext4_msg(sb, KERN_ERR, "Cannot load crc32c driver.");
-		ret = PTR_ERR(sbi->s_chksum_driver);
-		sbi->s_chksum_driver = NULL;
-		goto failed_mount;
-	}
-
-	/* Check superblock checksum */
-	if (!ext4_superblock_csum_verify(sb, es)) {
-		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
-			 "invalid superblock checksum.  Run e2fsck?");
-		ret = -EFSBADCRC;
+	err = ext4_handle_csum(sb, es);
+	if (err)
 		goto failed_mount;
-	}
-
-	/* Precompute checksum seed for all metadata */
-	if (ext4_has_feature_csum_seed(sb))
-		sbi->s_csum_seed = le32_to_cpu(es->s_checksum_seed);
-	else if (ext4_has_metadata_csum(sb) || ext4_has_feature_ea_inode(sb))
-		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
-					       sizeof(es->s_uuid));
 
 	ext4_set_def_opts(sb, es);
 
-- 
2.31.1

Re: [PATCH 08/13] ext4: factor out ext4_handle_csum()

[Jan Kara]

On Tue 30-08-22 20:04:06, Jason Yan wrote:
> Factor out ext4_handle_csum(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  fs/ext4/super.c | 83 +++++++++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9ee27472b802..96cf23787bba 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4563,6 +4563,50 @@ static int ext4_encoding_init(struct super_block *sb, struct ext4_super_block *e
>  	return 0;
>  }
>  
> +static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)

I'd rather call this function like ext4_init_metadata_csum().
ext4_handle_csum() seems to vague to me.

Otherwise the patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	/* Warn if metadata_csum and gdt_csum are both set. */
> +	if (ext4_has_feature_metadata_csum(sb) &&
> +	    ext4_has_feature_gdt_csum(sb))
> +		ext4_warning(sb, "metadata_csum and uninit_bg are "
> +			     "redundant flags; please run fsck.");
> +
> +	/* Check for a known checksum algorithm */
> +	if (!ext4_verify_csum_type(sb, es)) {
> +		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> +			 "unknown checksum algorithm.");
> +		return -EINVAL;
> +	}
> +	ext4_setup_csum_trigger(sb, EXT4_JTR_ORPHAN_FILE,
> +				ext4_orphan_file_block_trigger);
> +
> +	/* Load the checksum driver */
> +	sbi->s_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> +	if (IS_ERR(sbi->s_chksum_driver)) {
> +		int ret = PTR_ERR(sbi->s_chksum_driver);
> +		ext4_msg(sb, KERN_ERR, "Cannot load crc32c driver.");
> +		sbi->s_chksum_driver = NULL;
> +		return ret;
> +	}
> +
> +	/* Check superblock checksum */
> +	if (!ext4_superblock_csum_verify(sb, es)) {
> +		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> +			 "invalid superblock checksum.  Run e2fsck?");
> +		return -EFSBADCRC;
> +	}
> +
> +	/* Precompute checksum seed for all metadata */
> +	if (ext4_has_feature_csum_seed(sb))
> +		sbi->s_csum_seed = le32_to_cpu(es->s_checksum_seed);
> +	else if (ext4_has_metadata_csum(sb) || ext4_has_feature_ea_inode(sb))
> +		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
> +					       sizeof(es->s_uuid));
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4632,44 +4676,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	sbi->s_kbytes_written = le64_to_cpu(es->s_kbytes_written);
>  
> -	/* Warn if metadata_csum and gdt_csum are both set. */
> -	if (ext4_has_feature_metadata_csum(sb) &&
> -	    ext4_has_feature_gdt_csum(sb))
> -		ext4_warning(sb, "metadata_csum and uninit_bg are "
> -			     "redundant flags; please run fsck.");
> -
> -	/* Check for a known checksum algorithm */
> -	if (!ext4_verify_csum_type(sb, es)) {
> -		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> -			 "unknown checksum algorithm.");
> -		goto failed_mount;
> -	}
> -	ext4_setup_csum_trigger(sb, EXT4_JTR_ORPHAN_FILE,
> -				ext4_orphan_file_block_trigger);
> -
> -	/* Load the checksum driver */
> -	sbi->s_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> -	if (IS_ERR(sbi->s_chksum_driver)) {
> -		ext4_msg(sb, KERN_ERR, "Cannot load crc32c driver.");
> -		ret = PTR_ERR(sbi->s_chksum_driver);
> -		sbi->s_chksum_driver = NULL;
> -		goto failed_mount;
> -	}
> -
> -	/* Check superblock checksum */
> -	if (!ext4_superblock_csum_verify(sb, es)) {
> -		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> -			 "invalid superblock checksum.  Run e2fsck?");
> -		ret = -EFSBADCRC;
> +	err = ext4_handle_csum(sb, es);
> +	if (err)
>  		goto failed_mount;
> -	}
> -
> -	/* Precompute checksum seed for all metadata */
> -	if (ext4_has_feature_csum_seed(sb))
> -		sbi->s_csum_seed = le32_to_cpu(es->s_checksum_seed);
> -	else if (ext4_has_metadata_csum(sb) || ext4_has_feature_ea_inode(sb))
> -		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
> -					       sizeof(es->s_uuid));
>  
>  	ext4_set_def_opts(sb, es);
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 08/13] ext4: factor out ext4_handle_csum()

[Jason Yan]

On 2022/8/31 19:52, Jan Kara wrote:
> On Tue 30-08-22 20:04:06, Jason Yan wrote:
>> Factor out ext4_handle_csum(). No functional change.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   fs/ext4/super.c | 83 +++++++++++++++++++++++++++----------------------
>>   1 file changed, 46 insertions(+), 37 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 9ee27472b802..96cf23787bba 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4563,6 +4563,50 @@ static int ext4_encoding_init(struct super_block *sb, struct ext4_super_block *e
>>   	return 0;
>>   }
>>   
>> +static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)
> 
> I'd rather call this function like ext4_init_metadata_csum().
> ext4_handle_csum() seems to vague to me.
> 

Yes, much better.

Thanks.

> Otherwise the patch looks good to me. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza
> 

[PATCH 09/13] ext4: factor out ext4_compat_feature_check()

[Jason Yan]

Factor out ext4_compat_feature_check(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 144 ++++++++++++++++++++++++++----------------------
 1 file changed, 77 insertions(+), 67 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 96cf23787bba..1e7d6eb6a3aa 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4607,6 +4607,82 @@ static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)
 	return 0;
 }
 
+static int ext4_compat_feature_check(struct super_block *sb,
+				     struct ext4_super_block *es,
+				     int silent)
+{
+	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
+	    (ext4_has_compat_features(sb) ||
+	     ext4_has_ro_compat_features(sb) ||
+	     ext4_has_incompat_features(sb)))
+		ext4_msg(sb, KERN_WARNING,
+		       "feature flags set on rev 0 fs, "
+		       "running e2fsck is recommended");
+
+	if (es->s_creator_os == cpu_to_le32(EXT4_OS_HURD)) {
+		set_opt2(sb, HURD_COMPAT);
+		if (ext4_has_feature_64bit(sb)) {
+			ext4_msg(sb, KERN_ERR,
+				 "The Hurd can't support 64-bit file systems");
+			return -EINVAL;
+		}
+
+		/*
+		 * ea_inode feature uses l_i_version field which is not
+		 * available in HURD_COMPAT mode.
+		 */
+		if (ext4_has_feature_ea_inode(sb)) {
+			ext4_msg(sb, KERN_ERR,
+				 "ea_inode feature is not supported for Hurd");
+			return -EINVAL;
+		}
+	}
+
+	if (IS_EXT2_SB(sb)) {
+		if (ext2_feature_set_ok(sb))
+			ext4_msg(sb, KERN_INFO, "mounting ext2 file system "
+				 "using the ext4 subsystem");
+		else {
+			/*
+			 * If we're probing be silent, if this looks like
+			 * it's actually an ext[34] filesystem.
+			 */
+			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
+				return -EINVAL;
+			ext4_msg(sb, KERN_ERR, "couldn't mount as ext2 due "
+				 "to feature incompatibilities");
+			return -EINVAL;
+		}
+	}
+
+	if (IS_EXT3_SB(sb)) {
+		if (ext3_feature_set_ok(sb))
+			ext4_msg(sb, KERN_INFO, "mounting ext3 file system "
+				 "using the ext4 subsystem");
+		else {
+			/*
+			 * If we're probing be silent, if this looks like
+			 * it's actually an ext4 filesystem.
+			 */
+			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
+				return -EINVAL;
+			ext4_msg(sb, KERN_ERR, "couldn't mount as ext3 due "
+				 "to feature incompatibilities");
+			return -EINVAL;
+		}
+	}
+
+	/*
+	 * Check feature flags regardless of the revision level, since we
+	 * previously didn't change the revision level when setting the flags,
+	 * so there is a chance incompat flags are set on a rev 0 filesystem.
+	 */
+	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4761,73 +4837,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
 		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
 
-	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
-	    (ext4_has_compat_features(sb) ||
-	     ext4_has_ro_compat_features(sb) ||
-	     ext4_has_incompat_features(sb)))
-		ext4_msg(sb, KERN_WARNING,
-		       "feature flags set on rev 0 fs, "
-		       "running e2fsck is recommended");
-
-	if (es->s_creator_os == cpu_to_le32(EXT4_OS_HURD)) {
-		set_opt2(sb, HURD_COMPAT);
-		if (ext4_has_feature_64bit(sb)) {
-			ext4_msg(sb, KERN_ERR,
-				 "The Hurd can't support 64-bit file systems");
-			goto failed_mount;
-		}
-
-		/*
-		 * ea_inode feature uses l_i_version field which is not
-		 * available in HURD_COMPAT mode.
-		 */
-		if (ext4_has_feature_ea_inode(sb)) {
-			ext4_msg(sb, KERN_ERR,
-				 "ea_inode feature is not supported for Hurd");
-			goto failed_mount;
-		}
-	}
-
-	if (IS_EXT2_SB(sb)) {
-		if (ext2_feature_set_ok(sb))
-			ext4_msg(sb, KERN_INFO, "mounting ext2 file system "
-				 "using the ext4 subsystem");
-		else {
-			/*
-			 * If we're probing be silent, if this looks like
-			 * it's actually an ext[34] filesystem.
-			 */
-			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
-				goto failed_mount;
-			ext4_msg(sb, KERN_ERR, "couldn't mount as ext2 due "
-				 "to feature incompatibilities");
-			goto failed_mount;
-		}
-	}
-
-	if (IS_EXT3_SB(sb)) {
-		if (ext3_feature_set_ok(sb))
-			ext4_msg(sb, KERN_INFO, "mounting ext3 file system "
-				 "using the ext4 subsystem");
-		else {
-			/*
-			 * If we're probing be silent, if this looks like
-			 * it's actually an ext4 filesystem.
-			 */
-			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
-				goto failed_mount;
-			ext4_msg(sb, KERN_ERR, "couldn't mount as ext3 due "
-				 "to feature incompatibilities");
-			goto failed_mount;
-		}
-	}
-
-	/*
-	 * Check feature flags regardless of the revision level, since we
-	 * previously didn't change the revision level when setting the flags,
-	 * so there is a chance incompat flags are set on a rev 0 filesystem.
-	 */
-	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
+	if (ext4_compat_feature_check(sb, es, silent))
 		goto failed_mount;
 
 	if (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks) > (blocksize / 4)) {
-- 
2.31.1

Re: [PATCH 09/13] ext4: factor out ext4_compat_feature_check()

[Jan Kara]

On Tue 30-08-22 20:04:07, Jason Yan wrote:
> Factor out ext4_compat_feature_check(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  fs/ext4/super.c | 144 ++++++++++++++++++++++++++----------------------
>  1 file changed, 77 insertions(+), 67 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 96cf23787bba..1e7d6eb6a3aa 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4607,6 +4607,82 @@ static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)
>  	return 0;
>  }
>  
> +static int ext4_compat_feature_check(struct super_block *sb,
> +				     struct ext4_super_block *es,
> +				     int silent)

And here maybe ext4_check_feature_compatibility() might be a better name
because "compat_feature" is a name of a specific subset of ext4 features so
using it in function name is a bit confusing. Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> +{
> +	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
> +	    (ext4_has_compat_features(sb) ||
> +	     ext4_has_ro_compat_features(sb) ||
> +	     ext4_has_incompat_features(sb)))
> +		ext4_msg(sb, KERN_WARNING,
> +		       "feature flags set on rev 0 fs, "
> +		       "running e2fsck is recommended");
> +
> +	if (es->s_creator_os == cpu_to_le32(EXT4_OS_HURD)) {
> +		set_opt2(sb, HURD_COMPAT);
> +		if (ext4_has_feature_64bit(sb)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "The Hurd can't support 64-bit file systems");
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * ea_inode feature uses l_i_version field which is not
> +		 * available in HURD_COMPAT mode.
> +		 */
> +		if (ext4_has_feature_ea_inode(sb)) {
> +			ext4_msg(sb, KERN_ERR,
> +				 "ea_inode feature is not supported for Hurd");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (IS_EXT2_SB(sb)) {
> +		if (ext2_feature_set_ok(sb))
> +			ext4_msg(sb, KERN_INFO, "mounting ext2 file system "
> +				 "using the ext4 subsystem");
> +		else {
> +			/*
> +			 * If we're probing be silent, if this looks like
> +			 * it's actually an ext[34] filesystem.
> +			 */
> +			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
> +				return -EINVAL;
> +			ext4_msg(sb, KERN_ERR, "couldn't mount as ext2 due "
> +				 "to feature incompatibilities");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (IS_EXT3_SB(sb)) {
> +		if (ext3_feature_set_ok(sb))
> +			ext4_msg(sb, KERN_INFO, "mounting ext3 file system "
> +				 "using the ext4 subsystem");
> +		else {
> +			/*
> +			 * If we're probing be silent, if this looks like
> +			 * it's actually an ext4 filesystem.
> +			 */
> +			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
> +				return -EINVAL;
> +			ext4_msg(sb, KERN_ERR, "couldn't mount as ext3 due "
> +				 "to feature incompatibilities");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * Check feature flags regardless of the revision level, since we
> +	 * previously didn't change the revision level when setting the flags,
> +	 * so there is a chance incompat flags are set on a rev 0 filesystem.
> +	 */
> +	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4761,73 +4837,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
>  
> -	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV &&
> -	    (ext4_has_compat_features(sb) ||
> -	     ext4_has_ro_compat_features(sb) ||
> -	     ext4_has_incompat_features(sb)))
> -		ext4_msg(sb, KERN_WARNING,
> -		       "feature flags set on rev 0 fs, "
> -		       "running e2fsck is recommended");
> -
> -	if (es->s_creator_os == cpu_to_le32(EXT4_OS_HURD)) {
> -		set_opt2(sb, HURD_COMPAT);
> -		if (ext4_has_feature_64bit(sb)) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "The Hurd can't support 64-bit file systems");
> -			goto failed_mount;
> -		}
> -
> -		/*
> -		 * ea_inode feature uses l_i_version field which is not
> -		 * available in HURD_COMPAT mode.
> -		 */
> -		if (ext4_has_feature_ea_inode(sb)) {
> -			ext4_msg(sb, KERN_ERR,
> -				 "ea_inode feature is not supported for Hurd");
> -			goto failed_mount;
> -		}
> -	}
> -
> -	if (IS_EXT2_SB(sb)) {
> -		if (ext2_feature_set_ok(sb))
> -			ext4_msg(sb, KERN_INFO, "mounting ext2 file system "
> -				 "using the ext4 subsystem");
> -		else {
> -			/*
> -			 * If we're probing be silent, if this looks like
> -			 * it's actually an ext[34] filesystem.
> -			 */
> -			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
> -				goto failed_mount;
> -			ext4_msg(sb, KERN_ERR, "couldn't mount as ext2 due "
> -				 "to feature incompatibilities");
> -			goto failed_mount;
> -		}
> -	}
> -
> -	if (IS_EXT3_SB(sb)) {
> -		if (ext3_feature_set_ok(sb))
> -			ext4_msg(sb, KERN_INFO, "mounting ext3 file system "
> -				 "using the ext4 subsystem");
> -		else {
> -			/*
> -			 * If we're probing be silent, if this looks like
> -			 * it's actually an ext4 filesystem.
> -			 */
> -			if (silent && ext4_feature_set_ok(sb, sb_rdonly(sb)))
> -				goto failed_mount;
> -			ext4_msg(sb, KERN_ERR, "couldn't mount as ext3 due "
> -				 "to feature incompatibilities");
> -			goto failed_mount;
> -		}
> -	}
> -
> -	/*
> -	 * Check feature flags regardless of the revision level, since we
> -	 * previously didn't change the revision level when setting the flags,
> -	 * so there is a chance incompat flags are set on a rev 0 filesystem.
> -	 */
> -	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
> +	if (ext4_compat_feature_check(sb, es, silent))
>  		goto failed_mount;
>  
>  	if (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks) > (blocksize / 4)) {
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 09/13] ext4: factor out ext4_compat_feature_check()

[Jason Yan]

On 2022/8/31 19:55, Jan Kara wrote:
> On Tue 30-08-22 20:04:07, Jason Yan wrote:
>> Factor out ext4_compat_feature_check(). No functional change.
>>
>> Signed-off-by: Jason Yan<yanaijie@huawei.com>
>> ---
>>   fs/ext4/super.c | 144 ++++++++++++++++++++++++++----------------------
>>   1 file changed, 77 insertions(+), 67 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 96cf23787bba..1e7d6eb6a3aa 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4607,6 +4607,82 @@ static int ext4_handle_csum(struct super_block *sb, struct ext4_super_block *es)
>>   	return 0;
>>   }
>>   
>> +static int ext4_compat_feature_check(struct super_block *sb,
>> +				     struct ext4_super_block *es,
>> +				     int silent)
> And here maybe ext4_check_feature_compatibility() might be a better name
> because "compat_feature" is a name of a specific subset of ext4 features so
> using it in function name is a bit confusing. Otherwise feel free to add:
> 

Indeed.

Thanks.

> Reviewed-by: Jan Kara<jack@suse.cz>

[PATCH 10/13] ext4: factor out ext4_geometry_check()

[Jason Yan]

Factor out ext4_geometry_check(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 111 ++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 50 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 1e7d6eb6a3aa..4c6c4930e11b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4683,6 +4683,66 @@ static int ext4_compat_feature_check(struct super_block *sb,
 	return 0;
 }
 
+static int ext4_geometry_check(struct super_block *sb,
+			       struct ext4_super_block *es)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	__u64 blocks_count;
+
+	/* check blocks count against device size */
+	blocks_count = sb_bdev_nr_blocks(sb);
+	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
+		ext4_msg(sb, KERN_WARNING, "bad geometry: block count %llu "
+		       "exceeds size of device (%llu blocks)",
+		       ext4_blocks_count(es), blocks_count);
+		return -EINVAL;
+	}
+
+	/*
+	 * It makes no sense for the first data block to be beyond the end
+	 * of the filesystem.
+	 */
+	if (le32_to_cpu(es->s_first_data_block) >= ext4_blocks_count(es)) {
+		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
+			 "block %u is beyond end of filesystem (%llu)",
+			 le32_to_cpu(es->s_first_data_block),
+			 ext4_blocks_count(es));
+		return -EINVAL;
+	}
+	if ((es->s_first_data_block == 0) && (es->s_log_block_size == 0) &&
+	    (sbi->s_cluster_ratio == 1)) {
+		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
+			 "block is 0 with a 1k block and cluster size");
+		return -EINVAL;
+	}
+
+	blocks_count = (ext4_blocks_count(es) -
+			le32_to_cpu(es->s_first_data_block) +
+			EXT4_BLOCKS_PER_GROUP(sb) - 1);
+	do_div(blocks_count, EXT4_BLOCKS_PER_GROUP(sb));
+	if (blocks_count > ((uint64_t)1<<32) - EXT4_DESC_PER_BLOCK(sb)) {
+		ext4_msg(sb, KERN_WARNING, "groups count too large: %llu "
+		       "(block count %llu, first data block %u, "
+		       "blocks per group %lu)", blocks_count,
+		       ext4_blocks_count(es),
+		       le32_to_cpu(es->s_first_data_block),
+		       EXT4_BLOCKS_PER_GROUP(sb));
+		return -EINVAL;
+	}
+	sbi->s_groups_count = blocks_count;
+	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
+			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
+	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
+	    le32_to_cpu(es->s_inodes_count)) {
+		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
+			 le32_to_cpu(es->s_inodes_count),
+			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh, **group_desc;
@@ -4698,7 +4758,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	unsigned int db_count;
 	unsigned int i;
 	int needs_recovery, has_huge_files;
-	__u64 blocks_count;
 	int err = 0;
 	ext4_group_t first_not_zeroed;
 	struct ext4_fs_context *ctx = fc->fs_private;
@@ -4984,57 +5043,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		goto failed_mount;
 	}
 
-	/* check blocks count against device size */
-	blocks_count = sb_bdev_nr_blocks(sb);
-	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
-		ext4_msg(sb, KERN_WARNING, "bad geometry: block count %llu "
-		       "exceeds size of device (%llu blocks)",
-		       ext4_blocks_count(es), blocks_count);
+	if (ext4_geometry_check(sb, es))
 		goto failed_mount;
-	}
 
-	/*
-	 * It makes no sense for the first data block to be beyond the end
-	 * of the filesystem.
-	 */
-	if (le32_to_cpu(es->s_first_data_block) >= ext4_blocks_count(es)) {
-		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
-			 "block %u is beyond end of filesystem (%llu)",
-			 le32_to_cpu(es->s_first_data_block),
-			 ext4_blocks_count(es));
-		goto failed_mount;
-	}
-	if ((es->s_first_data_block == 0) && (es->s_log_block_size == 0) &&
-	    (sbi->s_cluster_ratio == 1)) {
-		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
-			 "block is 0 with a 1k block and cluster size");
-		goto failed_mount;
-	}
-
-	blocks_count = (ext4_blocks_count(es) -
-			le32_to_cpu(es->s_first_data_block) +
-			EXT4_BLOCKS_PER_GROUP(sb) - 1);
-	do_div(blocks_count, EXT4_BLOCKS_PER_GROUP(sb));
-	if (blocks_count > ((uint64_t)1<<32) - EXT4_DESC_PER_BLOCK(sb)) {
-		ext4_msg(sb, KERN_WARNING, "groups count too large: %llu "
-		       "(block count %llu, first data block %u, "
-		       "blocks per group %lu)", blocks_count,
-		       ext4_blocks_count(es),
-		       le32_to_cpu(es->s_first_data_block),
-		       EXT4_BLOCKS_PER_GROUP(sb));
-		goto failed_mount;
-	}
-	sbi->s_groups_count = blocks_count;
-	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
-			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
-	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
-	    le32_to_cpu(es->s_inodes_count)) {
-		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
-			 le32_to_cpu(es->s_inodes_count),
-			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
-		ret = -EINVAL;
-		goto failed_mount;
-	}
 	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
 		   EXT4_DESC_PER_BLOCK(sb);
 	if (ext4_has_feature_meta_bg(sb)) {
-- 
2.31.1

Re: [PATCH 10/13] ext4: factor out ext4_geometry_check()

[Jan Kara]

On Tue 30-08-22 20:04:08, Jason Yan wrote:
> Factor out ext4_geometry_check(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 111 ++++++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 1e7d6eb6a3aa..4c6c4930e11b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4683,6 +4683,66 @@ static int ext4_compat_feature_check(struct super_block *sb,
>  	return 0;
>  }
>  
> +static int ext4_geometry_check(struct super_block *sb,
> +			       struct ext4_super_block *es)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	__u64 blocks_count;
> +
> +	/* check blocks count against device size */
> +	blocks_count = sb_bdev_nr_blocks(sb);
> +	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
> +		ext4_msg(sb, KERN_WARNING, "bad geometry: block count %llu "
> +		       "exceeds size of device (%llu blocks)",
> +		       ext4_blocks_count(es), blocks_count);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * It makes no sense for the first data block to be beyond the end
> +	 * of the filesystem.
> +	 */
> +	if (le32_to_cpu(es->s_first_data_block) >= ext4_blocks_count(es)) {
> +		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
> +			 "block %u is beyond end of filesystem (%llu)",
> +			 le32_to_cpu(es->s_first_data_block),
> +			 ext4_blocks_count(es));
> +		return -EINVAL;
> +	}
> +	if ((es->s_first_data_block == 0) && (es->s_log_block_size == 0) &&
> +	    (sbi->s_cluster_ratio == 1)) {
> +		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
> +			 "block is 0 with a 1k block and cluster size");
> +		return -EINVAL;
> +	}
> +
> +	blocks_count = (ext4_blocks_count(es) -
> +			le32_to_cpu(es->s_first_data_block) +
> +			EXT4_BLOCKS_PER_GROUP(sb) - 1);
> +	do_div(blocks_count, EXT4_BLOCKS_PER_GROUP(sb));
> +	if (blocks_count > ((uint64_t)1<<32) - EXT4_DESC_PER_BLOCK(sb)) {
> +		ext4_msg(sb, KERN_WARNING, "groups count too large: %llu "
> +		       "(block count %llu, first data block %u, "
> +		       "blocks per group %lu)", blocks_count,
> +		       ext4_blocks_count(es),
> +		       le32_to_cpu(es->s_first_data_block),
> +		       EXT4_BLOCKS_PER_GROUP(sb));
> +		return -EINVAL;
> +	}
> +	sbi->s_groups_count = blocks_count;
> +	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
> +			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
> +	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
> +	    le32_to_cpu(es->s_inodes_count)) {
> +		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
> +			 le32_to_cpu(es->s_inodes_count),
> +			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh, **group_desc;
> @@ -4698,7 +4758,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	unsigned int db_count;
>  	unsigned int i;
>  	int needs_recovery, has_huge_files;
> -	__u64 blocks_count;
>  	int err = 0;
>  	ext4_group_t first_not_zeroed;
>  	struct ext4_fs_context *ctx = fc->fs_private;
> @@ -4984,57 +5043,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		goto failed_mount;
>  	}
>  
> -	/* check blocks count against device size */
> -	blocks_count = sb_bdev_nr_blocks(sb);
> -	if (blocks_count && ext4_blocks_count(es) > blocks_count) {
> -		ext4_msg(sb, KERN_WARNING, "bad geometry: block count %llu "
> -		       "exceeds size of device (%llu blocks)",
> -		       ext4_blocks_count(es), blocks_count);
> +	if (ext4_geometry_check(sb, es))
>  		goto failed_mount;
> -	}
>  
> -	/*
> -	 * It makes no sense for the first data block to be beyond the end
> -	 * of the filesystem.
> -	 */
> -	if (le32_to_cpu(es->s_first_data_block) >= ext4_blocks_count(es)) {
> -		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
> -			 "block %u is beyond end of filesystem (%llu)",
> -			 le32_to_cpu(es->s_first_data_block),
> -			 ext4_blocks_count(es));
> -		goto failed_mount;
> -	}
> -	if ((es->s_first_data_block == 0) && (es->s_log_block_size == 0) &&
> -	    (sbi->s_cluster_ratio == 1)) {
> -		ext4_msg(sb, KERN_WARNING, "bad geometry: first data "
> -			 "block is 0 with a 1k block and cluster size");
> -		goto failed_mount;
> -	}
> -
> -	blocks_count = (ext4_blocks_count(es) -
> -			le32_to_cpu(es->s_first_data_block) +
> -			EXT4_BLOCKS_PER_GROUP(sb) - 1);
> -	do_div(blocks_count, EXT4_BLOCKS_PER_GROUP(sb));
> -	if (blocks_count > ((uint64_t)1<<32) - EXT4_DESC_PER_BLOCK(sb)) {
> -		ext4_msg(sb, KERN_WARNING, "groups count too large: %llu "
> -		       "(block count %llu, first data block %u, "
> -		       "blocks per group %lu)", blocks_count,
> -		       ext4_blocks_count(es),
> -		       le32_to_cpu(es->s_first_data_block),
> -		       EXT4_BLOCKS_PER_GROUP(sb));
> -		goto failed_mount;
> -	}
> -	sbi->s_groups_count = blocks_count;
> -	sbi->s_blockfile_groups = min_t(ext4_group_t, sbi->s_groups_count,
> -			(EXT4_MAX_BLOCK_FILE_PHYS / EXT4_BLOCKS_PER_GROUP(sb)));
> -	if (((u64)sbi->s_groups_count * sbi->s_inodes_per_group) !=
> -	    le32_to_cpu(es->s_inodes_count)) {
> -		ext4_msg(sb, KERN_ERR, "inodes count not valid: %u vs %llu",
> -			 le32_to_cpu(es->s_inodes_count),
> -			 ((u64)sbi->s_groups_count * sbi->s_inodes_per_group));
> -		ret = -EINVAL;
> -		goto failed_mount;
> -	}
>  	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
>  		   EXT4_DESC_PER_BLOCK(sb);
>  	if (ext4_has_feature_meta_bg(sb)) {
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 11/13] ext4: factor out ext4_group_desc_init() and ext4_group_desc_free()

[Jason Yan]

Factor out ext4_group_desc_init() and ext4_group_desc_free(). No
functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 143 ++++++++++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 59 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4c6c4930e11b..40f155543df0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4743,9 +4743,89 @@ static int ext4_geometry_check(struct super_block *sb,
 	return 0;
 }
 
+static void ext4_group_desc_free(struct ext4_sb_info *sbi)
+{
+	struct buffer_head **group_desc;
+	int i;
+
+	rcu_read_lock();
+	group_desc = rcu_dereference(sbi->s_group_desc);
+	for (i = 0; i < sbi->s_gdb_count; i++)
+		brelse(group_desc[i]);
+	kvfree(group_desc);
+	rcu_read_unlock();
+}
+
+static int ext4_group_desc_init(struct super_block *sb,
+				struct ext4_super_block *es,
+				ext4_fsblk_t logical_sb_block,
+				ext4_group_t *first_not_zeroed)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	unsigned int db_count;
+	ext4_fsblk_t block;
+	int ret;
+	int i;
+
+	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
+		   EXT4_DESC_PER_BLOCK(sb);
+	if (ext4_has_feature_meta_bg(sb)) {
+		if (le32_to_cpu(es->s_first_meta_bg) > db_count) {
+			ext4_msg(sb, KERN_WARNING,
+				 "first meta block group too large: %u "
+				 "(group descriptor block count %u)",
+				 le32_to_cpu(es->s_first_meta_bg), db_count);
+			return -EINVAL;
+		}
+	}
+	rcu_assign_pointer(sbi->s_group_desc,
+			   kvmalloc_array(db_count,
+					  sizeof(struct buffer_head *),
+					  GFP_KERNEL));
+	if (sbi->s_group_desc == NULL) {
+		ext4_msg(sb, KERN_ERR, "not enough memory");
+		return -ENOMEM;
+	}
+
+	bgl_lock_init(sbi->s_blockgroup_lock);
+
+	/* Pre-read the descriptors into the buffer cache */
+	for (i = 0; i < db_count; i++) {
+		block = descriptor_loc(sb, logical_sb_block, i);
+		ext4_sb_breadahead_unmovable(sb, block);
+	}
+
+	for (i = 0; i < db_count; i++) {
+		struct buffer_head *bh;
+
+		block = descriptor_loc(sb, logical_sb_block, i);
+		bh = ext4_sb_bread_unmovable(sb, block);
+		if (IS_ERR(bh)) {
+			ext4_msg(sb, KERN_ERR,
+			       "can't read group descriptor %d", i);
+			sbi->s_gdb_count = i;
+			ret = PTR_ERR(bh);
+			goto out;
+		}
+		rcu_read_lock();
+		rcu_dereference(sbi->s_group_desc)[i] = bh;
+		rcu_read_unlock();
+	}
+	sbi->s_gdb_count = db_count;
+	if (!ext4_check_descriptors(sb, logical_sb_block, first_not_zeroed)) {
+		ext4_msg(sb, KERN_ERR, "group descriptors corrupted!");
+		ret = -EFSCORRUPTED;
+		goto out;
+	}
+	return 0;
+out:
+	ext4_group_desc_free(sbi);
+	return ret;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
-	struct buffer_head *bh, **group_desc;
+	struct buffer_head *bh;
 	struct ext4_super_block *es = NULL;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct flex_groups **flex_groups;
@@ -4755,7 +4835,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	struct inode *root;
 	int ret = -ENOMEM;
 	int blocksize;
-	unsigned int db_count;
 	unsigned int i;
 	int needs_recovery, has_huge_files;
 	int err = 0;
@@ -5046,57 +5125,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	if (ext4_geometry_check(sb, es))
 		goto failed_mount;
 
-	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
-		   EXT4_DESC_PER_BLOCK(sb);
-	if (ext4_has_feature_meta_bg(sb)) {
-		if (le32_to_cpu(es->s_first_meta_bg) > db_count) {
-			ext4_msg(sb, KERN_WARNING,
-				 "first meta block group too large: %u "
-				 "(group descriptor block count %u)",
-				 le32_to_cpu(es->s_first_meta_bg), db_count);
-			goto failed_mount;
-		}
-	}
-	rcu_assign_pointer(sbi->s_group_desc,
-			   kvmalloc_array(db_count,
-					  sizeof(struct buffer_head *),
-					  GFP_KERNEL));
-	if (sbi->s_group_desc == NULL) {
-		ext4_msg(sb, KERN_ERR, "not enough memory");
-		ret = -ENOMEM;
+	err = ext4_group_desc_init(sb, es, logical_sb_block, &first_not_zeroed);
+	if (err)
 		goto failed_mount;
-	}
-
-	bgl_lock_init(sbi->s_blockgroup_lock);
-
-	/* Pre-read the descriptors into the buffer cache */
-	for (i = 0; i < db_count; i++) {
-		block = descriptor_loc(sb, logical_sb_block, i);
-		ext4_sb_breadahead_unmovable(sb, block);
-	}
-
-	for (i = 0; i < db_count; i++) {
-		struct buffer_head *bh;
-
-		block = descriptor_loc(sb, logical_sb_block, i);
-		bh = ext4_sb_bread_unmovable(sb, block);
-		if (IS_ERR(bh)) {
-			ext4_msg(sb, KERN_ERR,
-			       "can't read group descriptor %d", i);
-			db_count = i;
-			ret = PTR_ERR(bh);
-			goto failed_mount2;
-		}
-		rcu_read_lock();
-		rcu_dereference(sbi->s_group_desc)[i] = bh;
-		rcu_read_unlock();
-	}
-	sbi->s_gdb_count = db_count;
-	if (!ext4_check_descriptors(sb, logical_sb_block, &first_not_zeroed)) {
-		ext4_msg(sb, KERN_ERR, "group descriptors corrupted!");
-		ret = -EFSCORRUPTED;
-		goto failed_mount2;
-	}
 
 	timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
 	spin_lock_init(&sbi->s_error_lock);
@@ -5540,13 +5571,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	flush_work(&sbi->s_error_work);
 	del_timer_sync(&sbi->s_err_report);
 	ext4_stop_mmpd(sbi);
-failed_mount2:
-	rcu_read_lock();
-	group_desc = rcu_dereference(sbi->s_group_desc);
-	for (i = 0; i < db_count; i++)
-		brelse(group_desc[i]);
-	kvfree(group_desc);
-	rcu_read_unlock();
+	ext4_group_desc_free(sbi);
 failed_mount:
 	if (sbi->s_chksum_driver)
 		crypto_free_shash(sbi->s_chksum_driver);
-- 
2.31.1

Re: [PATCH 11/13] ext4: factor out ext4_group_desc_init() and ext4_group_desc_free()

[Jan Kara]

On Tue 30-08-22 20:04:09, Jason Yan wrote:
> Factor out ext4_group_desc_init() and ext4_group_desc_free(). No
> functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 143 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 84 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4c6c4930e11b..40f155543df0 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4743,9 +4743,89 @@ static int ext4_geometry_check(struct super_block *sb,
>  	return 0;
>  }
>  
> +static void ext4_group_desc_free(struct ext4_sb_info *sbi)
> +{
> +	struct buffer_head **group_desc;
> +	int i;
> +
> +	rcu_read_lock();
> +	group_desc = rcu_dereference(sbi->s_group_desc);
> +	for (i = 0; i < sbi->s_gdb_count; i++)
> +		brelse(group_desc[i]);
> +	kvfree(group_desc);
> +	rcu_read_unlock();
> +}
> +
> +static int ext4_group_desc_init(struct super_block *sb,
> +				struct ext4_super_block *es,
> +				ext4_fsblk_t logical_sb_block,
> +				ext4_group_t *first_not_zeroed)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	unsigned int db_count;
> +	ext4_fsblk_t block;
> +	int ret;
> +	int i;
> +
> +	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
> +		   EXT4_DESC_PER_BLOCK(sb);
> +	if (ext4_has_feature_meta_bg(sb)) {
> +		if (le32_to_cpu(es->s_first_meta_bg) > db_count) {
> +			ext4_msg(sb, KERN_WARNING,
> +				 "first meta block group too large: %u "
> +				 "(group descriptor block count %u)",
> +				 le32_to_cpu(es->s_first_meta_bg), db_count);
> +			return -EINVAL;
> +		}
> +	}
> +	rcu_assign_pointer(sbi->s_group_desc,
> +			   kvmalloc_array(db_count,
> +					  sizeof(struct buffer_head *),
> +					  GFP_KERNEL));
> +	if (sbi->s_group_desc == NULL) {
> +		ext4_msg(sb, KERN_ERR, "not enough memory");
> +		return -ENOMEM;
> +	}
> +
> +	bgl_lock_init(sbi->s_blockgroup_lock);
> +
> +	/* Pre-read the descriptors into the buffer cache */
> +	for (i = 0; i < db_count; i++) {
> +		block = descriptor_loc(sb, logical_sb_block, i);
> +		ext4_sb_breadahead_unmovable(sb, block);
> +	}
> +
> +	for (i = 0; i < db_count; i++) {
> +		struct buffer_head *bh;
> +
> +		block = descriptor_loc(sb, logical_sb_block, i);
> +		bh = ext4_sb_bread_unmovable(sb, block);
> +		if (IS_ERR(bh)) {
> +			ext4_msg(sb, KERN_ERR,
> +			       "can't read group descriptor %d", i);
> +			sbi->s_gdb_count = i;
> +			ret = PTR_ERR(bh);
> +			goto out;
> +		}
> +		rcu_read_lock();
> +		rcu_dereference(sbi->s_group_desc)[i] = bh;
> +		rcu_read_unlock();
> +	}
> +	sbi->s_gdb_count = db_count;
> +	if (!ext4_check_descriptors(sb, logical_sb_block, first_not_zeroed)) {
> +		ext4_msg(sb, KERN_ERR, "group descriptors corrupted!");
> +		ret = -EFSCORRUPTED;
> +		goto out;
> +	}
> +	return 0;
> +out:
> +	ext4_group_desc_free(sbi);
> +	return ret;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
> -	struct buffer_head *bh, **group_desc;
> +	struct buffer_head *bh;
>  	struct ext4_super_block *es = NULL;
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct flex_groups **flex_groups;
> @@ -4755,7 +4835,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	struct inode *root;
>  	int ret = -ENOMEM;
>  	int blocksize;
> -	unsigned int db_count;
>  	unsigned int i;
>  	int needs_recovery, has_huge_files;
>  	int err = 0;
> @@ -5046,57 +5125,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	if (ext4_geometry_check(sb, es))
>  		goto failed_mount;
>  
> -	db_count = (sbi->s_groups_count + EXT4_DESC_PER_BLOCK(sb) - 1) /
> -		   EXT4_DESC_PER_BLOCK(sb);
> -	if (ext4_has_feature_meta_bg(sb)) {
> -		if (le32_to_cpu(es->s_first_meta_bg) > db_count) {
> -			ext4_msg(sb, KERN_WARNING,
> -				 "first meta block group too large: %u "
> -				 "(group descriptor block count %u)",
> -				 le32_to_cpu(es->s_first_meta_bg), db_count);
> -			goto failed_mount;
> -		}
> -	}
> -	rcu_assign_pointer(sbi->s_group_desc,
> -			   kvmalloc_array(db_count,
> -					  sizeof(struct buffer_head *),
> -					  GFP_KERNEL));
> -	if (sbi->s_group_desc == NULL) {
> -		ext4_msg(sb, KERN_ERR, "not enough memory");
> -		ret = -ENOMEM;
> +	err = ext4_group_desc_init(sb, es, logical_sb_block, &first_not_zeroed);
> +	if (err)
>  		goto failed_mount;
> -	}
> -
> -	bgl_lock_init(sbi->s_blockgroup_lock);
> -
> -	/* Pre-read the descriptors into the buffer cache */
> -	for (i = 0; i < db_count; i++) {
> -		block = descriptor_loc(sb, logical_sb_block, i);
> -		ext4_sb_breadahead_unmovable(sb, block);
> -	}
> -
> -	for (i = 0; i < db_count; i++) {
> -		struct buffer_head *bh;
> -
> -		block = descriptor_loc(sb, logical_sb_block, i);
> -		bh = ext4_sb_bread_unmovable(sb, block);
> -		if (IS_ERR(bh)) {
> -			ext4_msg(sb, KERN_ERR,
> -			       "can't read group descriptor %d", i);
> -			db_count = i;
> -			ret = PTR_ERR(bh);
> -			goto failed_mount2;
> -		}
> -		rcu_read_lock();
> -		rcu_dereference(sbi->s_group_desc)[i] = bh;
> -		rcu_read_unlock();
> -	}
> -	sbi->s_gdb_count = db_count;
> -	if (!ext4_check_descriptors(sb, logical_sb_block, &first_not_zeroed)) {
> -		ext4_msg(sb, KERN_ERR, "group descriptors corrupted!");
> -		ret = -EFSCORRUPTED;
> -		goto failed_mount2;
> -	}
>  
>  	timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
>  	spin_lock_init(&sbi->s_error_lock);
> @@ -5540,13 +5571,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	flush_work(&sbi->s_error_work);
>  	del_timer_sync(&sbi->s_err_report);
>  	ext4_stop_mmpd(sbi);
> -failed_mount2:
> -	rcu_read_lock();
> -	group_desc = rcu_dereference(sbi->s_group_desc);
> -	for (i = 0; i < db_count; i++)
> -		brelse(group_desc[i]);
> -	kvfree(group_desc);
> -	rcu_read_unlock();
> +	ext4_group_desc_free(sbi);
>  failed_mount:
>  	if (sbi->s_chksum_driver)
>  		crypto_free_shash(sbi->s_chksum_driver);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 12/13] ext4: factor out ext4_load_and_init_journal()

[Jason Yan]

This patch group the journal load and initialize code together and
factor out ext4_load_and_init_journal(). This patch also removes the
lable 'no_journal' which is not needed after refactor.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 157 +++++++++++++++++++++++++++---------------------
 1 file changed, 88 insertions(+), 69 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 40f155543df0..95e70f0316db 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4823,6 +4823,93 @@ static int ext4_group_desc_init(struct super_block *sb,
 	return ret;
 }
 
+static int ext4_load_and_init_journal(struct super_block *sb,
+				      struct ext4_super_block *es,
+				      struct ext4_fs_context *ctx)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int err;
+
+	err = ext4_load_journal(sb, es, ctx->journal_devnum);
+	if (err)
+		return err;
+
+	if (ext4_has_feature_64bit(sb) &&
+	    !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
+				       JBD2_FEATURE_INCOMPAT_64BIT)) {
+		ext4_msg(sb, KERN_ERR, "Failed to set 64-bit journal feature");
+		goto out;
+	}
+
+	if (!set_journal_csum_feature_set(sb)) {
+		ext4_msg(sb, KERN_ERR, "Failed to set journal checksum "
+			 "feature set");
+		goto out;
+	}
+
+	if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
+		!jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
+					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT)) {
+		ext4_msg(sb, KERN_ERR,
+			"Failed to set fast commit journal feature");
+		goto out;
+	}
+
+	/* We have now updated the journal if required, so we can
+	 * validate the data journaling mode. */
+	switch (test_opt(sb, DATA_FLAGS)) {
+	case 0:
+		/* No mode set, assume a default based on the journal
+		 * capabilities: ORDERED_DATA if the journal can
+		 * cope, else JOURNAL_DATA
+		 */
+		if (jbd2_journal_check_available_features
+		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
+			set_opt(sb, ORDERED_DATA);
+			sbi->s_def_mount_opt |= EXT4_MOUNT_ORDERED_DATA;
+		} else {
+			set_opt(sb, JOURNAL_DATA);
+			sbi->s_def_mount_opt |= EXT4_MOUNT_JOURNAL_DATA;
+		}
+		break;
+
+	case EXT4_MOUNT_ORDERED_DATA:
+	case EXT4_MOUNT_WRITEBACK_DATA:
+		if (!jbd2_journal_check_available_features
+		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
+			ext4_msg(sb, KERN_ERR, "Journal does not support "
+			       "requested data journaling mode");
+			goto out;
+		}
+		break;
+	default:
+		break;
+	}
+
+	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA &&
+	    test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
+		ext4_msg(sb, KERN_ERR, "can't mount with "
+			"journal_async_commit in data=ordered mode");
+		goto out;
+	}
+
+	set_task_ioprio(sbi->s_journal->j_task, ctx->journal_ioprio);
+
+	sbi->s_journal->j_submit_inode_data_buffers =
+		ext4_journal_submit_inode_data_buffers;
+	sbi->s_journal->j_finish_inode_data_buffers =
+		ext4_journal_finish_inode_data_buffers;
+
+	return 0;
+
+out:
+	/* flush s_error_work before journal destroy. */
+	flush_work(&sbi->s_error_work);
+	jbd2_journal_destroy(sbi->s_journal);
+	sbi->s_journal = NULL;
+	return err;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh;
@@ -5182,7 +5269,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	 * root first: it may be modified in the journal!
 	 */
 	if (!test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb)) {
-		err = ext4_load_journal(sb, es, ctx->journal_devnum);
+		err = ext4_load_and_init_journal(sb, es, ctx);
 		if (err)
 			goto failed_mount3a;
 	} else if (test_opt(sb, NOLOAD) && !sb_rdonly(sb) &&
@@ -5220,76 +5307,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		clear_opt2(sb, JOURNAL_FAST_COMMIT);
 		sbi->s_journal = NULL;
 		needs_recovery = 0;
-		goto no_journal;
 	}
 
-	if (ext4_has_feature_64bit(sb) &&
-	    !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
-				       JBD2_FEATURE_INCOMPAT_64BIT)) {
-		ext4_msg(sb, KERN_ERR, "Failed to set 64-bit journal feature");
-		goto failed_mount_wq;
-	}
-
-	if (!set_journal_csum_feature_set(sb)) {
-		ext4_msg(sb, KERN_ERR, "Failed to set journal checksum "
-			 "feature set");
-		goto failed_mount_wq;
-	}
-
-	if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
-		!jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
-					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT)) {
-		ext4_msg(sb, KERN_ERR,
-			"Failed to set fast commit journal feature");
-		goto failed_mount_wq;
-	}
-
-	/* We have now updated the journal if required, so we can
-	 * validate the data journaling mode. */
-	switch (test_opt(sb, DATA_FLAGS)) {
-	case 0:
-		/* No mode set, assume a default based on the journal
-		 * capabilities: ORDERED_DATA if the journal can
-		 * cope, else JOURNAL_DATA
-		 */
-		if (jbd2_journal_check_available_features
-		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
-			set_opt(sb, ORDERED_DATA);
-			sbi->s_def_mount_opt |= EXT4_MOUNT_ORDERED_DATA;
-		} else {
-			set_opt(sb, JOURNAL_DATA);
-			sbi->s_def_mount_opt |= EXT4_MOUNT_JOURNAL_DATA;
-		}
-		break;
-
-	case EXT4_MOUNT_ORDERED_DATA:
-	case EXT4_MOUNT_WRITEBACK_DATA:
-		if (!jbd2_journal_check_available_features
-		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
-			ext4_msg(sb, KERN_ERR, "Journal does not support "
-			       "requested data journaling mode");
-			goto failed_mount_wq;
-		}
-		break;
-	default:
-		break;
-	}
-
-	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA &&
-	    test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
-		ext4_msg(sb, KERN_ERR, "can't mount with "
-			"journal_async_commit in data=ordered mode");
-		goto failed_mount_wq;
-	}
-
-	set_task_ioprio(sbi->s_journal->j_task, ctx->journal_ioprio);
-
-	sbi->s_journal->j_submit_inode_data_buffers =
-		ext4_journal_submit_inode_data_buffers;
-	sbi->s_journal->j_finish_inode_data_buffers =
-		ext4_journal_finish_inode_data_buffers;
-
-no_journal:
 	if (!test_opt(sb, NO_MBCACHE)) {
 		sbi->s_ea_block_cache = ext4_xattr_create_cache();
 		if (!sbi->s_ea_block_cache) {
-- 
2.31.1

Re: [PATCH 12/13] ext4: factor out ext4_load_and_init_journal()

[Jan Kara]

On Tue 30-08-22 20:04:10, Jason Yan wrote:
> This patch group the journal load and initialize code together and
> factor out ext4_load_and_init_journal(). This patch also removes the
> lable 'no_journal' which is not needed after refactor.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/super.c | 157 +++++++++++++++++++++++++++---------------------
>  1 file changed, 88 insertions(+), 69 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 40f155543df0..95e70f0316db 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4823,6 +4823,93 @@ static int ext4_group_desc_init(struct super_block *sb,
>  	return ret;
>  }
>  
> +static int ext4_load_and_init_journal(struct super_block *sb,
> +				      struct ext4_super_block *es,
> +				      struct ext4_fs_context *ctx)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	int err;
> +
> +	err = ext4_load_journal(sb, es, ctx->journal_devnum);
> +	if (err)
> +		return err;
> +
> +	if (ext4_has_feature_64bit(sb) &&
> +	    !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
> +				       JBD2_FEATURE_INCOMPAT_64BIT)) {
> +		ext4_msg(sb, KERN_ERR, "Failed to set 64-bit journal feature");
> +		goto out;
> +	}
> +
> +	if (!set_journal_csum_feature_set(sb)) {
> +		ext4_msg(sb, KERN_ERR, "Failed to set journal checksum "
> +			 "feature set");
> +		goto out;
> +	}
> +
> +	if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
> +		!jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
> +					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT)) {
> +		ext4_msg(sb, KERN_ERR,
> +			"Failed to set fast commit journal feature");
> +		goto out;
> +	}
> +
> +	/* We have now updated the journal if required, so we can
> +	 * validate the data journaling mode. */
> +	switch (test_opt(sb, DATA_FLAGS)) {
> +	case 0:
> +		/* No mode set, assume a default based on the journal
> +		 * capabilities: ORDERED_DATA if the journal can
> +		 * cope, else JOURNAL_DATA
> +		 */
> +		if (jbd2_journal_check_available_features
> +		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
> +			set_opt(sb, ORDERED_DATA);
> +			sbi->s_def_mount_opt |= EXT4_MOUNT_ORDERED_DATA;
> +		} else {
> +			set_opt(sb, JOURNAL_DATA);
> +			sbi->s_def_mount_opt |= EXT4_MOUNT_JOURNAL_DATA;
> +		}
> +		break;
> +
> +	case EXT4_MOUNT_ORDERED_DATA:
> +	case EXT4_MOUNT_WRITEBACK_DATA:
> +		if (!jbd2_journal_check_available_features
> +		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
> +			ext4_msg(sb, KERN_ERR, "Journal does not support "
> +			       "requested data journaling mode");
> +			goto out;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA &&
> +	    test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
> +		ext4_msg(sb, KERN_ERR, "can't mount with "
> +			"journal_async_commit in data=ordered mode");
> +		goto out;
> +	}
> +
> +	set_task_ioprio(sbi->s_journal->j_task, ctx->journal_ioprio);
> +
> +	sbi->s_journal->j_submit_inode_data_buffers =
> +		ext4_journal_submit_inode_data_buffers;
> +	sbi->s_journal->j_finish_inode_data_buffers =
> +		ext4_journal_finish_inode_data_buffers;
> +
> +	return 0;
> +
> +out:
> +	/* flush s_error_work before journal destroy. */
> +	flush_work(&sbi->s_error_work);
> +	jbd2_journal_destroy(sbi->s_journal);
> +	sbi->s_journal = NULL;
> +	return err;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh;
> @@ -5182,7 +5269,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	 * root first: it may be modified in the journal!
>  	 */
>  	if (!test_opt(sb, NOLOAD) && ext4_has_feature_journal(sb)) {
> -		err = ext4_load_journal(sb, es, ctx->journal_devnum);
> +		err = ext4_load_and_init_journal(sb, es, ctx);
>  		if (err)
>  			goto failed_mount3a;
>  	} else if (test_opt(sb, NOLOAD) && !sb_rdonly(sb) &&
> @@ -5220,76 +5307,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		clear_opt2(sb, JOURNAL_FAST_COMMIT);
>  		sbi->s_journal = NULL;
>  		needs_recovery = 0;
> -		goto no_journal;
>  	}
>  
> -	if (ext4_has_feature_64bit(sb) &&
> -	    !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
> -				       JBD2_FEATURE_INCOMPAT_64BIT)) {
> -		ext4_msg(sb, KERN_ERR, "Failed to set 64-bit journal feature");
> -		goto failed_mount_wq;
> -	}
> -
> -	if (!set_journal_csum_feature_set(sb)) {
> -		ext4_msg(sb, KERN_ERR, "Failed to set journal checksum "
> -			 "feature set");
> -		goto failed_mount_wq;
> -	}
> -
> -	if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
> -		!jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
> -					  JBD2_FEATURE_INCOMPAT_FAST_COMMIT)) {
> -		ext4_msg(sb, KERN_ERR,
> -			"Failed to set fast commit journal feature");
> -		goto failed_mount_wq;
> -	}
> -
> -	/* We have now updated the journal if required, so we can
> -	 * validate the data journaling mode. */
> -	switch (test_opt(sb, DATA_FLAGS)) {
> -	case 0:
> -		/* No mode set, assume a default based on the journal
> -		 * capabilities: ORDERED_DATA if the journal can
> -		 * cope, else JOURNAL_DATA
> -		 */
> -		if (jbd2_journal_check_available_features
> -		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
> -			set_opt(sb, ORDERED_DATA);
> -			sbi->s_def_mount_opt |= EXT4_MOUNT_ORDERED_DATA;
> -		} else {
> -			set_opt(sb, JOURNAL_DATA);
> -			sbi->s_def_mount_opt |= EXT4_MOUNT_JOURNAL_DATA;
> -		}
> -		break;
> -
> -	case EXT4_MOUNT_ORDERED_DATA:
> -	case EXT4_MOUNT_WRITEBACK_DATA:
> -		if (!jbd2_journal_check_available_features
> -		    (sbi->s_journal, 0, 0, JBD2_FEATURE_INCOMPAT_REVOKE)) {
> -			ext4_msg(sb, KERN_ERR, "Journal does not support "
> -			       "requested data journaling mode");
> -			goto failed_mount_wq;
> -		}
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_ORDERED_DATA &&
> -	    test_opt(sb, JOURNAL_ASYNC_COMMIT)) {
> -		ext4_msg(sb, KERN_ERR, "can't mount with "
> -			"journal_async_commit in data=ordered mode");
> -		goto failed_mount_wq;
> -	}
> -
> -	set_task_ioprio(sbi->s_journal->j_task, ctx->journal_ioprio);
> -
> -	sbi->s_journal->j_submit_inode_data_buffers =
> -		ext4_journal_submit_inode_data_buffers;
> -	sbi->s_journal->j_finish_inode_data_buffers =
> -		ext4_journal_finish_inode_data_buffers;
> -
> -no_journal:
>  	if (!test_opt(sb, NO_MBCACHE)) {
>  		sbi->s_ea_block_cache = ext4_xattr_create_cache();
>  		if (!sbi->s_ea_block_cache) {
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 13/13] ext4: factor out ext4_journal_data_check()

[Jason Yan]

Factor out ext4_journal_data_check(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 fs/ext4/super.c | 60 ++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 95e70f0316db..c070d4f5ecc4 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4910,6 +4910,39 @@ static int ext4_load_and_init_journal(struct super_block *sb,
 	return err;
 }
 
+static int ext4_journal_data_check(struct super_block *sb)
+{
+	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
+		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with "
+			    "data=journal disables delayed allocation, "
+			    "dioread_nolock, O_DIRECT and fast_commit support!\n");
+		/* can't mount with both data=journal and dioread_nolock. */
+		clear_opt(sb, DIOREAD_NOLOCK);
+		clear_opt2(sb, JOURNAL_FAST_COMMIT);
+		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and delalloc");
+			return -EINVAL;
+		}
+		if (test_opt(sb, DAX_ALWAYS)) {
+			ext4_msg(sb, KERN_ERR, "can't mount with "
+				 "both data=journal and dax");
+			return -EINVAL;
+		}
+		if (ext4_has_feature_encrypt(sb)) {
+			ext4_msg(sb, KERN_WARNING,
+				 "encrypted files will use data=ordered "
+				 "instead of data journaling mode");
+		}
+		if (test_opt(sb, DELALLOC))
+			clear_opt(sb, DELALLOC);
+	} else {
+		sb->s_iflags |= SB_I_CGROUPWB;
+	}
+
+	return 0;
+}
+
 static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 {
 	struct buffer_head *bh;
@@ -5033,31 +5066,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 	if (ext4_encoding_init(sb, es))
 		goto failed_mount;
 
-	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
-		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with data=journal disables delayed allocation, dioread_nolock, O_DIRECT and fast_commit support!\n");
-		/* can't mount with both data=journal and dioread_nolock. */
-		clear_opt(sb, DIOREAD_NOLOCK);
-		clear_opt2(sb, JOURNAL_FAST_COMMIT);
-		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "both data=journal and delalloc");
-			goto failed_mount;
-		}
-		if (test_opt(sb, DAX_ALWAYS)) {
-			ext4_msg(sb, KERN_ERR, "can't mount with "
-				 "both data=journal and dax");
-			goto failed_mount;
-		}
-		if (ext4_has_feature_encrypt(sb)) {
-			ext4_msg(sb, KERN_WARNING,
-				 "encrypted files will use data=ordered "
-				 "instead of data journaling mode");
-		}
-		if (test_opt(sb, DELALLOC))
-			clear_opt(sb, DELALLOC);
-	} else {
-		sb->s_iflags |= SB_I_CGROUPWB;
-	}
+	if (ext4_journal_data_check(sb))
+		goto failed_mount;
 
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
 		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
-- 
2.31.1

Re: [PATCH 13/13] ext4: factor out ext4_journal_data_check()

[Jan Kara]

On Tue 30-08-22 20:04:11, Jason Yan wrote:
> Factor out ext4_journal_data_check(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  fs/ext4/super.c | 60 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 95e70f0316db..c070d4f5ecc4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4910,6 +4910,39 @@ static int ext4_load_and_init_journal(struct super_block *sb,
>  	return err;
>  }
>  
> +static int ext4_journal_data_check(struct super_block *sb)

Perhaps name this ext4_journal_data_mode_check()? Otherwise feel free to
add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> +{
> +	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
> +		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with "
> +			    "data=journal disables delayed allocation, "
> +			    "dioread_nolock, O_DIRECT and fast_commit support!\n");
> +		/* can't mount with both data=journal and dioread_nolock. */
> +		clear_opt(sb, DIOREAD_NOLOCK);
> +		clear_opt2(sb, JOURNAL_FAST_COMMIT);
> +		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
> +			ext4_msg(sb, KERN_ERR, "can't mount with "
> +				 "both data=journal and delalloc");
> +			return -EINVAL;
> +		}
> +		if (test_opt(sb, DAX_ALWAYS)) {
> +			ext4_msg(sb, KERN_ERR, "can't mount with "
> +				 "both data=journal and dax");
> +			return -EINVAL;
> +		}
> +		if (ext4_has_feature_encrypt(sb)) {
> +			ext4_msg(sb, KERN_WARNING,
> +				 "encrypted files will use data=ordered "
> +				 "instead of data journaling mode");
> +		}
> +		if (test_opt(sb, DELALLOC))
> +			clear_opt(sb, DELALLOC);
> +	} else {
> +		sb->s_iflags |= SB_I_CGROUPWB;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  {
>  	struct buffer_head *bh;
> @@ -5033,31 +5066,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  	if (ext4_encoding_init(sb, es))
>  		goto failed_mount;
>  
> -	if (test_opt(sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) {
> -		printk_once(KERN_WARNING "EXT4-fs: Warning: mounting with data=journal disables delayed allocation, dioread_nolock, O_DIRECT and fast_commit support!\n");
> -		/* can't mount with both data=journal and dioread_nolock. */
> -		clear_opt(sb, DIOREAD_NOLOCK);
> -		clear_opt2(sb, JOURNAL_FAST_COMMIT);
> -		if (test_opt2(sb, EXPLICIT_DELALLOC)) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "both data=journal and delalloc");
> -			goto failed_mount;
> -		}
> -		if (test_opt(sb, DAX_ALWAYS)) {
> -			ext4_msg(sb, KERN_ERR, "can't mount with "
> -				 "both data=journal and dax");
> -			goto failed_mount;
> -		}
> -		if (ext4_has_feature_encrypt(sb)) {
> -			ext4_msg(sb, KERN_WARNING,
> -				 "encrypted files will use data=ordered "
> -				 "instead of data journaling mode");
> -		}
> -		if (test_opt(sb, DELALLOC))
> -			clear_opt(sb, DELALLOC);
> -	} else {
> -		sb->s_iflags |= SB_I_CGROUPWB;
> -	}
> +	if (ext4_journal_data_check(sb))
> +		goto failed_mount;
>  
>  	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>  		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 13/13] ext4: factor out ext4_journal_data_check()

[Jason Yan]

On 2022/8/31 20:06, Jan Kara wrote:
> On Tue 30-08-22 20:04:11, Jason Yan wrote:
>> Factor out ext4_journal_data_check(). No functional change.
>>
>> Signed-off-by: Jason Yan<yanaijie@huawei.com>
>> ---
>>   fs/ext4/super.c | 60 ++++++++++++++++++++++++++++---------------------
>>   1 file changed, 35 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 95e70f0316db..c070d4f5ecc4 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4910,6 +4910,39 @@ static int ext4_load_and_init_journal(struct super_block *sb,
>>   	return err;
>>   }
>>   
>> +static int ext4_journal_data_check(struct super_block *sb)
> Perhaps name this ext4_journal_data_mode_check()? 

OK.

Thanks.

Otherwise feel free to
> add:
> 
> Reviewed-by: Jan Kara<jack@suse.cz>