Linux 5.3 compat #9029

behlendorf · 2019-07-12T22:06:56Z

Motivation and Context

Resolve Linux 5.3 build failures. @kusumi would you mind reviewing this.

Description

This PR contains three changes but only fa0047f is required to resolve the build issue with 5.3. The other two changes were included as additional cleanup in order to avoid potential future problems. Over the years the rwsems have seen numerous changes and going forward it's desirable to be more resilient to kernel changes.

fa0047f Linux 5.3 compat: rw_semaphore owner
e4d1839 Linux 5.3 compat: retire rw_tryupgrade()
4459f2b Retire unused spl_{mutex,rwlock}_{init_fini}

How Has This Been Tested?

Locally ran portions of the ZTS using the 5.3.0-0.rc0.git4.1.vanilla.knurd.1.fc30.x86_64 kernel. Pending CI results to verify other kernels. No significant performance regression was observed after disabling rw_tryupgrade.

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.

Commit torvalds/linux@94a9717b updated the rwsem's owner field to contain additional flags describing the rwsem's state. Rather then update the wrappers to mask out these bits, the code no longer relies on the owner stored by the kernel. This does increase the size of a krwlock_t but it makes the implementation less sensitive to future kernel changes. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

The Linux kernel's rwsem's have never provided an interface to allow a reader to be upgraded to a writer. Historically, this functionality has been implemented by a SPL wrapper function. However, this approach depends on internal knowledge of the rw_semaphore and is therefore rather brittle. Since the ZFS code must always be able to fallback to rw_exit() and rw_enter() when an rw_tryupgrade() fails; this functionality isn't critical. Therefore, it is being retired to make the build more robust and to simplify the rwlock implementation. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

These functions are unused and can be removed along with the spl-mutex.c and spl-rwlock.c source files. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

ahrens · 2019-07-12T22:35:33Z

<3 the code simplification here, but have you evaluated the potential performance implications of never upgrading rwlocks?

behlendorf · 2019-07-13T00:07:29Z

have you evaluated the potential performance implications of never upgrading rwlocks?

Yes, via code inspection of the callers. There are 4 callers of rw_tryupgrade(), of which I believe only one, dmu_zfetch(), may have an impact on performance. They are:

dmu_zfetch() - When creating a new zstream in dmu_zfetch(), if the code is unable to upgrade the zf->zf_rwlock the dnode's dn_struct_rwlock is dropped and reacquired, then the code rescans for a matching prefetch stream. This behavior could be improved.
zap_tryupgradedir() - Most callers already hold the lock as a writer, and zap upgrades aren't expected to be a high frequency events so any impact here is expected to be minimal.
dnode_new_blkid() - Negligible (if any) change in behavior, on upgrade failure the read lock is dropped and immediately acquired as a writer.
zfs_fuid_find_by_domain() - Results in one additional call to avl_find() on failure, only reachable from zfs_set_userquota().

If you can suggest a test case to stress dmu_zfetch() I'm happy to perform the testing. Though, I'd like to address any loss in performance by updating the caller and not re-introducing rw_tryupgrade().

codecov · 2019-07-13T05:58:14Z

Codecov Report

Merging #9029 into master will increase coverage by 0.15%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master    #9029      +/-   ##
==========================================
+ Coverage   78.62%   78.78%   +0.15%     
==========================================
  Files         401      399       -2     
  Lines      120158   120145      -13     
==========================================
+ Hits        94476    94658     +182     
+ Misses      25682    25487     -195

Flag	Coverage Δ
#kernel	`79.5% <45.45%> (+0.09%)`	⬆️
#user	`66.51% <ø> (+0.25%)`	⬆️

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 e5db313...4459f2b. Read the comment docs.

kusumi

Can't make any comment on impact of Linux 5.3 compat: retire rw_tryupgrade(), but Linux 5.3 compat: rw_semaphore owner (which addresses #9023) lgtm.

ahrens · 2019-07-15T18:03:04Z

@behlendorf A good workload to exercise this is random reads or writes of 8k blocks (on recordsize=8k) with several threads (>=8 threads on an 8-CPU system worked well for me).

I happen to be working on some changes to the dmu_zfetch locking anyway (because even the current rwlock implementation has extreme performance pathologies). So it's probably OK if this regresses performance of that somewhat.

behlendorf · 2019-07-17T21:55:01Z

@ahrens using the suggested workload (random 8k reads, 8k recordsize) I compared performance with and without this PR. Tested using an ec2 c5d.xlarge instance type (4 vcpu, local NVMe instance storage, Amazon Linux 2, 4.14.128-112.105.amzn2.x86_64 kernel). Based on these quick results I don't see any appreciable impact on performance.

processes	master (MiB/s)	patched (MiB/s)
1	52.9	52.3
2	109.0	109.5
4	189.9	191.1
8	188.3	186.7
16	186.9	187.4
32	189.2	191.9
64	190.3	188.4
128	189.8	188.6
256	191.0	190.1
512	195.2	192.8

8k-read.fio

[global]
filename=file
group_reporting=1
fallocate=0
overwrite=0
rw=randread
directory=/tank/fs/
runtime=10
bs=8192
ioengine=libaio
iodepth=1
sync=0
direct=1
size=32G
numjobs=${JOBS}
[job]

Note: I also didn't observe any pathological degradation of perform when threads > ncpus.

The Linux kernel's rwsem's have never provided an interface to allow a reader to be upgraded to a writer. Historically, this functionality has been implemented by a SPL wrapper function. However, this approach depends on internal knowledge of the rw_semaphore and is therefore rather brittle. Since the ZFS code must always be able to fallback to rw_exit() and rw_enter() when an rw_tryupgrade() fails; this functionality isn't critical. Furthermore, the only potentially performance sensitive consumer is dmu_zfetch() and no decrease in performance was observed with this change applied. See the PR comments for additional testing details. Therefore, it is being retired to make the build more robust and to simplify the rwlock implementation. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9029

These functions are unused and can be removed along with the spl-mutex.c and spl-rwlock.c source files. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #9029

Commit torvalds/linux@94a9717b updated the rwsem's owner field to contain additional flags describing the rwsem's state. Rather then update the wrappers to mask out these bits, the code no longer relies on the owner stored by the kernel. This does increase the size of a krwlock_t but it makes the implementation less sensitive to future kernel changes. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9029

The Linux kernel's rwsem's have never provided an interface to allow a reader to be upgraded to a writer. Historically, this functionality has been implemented by a SPL wrapper function. However, this approach depends on internal knowledge of the rw_semaphore and is therefore rather brittle. Since the ZFS code must always be able to fallback to rw_exit() and rw_enter() when an rw_tryupgrade() fails; this functionality isn't critical. Furthermore, the only potentially performance sensitive consumer is dmu_zfetch() and no decrease in performance was observed with this change applied. See the PR comments for additional testing details. Therefore, it is being retired to make the build more robust and to simplify the rwlock implementation. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9029

These functions are unused and can be removed along with the spl-mutex.c and spl-rwlock.c source files. Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#9029