Lustre devel - Jan 2007 - [Bug 11510] e2fsck doesn't handle all possible extents corruptions

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Alex has been inspecting the extents part of the ldiskfs code and has found a
few places where we shouldn''t get BUG''ed, but rather
ext3_error(). The
anticipates the fixes won''t be too complex. More to come shortly.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Alex, Girish,

During my conversation with Livermore today, we contemplated how we will test
e2fsck. While there are some tools available that produce corruption scenarios,
what we need here is some targeted tests that break the file system in ways that
we''ve known to have problems. For example, bug 11414 needs a specific
test case
in order to evaluate the ldiskfs fix by Andreas.

Regarding things to include in our corruption test suite, we can look to
Bugzilla tickets. I''ll ask Livermore to help coach us at relevant cases
and
attachments.

Brian Behlendorf expressed concerns that there may be several exceptions in
ldiskfs that are not handled effectively by e2fsck. This was in Terry''s
original
email and agreed to by Alex. This reinforces that a thorough code review is
warranted.

Finally, Livermore wonders which version of e2fsck we should be working with.
There is the current production version e2fsprogs-1.39.cfs2 from the customer
download site, but Andreas has a .cfs3 available on the ftp site.

All of this should be addressed in the planning worksheet and included in our
planning estimates.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



There is a serious existing corruption regression test in the e2fsck source
code, all we need to do is to extend it with more cases.  This is fortunately
quite simple.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



(In reply to comment #9)
> (In reply to comment #2)
> > 2. run Coverity checker on the e2fsprogs, file bugs from those
> >    findings
> 
> would you do that?
Yes, Brian is working on inspecting 40-50 Coverity hits, and
he will file bugs, with patches if doable, on the bug hits
that are not false positives.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Created an attachment (id=9351)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9351&action=view)
List of all possible corruptions in extents.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



We are putting together a test DDN with several
4 TB LUNs, specifically to provide a test sandbox
for broken ldiskfs filesystems and e2fsck
detection/repair of the various breakages.
   
I''m hoping this will be available early next week.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



CFS, is a patch available yet for LLNL testing?
We are anxious to try the improvements.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Created an attachment (id=9429)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9429&action=view)
Chaos e2fsprogs-1.39 patch series

Attached is Livermore''s full patch series which appies to the stock 
e2fsprogs-1.39 release.  It includes CFSs patches in addition to
Redhat patches and local chaos patches.  It also contains patches
for the bugs discovered by Coverity.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Created an attachment (id=9439)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9439&action=view)
Fix for e2fsck doesn''t display an error message when inode index block
is full.

The patch should be replaced with e2fsprogs-extents.patch in
e2fsprogs-1.39.cfs3.

[root@metis e2fsprogs-1.39.cfs3]# interdiff ../e2fsprogs-extents.patch.orig
patches/e2fsprogs-extents.patch 
diff -u e2fsprogs/lib/ext2fs/extents.c e2fsprogs/lib/ext2fs/extents.c
--- e2fsprogs/lib/ext2fs/extents.c	2006-12-27 16:48:54.000000000 -0700
+++ e2fsprogs/lib/ext2fs/extents.c	2006-12-27 16:48:54.000000000 -0700
@@ -348,6 +348,8 @@
				if (nh->eh_entries == 0)
					ix->ei_leaf = ix->ei_leaf_hi = 0;
			}
+			if (flags & BLOCK_ERROR)
+				ret |= BLOCK_ERROR;
			if (flags & BLOCK_ABORT) {
				ret |= BLOCK_ABORT;
				goto free_buf;

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #9446 is|1                           |0
            private|                            |
   Attachment #9446|review?(adilger@clusterfs.co|review+
               Flag|m)                          |


(From update of attachment 9446)
This is definitely a good start.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Created an attachment (id=9525)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9525&action=view)
Patch for multi level split.

The patch is only meant for a simple case where extent tree has just one level.

i.e. i_blocks contains the leaves. Needs more testing.

The change in the extent tree causes problem as block_iterate_extents is a
recursive function, so any change in the structure of the tree causes different

problems when the functions unravel. I am now moving to next part of the task
where the the tree is deeper.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9525|review?(adilger@clusterfs.co|review-
               Flag|m)                          |


(From update of attachment 9525)
>-errcode_t ext2fs_extent_split(struct ext3_extent_header *eh,
>+errcode_t ext2fs_extent_split2(struct ext3_extent_header *eh,
> 			      struct ext3_extent *ex, int count)
Is this function intended to be exported for external callers?	I''d
think this
should be an internal helper function only, and should be declared static and
be given a better name like "ext2fs_extent_split_internal()" or
similar.
>+errcode_t ext2fs_extent_split(ext2_filsys fs,
>+		struct ext3_extent_header **eh,
>+		struct ext3_extent **ex, int count, int *flag)
>+{
>+	struct ext3_extent_header *eh_l0;
>+	struct ext3_extent_header *eh_l1 = *eh;
>+	struct ext3_extent *ex_l0;
I find the names "eh_10" and "eh_11" not very informative. 
What do "10" and
"11" mean?  Could you pick some more descriptive names like
"eh_orig" and
"eh_parent" or something like that?  Same for ex_10.  Please also put
declarations of the same type on the same line.
>+	int entry = *ex - EXT_FIRST_EXTENT(eh_l1);
>+	int retval;
>+	blk_t new_block;
>+	char *buf;
>+	struct ext3_extent_idx *ei = EXT_FIRST_INDEX(eh_l1);
These should all be declared in the lowest scope in which they are used.
>+	if (entry < 0 || entry > eh_l1->eh_entries)
>+	                return EXT2_ET_EXTENT_LEAF_BAD;
Too much indenting.
>+			memcpy(buf, eh_l1, eh_l1->eh_entries * 
>+					sizeof(struct ext3_extent) + 
>+					sizeof(struct ext3_extent_header));
A minor note - I think this would be a bit clearer if you had:

			memcpy(buf, eh_11, sizeof(*eh_11) +
			       eh_11->eh_entries * sizeof(*ex_10));

Firstly, it is better because the eh struct is first in the array, and the
ex structs follow that.  Secondly, using "sizeof(*eh)" instead of
"sizeof(struct ext3_extent_header)" is shorter and also avoids
problems if
the variable type changes.
>+			eh_l0 = (struct ext3_extent_header *)buf;
>+
>+			printf("eh_l0 = %x\n", eh_l0);
>+			printf("eh_l1 = %x\n", eh_l1);
How about using show_header from EXT_DEBUG here, so we don''t have to
worry
about removing these later?
>+			eh_l0->eh_max = (fs->blocksize - 
>+					sizeof(struct ext3_extent_header)) /
>+					sizeof(struct ext3_extent);
>+			eh_l0->eh_depth = 0;
Shouldn''t the depth already be zero?
>+			ext2fs_new_block(fs, 0, 0, &new_block);
Need to check error code.  Also, the block should probably be allocated right
after the buffer is, to avoid doing work if this fails.  Also, please supply a
goal block to at least try and get decent allocation for the index.  You could
use the ex->ee_block as the goal, though if we had the inode block number
that
would be even better.
>+			*eh = eh_l0;
>+			*ex = EXT_FIRST_EXTENT(eh_l0)  + 
>+				sizeof(struct ext3_extent) * entry;
>+
>+			*flag = 1;
What does that flag mean?  Is it BLOCK_CHANGED, or something else?  Should we
also be setting BLOCK_ERROR in the error cases?
>+			return ext2fs_extent_split2(eh_l0, *ex, count);
>+		
Please do not add whitespace at the end of the line, like above.
>@@ -259,14 +321,22 @@
> 
> 				} else if (j > 0 && /* implies ex->ee_len > 1 */
> 					   (ctx->errcode >-					    ext2fs_extent_split(eh, ex++, j))) {
>+					    ext2fs_extent_split(ctx->fs, &eh, 
>+						    &ex, j, 
>+						    &extent_split_flag) 
>+						    /* ex++ doesnt affect the 
>+						       conditional statement
>+						     */
Please put "*/" on previous line.
>@@ -281,11 +351,25 @@
> 					}
> 					/* FIXME: 48-bit */
> 					ex->ee_start = block_address;
>+					
>+					
>+						
> 					ret |= BLOCK_CHANGED;
Why are these blank lines added?
>+		/* Multi level split at depth == 0. 
>+		   ex has been changed to point to  newly allocated block 
>+		   buffer. And after returning  in this scenario, only inode is
>+		   updated with changed i_block. Hence to explicitly write to 
>+		   block requried.
>+		 */
>+		if(extent_split_flag == 1) {
>+			struct ext3_extent_idx *ix = EXT_FIRST_INDEX(orig_eh);
>+			ctx->errcode = ext2fs_write_ext_block(ctx->fs,
>+								ix->ei_leaf, eh);
>+		}
I''m not sure I understand this.  Didn''t ext2fs_extent_split()
already write the
ei_leaf == new_block?  Should this instead be writing out the inode?  Would it
be enough to return BLOCK_CHANGED in this case to have the caller write out the
current block?

With the exception of the above issue, I think it only needs a bit of code
cleanup before it can land into the CFS e2fsprogs patch series.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510



Comment Tags: Analysis



> 
> I''m not sure I understand this.  Didn''t
ext2fs_extent_split() already write the
> ei_leaf == new_block?  Should this instead be writing out the inode?  Would
it
> be enough to return BLOCK_CHANGED in this case to have the caller write out
the
> current block?
> 
> With the exception of the above issue, I think it only needs a bit of code
> cleanup before it can land into the CFS e2fsprogs patch series.
>       
The block had to be explicitly written because, the extent tree structure has
been changed, now we have a tree with depth 2. And block_iterate_extents has
been called only once so far (original tree was of depth 1). So when
block_iterate_extents returns, the caller updates the inode.i_block, only. 

In ext2fs_extent_split, the new leaf block is written, which contains the
extents. But the ex_leaf fields are updated (It is updated with new blocks in
order to resolve multiply-claimed blocks problem) only after ext2fs_extent_split
returns in block_iterate_extents. 
Hence this block has to be written, again.

Note: eh & ex fields are changed in ext2fs_extents_split, as they should
always
point to last level of the tree i.e depth = 0.