Commit 94968e74 breaks f_lotsbad

Commit 94968e74 breaks f_lotsbad

[Andreas Dilger]

Ted,
I was going to send some more fixes, but it looks like your commit
94968e74 "libext2fs: teach bitmap functions about bigalloc/cluster"
breaks the "f_lotsbad" test, which I found via git bisect and verified
is the offending commit:

--- ./f_lotsbad/expect.1        2011-06-01 20:25:00.871615457 +0000
+++ f_lotsbad.1.log     2011-06-17 05:33:37.398073505 +0000
@@ -26,6 +26,7 @@
 
 Restarting e2fsck from the beginning...
 Pass 1: Checking inodes, blocks, and sizes
+Illegal block number passed to ext2fs_test_block_bitmap #0 for in-use block map
 Pass 2: Checking directory structure
 Entry 'termcap' in / (2) has deleted/unused inode 12.  Clear? yes


I couldn't see the obvious correlation between the failure and the patch,
though it is clear the patch is changing ext2fs_test_generic_bitmap(),
but all of the changes should be no-ops because cluster_bits should be 0
unless bigalloc is enabled.

My tree is up to b2e6c86d6, "mke2fs, e2fsck: fix i_blocks handling for bigalloc file systems", which AFAICS is the latest.

Cheers, Andreas
--
Andreas Dilger 
Principal Engineer
Whamcloud, Inc.

Re: Commit 94968e74 breaks f_lotsbad

