Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Sep 2022 14:30:25 +0530
From:   "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
To:     Jason Yan <yanaijie@huawei.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        lczerner@redhat.com, linux-ext4@vger.kernel.org
Subject: Re: [PATCH v2 10/13] ext4: factor out ext4_geometry_check()
Message-ID: <20220908090025.7aw2nsfnp5mzgewn@riteshh-domain>
References: <20220903030156.770313-1-yanaijie@huawei.com>
 <20220903030156.770313-11-yanaijie@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220903030156.770313-11-yanaijie@huawei.com>
Precedence: bulk

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
>