Message-Id: <C336EA03-5DB9-428B-A0A1-B124DBCAE6EF@gmail.com>
Date:   Tue, 6 Mar 2018 18:20:07 +0300
From:   Благодаренко Артём 
        <artem.blagodarenko@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2 5/7] ext2fs: add EXT4_FEATURE_INCOMPAT_64INODE support


> On 21 Nov 2017, at 02:09, Andreas Dilger <adilger@...ger.ca> wrote:
> 
> Note "Subject:" line needs to list "INODE64" instead of "64INODE"
> 
> On Nov 14, 2017, at 12:04 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
>> 
>> Inodes count and free inodes count should be 64 bit long.
>> This patch also changes s_inodes_count* to 64 bit
>> 
>> Lustre-bug: https://jira.hpdd.intel.com/browse/LU-9309
>> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@...il.com>
>> ---
> 
> At a high level this patch is good.
> 
> I think it would be a lot easier to review this patch if there was
> one patch that just added the ext2fs_{get,set}_inodes_count() and
> ext2fs_{get,set}_free_inodes_count() helper routines (that only
> use the low word) and "%lu" format, and then the second patch added
> the INODE64 functionality.
> 
> This would allow about 90% of this patch to be trivially reviewed,
> and even landed, and then the second patch contains the important
> 64-bit inode related changes that needs more thorough review.
> 
>> diff --git a/debugfs/util.c b/debugfs/util.c
>> index 452de749..3e4fcb5a 100644
>> --- a/debugfs/util.c
>> +++ b/debugfs/util.c
>> @@ -119,7 +119,8 @@ ext2_ino_t string_to_inode(char *str)
>> 	 */
>> 	if ((len > 2) && (str[0] == '<') && (str[len-1] == '>')) {
>> 		ino = strtoul(str+1, &end, 0);
>> -		if (*end=='>' && (ino <= current_fs->super->s_inodes_count))
>> +		if (*end == '>' &&
>> +			(ino <= ext2fs_get_inodes_count(current_fs->super)))
> 
> (style) align after '(' on previous line.
> (style) No need for () around <= comparison.
> 
>> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
>> index ef3146d8..d6bfcfb1 100644
>> --- a/e2fsck/extents.c
>> +++ b/e2fsck/extents.c
>> @@ -396,9 +396,9 @@ static void rebuild_extents(e2fsck_t ctx, const char *pass_name, int pr_header)
>> 		}
>> 		if (ctx->progress && !ctx->progress_fd)
>> 			e2fsck_simple_progress(ctx, "Rebuilding extents",
>> -					100.0 * (float) ino /
>> -					(float) ctx->fs->super->s_inodes_count,
>> -					ino);
>> +				100.0 * (float) ino /
>> +				(float) ext2fs_get_inodes_count(ctx->fs->super),
> 
> (style) no space after typecast.
> 
>> diff --git a/lib/ext2fs/alloc_stats.c b/lib/ext2fs/alloc_stats.c
>> index 3949f618..9a145b43 100644
>> --- a/lib/ext2fs/alloc_stats.c
>> +++ b/lib/ext2fs/alloc_stats.c
>> @@ -48,7 +48,9 @@ void ext2fs_inode_alloc_stats2(ext2_filsys fs, ext2_ino_t ino,
>> 		ext2fs_group_desc_csum_set(fs, group);
>> 	}
>> 
>> -	fs->super->s_free_inodes_count -= inuse;
>> +	ext2fs_set_free_inodes_count(fs->super,
>> +				     ext2fs_get_free_inodes_count(fs->super) -
>> +								  inuse);
> 
> This looks like "inuse" is an argument to ext2_get_free_inodes_count().
> It should be indented one tab from ext2fs_get_free_inodes_count() so it is
> clear this is a continued line.
> 
>> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
>> index 90294ed0..cb0f59d4 100644
>> --- a/lib/ext2fs/ext2_fs.h
>> +++ b/lib/ext2fs/ext2_fs.h
>> @@ -737,7 +737,10 @@ struct ext2_super_block {
>> 	__le32	s_lpf_ino;		/* Location of the lost+found inode */
>> 	__le32  s_prj_quota_inum;	/* inode for tracking project quota */
>> 	__le32	s_checksum_seed;	/* crc32c(orig_uuid) if csum_seed set */
>> -	__le32	s_reserved[98];		/* Padding to the end of the block */
>> +	__le32	s_inodes_count_hi;	/* higth part of inode count */
>> +	__le32	s_free_inodes_count_hi;	/* Free inodes count */
>> +	__le32	s_prj_quota_inum_hi;
>> +	__le32	s_reserved[93];		/* Padding to the end of the block */
> 
> 98 - 3 = 95?
> 
>> 	__u32	s_checksum;		/* crc32c(superblock) */
>> };
> 
> This misalignment of the s_checksum field should cause test failures in the
> checksum patches, and hopefully also some other checks to fail.  We should
> definitely have a "sizeof(ext2_super_block) == 1024" check somewhere.
> Did you run "make check" on these patches?
> 
>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
>> index b653012f..785042df 100644
>> --- a/lib/ext2fs/ext2fs.h
>> +++ b/lib/ext2fs/ext2fs.h
>> +static inline void ext2fs_set_inodes_count(struct ext2_super_block *sb,
>> +					   ext2_ino64_t val)
>> +{
>> +	if (ext2fs_has_feature_inode64(sb))
>> +		sb->s_inodes_count_hi =  val >> 32;
> 
> (style) two spaces before "val" here
> 
>> +static inline void ext2fs_set_free_inodes_count(struct ext2_super_block *sb,
>> +						ext2_ino64_t val)
>> +{
>> +	if (ext2fs_has_feature_inode64(sb))
>> +		sb->s_free_inodes_count_hi =  (__u32)(val >> 32);
> 
> (style) two spaces before "(__u32)" here
> 
>> diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
>> index 32f43210..2554b2dd 100644
>> --- a/lib/ext2fs/initialize.c
>> +++ b/lib/ext2fs/initialize.c
>> @@ -285,16 +285,18 @@ retry:
>> 
>> 	if (ext2fs_has_feature_64bit(super) &&
>> 	    (ext2fs_blocks_count(super) / i) > (1ULL << 32))
>> -		set_field(s_inodes_count, ~0U);
>> +		ext2fs_set_inodes_count(super, ~0U);
>> 	else
>> -		set_field(s_inodes_count, ext2fs_blocks_count(super) / i);
>> +		ext2fs_set_inodes_count(super, ext2fs_get_inodes_count(param) ?
>> +					ext2fs_get_inodes_count(param) :
>> +					ext2fs_blocks_count(super) / i);
>> 
>> 	/*
>> 	 * Make sure we have at least EXT2_FIRST_INO + 1 inodes, so
>> 	 * that we have enough inodes for the filesystem(!)
>> 	 */
>> -	if (super->s_inodes_count < EXT2_FIRST_INODE(super)+1)
>> -		super->s_inodes_count = EXT2_FIRST_INODE(super)+1;
>> +	if (ext2fs_get_inodes_count(super) < EXT2_FIRST_INODE(super)+1)
>> +		ext2fs_set_inodes_count(super, EXT2_FIRST_INODE(super)+1);
> 
> (style) space around those '+'
> 
>> @@ -355,9 +358,9 @@ ipg_retry:
>> 		ipg--;
>> 		goto ipg_retry;
>> 	}
>> -	super->s_inodes_count = super->s_inodes_per_group *
>> -		fs->group_desc_count;
>> -	super->s_free_inodes_count = super->s_inodes_count;
>> +	ext2fs_set_inodes_count(super, (ext2_ino64_t)super->s_inodes_per_group *
>> +		fs->group_desc_count);
> 
> (style) align continued line after '(' on previous line
> 
>> diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
>> index b9d8f557..b7c01005 100644
>> --- a/lib/ext2fs/swapfs.c
>> +++ b/lib/ext2fs/swapfs.c
>> @@ -26,10 +26,12 @@ void ext2fs_swap_super(struct ext2_super_block * sb)
>> {
>>  	int i;
>> 	sb->s_inodes_count = ext2fs_swab32(sb->s_inodes_count);
>> +	sb->s_inodes_count_hi = ext2fs_swab32(sb->s_inodes_count_hi);
>> 	sb->s_blocks_count = ext2fs_swab32(sb->s_blocks_count);
>> 	sb->s_r_blocks_count = ext2fs_swab32(sb->s_r_blocks_count);
>> 	sb->s_free_blocks_count = ext2fs_swab32(sb->s_free_blocks_count);
>> 	sb->s_free_inodes_count = ext2fs_swab32(sb->s_free_inodes_count);
>> +	sb->s_free_inodes_count_hi = ext2fs_swab32(sb->s_free_inodes_count_hi);
>> 	sb->s_first_data_block = ext2fs_swab32(sb->s_first_data_block);
>> 	sb->s_log_block_size = ext2fs_swab32(sb->s_log_block_size);
>> 	sb->s_log_cluster_size = ext2fs_swab32(sb->s_log_cluster_size);
> 
> (style) this should be consistent with the rest of the function, and swab
> these new fields in ext2_super_block offset order (i.e. at the end)
> 
>> diff --git a/lib/ext2fs/tst_super_size.c b/lib/ext2fs/tst_super_size.c
>> index 0adac411..e3dc608a 100644
>> --- a/lib/ext2fs/tst_super_size.c
>> +++ b/lib/ext2fs/tst_super_size.c
>> @@ -142,7 +142,10 @@ int main(int argc, char **argv)
>> 	check_field(s_lpf_ino, 4);
>> 	check_field(s_prj_quota_inum, 4);
>> 	check_field(s_checksum_seed, 4);
>> -	check_field(s_reserved, 98 * 4);
>> +	check_field(s_inodes_count_hi, 4);
>> +	check_field(s_free_inodes_count_hi, 4);
>> +	check_field(s_prj_quota_inum_hi, 4);
>> +	check_field(s_reserved, 93 * 4);
> 
>> 	check_field(s_checksum, 4);
>> 	do_field("Superblock end", 0, 0, cur_offset, 1024);
> 
> This test should have failed, since 98 - 3 = 95, and the superblock
> size was no longer 1024 bytes.  You should run "make check" after
> every patch in your series to catch problems like this.
> 
>> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
>> index 1edc0cd1..64102b79 100644
>> --- a/misc/mke2fs.c
>> +++ b/misc/mke2fs.c
>> @@ -2476,10 +2479,14 @@ profile_error:
>> 	/*
>> 	 * Calculate number of inodes based on the inode ratio
>> 	 */
>> -	fs_param.s_inodes_count = num_inodes ? num_inodes :
>> -		(ext2fs_blocks_count(&fs_param) * blocksize) / inode_ratio;
>> +	if (num_inodes == 0)
>> +		num_inodes = (ext2fs_blocks_count(&fs_param) * blocksize) /
>> +			inode_ratio;
>> 
>> -	if ((((unsigned long long)fs_param.s_inodes_count) *
>> +	ext2fs_set_inodes_count(&fs_param, num_inodes);
>> +
>> +
> 
> (style) two empty lines here
> 
> Should this set FEATURE_INCOMPAT_INODE64 if num_inodes >= 2^32, or is
> that handled internally if more than 2^32 inodes are created?  Same
> for setting the DIRDATA feature.

I expect user user need to enable inode64 if want to format system with >2^32 nodes.
I added message that suggest to enable "dirdata, inode64” options.

> 
> 
> I know for block count that we rounded the block count down to 2^32-1
> if it was only a little bit higher (e.g. less than 1M inodes over), so
> that we didn't introduce an incompatible feature if it wasn't needed.

Currently, if 64bit option is enabled node count for given > 2^32 nodes, 
Set to the most large inodes count without inode64 option - MAX_32_NUM.
> 
>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
>> index 44dd41a5..3538ab9c 100644
>> --- a/misc/tune2fs.c
>> +++ b/misc/tune2fs.c
>> @@ -2614,21 +2615,22 @@ static errcode_t ext2fs_calculate_summary_stats(ext2_filsys fs)
>> 	group = 0;
>> 
>> 	/* Protect loop from wrap-around if s_inodes_count maxed */
>> -	for (ino = 1; ino <= fs->super->s_inodes_count && ino > 0; ino++) {
>> +	for (ino = 1;
>> +	     ino <= ext2fs_get_inodes_count(fs->super) && ino > 0; ino++) {
>> 		if (!ext2fs_fast_test_inode_bitmap2(fs->inode_map, ino)) {
>> 			group_free++;
>> 			total_free++;
>> 		}
>> 		count++;
>> 		if ((count == fs->super->s_inodes_per_group) ||
>> -		    (ino == fs->super->s_inodes_count)) {
>> +		    (ino == ext2fs_get_inodes_count(fs->super))) {
>> 			ext2fs_bg_free_inodes_count_set(fs, group++,
>> 							group_free);
>> 			count = 0;
>> 			group_free = 0;
>> 		}
>> 	}
>> -	fs->super->s_free_inodes_count = total_free;
>> +	ext2fs_set_free_inodes_count(fs->super, total_free);
>> 	ext2fs_mark_super_dirty(fs);
>> 	return 0;
>> }
> 
> The tune2fs code should check and prevent the INODE64 and DIRDATA features
> from being cleared if the filesystem has more than 2^32 inodes.
> 
> 
> This patch adds support for tools to accept the INODE64 feature, but I don't
> see anywhere in this patch that adds the 64-bit inode numbers to the dirent
> using EXT2_DIRENT_INODE64.  That means if these patches landed without the
> dirent support then e2fsck would probably corrupt the filesystem.  That
> means that EXT2_FEATURE_INCOMPAT_INODE64 shouldn't be added to the list of
> supported features until the rest of the functionality is complete.
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

Powered by blists - more mailing lists