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

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

[Damien Guibouret]

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

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

[Theodore Tso]

On Fri, Sep 11, 2009 at 07:57:00PM +0200, Damien Guibouret wrote:
>
> 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 ?

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

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

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

[PATCH] ext4: Fix initalization of s_flex_groups

[Theodore Ts'o]

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!

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/super.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9f6fa3f..04c6933 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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

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

[Damien Guibouret]

Theodore Tso wrote:
> On Fri, Sep 11, 2009 at 07:57:00PM +0200, Damien Guibouret wrote:
> 
>>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 ?
> 
> 
> Good eye; that's a bug; thanks for pointing that out.
> 
> 
>>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 ?
> 
> 
> 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
> 
> 

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

--- fs/ext4/ext4.h.old  2009-09-12 09:45:42.161490080 +0200
+++ fs/ext4/ext4.h      2009-09-12 09:47:43.808996848 +0200
@@ -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