[PATCH 0/2 V2] Fix an e2fsck bug for fallocated extent.

[PATCH 0/2 V2] Fix an e2fsck bug for fallocated extent.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

Hi all,
	This is the V2 for the bug of e2fsck. patch 1 is
the fix and patch 2 is the regression test case.

Tao Ma (2):
  e2fsck: Let end_blk to be the maximum value of u32.
  e2fsck/tests: Add the regression test case for e2fsck.

 lib/ext2fs/extent.c                 |    4 +---
 tests/f_zero_extent_length/expect.1 |    7 +++++++
 tests/f_zero_extent_length/expect.2 |    7 +++++++
 tests/f_zero_extent_length/image.gz |  Bin 0 -> 5102 bytes
 tests/f_zero_extent_length/name     |    1 +
 5 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 tests/f_zero_extent_length/expect.1
 create mode 100644 tests/f_zero_extent_length/expect.2
 create mode 100644 tests/f_zero_extent_length/image.gz
 create mode 100644 tests/f_zero_extent_length/name

[PATCH 1/2] e2fsck: Let end_blk to be the maximum value of u32.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

Now we can use fallocate to create a large file while keep the size
to be small. It will cause the e2fsck complain about it. The test
script is simple and I have pasted it here.

DEVICE=/dev/sdb1
mount -t ext4 $DEVICE /mnt/ext4
for((i=0;i<10;i++))do fallocate -n -o $[$i*8192] -l 4096 /mnt/ext4/a;done
umount $DEVICE
e2fsck -fn $DEVICE

The error message will be like this:
e2fsck 1.42.3 (14-May-2012)
Pass 1: Checking inodes, blocks, and sizes
Inode 12 has zero length extent
	(invalid logical block 0, physical block 32775)
Clear? no

Inode 12, i_blocks is 88, should be 0.  Fix? no

Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Block bitmap differences:  -(8231--8232) -(32770--32778)
Fix? no

