flex_bg information initialization and question on resize/bad inodes with 48 bits filesystem

Hello,

I have looked at the new features provided by ext4 and have a question on flex_bg information initialization:
into ext4_fill_flex_info function of fs/ext4/super.c (lines 1698, 1700 and 1702 for kernel 2.6.31) doesn't the atomic_set calls be atomic_add to sum statistics of each group composing a flex group, or do I misunderstand something ?

For the extension to manage 48 bits blocks number, I do not see anything to treat this for resize and bad inodes into kernel or e2fsprogs. For the resize inode, it is perhaps an incompatibility of this feature with 48 bits blocks number, but for the bad inode ?

For information, I did not subscribe to the list, so could you please CC-me in your answer ?

Thanks,

Damien

Good eye; that's a bug; thanks for pointing that out.

There is a plan for how to handle online resizing for > 2^32 block filesystems, but it hasn't been implemented yet. The basic support for it is there; that's what the META_BG feature is designed to support, so existing kernels will be able to deal with resized large filesystemes. But the code to actually do the on-line resizing hasn't been implemented yet.

For the bad block inode, the solution is to make it be extent mapped inode. This also hasn't been implemented yet, but this is a much simpler one to write. The main reason why we haven't is that modern disks rarely have system-visible bad blocks; normally the hard drive has its own bad block remapping layer in hardware so we never see a bad block until the disk is failing so badly it needs to be replaced ASAP.

- Ted

The s_flex_groups array should have been initialized using atomic_add to sum up the free counts from the block groups that make up a flex_bg. By using atomic_set, the value of the s_flex_groups array was set to the values of the last block group in the flex_bg.

The impact of this bug is that the block and inode allocation algorithms might not pick the best flex_bg for new allocation.

Thanks to Damien Guibouret for pointing out this problem!

---
 fs/ext4/super.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

index 9f6fa3f..04c6933 100644
@@ -1694,12 +1694,12 @@ static int ext4_fill_flex_info(struct super_block *sb)
 		gdp = ext4_get_group_desc(sb, i, NULL);
 
 		flex_group = ext4_flex_group(sbi, i);
-		atomic_set(&sbi->s_flex_groups[flex_group].free_inodes,
-			   ext4_free_inodes_count(sb, gdp));
-		atomic_set(&sbi->s_flex_groups[flex_group].free_blocks,
-			   ext4_free_blks_count(sb, gdp));
-		atomic_set(&sbi->s_flex_groups[flex_group].used_dirs,
-			   ext4_used_dirs_count(sb, gdp));
+		atomic_add(ext4_free_inodes_count(sb, gdp),
+			   &sbi->s_flex_groups[flex_group].free_inodes);
+		atomic_add(ext4_free_blks_count(sb, gdp),
+			   &sbi->s_flex_groups[flex_group].free_blocks);
+		atomic_add(ext4_used_dirs_count(sb, gdp),
+			   &sbi->s_flex_groups[flex_group].used_dirs);
 	}
 
 	return 1;

1.6.3.2.1.gb9f7d.dirty

Hi,

Thanks for the information.

Looking at ext4.h, I think the setting of extra time fields forgets to mask the epoch bits so the epoch part overwrites nsec part. The second change is only for coherency (2 -> EXT4_EPOCH_BITS):

@@ -481,8 +481,8 @@
 static inline __le32 ext4_encode_extra_time(struct timespec *time)
 {
        return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
-                          time->tv_sec >> 32 : 0) |
-                          ((time->tv_nsec << 2) & EXT4_NSEC_MASK));
+                          (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
+                          ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
 }
 
 static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra)

@@ -490,7 +490,7 @@
        if (sizeof(time->tv_sec) > 4)
               time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK)
                               << 32;
-       time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2;
+       time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
 }
 
 #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode)                         \

Regards,

Damien