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

[PATCH v2 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.

v1->v2: 
  some code improvements suggested by Jan Kara and add review tags.

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_init_metadata_csum()
  ext4: factor out ext4_check_feature_compatibility()
  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_mode_check()

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

-- 
2.31.1

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 v2 01/13] ext4: goto right label 'failed_mount3a'

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

This looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

> 
> 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
> 

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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..6fced457ba3f 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 || sbi->s_blocks_per_group == 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 v2 02/13] ext4: remove cantfind_ext4 error handler

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)

Looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 6fced457ba3f..7cc499a221ff 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 v2 03/13] ext4: factor out ext4_set_def_opts()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_set_def_opts(). No functional change.

       if (blocksize == PAGE_SIZE)
               set_opt(sb, DIOREAD_NOLOCK);
The patch looks good however, even this ^^ could be moved in
ext4_set_def_opts() via some refactoring.

If required you could even submit a seperate change for above. 

Otherwise this change looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  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 6fced457ba3f..7cc499a221ff 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 v2 03/13] ext4: factor out ext4_set_def_opts()

[Jason Yan]

Hi Ritesh, thanks for the review.

On 2022/9/8 16:44, Ritesh Harjani (IBM) wrote:
> On 22/09/03 11:01AM, Jason Yan wrote:
>> Factor out ext4_set_def_opts(). No functional change.
>         if (blocksize == PAGE_SIZE)
>                 set_opt(sb, DIOREAD_NOLOCK);
> The patch looks good however, even this ^^ could be moved in
> ext4_set_def_opts() via some refactoring.
> 
> If required you could even submit a seperate change for above.
> 

Yes, this needs some refactoring. I would like to make a separate patch 
in my next series.

Jason

> Otherwise this change looks good to me.
> Reviewed-by: Ritesh Harjani (IBM)<ritesh.list@gmail.com>
> 
> 

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 7cc499a221ff..09b3c51d472b 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 v2 04/13] ext4: factor out ext4_handle_clustersize()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_handle_clustersize(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 110 +++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 49 deletions(-)

Changes looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 09b3c51d472b..3d58c2d889d5 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 v2 05/13] ext4: factor out ext4_fast_commit_init()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_fast_commit_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)

Nice cleanups. 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 09b3c51d472b..3d58c2d889d5 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
> 

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 3d58c2d889d5..f8806226b796 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 v2 06/13] ext4: factor out ext4_inode_info_init()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_inode_info_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 137 ++++++++++++++++++++++++++----------------------
>  1 file changed, 75 insertions(+), 62 deletions(-)

Changes looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 f8806226b796..67972b0218c0 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 v2 07/13] ext4: factor out ext4_encoding_init()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_encoding_init(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  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 f8806226b796..67972b0218c0 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)

How about simplying it like below.
		if (!IS_ENABLED(CONFIG_UNICODE))
			return 0;
	
		<...>	

Then we don't need #ifdef CONFIG_UNICODE

-ritesh

> +	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 v2 07/13] ext4: factor out ext4_encoding_init()

[Jason Yan]

On 2022/9/8 16:56, Ritesh Harjani (IBM) wrote:
> On 22/09/03 11:01AM, Jason Yan wrote:
>> Factor out ext4_encoding_init(). No functional change.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> ---
>>   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 f8806226b796..67972b0218c0 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)
> 
> How about simplying it like below.
> 		if (!IS_ENABLED(CONFIG_UNICODE))
> 			return 0;
> 	
> 		<...>	
> 
> Then we don't need #ifdef CONFIG_UNICODE
> 

Nice idea. Will update.

Thanks
Jason

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

[Jason Yan]

On 2022/9/12 10:30, Jason Yan wrote:
> 
> On 2022/9/8 16:56, Ritesh Harjani (IBM) wrote:
>> On 22/09/03 11:01AM, Jason Yan wrote:
>>> Factor out ext4_encoding_init(). No functional change.
>>>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>> ---
>>>   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 f8806226b796..67972b0218c0 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)
>>
>> How about simplying it like below.
>>         if (!IS_ENABLED(CONFIG_UNICODE))
>>             return 0;
>>
>>         <...>
>>
>> Then we don't need #ifdef CONFIG_UNICODE
>>
> 
> Nice idea. Will update.
> 

Sorry I tried to compile with this change but the compiler is not clever 
enough to ignore the code down if CONFIG_UNICODE is not enabled.


fs/ext4/super.c: In function ‘ext4_encoding_init’:
fs/ext4/super.c:4529:2: warning: ISO C90 forbids mixed declarations and 
code [-Wdeclaration-after-statement]
  4529 |  const struct ext4_sb_encodings *encoding_info;
       |  ^~~~~