[Ted Ts'o]

On Fri, Jun 17, 2011 at 12:28:01AM -0600, Andreas Dilger wrote:
> Ted,
> I was going to send some more fixes, but it looks like your commit
> 94968e74 "libext2fs: teach bitmap functions about bigalloc/cluster"
> breaks the "f_lotsbad" test, which I found via git bisect and verified
> is the offending commit:

The problem was a bug in lib/ext2fs/Makefile.in; the $(SRCS) macro
included "$(srcdir)/blkmap64_ba.o" (note ".o" instead of ".o"; Oops.)

As a result we didn't have a dependency for blkmap64_ba.c in the
Makefile, and so blkmap64_ba.o wasn't getting regenerated when
bmap64.h changed.  You can fix it by rm'ing blkmap64_ba.o and then
rebuilding.  I'll fixed the Makefile.in, rerun "make depend", and then
check in a fix.

Regards,

						- Ted

Re: Commit 94968e74 breaks f_lotsbad

[Andreas Dilger]

On 2011-06-19, at 7:58 PM, Ted Ts'o wrote:
> On Fri, Jun 17, 2011 at 12:28:01AM -0600, Andreas Dilger wrote:
>> 
>> I was going to send some more fixes, but it looks like your commit
>> 94968e74 "libext2fs: teach bitmap functions about bigalloc/cluster"
>> breaks the "f_lotsbad" test, which I found via git bisect and verified
>> is the offending commit:
> 
> The problem was a bug in lib/ext2fs/Makefile.in; the $(SRCS) macro
> included "$(srcdir)/blkmap64_ba.o" (note ".o" instead of ".o"; Oops.)
> 
> As a result we didn't have a dependency for blkmap64_ba.c in the
> Makefile, and so blkmap64_ba.o wasn't getting regenerated when
> bmap64.h changed.  You can fix it by rm'ing blkmap64_ba.o and then
> rebuilding.  I'll fixed the Makefile.in, rerun "make depend", and then
> check in a fix.

I retested after pulling your commit 1ca87790b914a1958f3cbd8b5e1e7037f2cf30ed
"libext2fs: fix makefile dependency problem", but even when I do "make clean"
before each test I still get a failure on the "f_lotsbad" test:

--- ./f_lotsbad/expect.1        2011-06-01 20:25:00.871615457 +0000
+++ f_lotsbad.1.log     2011-06-28 05:09:16.484198659 +0000
@@ -26,6 +26,7 @@
 
 Restarting e2fsck from the beginning...
 Pass 1: Checking inodes, blocks, and sizes
+Illegal block number passed to ext2fs_test_block_bitmap #0 for in-use block map
 Pass 2: Checking directory structure
 Entry 'termcap' in / (2) has deleted/unused inode 12.  Clear? yes

I verified that no "*.o" files remained before rebuilding, but it didn't
fix the problem.  I also tested on both the "maint" and "next" branches
in case they were different, but it failed on both.

Cheers, Andreas
--
Andreas Dilger 
Principal Engineer
Whamcloud, Inc.

Re: Commit 94968e74 breaks f_lotsbad

[Ted Ts'o]

On Mon, Jun 27, 2011 at 11:16:14PM -0600, Andreas Dilger wrote:
> 
> I retested after pulling your commit 1ca87790b914a1958f3cbd8b5e1e7037f2cf30ed
> "libext2fs: fix makefile dependency problem", but even when I do "make clean"
> before each test I still get a failure on the "f_lotsbad" test:

I can't replicate this problem on my end.  What gcc version are you
using, and what are your configure options?

This is what I'm using

% ./config.status --version
config.status
configured by ../configure, generated by GNU Autoconf 2.65,
  with options "'--enable-elf-shlibs' '--enable-maintainer-mode' 'CFLAGS=-g' '--enable-symlink-build' '--enable-symlink-install' '--disable-libuuid' '--disable-libblkid'"

Copyright (C) 2009 Free Software Foundation, Inc.
This config.status script is free software; the Free Software Foundation
gives unlimited permission to copy, distribute and modify it.

% gcc --version
gcc (Ubuntu 4.4.3-4ubuntu5) 4.4.3

						- Ted

Re: Commit 94968e74 breaks f_lotsbad

[Andreas Dilger]

On 2011-06-28, at 8:23 AM, Ted Ts'o wrote:
> On Mon, Jun 27, 2011 at 11:16:14PM -0600, Andreas Dilger wrote:
>> 
>> I retested after pulling your commit 1ca87790b914a1958f3cbd8b5e1e7037f2cf30ed
>> "libext2fs: fix makefile dependency problem", but even when I do "make clean"
>> before each test I still get a failure on the "f_lotsbad" test:
> 
> I can't replicate this problem on my end.  What gcc version are you
> using, and what are your configure options?
> 
> This is what I'm using
> 
> % ./config.status --version
> config.status
> configured by ../configure, generated by GNU Autoconf 2.65,
>  with options "'--enable-elf-shlibs' '--enable-maintainer-mode' 'CFLAGS=-g' '--enable-symlink-build' '--enable-symlink-install' '--disable-libuuid' '--disable-libblkid'"
> 
> Copyright (C) 2009 Free Software Foundation, Inc.
> This config.status script is free software; the Free Software Foundation
> gives unlimited permission to copy, distribute and modify it.
> 
> % gcc --version
> gcc (Ubuntu 4.4.3-4ubuntu5) 4.4.3

I'm testing on an FC13 system, with a downgraded 2.6.32 FC12 kernel:

[root@sookie e2fsprogs-git]# git describe
v1.41.14-228-g1ca8779
[root@sookie e2fsprogs-git]# git diff
[root@sookie e2fsprogs-git]# ./config.status --version
config.status
configured by ./configure, generated by GNU Autoconf 2.65,
  with options ""

Copyright (C) 2009 Free Software Foundation, Inc.
This config.status script is free software; the Free Software Foundation
gives unlimited permission to copy, distribute and modify it.

[root@sookie e2fsprogs-git]# gcc --version
gcc (GCC) 4.4.5 20101112 (Red Hat 4.4.5-2)
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
[root@sookie e2fsprogs-git]# cd tests
[root@sookie tests]# make test_script
/bin/cp ./mke2fs.conf.in mke2fs.conf
Creating test_script...
[root@sookie tests]# ./test_script f_lotsbad
f_lotsbad: too many illegal blocks in inode: failed
0 tests succeeded       1 tests failed

Very strangely, the test passes on another node (older FC3 with many updates)
when run as a non-root user, though this isn't true on the original test node:

[adilger@mookie tests]$ ./test_script f_lotsbad
f_lotsbad: too many illegal blocks in inode: ok
1 tests succeeded       0 tests failed
[adilger@mookie tests]$ logout
[root@mookie e2fsprogs-git]# cd tests/
[root@mookie tests]# ./test_script f_lotsbad
f_lotsbad: too many illegal blocks in inode: failed
0 tests succeeded       1 tests failed
root@mookie tests]# su adilger
Agent pid 29339
[adilger@mookie tests]$ ./test_script f_lotsbad
f_lotsbad: too many illegal blocks in inode: ok
1 tests succeeded       0 tests failed
[adilger@mookie tests]$ ./config.status --version
config.status
configured by ./configure, generated by GNU Autoconf 2.65,
  with options "'CFLAGS=-Wall -g'"

Copyright (C) 2009 Free Software Foundation, Inc.
This config.status script is free software; the Free Software Foundation
gives unlimited permission to copy, distribute and modify it.
[adilger@mookie tests]$ gcc -V
gcc: `-V' option must have argument
[adilger@mookie tests]$ gcc --version
gcc (GCC) 3.4.4 20050721 (Red Hat 3.4.4-2)
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Comparing the environments between user and root don't show any
(apparently) significant differences (PATH is different, but I don't
think this test uses anything outside the local e2fsck) and even
running strace the only noticeable differences are the getuid/getgid
calls, and umask (which impacts the mode on the test.img file, but
I don't think that should be relevant either).


Cheers, Andreas
--
Andreas Dilger 
Principal Engineer
Whamcloud, Inc.

[PATCH] libext2fs: copy cluster_bits in ext2fs_copy_generic_bmap

[Eric Sandeen]

The f_lotsbad regression test was failing on some systems
with:

@@ -26,6 +61,7 @@

 Restarting e2fsck from the beginning...
 Pass 1: Checking inodes, blocks, and sizes
+Illegal block number passed to ext2fs_test_block_bitmap #0 for in-use block map
 Pass 2: Checking directory structure
 Entry 'termcap' in / (2) has deleted/unused inode 12.  Clear? yes

Running with valgrind (./test_script --valgrind f_lotsbad) we
see:

+==31409== Conditional jump or move depends on uninitialised value(s)
+==31409==    at 0x42927A: ext2fs_test_generic_bmap (gen_bitmap64.c:378)

among others.

Looking at gen_bitmap64.c:
376:        arg >>= bitmap->cluster_bits;
377:
378:        if ((arg < bitmap->start) || (arg > bitmap->end)) {

A little more debugging showed that it was actually
bitmap->cluster_bits which was uninitialized, because it never
gets copied over in ext2fs_copy_generic_bmap()

Patch below resolves the issue.

Reported-by: Andreas Dilger <adilger@whamcloud.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
index f75876d..80c9f7a 100644
--- a/lib/ext2fs/gen_bitmap64.c
+++ b/lib/ext2fs/gen_bitmap64.c
@@ -193,6 +193,7 @@ errcode_t ext2fs_copy_generic_bmap(ext2fs_generic_bitmap src,
 	new_bmap->real_end = src->real_end;
 	new_bmap->bitmap_ops = src->bitmap_ops;
 	new_bmap->base_error_code = src->base_error_code;
+	new_bmap->cluster_bits = src->cluster_bits;
 
 	descr = src->description;
 	if (descr) {

Re: [PATCH] libext2fs: copy cluster_bits in ext2fs_copy_generic_bmap

[Ted Ts'o]

Oops, thanks for catching this!  Applied to the e2fsprogs next branch.

      	     	 	  	 	 - Ted