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

RHEL 7.5 compat: FMODE_KABI_ITERATE #7463

Merged
merged 1 commit into from
May 2, 2018

Conversation

behlendorf
Copy link
Contributor

Description

As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure.

This wouldn't have been a problem however in order to maintain
KABI compatibility it was added to the very end of the structure.
Callers intending to use this extended interface were additionally
required to set the FMODE_KABI_ITERATE flag on the file struct
when opening the directory.

Update the ZPL to set this RHEL specific flag when required.
Using this interface will mean that kmods built for RHEL 7.5
will not work on 7.4 without being rebuilt.

Motivation and Context

Issue #7460

How Has This Been Tested?

Local run of the ZTS using the 3.10.0-862.el7.x86_64 kernel.

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.

@sandeen
Copy link

sandeen commented Apr 20, 2018

FYI this is probably OK, but a couple things to note:

  • As you mentioned, this means the new kmod can't work on older RHEL7 (it might be worth checking what does happen if it gets loaded on, say, RHEL7.4, I haven't thought through that).
  • For what it's worth no other disk filesystem shipped in RHEL7 uses ->iterate today, it was added for a different purpose, so YMMV. Not saying it doesn't work, just saying it was tested only for a narrow purpose.

-Eric

@truatpasteurdotfr
Copy link

truatpasteurdotfr commented May 1, 2018

patching both 0.6.5.11 and 0.7.8 with this changes allow 3.10.0-862.el7.x86_64 from CentOS 7.4/CR repository to run in my limited testing.

@behlendorf
Copy link
Contributor Author

@sandeen thanks for taking the time to comment. For all the reasons mentioned above I've gone back and reworked this PR to take the conservative approach and fallback to using the .readdir interface. I've also verified the kmods are in fact compatible with RHEL 7.4, which is nice.

@truatpasteurdotfr any additional testing/verification of the updated PR would be appreciated.

@behlendorf behlendorf requested a review from ofaaland May 1, 2018 21:37
@truatpasteurdotfr
Copy link

behlendorf@1dd2548 could be applied cleanly on both 0.6.5.11 and 0.7.8, but the updated one d177418 only be applied cleanly for 0.7.8.
Updated PR builds and runs fine for my 0.7.8 limited testing.

@truatpasteurdotfr
Copy link

trvial manual cleanup allows to fix the 2 rejections ( ./include/sys/zpl.h.rej and ./module/zfs/zpl_ctldir.c.rej) on 0.6.5.11

@truatpasteurdotfr
Copy link

truatpasteurdotfr commented May 1, 2018

0.6.5.11 patched https://gist.github.com/truatpasteurdotfr/7508b86a692244340f3debeb6ae855d9 seems to work fine too (released version number bumped to .7463.2)

As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#7460
@codecov
Copy link

codecov bot commented May 2, 2018

Codecov Report

Merging #7463 into master will increase coverage by 0.16%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7463      +/-   ##
==========================================
+ Coverage   76.94%   77.11%   +0.16%     
==========================================
  Files         336      336              
  Lines      107139   107139              
==========================================
+ Hits        82436    82617     +181     
+ Misses      24703    24522     -181
Flag Coverage Δ
#kernel 77.63% <91.66%> (+0.02%) ⬆️
#user 66.28% <ø> (+0.29%) ⬆️

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 2c24b5b...983bbc7. Read the comment docs.

@truatpasteurdotfr
Copy link

truatpasteurdotfr commented May 2, 2018

correction :( 0.6.5.11 + patched failed* to enumerate :( -> my bad, upgrading kmod-zfs was not enough, rebooting shows the issue... sorry for the mishap!
summary:
https://gist.github.com/truatpasteurdotfr/f0a1f34b54568475a12e313a663dfefc <- initial version works
https://gist.github.com/truatpasteurdotfr/7508b86a692244340f3debeb6ae855d9 <- latter version fails for me

@behlendorf
Copy link
Contributor Author

@truatpasteurdotfr are you sure? The patch worked as intended for me when applied to 0.6.5.11. I was able to enumerate directories and ran several benchmarks successfully. Perhaps you didn't run autogen.sh after applying the patch when building for 0.6.5.11?

@truatpasteurdotfr
Copy link

Perhaps you didn't run autogen.sh after applying the patch when building for 0.6.5.11?

facepalm... I will add the autogen.sh step and report back later.

Copy link
Contributor

@ofaaland ofaaland left a comment

Choose a reason for hiding this comment

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

LGTM

@truatpasteurdotfr
Copy link

@behlendorf you are right, running autogen.sh after patching yields a 0.6.5.11 running version on 3.10.0-862.el7.x86_64.

@behlendorf behlendorf merged commit 9464b95 into openzfs:master May 2, 2018
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 3, 2018
As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7460 
Closes openzfs#7463
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 3, 2018
As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7460 
Closes openzfs#7463
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 4, 2018
As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7460 
Closes openzfs#7463
tonyhutter pushed a commit to LLNL/zfs that referenced this pull request May 4, 2018
As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7460
Closes openzfs#7463
tonyhutter pushed a commit that referenced this pull request May 10, 2018
As of RHEL 7.5 the mainline fops.iterate() method was added to
the file_operations structure and is correctly detected by the
configure script.

Normally this is what we want, but in order to maintain KABI
compatibility the RHEL change additionally does the following:

* Requires that callers intending to use this extended interface
  set the FMODE_KABI_ITERATE flag on the file structure when
  opening the directory.
* Adds the fops.iterate() method to the end of the structure,
  without removing fops.readdir().

This change updates the configure check to ignore the RHEL 7.5+
variant of fops.iterate() when detected.  Instead fallback to
the fops.readdir() interface which will be available.

Finally, add the 'zpl_' prefix to the directory context wrappers
to avoid colliding with the kernel provided symbols when both
the fops.iterate() and fops.readdir() are provided by the kernel.

Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #7460
Closes #7463
pruiz added a commit to pruiz/zfs that referenced this pull request Aug 27, 2018
@behlendorf behlendorf deleted the issue-7460 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.

5 participants