fs/ext4/super.c:4533:42: error: ‘struct super_block’ has no member named 
‘s_encoding’
  4533 |  if (!ext4_has_feature_casefold(sb) || sb->s_encoding)
       |                                          ^~
fs/ext4/super.c:4536:18: error: implicit declaration of function 
‘ext4_sb_read_encoding’; did you mean ‘ext4_sb_bread_unmovable’? 
[-Werror=implicit-function-declaration]
  4536 |  encoding_info = ext4_sb_read_encoding(es);
       |                  ^~~~~~~~~~~~~~~~~~~~~
       |                  ext4_sb_bread_unmovable
fs/ext4/super.c:4536:16: warning: assignment to ‘const struct 
ext4_sb_encodings *’ from ‘int’ makes pointer from integer without a 
cast [-Wint-conversion]
  4536 |  encoding_info = ext4_sb_read_encoding(es);
       |                ^
fs/ext4/super.c:4543:36: error: dereferencing pointer to incomplete type 
‘const struct ext4_sb_encodings’
  4543 |  encoding = utf8_load(encoding_info->version);
       |                                    ^~
fs/ext4/super.c:4562:4: error: ‘struct super_block’ has no member named 
‘s_encoding’
  4562 |  sb->s_encoding = encoding;
       |    ^~
fs/ext4/super.c:4563:4: error: ‘struct super_block’ has no member named 
‘s_encoding_flags’
  4563 |  sb->s_encoding_flags = encoding_flags;
       |    ^~
cc1: some warnings being treated as errors
make[2]: *** [scripts/Makefile.build:249: fs/ext4/super.o] Error 1
make[1]: *** [scripts/Makefile.build:465: fs/ext4] Error 2
make: *** [Makefile:1852: fs] Error 2




> Thanks
> Jason
> .

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

[Jason Yan]

Factor out ext4_init_metadata_csum(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 67972b0218c0..09a1eef24cdc 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_init_metadata_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_init_metadata_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 v2 08/13] ext4: factor out ext4_init_metadata_csum()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_init_metadata_csum(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 83 +++++++++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 37 deletions(-)

Looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

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

[Jason Yan]

Factor out ext4_check_feature_compatibility(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 09a1eef24cdc..5f0e7c5188a3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4607,6 +4607,82 @@ static int ext4_init_metadata_csum(struct super_block *sb, struct ext4_super_blo
 	return 0;
 }
 
+static int ext4_check_feature_compatibility(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_check_feature_compatibility(sb, es, silent))
 		goto failed_mount;
 
 	if (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks) > (blocksize / 4)) {
-- 
2.31.1

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

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_check_feature_compatibility(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c | 144 ++++++++++++++++++++++++++----------------------
>  1 file changed, 77 insertions(+), 67 deletions(-)
> 

Looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 5f0e7c5188a3..69921a850644 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4683,6 +4683,66 @@ static int ext4_check_feature_compatibility(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 v2 10/13] ext4: factor out ext4_geometry_check()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_geometry_check(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  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 5f0e7c5188a3..69921a850644 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4683,6 +4683,66 @@ static int ext4_check_feature_compatibility(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;

This had initially confused me. But then I saw we by default always have
ret = -EINVAL.

This patch looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


> -		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
> 

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 69921a850644..468a958cf414 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 v2 11/13] ext4: factor out ext4_group_desc_init() and ext4_group_desc_free()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  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 69921a850644..468a958cf414 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();
> +}

I thought we could use ext4_group_desc_free() in ext4_put_super() too. 
But I guess in there within the same rcu_read_lock/unlock() we call for 
kvfree of flex_groups as well. 

But this change looks good to me. 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 468a958cf414..a464223b2913 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 v2 12/13] ext4: factor out ext4_load_and_init_journal()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, 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.
 
Clever cleanup.


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

Looks good.
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

[PATCH v2 13/13] ext4: factor out ext4_journal_data_mode_check()

[Jason Yan]

Factor out ext4_journal_data_mode_check(). No functional change.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jan Kara<jack@suse.cz>
---
 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 a464223b2913..cc9e834bc18c 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_mode_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_mode_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 v2 13/13] ext4: factor out ext4_journal_data_mode_check()

[Ritesh Harjani (IBM)]

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_journal_data_mode_check(). No functional change.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jan Kara<jack@suse.cz>
> ---
>  fs/ext4/super.c | 60 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 25 deletions(-)

Looks good to me. Thanks overall for the nice refactoring.
Function __ext4_fill_super() indeed looks a lot better now :)

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>