2022-09-08 09:15:00

by Ritesh Harjani

[permalink] [raw]
Subject: Re: [PATCH v2 10/13] ext4: factor out ext4_geometry_check()

On 22/09/03 11:01AM, Jason Yan wrote:
> Factor out ext4_geometry_check(). No functional change.
>
> Signed-off-by: Jason Yan <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
> ---
> 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) <[email protected]>


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