tunables: general code cleanup #17377

robn · 2025-05-24T11:18:50Z

Motivation and Context

Our tunables/module parameters infrastructure has accumulated a lot of cruft over time, making it pretty difficult to work with. I've been working quietly over the last year on a general uplift. The first thing to do though is make a run at the obvious things, which is this PR: a goodie bag of miscellaneous tunable-related cleanups.

This PR is just the internal code cleanup. See #17375 and #17376 for the user-visible changes (removing old tunables proper).

Description

See the individual commits. Commit summaries:

zfs_log: make zfs_immediate_write_sz uint
tunables: don't assert initialisation in impl getters
tunables: ensure tunable and variable have same define gate
tunables: remove FreeBSD compat macros for Linux module params
tunables: remove direct use of module_param_cb
tunables: remove support for s64 tunables
tunables: use Linux ullong param ops for u64
tunables: remove unused param get/set aliases
tunables: remove __check_old_set_param workaround
tunables: fix spelling

Includes e2345d1 from #17376 to keep it compiling on FreeBSD after the no-op compat macros module_param_named and MODULE_PARAM_DESC removed. However, that commit is part of that PR, not this, and if its merged earlier, I will rebase this one on top of it.

How Has This Been Tested?

Compile checked on Linux and FreeBSD.

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)
Quality assurance (non-breaking change which makes the code more robust against bugs)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
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.

module/os/freebsd/zfs/zio_crypt.c

amotin · 2025-05-25T01:57:46Z

@robn Does not build on FreeBSD.

robn · 2025-05-25T02:16:25Z

But! It! I!... gragh!

robn · 2025-05-25T02:25:07Z

@amotin fixed by pulling in a commit from #17376 temporarily. This all used to be one branch, then I decided I should split cleanup from user-visible changes for review, and I clearly didn't test properly after that. Thanks.

amotin

Generally looks good. Just one comment:

module/zfs/zfs_log.c

behlendorf

Looks good. It's nice to have the additional cleanup. Can you just rebase this now that #17376 was merged.

Likely it's only int64 for comparison with ssize_t, which is signed. However, it would make no sense for it to be less than 0 or greater than 4G, so making it a regular uint will make it safe for comparison and remove the only S64 tunable in core. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

It actually doesn't matter if it's not initialised when we first query the current value; it just returns empty-string. A crash is quite obnoxious even if it is a rare case. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

If a variable is only available in the kernel, then the tunable should also only be available there. This matters very little so long as we don't have userspace tunables, but its still good hygeine. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

The use for spl_taskq_kick was the only use, and the comment that module_param_call is obsolete is no longer true - it's still very much used even in recent kernels. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

Since 3.17 Linux has provided param ops for 64-bit ints, so we don't need to use our own anymore. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

This was fully removed from Linux in 4.15, so we won't be seeing it again. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

Three occurences with an 'e', and all of them mine. Maybe it's an British thing? Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

robn · 2025-05-28T00:17:54Z

@behlendorf done, ty.

It actually doesn't matter if it's not initialised when we first query the current value; it just returns empty-string. A crash is quite obnoxious even if it is a rare case. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

If a variable is only available in the kernel, then the tunable should also only be available there. This matters very little so long as we don't have userspace tunables, but its still good hygeine. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

The use for spl_taskq_kick was the only use, and the comment that module_param_call is obsolete is no longer true - it's still very much used even in recent kernels. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Likely it's only int64 for comparison with ssize_t, which is signed. However, it would make no sense for it to be less than 0 or greater than 4G, so making it a regular uint will make it safe for comparison and remove the only S64 tunable in core. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

It actually doesn't matter if it's not initialised when we first query the current value; it just returns empty-string. A crash is quite obnoxious even if it is a rare case. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

If a variable is only available in the kernel, then the tunable should also only be available there. This matters very little so long as we don't have userspace tunables, but its still good hygeine. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

The use for spl_taskq_kick was the only use, and the comment that module_param_call is obsolete is no longer true - it's still very much used even in recent kernels. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Since 3.17 Linux has provided param ops for 64-bit ints, so we don't need to use our own anymore. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

This was fully removed from Linux in 4.15, so we won't be seeing it again. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Three occurences with an 'e', and all of them mine. Maybe it's an British thing? Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Likely it's only int64 for comparison with ssize_t, which is signed. However, it would make no sense for it to be less than 0 or greater than 4G, so making it a regular uint will make it safe for comparison and remove the only S64 tunable in core. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

