[PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[Justin Maggard]

Creating a 4TB file on a filesystem with the 64bit flag set results in e2fsck consistently complaining about i_blocks being wrong, with confusing messages like this:

Inode 29818882, i_blocks is 8388608816, should be 8388608816.  Fix? no

That appears to be caused by ext2fs_inode_i_blocks() checking for the EXT4_FEATURE_RO_COMPAT_HUGE_FILE in the wrong place.  Fix it.

Signed-off-by: Justin Maggard <jmaggard10@gmail.com>
---
 lib/ext2fs/blknum.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c
index a48b696..560edc7 100644
--- a/lib/ext2fs/blknum.c
+++ b/lib/ext2fs/blknum.c
@@ -49,7 +49,7 @@ blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs,
 					struct ext2_inode *inode)
 {
 	return (inode->i_blocks |
-		((fs->super->s_feature_incompat &
+		((fs->super->s_feature_ro_compat &
 		  EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ?
 		 (__u64) inode->osd2.linux2.l_i_blocks_hi << 32 : 0)) -
 		(inode->i_file_acl ? fs->blocksize >> 9 : 0);
@@ -62,7 +62,7 @@ blk64_t ext2fs_inode_i_blocks(ext2_filsys fs,
 					struct ext2_inode *inode)
 {
 	return (inode->i_blocks |
-		((fs->super->s_feature_incompat & 
+		((fs->super->s_feature_ro_compat & 
 		  EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ?
 		 (__u64)inode->osd2.linux2.l_i_blocks_hi << 32 : 0));
 }
-- 
1.5.6.5

Re: [PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[Andreas Dilger]

On 2010-08-23, at 19:34, Justin Maggard wrote:
> Creating a 4TB file on a filesystem with the 64bit flag set results in e2fsck consistently complaining about i_blocks being wrong, with confusing messages like this:
> 
> That appears to be caused by ext2fs_inode_i_blocks() checking for the EXT4_FEATURE_RO_COMPAT_HUGE_FILE in the wrong place.  Fix it.
> 
> -		((fs->super->s_feature_incompat &
> +		((fs->super->s_feature_ro_compat &
> 		  EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ?

This isn't the first time that we've had problems like this, and it won't be the last time unless there is a better API for checking these flags.

I've long thought about having something like:

#define EXT4_CHECK_COMPAT(sb, flag)    ((super)->s_feature_compat &     \
                                          EXT4_FEATURE_COMPAT_ ## flag)
#define EXT4_CHECK_RO_COMPAT(sb, flag) ((super)->s_feature_ro_compat &     \
                                          EXT4_FEATURE_RO_COMPAT_ ## flag)
#define EXT4_CHECK_INCOMPAT(sb, flag)  ((super)->s_feature_incompat &     \
                                          EXT4_FEATURE_INCOMPAT_ ## flag)
#define EXT4_SET_COMPAT(sb, flag)    do { (super)->s_feature_compat |=     \
                                         EXT4_FEATURE_COMPAT_ ## flag; } while(0)
#define EXT4_SET_RO_COMPAT(sb, flag) do { (super)->s_feature_ro_compat |=     \
                                         EXT4_FEATURE_RO_COMPAT_ ##flag;} while(0)
#define EXT4_SET_INCOMPAT(sb, flag)  do { (super)->s_feature_incompat |=     \
                                         EXT4_FEATURE_INCOMPAT_ ## flag;} while(0)

This will produce a compile error if the flag name and the macro name don't match. Unfortunately, for e2fsprogs we will need 3 sets of such macros because the feature flags are named EXT2_*, EXT3_*, and EXT4_*, depending on when they were added.  For the kernel code that wouldn't be needed.

Cheers, Andreas

Re: [PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[Eric Sandeen]

Andreas Dilger wrote:

> This isn't the first time that we've had problems like this, and it
> won't be the last time unless there is a better API for checking
> these flags.

*nod*
 
> I've long thought about having something like:

Same thing crossed my mind; seems like a very good idea.

> #define EXT4_CHECK_COMPAT(sb, flag)    ((super)->s_feature_compat &     \
>                                           EXT4_FEATURE_COMPAT_ ## flag)
> #define EXT4_CHECK_RO_COMPAT(sb, flag) ((super)->s_feature_ro_compat &     \
>                                           EXT4_FEATURE_RO_COMPAT_ ## flag)
> #define EXT4_CHECK_INCOMPAT(sb, flag)  ((super)->s_feature_incompat &     \
>                                           EXT4_FEATURE_INCOMPAT_ ## flag)
> #define EXT4_SET_COMPAT(sb, flag)    do { (super)->s_feature_compat |=     \
>                                          EXT4_FEATURE_COMPAT_ ## flag; } while(0)
> #define EXT4_SET_RO_COMPAT(sb, flag) do { (super)->s_feature_ro_compat |=     \
>                                          EXT4_FEATURE_RO_COMPAT_ ##flag;} while(0)
> #define EXT4_SET_INCOMPAT(sb, flag)  do { (super)->s_feature_incompat |=     \
>                                          EXT4_FEATURE_INCOMPAT_ ## flag;} while(0)
> 
> This will produce a compile error if the flag name and the macro name
> don't match. Unfortunately, for e2fsprogs we will need 3 sets of such
> macros because the feature flags are named EXT2_*, EXT3_*, and
> EXT4_*, depending on when they were added. For the kernel code that
> wouldn't be needed.

Time to name them consistently in e2fsprogs, then, I think?

-Eric

> Cheers, Andreas

Re: [PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[Justin Maggard]

On Tue, Aug 24, 2010 at 9:41 AM, Eric Sandeen <sandeen@redhat.com> wrote:
> Andreas Dilger wrote:
>
>> This isn't the first time that we've had problems like this, and it
>> won't be the last time unless there is a better API for checking
>> these flags.
>
> *nod*
>

On a related note (to my original problem)... is the bad i_blocks
count fixup code in the check_blocks() function of pass1.c broken for
large files, or am I misunderstanding the code?  pb.num_blocks and
ext2fs_inode_i_blocks() are both >32 bits, but the fixup code appears
to always try to shove pb.num_blocks into the 32-bit inode->i_blocks
and set inode->osd2.linux2.l_i_blocks_hi = 0.

-Justin

Re: [PATCH] Fix EXT4_FEATURE_RO_COMPAT_HUGE_FILE check

[Ted Ts'o]

On Tue, Aug 24, 2010 at 01:34:16AM +0000, Justin Maggard wrote:
> Creating a 4TB file on a filesystem with the 64bit flag set results
> in e2fsck consistently complaining about i_blocks being wrong, with
> confusing messages like this:
> 
> Inode 29818882, i_blocks is 8388608816, should be 8388608816.  Fix? no
> 
> That appears to be caused by ext2fs_inode_i_blocks() checking for
> the EXT4_FEATURE_RO_COMPAT_HUGE_FILE in the wrong place.  Fix it.
> 
> Signed-off-by: Justin Maggard <jmaggard10@gmail.com>

Applied to the e2fsprogs master branch.  Sorry for the delay in
attending to this patch.  This Fall has been incredibly busy for me;
now that I have the maint branch tidied up for the 1.41.12 release,
I'm not catching up with the backlog on the master branch.

    		    	     	     	- Ted