[LU-7267] e2fsck kill all files with Large EA.

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.8.0
• Fix Version/s: Lustre 2.8.0
• Type: Bug
• Priority: Blocker
• Reporter: Alexey Lyashkov
• Assignee: Niu Yawei (Inactive)
• Resolution: Fixed

Environment

e2fsck 1.42.12 (and looks 1.42.13)

• Severity: 3

Description

e2fsck start a killing any Large EA enabled files after wrong merging in 1.42.9 e2fsprogs.
after it point any run e2fsck produce an output

Pass 1: Checking inodes, blocks, and sizes
Extended attribute in inode 755994480 has a value size (5674) which is invalid

fix is simple - just restore old behavior in pass1.c : check_ea_in_inode() function.

                /* check value size */
-                if (entry->e_value_size > remain) {
+               if (entry->e_value_size == 0) {
                        pctx->num = entry->e_value_size;
                        problem = PR_1_ATTR_VALUE_SIZE;
                        goto fix;
                }
 
                if (entry->e_value_inum == 0) {
                        /* check value size */
                        if (entry->e_value_size > remain) {
                                pctx->num = entry->e_value_size;
                                problem = PR_1_ATTR_VALUE_SIZE;
                                goto fix;
                        }

Comments

[Andreas Dilger]

There also needs to be an e2fsprogs test case for large xattrs. I'm really surprised that there isn't already a test case that would have caught this regression.

[Joseph Gmitter (Inactive)]

Hi Niu,
Can you have a look at this one?
Thanks.
Joe

[Alexey Lyashkov]

was wrong about checking to zero, we need a kill first check completely. corresponded commit in upstream.

Author: Eric Sandeen <[e-mail]>
Date:   Thu Apr 25 00:14:33 2013 -0400

    e2fsprogs: allow 0-length xattr values in e2fsck
    
    e2fsck thinks that this:
    
    # touch mnt/testfile1
    # setfattr -n "user.test" mnt/testfile1
    
    results in a filesystem with corruption:
    
    Pass 1: Checking inodes, blocks, and sizes
    Extended attribute in inode 12 has a value size (0) which is invalid
    Clear? yes
    
    but as far as I can tell, there is absolutely nothing wrong with
    a 0-length value on an extended attribute.  Just remove the check.
    
    Reported-by: David Shaw <[e-mail]>
    Reported-by: Harald Reindl <[e-mail]>
    Addresses-Red-Hat-Bugzilla: #557959
    Signed-off-by: Eric Sandeen <[e-mail]>
    Signed-off-by: Theodore Ts'o <[e-mail]>

[Niu Yawei (Inactive)]

There must be a wrong merge at some point which created this duplicated check, probably when merging the 10fc3a63d9b7efb14e810ee94ad1d2f254d44eae (the fix mentioned by Alexey above)

               /* check value size */
                if (entry->e_value_size > remain) {
                        pctx->num = entry->e_value_size;
                        problem = PR_1_ATTR_VALUE_SIZE;
                        goto fix;
                }

                if (entry->e_value_inum == 0) {
                        /* check value size */
                        if (entry->e_value_size > remain) {
                                pctx->num = entry->e_value_size;
                                problem = PR_1_ATTR_VALUE_SIZE;
                                goto fix;
                        }
                } else {
                        int ret, tmp;

                        ret = check_large_ea_inode(ctx, entry, pctx, &tmp);
                        if (ret == 0)
                                mark_inode_ea_map(ctx, pctx,
                                                  entry->e_value_inum);
                }

Obviously, the duplicated check above the " if (entry->e_value_inum == 0) " should be completely removed.

[Niu Yawei (Inactive)]

Andreas, I found that there is a test for the large xattr:

commit c204ac83b8889a81d8aeb2ea421ca14db796e5c6
Author: Andreas Dilger <[e-mail]>
Date:   Fri Apr 13 02:14:16 2012 -0600

    tests: verify large xattr inode support

    Verify that inodes with large EAs in a secondary inode are working:
    * EA inode needs to have EA_INODE_FL set
    * EA inode should reference parent inode number+generation

    Signed-off-by: Kalpak Shah <[e-mail]>
    Signed-off-by: Andreas Dilger <[e-mail]>

But when I check into the image file under f_large_ea, seems none of the files have any xattr set (getfattr can't list any ea entries), and the inode size in the superblock is 128 (which is EXT2_GOOD_OLD_INODE_SIZE), so the check_ea_in_inode() won't be called at all.

The troublesome here is that EA can be set on fs with 128 inode size (with EA_INODE enabled)? Looks a lot more changes are required to check large ea inode. (see check_inode_extra_space(), check_ea_in_inode()).

[Niu Yawei (Inactive)]

I was wrong about the large ea tests, the ea inode was checked when checking extend attribute blocks, so just removing the duplicated checking code in check_ea_in_inode() should be enough.

[Gerrit Updater]

Niu Yawei ([e-mail]) uploaded a new patch: http://review.whamcloud.com/16779
Subject: LU-7267 e2fsck: remove duplicated ea value size check
Project: tools/e2fsprogs
Branch: master-lustre
Current Patch Set: 1
Commit: d2498fe35d2a12ef085f865f2edc9350a8b47f2b

[Andreas Dilger]

Niu, I was looking at the f_large_ea/image file, and it does appear to have large xattrs:

e2fsck -fn image
:
:
Pass 1: Checking inodes, blocks, and sizes
Inode 12 has invalid extended attribute. EA inode 79 missing EA_INODE flag.
Clear? yes

Invalid backpointer from extended attribute inode 19 to parent inode 13.
Clear? yes

Inode 14 has invalid extended attribute. EA inode 20 missing EA_INODE flag.
Clear? yes

debugfs:  stat <19>
Inode: 19   Type: regular    Mode:  0600   Flags: 0x280000
Generation: 3943742063    Version: 0x00000001
User:     0   Group:     0   Size: 65536
File ACL: 0    Directory ACL: 0
Links: 1   Blockcount: 128
Fragment:  Address: 0    Number: 0    Size: 0
ctime: 0x49193fdf -- Tue Nov 11 01:18:39 2008
atime: 0x49193fdf -- Tue Nov 11 01:18:39 2008
mtime: 0x0000000f -- Wed Dec 31 17:00:15 1969
EXTENTS:
(0-14):1567-1581, (15-63):1261-1309

[Gerrit Updater]

Andreas Dilger ([e-mail]) merged in patch http://review.whamcloud.com/16779/
Subject: LU-7267 e2fsck: remove duplicated ea value size check
Project: tools/e2fsprogs
Branch: master-lustre
Current Patch Set:
Commit: 709edd7febe7831c01c406cc1552b7c288502a8b

[Andreas Dilger]

Patch is landed, but still need to make a new e2fsprogs release with this patch included.

[Andreas Dilger]

Close this ticket, as we also need to land LU-7368 as well and we can use that for tracking.