Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks.

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

ext4/super.c renamed from ext4-set_64-bit_JBD_incompat.patch/fs/ext4/super.c +0 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +4 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 
+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */

Quoted 17 lines · Jose R. Santos

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +0 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 

fs/ext4/super.c static int ext4_fill_super (struct super · line 1831

Quoted 5 lines

fs/ext4/super.c +5 -0

+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */
+	if(es->s_blocks_count_hi &&

Quoted 5 lines

fs/ext4/super.c +5 -0

+	   !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
+				      JBD2_FEATURE_INCOMPAT_64BIT)){
+		printk(KERN_ERR "ext4: Failed to set 64-bit journal feature\n");
+		goto failed_mount4;
+	}

fs/ext4/super.c static int ext4_fill_super (struct super · line 1840

Quoted 4 lines

fs/ext4/super.c +1 -0

+
 	/* We have now updated the journal if required, so we can
 	 * validate the data journaling mode. */
 	switch (test_opt(sb, DATA_FLAGS)) {

This is fine, but Linux CodingStyle would have "if (" and have ")) {". Don't bother reposting, but whoever adds this to the ext4 git tree and/or sending it to Andrew should make the fix.

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Mingming Cao <[email protected]>

Signed-off-by Jose R. Santos <[email protected]>

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +4 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 
+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */

Quoted 18 lines · Andreas Dilger

Quoted 17 lines · Jose R. Santos

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +0 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 

Quoted 5 lines

fs/ext4/super.c static int ext4_fill_super (struct super

Quoted 5 lines

fs/ext4/super.c +5 -0

+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */
+	if(es->s_blocks_count_hi &&

This need to be le32_to_cpu(es->s_blocks_count_hi)

Quoted 19 lines · On Mon, 04 Jun 2007 16:01:45 -0700 Mingming Cao <[email protected]> wrote:

Quoted 18 lines · Andreas Dilger

Quoted 17 lines · Jose R. Santos

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +0 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 

Quoted 7 lines

Quoted 5 lines

fs/ext4/super.c static int ext4_fill_super (struct super

Quoted 5 lines

fs/ext4/super.c +5 -0

+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */
+	if(es->s_blocks_count_hi &&

This need to be le32_to_cpu(es->s_blocks_count_hi)

I'm curious,

Why do we need to do an endian conversion to check for a non-zero value in s_blocks_count_hi? Seems unnecessary here.

Quoted 28 lines · Jose R. Santos

Quoted 25 lines · On Mon, 04 Jun 2007 16:01:45 -0700 Mingming Cao <[email protected]> wrote:

Quoted 18 lines · Andreas Dilger

Quoted 17 lines · Jose R. Santos

Set the journals JBD2_FEATURE_INCOMPAT_64BIT on devices with more than 32bit block sizes during mount time. This ensure proper record lenth when writing to the journal.

Signed-off-by Jose R. Santos <[email protected]>

---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================

fs/ext4/super.c +0 -0

@@ -1824,6 +1824,17 @@ static int ext4_fill_super (struct super
 		goto failed_mount3;
 	}
 

Quoted 5 lines

fs/ext4/super.c static int ext4_fill_super (struct super

Quoted 5 lines

fs/ext4/super.c +5 -0

+        /*
+	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
+	 * with more that 32-bit block counts
+	 */
+	if(es->s_blocks_count_hi &&

This need to be le32_to_cpu(es->s_blocks_count_hi)

Quoted 4 lines

I'm curious,

Why do we need to do an endian conversion to check for a non-zero value in s_blocks_count_hi? Seems unnecessary here.

Jose is right. The endian conversion is unnecessary.

Shaggy

Quoted 3 lines · Dave Kleikamp

Jose is right. The endian conversion is unnecessary.

Shaggy

But by using le32_to_cpu(es->s_blocks_count_hi) you explicitly mark the variable as a little-endian.
So if someone reads the code, he knows this is a little-endian value and this allows to avoid errors if later variable must be tested for other value than 0.

For instance, you have :

if(es->s_blocks_count_hi)

and later the value should be compared to 10, how do you know easily you should use:

if (le32_to_cpu(es->s_blocks_count_hi) == 10)

instead of

if(es->s_blocks_count_hi == 10)

I think writing like Mingming asks should allow to avoid errors later.

(and code becomes really self-explicit...)

Regards,
Laurent

Quoted 20 lines · On Tue, 05 Jun 2007 15:26:53 +0200 Laurent Vivier <[email protected]>…

Quoted 3 lines · Dave Kleikamp

Jose is right. The endian conversion is unnecessary.

Shaggy

But by using le32_to_cpu(es->s_blocks_count_hi) you explicitly mark the variable as a little-endian. So if someone reads the code, he knows this is a little-endian value and this allows to avoid errors if later variable must be tested for other value than 0.

For instance, you have :

if(es->s_blocks_count_hi)

and later the value should be compared to 10, how do you know easily you should use:

if (le32_to_cpu(es->s_blocks_count_hi) == 10)

instead of

Quoted 7 lines

if(es->s_blocks_count_hi == 10)

I think writing like Mingming asks should allow to avoid errors later.

(and code becomes really self-explicit...)

Regards, Laurent

Hi Laurent,

In this particular case though, the value of s_blocks_count_hi should not be uses on its own. The correct way would be to use ext4_blocks_count() which already does the endian conversion. If you think the code could confuse people as to how to access the data in s_blocks_count_hi, wouldn't hiding it through the use of a macro make more sense than doing an unnecessary endian conversion?

Quoted 9 lines · Jose R. Santos

Hi Laurent,

In this particular case though, the value of s_blocks_count_hi should not be uses on its own. The correct way would be to use ext4_blocks_count() which already does the endian conversion. If you think the code could confuse people as to how to access the data in s_blocks_count_hi, wouldn't hiding it through the use of a macro make more sense than doing an unnecessary endian conversion?

Yes, I think the code could confuse people, but I don't think defining "Yet Another Macro" is a good choice (IMHO).

I think we can resolve this (non-)issue by two ways:
- using le32_to_cpu() (but I agree it does an unnecessary endian conversion on big-endian systems)

- put a comment on the line (but are we allowed to put comments in kernel source code... ;-) )

Regards
Laurent

Quoted 10 lines · On Tue, 05 Jun 2007 16:03:44 +0200 Laurent Vivier <[email protected]>…

Quoted 9 lines · Jose R. Santos

Hi Laurent,

In this particular case though, the value of s_blocks_count_hi should not be uses on its own. The correct way would be to use ext4_blocks_count() which already does the endian conversion. If you think the code could confuse people as to how to access the data in s_blocks_count_hi, wouldn't hiding it through the use of a macro make more sense than doing an unnecessary endian conversion?

Quoted 6 lines

Yes, I think the code could confuse people, but I don't think defining "Yet Another Macro" is a good choice (IMHO).

I think we can resolve this (non-)issue by two ways:
- using le32_to_cpu() (but I agree it does an unnecessary endian conversion on big-endian systems)

I just think that adding extra instructions for the sake of slightly better code readability is wrong, especially when the value s_blocks_count_hi should not be used on its own.

Quoted 2 lines

- put a comment on the line (but are we allowed to put comments in kernel source code... ;-) )

One advantage of a macro here is that we would make the code more explicit and should be able to eliminate the need for those 4 lines of comments that this patch adds.

Quoted 26 lines · Jose R. Santos

Quoted 10 lines · On Tue, 05 Jun 2007 16:03:44 +0200 Laurent Vivier <[email protected]>…

Quoted 9 lines · Jose R. Santos

Hi Laurent,

In this particular case though, the value of s_blocks_count_hi should not be uses on its own. The correct way would be to use ext4_blocks_count() which already does the endian conversion. If you think the code could confuse people as to how to access the data in s_blocks_count_hi, wouldn't hiding it through the use of a macro make more sense than doing an unnecessary endian conversion?

Quoted 6 lines

Yes, I think the code could confuse people, but I don't think defining "Yet Another Macro" is a good choice (IMHO).

I think we can resolve this (non-)issue by two ways:
- using le32_to_cpu() (but I agree it does an unnecessary endian conversion on big-endian systems)

I just think that adding extra instructions for the sake of slightly better code readability is wrong, especially when the value s_blocks_count_hi should not be used on its own.

Quoted 2 lines

- put a comment on the line (but are we allowed to put comments in kernel source code... ;-) )

Quoted 3 lines

One advantage of a macro here is that we would make the code more explicit and should be able to eliminate the need for those 4 lines of comments that this patch adds.

IMHO, you should do as _you_ think it is better... but as Mingming did the first comment perhaps she can explain what she thought.

Regards,
Laurent

The better choice to me is using ext4_blocks_count() to hide the details of the little endian. It's fine to use s_blocks_count_hi directly, just to make it clear, this is on-disk superblock data and better to do little endian conversion like read-in other on-disk superblock fields.

Yeah, it probably unnecessary in this case, but I don't think the extra instruction plays an important role in the performance, (this is only called at mount time, and there are lots of other places doing little endian conversion in ext4_fill_super() anyway).