Skip to content

Conversation

@behlendorf
Copy link
Contributor

Description

The RT rwsem implementation was changed to allow multiple readers
as of the 4.9.20-rt16 patch set. This results in a build failure
because the existing implementation was forced to directly access
the rwsem strucgture which has changed.

While this could be accommodated by adding additional compatibility
code. This patch resolves the build issue by simply assuming the
rwsem can never be upgraded. This functionality is a performance
optimization and all callers must already handle this case.

Motivation and Context

openzfs/spl#611

How Has This Been Tested?

Locally built, which isn't saying much. But it should work @clefru, @kernelOfTruth, or @steven-omaha would you be able to test that this resolves the build issue.

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.

@steven-omaha
Copy link

steven-omaha commented Jun 1, 2018

Awesome! Thanks for working on this. I will build it on one of my machines next week.

@steven-omaha
Copy link

steven-omaha commented Jun 2, 2018

@behlendorf I checked your patch on one of my machines, but it didn't work for me. Maybe I didn't do the right things, but I'll try to explain as best as I can. Sorry if I did something stupid, this is my first time fiddling around with these things.

This is on Arch Linux. Here ZFS is a user-maintained package that is actually divided into 4 packages spl-dkms, spl-utils, zfs-dkms, zfs-utils. I used the latest stable release for spl (0.7.9), and applied your patch, see the build-file here. Specifically, I added the lines 15 and 22 to fetch and apply your patch. I checked to build directory to verify that the patch was actually applied.

Building the package works, and building the dkms-modules for the vanilla-, lts- and zen-kernel works, but not for the rt-kernel.

sudo dkms install spl/0.7.9 -k 4.16.8-rt3-1-rt leads to the output here. (/var/lib/dkms/spl/0.7.9/build/make.log)

I can make further tests, just tell me.

@behlendorf
Copy link
Contributor Author

@steven-omaha thanks for testing. When I get a chance, I'll give this a test run with the Arch Linux RT kernel which looks like a convenient way to test this.

@behlendorf
Copy link
Contributor Author

@steven-omaha I've updated this PR and addressed the issue you likely encountered. Without this change I was able to reproduce the reported build issue when using the Debian 4.9.0-7-rt-amd64 kernel. With the change applied I'm able to build cleanly. Sorry about the delay.

@behlendorf behlendorf requested a review from kernelOfTruth July 27, 2018 23:19
The RT rwsem implementation was changed to allow multiple readers
as of the 4.9.20-rt16 patch set.  This results in a build failure
because the existing implementation was forced to directly access
the rwsem structure which has changed.

While this could be accommodated by adding additional compatibility
code.  This patch resolves the build issue by simply assuming the
rwsem can never be upgraded.  This functionality is a performance
optimization and all callers must already handle this case.

Converting the last remaining use of __SPIN_LOCK_UNLOCKED to
spin_lock_init() was additionally required to get a clean build.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@steven-omaha
Copy link

steven-omaha commented Jul 27, 2018

@behlendorf Thanks for working on this. I tried to build it, but to no avail. I think the error is on my side. I'm not really familiar with building projects as complex as zfs / spl. I'm using the same building steps again (see above), but I'm now patching the source using your commit a38d724. It doesn't matter if I try this from the last tagged version, or the recent master. I always get failed patching.

patching file /tmp/makepkg/spl-dkms/src/spl/module/spl/spl-rwlock.c
Hunk #1 FAILED at 172.
Hunk #2 FAILED at 188.
2 out of 2 hunks FAILED -- saving rejects to file /tmp/makepkg/spl-dkms/src/spl/module/spl/spl-rwlock.c.rej
patching file /tmp/makepkg/spl-dkms/src/spl/module/spl/spl-rwlock.c
Hunk #1 succeeded at 34 (offset -1 lines).
Hunk #2 succeeded at 67 (offset -1 lines).
patching file /tmp/makepkg/spl-dkms/src/spl/module/spl/spl-rwlock.c
Hunk #1 FAILED at 744.
1 out of 1 hunk FAILED -- saving rejects to file /tmp/makepkg/spl-dkms/src/spl/module/spl/spl-rwlock.c.rej

EDIT: If you could help me out with this issue, I'm more than willing to test this on my machine.

@behlendorf
Copy link
Contributor Author

@steven-omaha try this. Instead of wrangling the patch and manually applying it you can download the pre-patched source as a tarball from github like this.

curl https://codeload.github.com/zfsonlinux/zfs/legacy.tar.gz/a38d724 | tar -xz
cd zfsonlinux-zfs-a38d724
sh autogen.sh
./configure
make

@codecov
Copy link

codecov bot commented Jul 28, 2018

Codecov Report

Merging #7589 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7589      +/-   ##
==========================================
- Coverage   78.25%   78.16%   -0.09%     
==========================================
  Files         368      368              
  Lines      111981   111981              
==========================================
- Hits        87628    87529      -99     
- Misses      24353    24452      +99
Flag Coverage Δ
#kernel 78.74% <100%> (-0.01%) ⬇️
#user 67.24% <ø> (-0.05%) ⬇️

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 fb7307b...a38d724. Read the comment docs.

@steven-omaha
Copy link

@behlendorf I checked this on Arch using 4.16.18-rt9, and I could compile it using your steps.

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