| Message ID | 200811061651.49602.borntraeger@de.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Christian Borntraeger wrote: > Ok, I think I found it: > > On big endianess plattforms newly created ext4 file systems cannot be mounted > and show messages like: > [6923911.715968] EXT4-fs: not enough memory for 522250 flex groups > [6923911.715973] EXT4-fs: unable to initialize flex_bg meta info! > > We have to access s_reserved_gdb_blocks with le16_to_cpu. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > fs/ext4/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: kvm/fs/ext4/super.c > =================================================================== > --- kvm.orig/fs/ext4/super.c > +++ kvm/fs/ext4/super.c > @@ -1455,7 +1455,7 @@ static int ext4_fill_flex_info(struct su > > /* We allocate both existing and potentially added groups */ > flex_group_count = ((sbi->s_groups_count + groups_per_flex - 1) + > - ((sbi->s_es->s_reserved_gdt_blocks +1 ) << > + (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks + 1) << > I suspect that you want to do the le16_to_cpu() and _then_ add the 1. Otherwise, adding 1 to a different byte order value won't do quite what is expected or hoped for... ps > EXT4_DESC_PER_BLOCK_BITS(sb))) / > groups_per_flex; > sbi->s_flex_groups = kzalloc(flex_group_count * > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christian Borntraeger <borntraeger@de.ibm.com> writes: > Index: kvm/fs/ext4/super.c > =================================================================== > --- kvm.orig/fs/ext4/super.c > +++ kvm/fs/ext4/super.c > @@ -1455,7 +1455,7 @@ static int ext4_fill_flex_info(struct su > > /* We allocate both existing and potentially added groups */ > flex_group_count = ((sbi->s_groups_count + groups_per_flex - 1) + > - ((sbi->s_es->s_reserved_gdt_blocks +1 ) << > + (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks + 1) << You need to put the "+ 1" after the swap. Andreas.
On Thu, Nov 06, 2008 at 11:06:25AM -0500, Peter Staubach wrote: >> Index: kvm/fs/ext4/super.c >> =================================================================== >> --- kvm.orig/fs/ext4/super.c >> +++ kvm/fs/ext4/super.c >> @@ -1455,7 +1455,7 @@ static int ext4_fill_flex_info(struct su >> /* We allocate both existing and potentially added groups */ >> flex_group_count = ((sbi->s_groups_count + groups_per_flex - 1) + >> - ((sbi->s_es->s_reserved_gdt_blocks +1 ) << >> + (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks + 1) << >> > > I suspect that you want to do the le16_to_cpu() and _then_ > add the 1. Otherwise, adding 1 to a different byte order > value won't do quite what is expected or hoped for... Yes. And if someone ran sparse over the code both the initial error and this varaint would be trivial to spot.. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2008 at 04:51:49PM +0100, Christian Borntraeger wrote: > Ok, I think I found it: > > On big endianess plattforms newly created ext4 file systems cannot be mounted > and show messages like: > [6923911.715968] EXT4-fs: not enough memory for 522250 flex groups > [6923911.715973] EXT4-fs: unable to initialize flex_bg meta info! > > We have to access s_reserved_gdb_blocks with le16_to_cpu. Aneesh has already submitted a patch to fix this; I'll be pushing it to Linus shortly. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 6. November 2008 schrieb Christoph Hellwig: > > I suspect that you want to do the le16_to_cpu() and _then_ > > add the 1. Otherwise, adding 1 to a different byte order > > value won't do quite what is expected or hoped for... > > Yes. And if someone ran sparse over the code both the initial error > and this varaint would be trivial to spot.. I think the problem is, that sparse now requires __CHECK_ENDIAN__ to check for endianess problems. Seems that lots of people are not aware of this. Christian -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2008 at 11:38:46AM -0500, Christoph Hellwig wrote: > > Yes. And if someone ran sparse over the code both the initial error > and this varaint would be trivial to spot.. > That's how Aneesh found it. :-) - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2008 at 12:33:27PM -0500, Theodore Tso wrote: > On Thu, Nov 06, 2008 at 11:38:46AM -0500, Christoph Hellwig wrote: > > > > Yes. And if someone ran sparse over the code both the initial error > > and this varaint would be trivial to spot.. > > > > That's how Aneesh found it. :-) Heh, okay. We really should try to find some annotations that run sparse with endian checking by default for those parts of the tree where it makes sense.. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 06, 2008 at 12:39:44PM -0500, Christoph Hellwig wrote: > On Thu, Nov 06, 2008 at 12:33:27PM -0500, Theodore Tso wrote: > > On Thu, Nov 06, 2008 at 11:38:46AM -0500, Christoph Hellwig wrote: > > > > > > Yes. And if someone ran sparse over the code both the initial error > > > and this varaint would be trivial to spot.. > > > > > > > That's how Aneesh found it. :-) > > Heh, okay. We really should try to find some annotations that run > sparse with endian checking by default for those parts of the tree > where it makes sense.. In fact endian warnings were off by default, because there were too much of them in networking stack and elsewhere, but situations definitely improved. Out of head, only drivers/ieee1394/ is not done. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 6 November 2008 12:39:44 -0500, Christoph Hellwig wrote: > > Heh, okay. We really should try to find some annotations that run > sparse with endian checking by default for those parts of the tree > where it makes sense.. #define __CHECK_ENDIAN__ in some ext4 header. Made life much easier for me. Jörn
On Sat, 2008-11-08 at 23:35 +0100, Jörn Engel wrote: > On Thu, 6 November 2008 12:39:44 -0500, Christoph Hellwig wrote: > > > > Heh, okay. We really should try to find some annotations that run > > sparse with endian checking by default for those parts of the tree > > where it makes sense.. > > #define __CHECK_ENDIAN__ in some ext4 header. Made life much easier for > me. > I'd say that it's getting close to just being able to turn it on by default. A lot of the really verbose offenders have been annotated now, drivers/ieee1394, drivers/scsi, drivers/message and some of the older areas of drivers/net will get noisy, I was going to send an RFC for 2.6.29 and continue to chip away at the output. Harvey -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 10 November 2008 10:09:38 -0800, Harvey Harrison wrote: > > I'd say that it's getting close to just being able to turn it on by > default. A lot of the really verbose offenders have been annotated > now, drivers/ieee1394, drivers/scsi, drivers/message and some of the > older areas of drivers/net will get noisy, I was going to send an RFC > for 2.6.29 and continue to chip away at the output. My argument for the define is that it doesn't hurt much if endian checks are enabled globally, but helps a lot if they are not. So until it is clear that endian checks will be enabled, it may still be a good idea. Jörn
Index: kvm/fs/ext4/super.c =================================================================== --- kvm.orig/fs/ext4/super.c +++ kvm/fs/ext4/super.c @@ -1455,7 +1455,7 @@ static int ext4_fill_flex_info(struct su /* We allocate both existing and potentially added groups */ flex_group_count = ((sbi->s_groups_count + groups_per_flex - 1) + - ((sbi->s_es->s_reserved_gdt_blocks +1 ) << + (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks + 1) << EXT4_DESC_PER_BLOCK_BITS(sb))) / groups_per_flex; sbi->s_flex_groups = kzalloc(flex_group_count *
Ok, I think I found it: On big endianess plattforms newly created ext4 file systems cannot be mounted and show messages like: [6923911.715968] EXT4-fs: not enough memory for 522250 flex groups [6923911.715973] EXT4-fs: unable to initialize flex_bg meta info! We have to access s_reserved_gdb_blocks with le16_to_cpu. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- fs/ext4/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html