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

{zfs,spl}-module-parameters.5: fix nonexistent parameters #12157

Closed

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented May 29, 2021

Motivation and Context

#12125 (comment), I hope for this to be pretty uncontroversial

Description

Commit messages outline history for all of these

How Has This Been Tested?

Looked at it, I guess? I made the selection through the very scientific process of getting all the names in the zfs-m-p page and contrasting that to a modinfo listing from zfs-2.0.3-6~bpo10+1, which yielded a list short enough for manual extraxion, like spl-m-p.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Both were removed in 4fbdb10 ("remove
kmem_cache module parameter KMC_EXPIRE_AGE")

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
zfs_arc_overflow_shift was never a parameter:
ca0bf58 ("Illumos 5497 - lock
contention on arcs_mtx") is the only result in
git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in
e53d678 ("Share zfs_fsync, zfs_read,
zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in
c3bd3fb ("OpenZFS 9403 - assertion
failed in arc_buf_destroy()") as a developer aid for setting in zdb, but
it's a dangerous test tunable and has no place in public documentation,
(not to mention that it obviously doesn't work):
> Although this did uncover a few low priority issues, this
  unfortuantely also causes ztest to ASSERT in many locations where the
  code is working correctly since it is designed to fail on IO errors.
  Developers can manually set this variable with the '-o' option to find
  and debug issues.

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
@allanjude
Copy link
Contributor

zio_decompress_fail_fraction still exists, although it is not exposed as a module parameter, it is used by zdb to simulate failures.
Not sure if it makes sense to remove the documentation for it, but maybe it should explain that it is not a full module parameter or something.

Although there is a similar variable for decryption failure, that doesn't appear to be documented.

@nabijaczleweli
Copy link
Contributor Author

It seems that I have to quote the messages

For spl-m-p:

Both were removed in 4fbdb10 ("remove kmem_cache module parameter KMC_EXPIRE_AGE")

For zfs-m-p:

zfs_arc_overflow_shift was never a parameter: ca0bf58 ("Illumos 5497 - lock contention on arcs_mtx") is the only result in git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in e53d678 ("Share zfs_fsync, zfs_read, zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in c3bd3fb ("OpenZFS 9403 - assertion failed in arc_buf_destroy()") as a developer aid for setting in zdb, but it's a dangerous test tunable and has no place in public documentation, (not to mention that it obviously doesn't work):

Although this did uncover a few low priority issues, this unfortuantely also causes ztest to ASSERT in many locations where the code is working correctly since it is designed to fail on IO errors. Developers can manually set this variable with the '-o' option to find and debug issues.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 30, 2021
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Since zio_decompress_fail_fraction was added to the documentation it makes me wonder if the original intent was to make it available as a tunable option. However, since it clearly never was and is very developer focused I agree let's drop from from the user visible docs.

@rlaager
Copy link
Member

rlaager commented Jun 1, 2021

Before anyone requests a review from me (like with the other man page changes), I figured I'd take a look.

The only question I have is about spl_kmem_cache_obj_per_slab. That one seems to still exist:
https://github.com/openzfs/zfs/blob/master/module/os/linux/spl/spl-kmem-cache.c#L93

4fbdb10 says it removes spl_kmem_cache_obj_per_slab**_min**

This comment is the limit of my expertise in this area.

@nabijaczleweli
Copy link
Contributor Author

How fortunate that this doesn't touch spl_kmem_cache_obj_per_slab, then

@rlaager
Copy link
Member

rlaager commented Jun 1, 2021

How fortunate that this doesn't touch spl_kmem_cache_obj_per_slab, then

Hah, yep, you're right. My bad!

@jwk404 jwk404 added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 2, 2021
@behlendorf behlendorf closed this in ace760a Jun 2, 2021
behlendorf pushed a commit that referenced this pull request Jun 2, 2021
zfs_arc_overflow_shift was never a parameter:
ca0bf58 ("Illumos 5497 - lock
contention on arcs_mtx") is the only result in
git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in
e53d678 ("Share zfs_fsync, zfs_read,
zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in
c3bd3fb ("OpenZFS 9403 - assertion
failed in arc_buf_destroy()") as a developer aid for setting in zdb, but
it's a dangerous test tunable and has no place in public documentation,
(not to mention that it obviously doesn't work):
> Although this did uncover a few low priority issues, this
  unfortuantely also causes ztest to ASSERT in many locations where the
  code is working correctly since it is designed to fail on IO errors.
  Developers can manually set this variable with the '-o' option to find
  and debug issues.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 3, 2021
Both were removed in 4fbdb10 ("remove
kmem_cache module parameter KMC_EXPIRE_AGE")

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 3, 2021
zfs_arc_overflow_shift was never a parameter:
ca0bf58 ("Illumos 5497 - lock
contention on arcs_mtx") is the only result in
git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in
e53d678 ("Share zfs_fsync, zfs_read,
zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in
c3bd3fb ("OpenZFS 9403 - assertion
failed in arc_buf_destroy()") as a developer aid for setting in zdb, but
it's a dangerous test tunable and has no place in public documentation,
(not to mention that it obviously doesn't work):
> Although this did uncover a few low priority issues, this
  unfortuantely also causes ztest to ASSERT in many locations where the
  code is working correctly since it is designed to fail on IO errors.
  Developers can manually set this variable with the '-o' option to find
  and debug issues.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
Both were removed in 4fbdb10 ("remove
kmem_cache module parameter KMC_EXPIRE_AGE")

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
zfs_arc_overflow_shift was never a parameter:
ca0bf58 ("Illumos 5497 - lock
contention on arcs_mtx") is the only result in
git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in
e53d678 ("Share zfs_fsync, zfs_read,
zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in
c3bd3fb ("OpenZFS 9403 - assertion
failed in arc_buf_destroy()") as a developer aid for setting in zdb, but
it's a dangerous test tunable and has no place in public documentation,
(not to mention that it obviously doesn't work):
> Although this did uncover a few low priority issues, this
  unfortuantely also causes ztest to ASSERT in many locations where the
  code is working correctly since it is designed to fail on IO errors.
  Developers can manually set this variable with the '-o' option to find
  and debug issues.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
Both were removed in 4fbdb10 ("remove
kmem_cache module parameter KMC_EXPIRE_AGE")

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
zfs_arc_overflow_shift was never a parameter:
ca0bf58 ("Illumos 5497 - lock
contention on arcs_mtx") is the only result in
git log -Soverflow_shift, and it wasn't exposed then, nor is it now

zfs_read_chunk_size was renamed to zfs_vnops_read_chunk_size in
e53d678 ("Share zfs_fsync, zfs_read,
zfs_write, et al between Linux and FreeBSD")

zio_decompress_fail_fraction was never a parameter: it was added in
c3bd3fb ("OpenZFS 9403 - assertion
failed in arc_buf_destroy()") as a developer aid for setting in zdb, but
it's a dangerous test tunable and has no place in public documentation,
(not to mention that it obviously doesn't work):
> Although this did uncover a few low priority issues, this
  unfortuantely also causes ztest to ASSERT in many locations where the
  code is working correctly since it is designed to fail on IO errors.
  Developers can manually set this variable with the '-o' option to find
  and debug issues.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants