Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-9723

test large xattr (ea_inode) patch interoperability

Details

    • Task
    • Resolution: Fixed
    • Minor
    • None
    • None
    • None
    • 9223372036854775807

    Description

      The large xattr (ea_inode) feature patches are being merged into the upstream 4.13 kernel, but additional changes are being built on top of the EA inode functionality that we use in the ldiskfs patch series. In particular, in the upstream patch series the EA inodes can be shared among multiple parent inodes, and instead of having a backpointer to the parent inode/generation the shared inodes have a refcount and a hash of the xattr value to verify that the correct xattr is referenced. Once we update to a newer vendor kernel that includes these changes, we can remove the patch from ldiskfs to reduce ongoing maintenance efforts, but we can't wait until that time to verify that the feature works properly with existing Lustre filesystems.

      There is supposed to be interoperability functionality in the upstream feature to allow access to existing Lustre EA inodes. We need to test that the ext4 patches being landed to the upstream kernel are able to work with xattrs created by Lustre, and that the upstream e2fsprogs will not consider Lustre EA inodes to be corrupt. This testing needs to be done in the next week or two, to ensure that we can feed back any issues to the upstream ext4 maintainers before that feature is released in an upstream kernel.

      For testing, something like the following process should be sufficient:

      • create an MDT filesystem with an existing RHEL7 kernel with --mkfsoptions="-O ea_inode"
      • mount filesystem as type lustre
      • create large user.test xattrs via setfattr (up to 64KiB) with verifiable data (e.g. filename repeated many times)
      • dump all xattrs via getfattr -d -m user.test testfiles > xattrs.lustre
      • upgrade the kernel to upstream kernel
      • disable the dirdata feature via debugfs -w -R "feature ^dirdata" /dev/MDT and mount it as type ext4
      • dump all xattrs via getfattr -d -m user.test testfiles > xattrs.ext4 and compare to xattrs.lustre to verify xattr consistency and ensure that no errors are generated by the kernel
      • run new e2fsck on updated filesystem to verify that it does not consider the EA inodes as corrupted, at worst it should offer to fix the refcount and hash values of those inodes

      The upstream kernel patches are included on the dev branch of https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git (all of the patches from author "Tahsin Erdogan") but could potentially also be pushed as a series to the fs/linux-staging branch (with Test-Parameters: forbuildonly since there is no benefit to Lustre testing on them) in order to have Jenkins build the patches for testing via loadjenkinsbuild.

      Attachments

        1. e2fsck_new.log
          2 kB
        2. e2fsck.log
          2 kB
        3. xattrs_after_new_e2fs.lustre
          64 kB
        4. xattrs.ext4
          64 kB
        5. xattrs.lustre
          64 kB

        Issue Links

          Activity

            [LU-9723] test large xattr (ea_inode) patch interoperability

            Many of the features in e2fsprogs-1.42.13 have been merged into e2fsprogs-1.44. The main exception is the dirdata patch. The remaining patches are mostly .spec files for different distros and functional improvements that do not affect the on-disk format. It may be that we could use an unpatched e2fsprogs-1.44 on the OST, but not yet on the MDT (this has not been tested).

            adilger Andreas Dilger added a comment - Many of the features in e2fsprogs-1.42.13 have been merged into e2fsprogs-1.44. The main exception is the dirdata patch. The remaining patches are mostly .spec files for different distros and functional improvements that do not affect the on-disk format. It may be that we could use an unpatched e2fsprogs-1.44 on the OST, but not yet on the MDT (this has not been tested).

            Actually Ubuntu e2fsprogs is at version 1.44 so it has support for ea_inode already. Its Ubuntu16 that is lacking. BTW how much a difference is their between lustre e2fsprogs version and the normal e2fsprog 1.44 version?

            simmonsja James A Simmons added a comment - Actually Ubuntu e2fsprogs is at version 1.44 so it has support for ea_inode already. Its Ubuntu16 that is lacking. BTW how much a difference is their between lustre e2fsprogs version and the normal e2fsprog 1.44 version?

            We are updating to e2fsprogs-1.44 for the 2.12 release, so we will now be using the upstream ea_inode code in e2fsprogs at least. At some point, maybe Ubuntu 18, we will also get the ea_inode feature in ldiskfs, so that will be tested as part of those releases.

            adilger Andreas Dilger added a comment - We are updating to e2fsprogs-1.44 for the 2.12 release, so we will now be using the upstream ea_inode code in e2fsprogs at least. At some point, maybe Ubuntu 18, we will also get the ea_inode feature in ldiskfs, so that will be tested as part of those releases.
            emoly.liu Emoly Liu added a comment -

            This patch has been landed to ext4 upstream:

            commit 191eac33009e6a6d31e87cfa425a20d0e79704b4
            Author: Emoly Liu <emoly.liu@intel.com>
            Date:   Mon Jul 31 00:40:22 2017 -0400
            
                ext4: error should be cleared if ea_inode isn't added to the cache
            
                For Lustre, if ea_inode fails in hash validation but passes parent
                inode and generation checks, it won't be added to the cache as well
                as the error "-EFSCORRUPTED" should be cleared, otherwise it will
                cause "Structure needs cleaning" when running getfattr command.
            
                Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9723
            
                Cc: stable@vger.kernel.org
                Fixes: dec214d00e0d78a08b947d7dccdfdb84407a9f4d
                Signed-off-by: Emoly Liu <emoly.liu@intel.com>
                Signed-off-by: Theodore Ts'o <tytso@mit.edu>
                Reviewed-by: Andreas Dilger <adilger@dilger.ca>
                Reviewed-by: tahsin@google.com
            
            emoly.liu Emoly Liu added a comment - This patch has been landed to ext4 upstream: commit 191eac33009e6a6d31e87cfa425a20d0e79704b4 Author: Emoly Liu <emoly.liu@intel.com> Date: Mon Jul 31 00:40:22 2017 -0400 ext4: error should be cleared if ea_inode isn't added to the cache For Lustre, if ea_inode fails in hash validation but passes parent inode and generation checks, it won't be added to the cache as well as the error "-EFSCORRUPTED" should be cleared, otherwise it will cause "Structure needs cleaning" when running getfattr command. Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9723 Cc: stable@vger.kernel.org Fixes: dec214d00e0d78a08b947d7dccdfdb84407a9f4d Signed-off-by: Emoly Liu <emoly.liu@intel.com> Signed-off-by: Theodore Ts'o <tytso@mit.edu> Reviewed-by: Andreas Dilger <adilger@dilger.ca> Reviewed-by: tahsin@google.com

            I just talked with Ted, and the patch will be included into the next release. 

            adilger Andreas Dilger added a comment - I just talked with Ted, and the patch will be included into the next release. 
            emoly.liu Emoly Liu added a comment -

            Done.

            emoly.liu Emoly Liu added a comment - Done.
            emoly.liu Emoly Liu added a comment -

            adilger, do you mean ext4 "dev" branch? There is no "next" branch at https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git

            -sh-4.1$ git branch -a
              dev
            * master
              remotes/origin/3.2-punch-fix
              remotes/origin/HEAD -> origin/master
              remotes/origin/backport-to-3.10
              remotes/origin/crypto
              remotes/origin/crypto-3.14
              remotes/origin/dev
              remotes/origin/ext4-tools
              remotes/origin/for-stable
              remotes/origin/fscrypt
              remotes/origin/lazy_journal
              remotes/origin/master
              remotes/origin/origin
              remotes/origin/test
              remotes/origin/test-mb_generate_buddy-failure
              remotes/origin/unstable
            
            emoly.liu Emoly Liu added a comment - adilger , do you mean ext4 "dev" branch? There is no "next" branch at https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git -sh-4.1$ git branch -a dev * master remotes/origin/3.2-punch-fix remotes/origin/HEAD -> origin/master remotes/origin/backport-to-3.10 remotes/origin/crypto remotes/origin/crypto-3.14 remotes/origin/dev remotes/origin/ext4-tools remotes/origin/for-stable remotes/origin/fscrypt remotes/origin/lazy_journal remotes/origin/master remotes/origin/origin remotes/origin/test remotes/origin/test-mb_generate_buddy-failure remotes/origin/unstable

            Emoly, can you please make a patch (git commit) for this on the ext4 "next" branch, with a proper commit comment with Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9723 and Signed-off-by: line, and then send it to the list with "git send-email --to tytso@mit.edu --cc linux-ext4@vger.kernel.org". This should be done as soon as possible so that it will be included with the patch series going upstream.

            adilger Andreas Dilger added a comment - Emoly, can you please make a patch (git commit) for this on the ext4 "next" branch, with a proper commit comment with Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9723 and Signed-off-by: line, and then send it to the list with " git send-email --to tytso@mit.edu --cc linux-ext4@vger.kernel.org ". This should be done as soon as possible so that it will be included with the patch series going upstream.
            emoly.liu Emoly Liu added a comment -

            I just uploaded the latest testing results here.

            emoly.liu Emoly Liu added a comment - I just uploaded the latest testing results here.
            emoly.liu Emoly Liu added a comment -

            adilger, with the following fix, all the tests can pass.

            diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
            index ce12c3f..c7876c2 100644
            --- a/fs/ext4/xattr.c
            +++ b/fs/ext4/xattr.c
            @@ -451,6 +451,7 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
                            }
                            /* Do not add ea_inode to the cache. */
                            ea_inode_cache = NULL;
            +               err = 0;
                    } else if (err)
                            goto out;
            
            emoly.liu Emoly Liu added a comment - adilger , with the following fix, all the tests can pass. diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index ce12c3f..c7876c2 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -451,6 +451,7 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino, } /* Do not add ea_inode to the cache. */ ea_inode_cache = NULL; + err = 0; } else if (err) goto out;

            People

              emoly.liu Emoly Liu
              adilger Andreas Dilger
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: