Accumulated fixes for 5.10 kernel-introduced build breakages #11085

ckane · 2020-10-18T17:40:40Z

Motivation and Context

The latest pulls ahead of the Linux 5.10 release cycle introduced some API changes that break the build. In particular check_disk_change() and revalidate_disk() have been removed, and the "count" variable for the percpu_ref data structure has now been moved into a child data structure of type percpu_ref_data.

Description

I have replaced revalidate_disk() with revalidate_disk_size() where the former is unavailable, as the context in which it is called within the driver suggests that the latter is the appropriate function to call.
check_disk_change() was a combination of checking for the change and also forcing revalidation of the device. The new preferred function is bdev_check_media_change() which does the check, but doesn't force a revalidation, which is now left up to the caller if it wants to. To preserve the old behavior, I have created a wrapper function when check_disk_change() doesn't exist that will call bdev_check_media_change() and then also perform the revalidate call before returning. It may be worth revisiting whether this behavior is needed in all cases where the code is used.
The vdev_blkg_tryget() function implements a local version of function calls that are implemented in GPL'd Linux code. The API changes break our version, and the solution to address this was just to switch ref->count to using ref->data->count instead. The underlying change helps reduce memory usage where local copies of struct percpu_ref are created, so that the underlying metadata only links to a single instance, which also now remains consistent across all copies of the struct percpu_ref instance.

How Has This Been Tested?

So far I have compiled on 5.10 (Linus' tree), 5.9.0, and 5.8.14 and the build worked fine. I have not yet run-time tested this version of the module, which will take some more time to do. I wanted to get the PR up here so that all the build/test automation you've got can take a crack at it in parallel with me testing it.

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 OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

codecov · 2020-10-19T01:43:38Z

Codecov Report

Merging #11085 into master will decrease coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #11085      +/-   ##
==========================================
- Coverage   79.81%   79.56%   -0.25%     
==========================================
  Files         398      398              
  Lines      125754   125754              
==========================================
- Hits       100367   100055     -312     
- Misses      25387    25699     +312

Flag	Coverage Δ
#kernel	`80.40% <100.00%> (-0.07%)`	⬇️
#user	`64.43% <ø> (-1.76%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
include/os/linux/kernel/linux/blkdev_compat.h	`75.40% <ø> (ø)`
module/os/linux/zfs/vdev_disk.c	`78.92% <100.00%> (ø)`
module/os/linux/zfs/zvol_os.c	`87.36% <100.00%> (-0.89%)`	⬇️
module/zfs/vdev_indirect.c	`72.25% <0.00%> (-12.63%)`	⬇️
module/zcommon/zfs_fletcher.c	`68.09% <0.00%> (-10.20%)`	⬇️
cmd/ztest/ztest.c	`74.70% <0.00%> (-3.49%)`	⬇️
module/zfs/spa_errlog.c	`90.83% <0.00%> (-3.06%)`	⬇️
module/zcommon/zfs_fletcher_superscalar.c	`97.05% <0.00%> (-2.95%)`	⬇️
module/zfs/vdev_removal.c	`93.77% <0.00%> (-2.72%)`	⬇️
module/zfs/vdev_raidz.c	`91.63% <0.00%> (-2.29%)`	⬇️
... and 56 more

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 5c810ac...55c52ed. Read the comment docs.

Kernel commit 2b0d3d3e4fcfb on Oct 1 brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Signed-off-by: Coleman Kane <ckane@colemankane.org>

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Signed-off-by: Coleman Kane <ckane@colemankane.org>

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, I think that swapping the old function call out for the new one should be all that is required. Signed-off-by: Coleman Kane <ckane@colemankane.org>

This commit contains a number of style fixes to correct some style bugs introduced into include/os/linux/kernel/linux/blkdev_compat.h during the recent set of commits updating this module to 5.10 kernel API changes. Signed-off-by: Coleman Kane <ckane@colemankane.org>

Signed-off-by: Coleman Kane <ckane@colemankane.org>

The original version I wrote performed some dereference logic which worked in my own builds, but appears to not work in the autotools setup used in the style checker. This commit simplifies the test and hopefully will appease more variations of autotools. Signed-off-by: Coleman Kane <ckane@colemankane.org>

ckane · 2020-10-27T13:30:12Z

Rebased this with master on 2020-10-27 in to determine if the buildbot test errors are affected by other code in master or not. The ZFS tests mostly work for me when run in a VM, but there are a handful that seem to be failing which seem also outside of the scope of these changes, and I am unsure if these were introduced by my changes or not. Going to do a zfstest comparison between "old kernel w/ 2020-10-27 master" and "new kernel w/ my changes" to see what the test suite says.

A #define for zfs_check_media_change contained a space between the macro name and the expansion, rather than a tab. The other #define's around it use tabs to separate the fields. This fixes that small style defect. Signed-off-by: Coleman Kane <ckane@colemankane.org>

ckane · 2020-10-30T17:42:40Z

Apologies for the delay in getting anything back on this. I have tested this in a VM on kernel 5.9.1, but one of the issues with the zfs-test suite is that it looks like there are a bunch of tests that fail unexpectedly already on the current HEAD of both 2.0-release and master.... so I'm doing a zfs-test against 2.0-release branch with & w/out these changes applied, so that I can identify if they introduce any new testing regressions after they've been applied. Looks like in the buildbot stuff above all of the tests are completing successfully, minus the FreeBSD one, which I doubt was introduced by my changes.

ckane · 2020-10-30T19:25:06Z

Running zfs-tests.sh with & without my patches produced the following failures. Since this indicates that there's no regressions and the code responsible for test failures is outside of these patches, I'm going to mark this as passing the tests in my VM environment:

Tests with results other than PASS that are unexpected:
    FAIL casenorm/insensitive_formd_delete (expected PASS)
    FAIL casenorm/insensitive_formd_lookup (expected PASS)
    FAIL casenorm/insensitive_none_delete (expected PASS)
    FAIL casenorm/insensitive_none_lookup (expected PASS)
    FAIL casenorm/mixed_none_delete (expected PASS)
    FAIL casenorm/mixed_none_lookup (expected PASS)
    FAIL casenorm/sensitive_none_delete (expected PASS)
    FAIL casenorm/sensitive_none_lookup (expected PASS)
    FAIL cli_root/zfs_share/zfs_share_005_pos (expected PASS)
    FAIL cli_root/zpool_events/zpool_events_duplicates (expected PASS)
    FAIL delegate/zfs_allow_001_pos (expected PASS)
    FAIL delegate/zfs_allow_002_pos (expected PASS)
    FAIL delegate/zfs_allow_006_pos (expected PASS)
    FAIL delegate/zfs_allow_007_pos (expected PASS)
    FAIL delegate/zfs_allow_010_pos (expected PASS)
    FAIL slog/slog_replay_fs_001 (expected PASS)
    FAIL xattr/xattr_004_pos (expected PASS)

ckane · 2020-10-30T19:26:29Z

As no new functionality is added and no behavioral changes are expected, I checked the "added tests" box, but there were no new tests to add. Similarly, the documentation in the source code I added should be sufficient documentation for these changes.

ckane · 2020-10-30T21:34:29Z

Ok I'm running these changes on main now (patched against the 2.0.0rc4 sources)

PrivatePuffin · 2020-11-01T17:16:02Z

@behlendorf and @ahrens I think this one requires a review and removal of the WIP tag.

@ckane You might at least squash the style fixes (or even better: cherrypick and squash them into their respective commits)

behlendorf

@ckane thanks for putting together these fixes. They look good and work correctly for me with the latest mainline kernel. I'll squash them accordingly when merging.

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, swapping the old function call out for the new one should be all that is required. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

behlendorf · 2020-11-02T22:02:12Z

Merged and squashed.

tonyhutter · 2020-11-02T22:15:38Z

This would be nice to have backported to the zfs-0.8.6-staging branch. If someone is interested in doing that, please open a PR against that branch, and we'll try to pull it in.

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11085

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, swapping the old function call out for the new one should be all that is required. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes #11085

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, swapping the old function call out for the new one should be all that is required. Backported-by: Aron Xu <aron@debian.org> Signed-off-by: Aron Xu <aron@debian.org> Closes openzfs#11085

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11085

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, swapping the old function call out for the new one should be all that is required. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

In Linux 5.10 the linux/frame.h header was renamed linux/objtool.h. Add a configure check to detect and use the correctly named header. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#11085

Kernel commit 2b0d3d3e4fcfb brought in some changes to the struct percpu_ref structure that moves most of its fields into a member struct named "data" of type struct percpu_ref_data. This includes the "count" member which is updated by vdev_blkg_tryget(), so update this function to chase the API change, and detect it via configure. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

Kernel 5.10 removed check_disk_change() in favor of callers using the faster bdev_check_media_change() instead, and explicitly forcing bdev revalidation when they desire that behavior. To preserve prior behavior, I have wrapped this into a zfs_check_media_change() macro that calls an inline function for the new API that mimics the old behavior when check_disk_change() doesn't exist, and just calls check_disk_change() if it exists. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

A new function was added named revalidate_disk_size() and the old revalidate_disk() appears to have been deprecated. As the only ZFS code that calls this function is zvol_update_volsize, swapping the old function call out for the new one should be all that is required. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Coleman Kane <ckane@colemankane.org> Closes openzfs#11085

behlendorf added the Status: Work in Progress Not yet ready for general review label Oct 19, 2020

ckane added 6 commits October 27, 2020 09:12

Fix block comments to adhere to style

9200964

Signed-off-by: Coleman Kane <ckane@colemankane.org>

ckane force-pushed the fixes-5.10-20201018 branch from bd9a50d to 88575e6 Compare October 27, 2020 13:21

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Work in Progress Not yet ready for general review labels Nov 2, 2020

behlendorf approved these changes Nov 2, 2020

View reviewed changes

behlendorf closed this in 8c7d604 Nov 2, 2020

softminus mentioned this pull request Nov 2, 2020

errors building latest ZFS against latest linux kernel; since check_disk_change seems to have disappeared and struct percpu_ref doesn't have a "count" member #11074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accumulated fixes for 5.10 kernel-introduced build breakages #11085

Accumulated fixes for 5.10 kernel-introduced build breakages #11085

ckane commented Oct 18, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

ckane commented Oct 27, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

PrivatePuffin commented Nov 1, 2020

behlendorf left a comment

behlendorf commented Nov 2, 2020

tonyhutter commented Nov 2, 2020 •

edited

Loading

Accumulated fixes for 5.10 kernel-introduced build breakages #11085

Accumulated fixes for 5.10 kernel-introduced build breakages #11085

Conversation

ckane commented Oct 18, 2020 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 19, 2020 • edited Loading

Codecov Report

ckane commented Oct 27, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

ckane commented Oct 30, 2020

PrivatePuffin commented Nov 1, 2020

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Nov 2, 2020

tonyhutter commented Nov 2, 2020 • edited Loading

ckane commented Oct 18, 2020 •

edited

Loading

codecov bot commented Oct 19, 2020 •

edited

Loading

tonyhutter commented Nov 2, 2020 •

edited

Loading