It actually doesn't matter if it's not initialised when we first query the current value; it just returns empty-string. A crash is quite obnoxious even if it is a rare case. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

If a variable is only available in the kernel, then the tunable should also only be available there. This matters very little so long as we don't have userspace tunables, but its still good hygeine. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

The use for spl_taskq_kick was the only use, and the comment that module_param_call is obsolete is no longer true - it's still very much used even in recent kernels. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Since 3.17 Linux has provided param ops for 64-bit ints, so we don't need to use our own anymore. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

This was fully removed from Linux in 4.15, so we won't be seeing it again. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Three occurences with an 'e', and all of them mine. Maybe it's an British thing? Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #17377

Likely it's only int64 for comparison with ssize_t, which is signed. However, it would make no sense for it to be less than 0 or greater than 4G, so making it a regular uint will make it safe for comparison and remove the only S64 tunable in core. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

It actually doesn't matter if it's not initialised when we first query the current value; it just returns empty-string. A crash is quite obnoxious even if it is a rare case. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

If a variable is only available in the kernel, then the tunable should also only be available there. This matters very little so long as we don't have userspace tunables, but its still good hygeine. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

The use for spl_taskq_kick was the only use, and the comment that module_param_call is obsolete is no longer true - it's still very much used even in recent kernels. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Since 3.17 Linux has provided param ops for 64-bit ints, so we don't need to use our own anymore. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

This was fully removed from Linux in 4.15, so we won't be seeing it again. Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

Three occurences with an 'e', and all of them mine. Maybe it's an British thing? Sponsored-by: https://despairlabs.com/sponsor/ Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Pavel Snajdr <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#17377

snajpa reviewed May 24, 2025

View reviewed changes

module/os/freebsd/zfs/zio_crypt.c Show resolved Hide resolved

amotin added the Status: Revision Needed Changes are required for the PR to be accepted label May 25, 2025

robn force-pushed the tunables-cleanup branch from 2d1de36 to 9a12d57 Compare May 25, 2025 02:21

github-actions bot removed the Status: Revision Needed Changes are required for the PR to be accepted label May 25, 2025

amotin approved these changes May 26, 2025

View reviewed changes

module/zfs/zfs_log.c Show resolved Hide resolved

amotin added the Status: Code Review Needed Ready for review and testing label May 27, 2025

behlendorf approved these changes May 27, 2025

View reviewed changes

robn added 10 commits May 28, 2025 10:15

tunables: remove FreeBSD compat macros for Linux module params

240607a

Nothing in any FreeBSD code uses them. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

tunables: remove support for s64 tunables

5a05fd2

Nothing uses them now. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

tunables: use Linux ullong param ops for u64

92ab041

Since 3.17 Linux has provided param ops for 64-bit ints, so we don't need to use our own anymore. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

tunables: remove unused param get/set aliases

cc564ac

Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

tunables: remove __check_old_set_param workaround

f5785b7

This was fully removed from Linux in 4.15, so we won't be seeing it again. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

tunables: fix spelling

68561cd

Three occurences with an 'e', and all of them mine. Maybe it's an British thing? Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <[email protected]>

robn force-pushed the tunables-cleanup branch from 9a12d57 to 68561cd Compare May 28, 2025 00:16

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 28, 2025

behlendorf closed this in 5ef91c2 May 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tunables: general code cleanup #17377

tunables: general code cleanup #17377

Uh oh!

robn commented May 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

amotin commented May 25, 2025

Uh oh!

robn commented May 25, 2025

Uh oh!

robn commented May 25, 2025

Uh oh!

amotin left a comment

Uh oh!

Uh oh!

behlendorf left a comment

Uh oh!

robn commented May 28, 2025

Uh oh!

Uh oh!

tunables: general code cleanup #17377

tunables: general code cleanup #17377

Uh oh!

Conversation

robn commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Uh oh!

amotin commented May 25, 2025

Uh oh!

robn commented May 25, 2025

Uh oh!

robn commented May 25, 2025

Uh oh!

amotin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

robn commented May 28, 2025

Uh oh!

Uh oh!

robn commented May 24, 2025 •

edited

Loading