Skip to content

Conversation

@behlendorf
Copy link
Contributor

@behlendorf behlendorf commented May 11, 2018

Description

In CentOS 7.5 the kernel provided a compatibility wrapper to support O_TMPFILE. This results in the test setup script correctly detecting kernel support. But the ZFS module was built without O_TMPFILE support due to the non-standard CentOS kernel interface.

Handle this case by updating the setup check to fail either when the kernel or the ZFS module fail to provide support. The reason will be clearly logged in the test results.

Motivation and Context

Fix test failures with CentOS 7.5.

How Has This Been Tested?

Locally on CentOS 7.5. Pending results from bots on other distributions.

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)
  • Documentation (a change to man pages or other documentation)

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.

@behlendorf behlendorf requested a review from tuxoko May 11, 2018 18:08
@codecov
Copy link

codecov bot commented May 11, 2018

Codecov Report

Merging #7528 into master will increase coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7528      +/-   ##
==========================================
+ Coverage   77.43%   77.46%   +0.02%     
==========================================
  Files         336      336              
  Lines      107521   107524       +3     
==========================================
+ Hits        83263    83295      +32     
+ Misses      24258    24229      -29
Flag Coverage Δ
#kernel 77.9% <ø> (-0.11%) ⬇️
#user 66.58% <0%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b669ab8...3819321. Read the comment docs.

* support O_TMPFILE.
*/
if (errno == EISDIR) {
if (errno == ENOENT || errno == EOPNOTSUPP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

http://man7.org/linux/man-pages/man2/open.2.html

One must check for two different error codes, EISDIR and ENOENT, when trying to determine whether the kernel supports O_TMPFILE functionality.

I don't know why I didn't check for both EISDIR and ENOENT, but we shouldn't check for EOPNOTSUPP because that flag means kernel supports it, but filesystem doesn't. And as long as kernel supports, we should not skip the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I should have dug a little deeper before assuming the new CentOS 7.5 kernel didn't support it. Apparently it does, in a non-standard way, to preserve the existing KABI. We could probably update ZFS to use the extended interface with a little work. But it would probably be better for compatibility to add a way to make it clear we know that the kernel interface is available but we've intentionally decided to not use it.

@tuxoko would you mind taking over this issue and sorting it out? I'll just close this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@behlendorf
Can you point out where I can find about what CentOS does differently than others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DISK=${DISKS%% *}
default_setup_noexit $DISK

if ! $STF_SUITE/tests/functional/tmpfile/tmpfile_test $TESTDIR/tmpfile; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Is $TESTDIR/tmpfile a file or directory?
O_TMPFILE should take a directory as pathname argument.

In CentOS 7.5 the kernel provided a compatibility wrapper to support
O_TMPFILE.  This results in the test setup script correctly detecting
kernel support.  But the ZFS module was built without O_TMPFILE
support due to the non-standard CentOS kernel interface.

Handle this case by updating the setup check to fail either when
the kernel or the ZFS module fail to provide support.  The reason
will be clearly logged in the test results.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf
Copy link
Contributor Author

@tuxoko I've refreshed this PR so the tests are skipped if either the kernel or ZFS filesystem lackO_TMPFILE support for whatever reason.

I looked in to updating the kernel code to use the new O_TMPFILE interface and while it wouldn't be technically difficult, it would be very very ugly so in the end I decided against it.

@tuxoko
Copy link
Contributor

tuxoko commented May 14, 2018

@behlendorf
I see, so basically CentOS 7.5 add support for O_TMPFILE in older kernel using some weird hack that is not compatible with what's in new kernel. I'm OK with not supporting that.

@behlendorf behlendorf merged commit 8c64fe0 into openzfs:master May 15, 2018
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 6, 2018
In CentOS 7.5 the kernel provided a compatibility wrapper to support
O_TMPFILE.  This results in the test setup script correctly detecting
kernel support.  But the ZFS module was built without O_TMPFILE
support due to the non-standard CentOS kernel interface.

Handle this case by updating the setup check to fail either when
the kernel or the ZFS module fail to provide support.  The reason
will be clearly logged in the test results.

Reviewed-by: Chunwei Chen <tuxoko@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7528
tonyhutter pushed a commit that referenced this pull request Nov 13, 2018
In CentOS 7.5 the kernel provided a compatibility wrapper to support
O_TMPFILE.  This results in the test setup script correctly detecting
kernel support.  But the ZFS module was built without O_TMPFILE
support due to the non-standard CentOS kernel interface.

Handle this case by updating the setup check to fail either when
the kernel or the ZFS module fail to provide support.  The reason
will be clearly logged in the test results.

Reviewed-by: Chunwei Chen <tuxoko@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #7528
@behlendorf behlendorf deleted the tmpfile branch April 19, 2021 19:30
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.

2 participants