Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linux 4.9 compat: fix zfs_ctldir xattr handling #6189

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Jun 3, 2017

Description

Since torvalds/linux@d0a5b99 IOP_XATTR is used to indicate the inode has xattr support: we should clear it for the ctldir inodes to avoid IO errors.

Motivation and Context

On Linux 4.9+ ls -la on the ".zfs" directory produces IO errors:

root@linux:~# uname -r
4.9.0
root@linux:~# POOLNAME='testpool'
root@linux:~# TMPDIR='/var/tmp'
root@linux:~# mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
root@linux:~# zpool destroy $POOLNAME
root@linux:~# rm -f $TMPDIR/zpool.dat
root@linux:~# fallocate -l 128m $TMPDIR/zpool.dat
root@linux:~# zpool create -O mountpoint=/tmp/$POOLNAME $POOLNAME $TMPDIR/zpool.dat
root@linux:~# zfs create $POOLNAME/fs1
root@linux:~# zfs snap $POOLNAME@snap1
root@linux:~# ls -lah /tmp/$POOLNAME/.zfs/
ls: /tmp/testpool/.zfs/: Input/output error
ls: /tmp/testpool/.zfs/.: Input/output error
ls: /tmp/testpool/.zfs/snapshot: Input/output error
ls: /tmp/testpool/.zfs/shares: Input/output error
total 512
drwxrwxrwx 1 root root 0 Jun  3 11:56 .
drwxr-xr-x 3 root root 3 Jun  3 11:56 ..
drwxrwxrwx 2 root root 2 Jun  3 11:56 shares
drwxrwxrwx 2 root root 2 Jun  3 11:56 snapshot
root@linux:~# strace -e trace=lgetxattr ls -lah /tmp/$POOLNAME/.zfs/
lgetxattr("/tmp/testpool/.zfs/", "security.selinux", 0x249dd90, 255) = -1 EIO (Input/output error)
ls: /tmp/testpool/.zfs/: Input/output error
lgetxattr("/tmp/testpool/.zfs/.", "security.selinux", 0x24a8aa0, 255) = -1 EIO (Input/output error)
ls: /tmp/testpool/.zfs/.: Input/output error
lgetxattr("/tmp/testpool/.zfs/..", "security.selinux", 0x24a8ae0, 255) = -1 ENODATA (No data available)
lgetxattr("/tmp/testpool/.zfs/snapshot", "security.selinux", 0x24a8b00, 255) = -1 EIO (Input/output error)
ls: /tmp/testpool/.zfs/snapshot: Input/output error
lgetxattr("/tmp/testpool/.zfs/shares", "security.selinux", 0x24a8b20, 255) = -1 EIO (Input/output error)
ls: /tmp/testpool/.zfs/shares: Input/output error
total 512
drwxrwxrwx 1 root root 0 Jun  3 11:56 .
drwxr-xr-x 3 root root 3 Jun  3 11:56 ..
drwxrwxrwx 2 root root 2 Jun  3 11:56 shares
drwxrwxrwx 2 root root 2 Jun  3 11:56 snapshot
+++ exited with 0 +++
root@linux:~# 

How Has This Been Tested?

Manual testing. We could also add a new test to the ZTS in "functional/xattr".

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@mention-bot
Copy link

@loli10K, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @tuxoko and @ahrens to be potential reviewers.

@loli10K loli10K requested a review from tuxoko June 3, 2017 10:02
@@ -492,6 +492,9 @@ zfsctl_inode_alloc(zfsvfs_t *zfsvfs, uint64_t id,
ip->i_ctime = now;
ip->i_fop = fops;
ip->i_op = ops;
#ifndef HAVE_GENERIC_SETXATTR
ip->i_opflags = ~IOP_XATTR;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find, yes we should definitely do this but I'd suggest adding this same logic a little differently. The following has a few advantages.

  • Checking for IOP_XATTR instead of HAVE_GENERIC_SETXATTR ensures we're depending on exactly this interface change. That way in the unlikely case this change gets cherry picked in to an older kernel this will still work correctly.

  • Using &= ~IOP_XATTR just clears the one bit we care about.

#if defined(IOP_XATTR)
        ip->i_opflags &= ~IOP_XATTR;
#endif

Since torvalds/linux@d0a5b99 IOP_XATTR is used to indicate the inode
has xattr support: clear it for the ctldir inodes to avoid EIO errors.

Signed-off-by: loli10K <[email protected]>
@loli10K loli10K force-pushed the linux-4.9-compat_iop_xattr branch from 882cb82 to e623855 Compare June 5, 2017 05:42
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. And the test case failures are unrelated.

@behlendorf behlendorf merged commit 9f7b066 into openzfs:master Jun 5, 2017
@loli10K loli10K deleted the linux-4.9-compat_iop_xattr branch June 5, 2017 19:40
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 5, 2017
Since torvalds/linux@d0a5b99 IOP_XATTR is used to indicate the inode
has xattr support: clear it for the ctldir inodes to avoid EIO errors.

Reviewed-by: Chunwei Chen <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes openzfs#6189
tonyhutter pushed a commit that referenced this pull request Jun 9, 2017
Since torvalds/linux@d0a5b99 IOP_XATTR is used to indicate the inode
has xattr support: clear it for the ctldir inodes to avoid EIO errors.

Reviewed-by: Chunwei Chen <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes #6189
@loli10K loli10K restored the linux-4.9-compat_iop_xattr branch November 1, 2018 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants