[RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks.

[RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks.

[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 <jrs@us.ibm.com>

--- .pc/ext4-set_64-bit_JBD_incompat.patch/fs/ext4/super.c	2007-05-31 22:25:21.843984697 -0500
+++ fs/ext4/super.c	2007-05-31 22:26:14.913670672 -0500
@@ -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
+	 */
+	if(es->s_blocks_count_hi)
+		if (!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;
+		}
+
 	/* We have now updated the journal if required, so we can
 	 * validate the data journaling mode. */
 	switch (test_opt(sb, DATA_FLAGS)) {


Thanks
-JRS

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks.

[Andreas Dilger]

On Jun 01, 2007  10:52 -0500, Jose R. Santos wrote:
> 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 <jrs@us.ibm.com>

Content is good, but indenting should be fixed, spaces removed.

> --- .pc/ext4-set_64-bit_JBD_incompat.patch/fs/ext4/super.c	2007-05-31 22:25:21.843984697 -0500
> +++ fs/ext4/super.c	2007-05-31 22:26:14.913670672 -0500
> @@ -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
> +	 */
> +	if(es->s_blocks_count_hi)
> +		if (!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;
> +		}

	/* 
	 * Make sure to set JBD2_FEATURE_INCOMPAT_64BIT on filesystems
	 * with more that 32-bit block counts
	 */
	if (es->s_blocks_count_hi &&
	    !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;
	}

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

[RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[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 <jrs@us.ibm.com>
---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================
--- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
+++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
@@ -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
+	 */
+	if(es->s_blocks_count_hi &&
+	   !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;
+	}
+
 	/* We have now updated the journal if required, so we can
 	 * validate the data journaling mode. */
 	switch (test_opt(sb, DATA_FLAGS)) {


Thanks
-JRS

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Andreas Dilger]

On Jun 04, 2007  11:32 -0500, Jose R. Santos wrote:
> 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 <jrs@us.ibm.com>
> ---
>  fs/ext4/super.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> Index: linux-2.6.22-rc3/fs/ext4/super.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
> +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
> @@ -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
> +	 */
> +	if(es->s_blocks_count_hi &&
> +	   !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;
> +	}
> +
>  	/* 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.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Mingming Cao]

On Mon, 2007-06-04 at 11:57 -0600, Andreas Dilger wrote:
> On Jun 04, 2007  11:32 -0500, Jose R. Santos wrote:
> > 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 <jrs@us.ibm.com>
> > ---
> >  fs/ext4/super.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > Index: linux-2.6.22-rc3/fs/ext4/super.c
> > ===================================================================
> > --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
> > +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
> > @@ -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
> > +	 */
> > +	if(es->s_blocks_count_hi &&

This need to be le32_to_cpu(es->s_blocks_count_hi)

> > +	   !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;
> > +	}
> > +
> >  	/* 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.
> 
Okay, I can added the le32_to_cpu() and fix the style, and add the
following patch to ext4 patch queue.


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 <cmm@us.ibm.com>
Signed-off-by: Jose R. Santos <jrs@us.ibm.com>
---
 fs/ext4/super.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6.22-rc3/fs/ext4/super.c
===================================================================
--- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
+++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
@@ -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
+	 */
+	if (le32_to_cpu(es->s_blocks_count_hi) &&
+	   !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;
+	}
+
 	/* We have now updated the journal if required, so we can
 	 * validate the data journaling mode. */
 	switch (test_opt(sb, DATA_FLAGS)) {

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Andreas Dilger]

On Jun 04, 2007  16:01 -0700, Mingming Cao wrote:
> 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 <cmm@us.ibm.com>
> Signed-off-by: Jose R. Santos <jrs@us.ibm.com>

You can add Signed-off-by: Andreas Dilger <adilger@clusterfs.com> also.

> Index: linux-2.6.22-rc3/fs/ext4/super.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
> +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
> @@ -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
> +	 */
> +	if (le32_to_cpu(es->s_blocks_count_hi) &&
> +	   !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;
> +	}
> +
>  	/* We have now updated the journal if required, so we can
>  	 * validate the data journaling mode. */
>  	switch (test_opt(sb, DATA_FLAGS)) {

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Jose R. Santos]

On Mon, 04 Jun 2007 16:01:45 -0700
Mingming Cao <cmm@us.ibm.com> wrote:

> On Mon, 2007-06-04 at 11:57 -0600, Andreas Dilger wrote:
> > On Jun 04, 2007  11:32 -0500, Jose R. Santos wrote:
> > > 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 <jrs@us.ibm.com>
> > > ---
> > >  fs/ext4/super.c |   11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > Index: linux-2.6.22-rc3/fs/ext4/super.c
> > > ===================================================================
> > > --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
> > > +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
> > > @@ -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
> > > +	 */
> > > +	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.

-JRS

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Dave Kleikamp]

On Tue, 2007-06-05 at 06:41 -0500, Jose R. Santos wrote:
> On Mon, 04 Jun 2007 16:01:45 -0700
> Mingming Cao <cmm@us.ibm.com> wrote:
> 
> > On Mon, 2007-06-04 at 11:57 -0600, Andreas Dilger wrote:
> > > On Jun 04, 2007  11:32 -0500, Jose R. Santos wrote:
> > > > 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 <jrs@us.ibm.com>
> > > > ---
> > > >  fs/ext4/super.c |   11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > Index: linux-2.6.22-rc3/fs/ext4/super.c
> > > > ===================================================================
> > > > --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
> > > > +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
> > > > @@ -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
> > > > +	 */
> > > > +	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.

Jose is right.  The endian conversion is unnecessary.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

Dave Kleikamp wrote:
> On Tue, 2007-06-05 at 06:41 -0500, Jose R. Santos wrote:
>> On Mon, 04 Jun 2007 16:01:45 -0700
>> Mingming Cao <cmm@us.ibm.com> wrote:
>>
>>> On Mon, 2007-06-04 at 11:57 -0600, Andreas Dilger wrote:
>>>> On Jun 04, 2007  11:32 -0500, Jose R. Santos wrote:
>>>>> 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 <jrs@us.ibm.com>
>>>>> ---
>>>>>  fs/ext4/super.c |   11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> Index: linux-2.6.22-rc3/fs/ext4/super.c
>>>>> ===================================================================
>>>>> --- linux-2.6.22-rc3.orig/fs/ext4/super.c	2007-06-04 11:01:20.028360650 -0500
>>>>> +++ linux-2.6.22-rc3/fs/ext4/super.c	2007-06-04 11:05:11.389126418 -0500
>>>>> @@ -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
>>>>> +	 */
>>>>> +	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.
> 
> 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
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Jose R. Santos]

On Tue, 05 Jun 2007 15:26:53 +0200
Laurent Vivier <Laurent.Vivier@bull.net> wrote:

> Dave Kleikamp wrote:
> > 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

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?

-JRS

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

Jose R. Santos wrote:
> On Tue, 05 Jun 2007 15:26:53 +0200 Laurent Vivier <Laurent.Vivier@bull.net>
> wrote:
> 
>> Dave Kleikamp wrote:
>>> 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
> 
> 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
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Jose R. Santos]

On Tue, 05 Jun 2007 16:03:44 +0200
Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> Jose R. Santos wrote:
> > 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)

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.

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

> Regards
> Laurent


-JRS

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

Jose R. Santos wrote:
> On Tue, 05 Jun 2007 16:03:44 +0200
> Laurent Vivier <Laurent.Vivier@bull.net> wrote:
>> Jose R. Santos wrote:
>>> 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)
> 
> 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.
> 
>> - 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.

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
-- 
------------- Laurent.Vivier@bull.net  --------------
       "Any sufficiently advanced technology is
  indistinguishable from magic." - Arthur C. Clarke


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Mingming Cao]

On Tue, 2007-06-05 at 18:07 +0200, Laurent Vivier wrote:
> Jose R. Santos wrote:
> > On Tue, 05 Jun 2007 16:03:44 +0200
> > Laurent Vivier <Laurent.Vivier@bull.net> wrote:
> >> Jose R. Santos wrote:
> >>> 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)
> > 
> > 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.
> > 
> >> - 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.
> 
> IMHO, you should do as _you_ think it is better... but as Mingming did the first
> comment perhaps she can explain what she thought.
> 

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

Mingming
> Regards,
> Laurent

Re: [RFC][PATCH] Set JBD2_FEATURE_INCOMPAT_64BIT on filesystems larger than 32-bit blocks (take 2).

[Jose R. Santos]

On Tue, 05 Jun 2007 10:46:43 -0700
Mingming Cao <cmm@us.ibm.com> wrote:
> 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. 

On the grounds of avoiding confusion regarthing the use of
s_blocks_count_hi, I agree that using ext4_blocks_count() is the right
thing to do.  I will resubmit the patch and also eliminate the the 4
lines of comments since the code would be more explicit as to what its
doing.

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

I originally wrote this patch using ext4_blocks_count() but later
changed it since it was faster to do it this way.  While mounting is
not always a performance critical section, I still see those few extra
instructions a little wasteful. :)


-JRS