Now actually the end_blk can be any value which is less than
u32, so make end_blk be the maximum value of u32.

Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 lib/ext2fs/extent.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index eb096d6..e2815c2 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -253,9 +253,7 @@ extern errcode_t ext2fs_extent_open2(ext2_filsys fs, ext2_ino_t ino,
 		ext2fs_le16_to_cpu(eh->eh_entries);
 	handle->path[0].max_entries = ext2fs_le16_to_cpu(eh->eh_max);
 	handle->path[0].curr = 0;
-	handle->path[0].end_blk =
-		(EXT2_I_SIZE(handle->inode) + fs->blocksize - 1) >>
-		 EXT2_BLOCK_SIZE_BITS(fs->super);
+	handle->path[0].end_blk = ((((unsigned long long) 1) << 32) - 1);
 	handle->path[0].visit_num = 1;
 	handle->level = 0;
 	handle->magic = EXT2_ET_MAGIC_EXTENT_HANDLE;
-- 
1.7.1

Re: [PATCH 1/2] e2fsck: Let end_blk to be the maximum value of u32.

[Ted Ts'o]

Hi Tao,

Thanks for reporting the problem and submitting a test case.  I've
kept the regression test case (although I ended up renaming it).
However, I think a better fix is to change e2fsck, since it wasn't
making a check that it shouldn't have been making in the first place.
The commit description describes the fix which I think is better....

    	   	       		     	 - Ted

commit 9c40d14841f04811097a123d6e8555e78ce56811
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Mon Jun 11 00:25:45 2012 -0400

    e2fsck: only check for zero-length leaf extents
    
    The on-disk format for interior nodes in the extent tree does not
    encode the length of each entry in the interior node; instead, it is
    synthesized/simulated by the extent library code in libext2fs.
    Unfortunately, this simulation is not perfect; in particular it does
    not work for the last extent in the extent tree if there are
    uninitialized blocks allocated using fallocate with
    FALLOC_FL_KEEP_SIZE, and it leads to e2fsck incorrectly complaining
    about an invalid zero-length extent.
    
    We only need to worry about the extent length for the leaves of the
    tree, since it is there were we are checking an on-disk value, as
    opposed to a software-generated simulation.  So restrict the check of
    extent length to leaf nodes in the extent tree.
    
    Reported-by: Tao Ma <tm@tao.ma>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index c6aae6e..72dcd97 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -1779,7 +1779,7 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
 			problem = PR_1_EXTENT_BAD_START_BLK;
 		else if (extent.e_lblk < start_block)
 			problem = PR_1_OUT_OF_ORDER_EXTENTS;
-		else if (extent.e_len == 0)
+		else if (is_leaf && extent.e_len == 0)
 			problem = PR_1_EXTENT_LENGTH_ZERO;
 		else if (is_leaf &&
 			 (extent.e_pblk + extent.e_len) >

[PATCH 2/2] e2fsck/tests: Add the regression test case for e2fsck.

[Tao Ma]

From: Tao Ma <boyu.mt@taobao.com>

In commit "e2fsck: Let end_blk to be the maximum value of u32."
we fix a bug where a normal fallocate will cause e2fsck complain.
So add it to the regression test suite.

Signed-off-by: Tao Ma <boyu.mt@taobao.com>
---
 tests/f_zero_extent_length/expect.1 |    7 +++++++
 tests/f_zero_extent_length/expect.2 |    7 +++++++
 tests/f_zero_extent_length/image.gz |  Bin 0 -> 5102 bytes
 tests/f_zero_extent_length/name     |    1 +
 4 files changed, 15 insertions(+), 0 deletions(-)
 create mode 100644 tests/f_zero_extent_length/expect.1
 create mode 100644 tests/f_zero_extent_length/expect.2
 create mode 100644 tests/f_zero_extent_length/image.gz
 create mode 100644 tests/f_zero_extent_length/name

diff --git a/tests/f_zero_extent_length/expect.1 b/tests/f_zero_extent_length/expect.1
new file mode 100644
index 0000000..762159c
--- /dev/null
+++ b/tests/f_zero_extent_length/expect.1
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 12/1024 files (16.7% non-contiguous), 1227/4096 blocks
+Exit status is 0
diff --git a/tests/f_zero_extent_length/expect.2 b/tests/f_zero_extent_length/expect.2
new file mode 100644
index 0000000..762159c
--- /dev/null
+++ b/tests/f_zero_extent_length/expect.2
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 12/1024 files (16.7% non-contiguous), 1227/4096 blocks
+Exit status is 0
diff --git a/tests/f_zero_extent_length/image.gz b/tests/f_zero_extent_length/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..0b6915805bc7c3d67b9bf0f944f980b36012268e
GIT binary patch
literal 5102
zcmb2|=HM{n-0IK3oSB=Lp33m{u78e5s070Y^WBScwzp2RH+-<N(S>t{lY_{A6(w~8
zL)9)$^`=g#<SjBsT>RICD+?rfbW|xE5jfl^Fk47()~U$+s*edxoa}rp6SlP%t;{b=
zOY|0Bx$rI5=ijRG?`Pk<Idl8%@4o5V4(D(<WG*#ibNXRwW?khxV@=zHE%tU1E-~*;
z&fEC#d6G(E+OxCLp2a&w3wLeJcwJd&x=`@(Q;FT$iZ#>DN?U*Zbz%DYuYW#-zWv;o
zzyIfhcX>HKGoGB7R&H<m=?(MV8UHMcvvPl)kN>}Q*|pujU(VlG{qKRkzl_bqBYfXQ
z`%~91whPSNC#$>h_1Wohn%DLy9y=vr{;1mIR^e~v;%6$d(@t&sSa<6oV?@&Gq&58;
zbyL3DN3PuxdLX6rSJC!gtPH3i)#!8m7q9L2rxta*|MD^K|B9FE-rO^*Rb0u_`p3?^
zc43gD)T#FmH>+LImY4hY<HWP?eDy}3{OZ=vxxd=;Q(3sX?uLL(R{87ypUjH;@8G#Z
zTz1OzU+tfQ{s+Xpl0W7B*ST_Ky@vi*{-;a-E!=a(9wzDaSbuMU)}4FIOV*lje!rnz
z@<Z;uUwk324NKRr^I1EBT-NTkWtw)%r+)f&{*tq+BAQ<(=X4itC|?zoY5ivYs(q!a
zqkpp|d=x$Ye{J&rN=dobl6St`?!Mf8T{}T@%iRCr&UG3WJhT6A*qgCk^zExfu~WCd
z{CVS=`0n!u4i)_4e|oj<-~PqrMc01*2M5YGZiepMKUaU8za|j&rH6OBQvDL4Yi76q
zoL~60Gvykq;LW`O*L4nj+y7rFe}32PTvt1F&U};Gwr3}AuK8WAx78xPMD2C#Eslpd
zbL2EW6&uD!KM8;I@vi*C{NIKDuHN~$-(+3-*Xhr0&$s=z`Y8LPt=m4<yPM^I4Jw=B
z9;AHxf1dB<osaD~oB4OVlYhSZ?~dp1w*37Q?*IC`f9bU!=d+SGWNZ%p!D|?HY)2q(
z;urfb7ys^GxM$bjpU&?4{wI5$`SGv+(rW)JhyQIeoM`4`G5Os4Jw+Q%tUp$hJTd-t
z{yabP&#w*dM?Nu6-naV1@?V=b-?Ewi?$dqw7kysJxpgx(zJGG>*_W5y?9);oKfADW
zPi}0BUEP<8&9UJ=_ut#kj*+muR#=z(<%n(gggaOD@=xd6zZbk(_Se63-+5rDSEgN_
zf7@I?ZapYXHMBnYC4Xwgo9q9l%za(Yh?>mk%UiHHtH<P6vY}l6Mjba@H~*fho_i9t
zhSQEm2rjli=Fl_s?h76Ix@gq4(GVC7fdLMI4;NN~3(A*U1P{$LX^EORzxSuZ#QEJn
zZz=7+B!4JjVtnt<V@mtQKCgAHvwddW{U_~Ax#-W3Gv7simi%|xuO!Dd)ooVF+kKO(
zXJ1~fFBdVbceeBNPus$D%%Mf5`uE89trL@FpPRe+-zjE28T<Crwm;uK53sjKtsf16
n(GVC7fzc2c4S~@R7!85Z5Eu=CArb-&ANXYirY~S%a9{uc`yfdH

literal 0
HcmV?d00001

diff --git a/tests/f_zero_extent_length/name b/tests/f_zero_extent_length/name
new file mode 100644
index 0000000..9e0c6e0
--- /dev/null
+++ b/tests/f_zero_extent_length/name
@@ -0,0 +1 @@
+fallocated extents after i_size
-- 
1.7.1

Re: [PATCH 2/2] e2fsck/tests: Add the regression test case for e2fsck.

[Ted Ts'o]

On Thu, May 17, 2012 at 03:16:07PM +0800, Tao Ma wrote:
> From: Tao Ma <boyu.mt@taobao.com>
> 
> In commit "e2fsck: Let end_blk to be the maximum value of u32."
> we fix a bug where a normal fallocate will cause e2fsck complain.
> So add it to the regression test suite.
> 
> Signed-off-by: Tao Ma <boyu.mt@taobao.com>

I've changed the description of this commit to reflect the change in
how I propose to fix the problem....

					- Ted

commit 8d12c46a22965179cae1e3b47778fdee5efeb513
Author: Tao Ma <boyu.mt@taobao.com>
Date:   Sun Jun 10 23:56:30 2012 -0400

    tests: add new test f_zero_extent_length
    
    If all of the extents in the last extent tree block (ETB) in a
    non-trivial extent tree contain uninitialized extents which are after
    the end of the file as defined by i_size, the hueristics will
    incorrectly estimate the last entry (and hence the node's e_len field)
    in the last entry of each level of the extent tree.
    
    As Tao Ma has noted, since e2fsck was requiring that the length
    (e_len) field of interior nodes be non-zero, this was causing false
    failures where e2fsck would declare that the extent tree was
    corrupted.
    
    This was fixed in commit 9c40d14841: "e2fsck: only check for
    zero-length leaf extents".  Add a regression test case to ensure that
    this issue remains fixed.
    
    Signed-off-by: Tao Ma <boyu.mt@taobao.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>