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

FreeBSD: Organize sysctls #13756

Closed
wants to merge 2 commits into from
Closed

FreeBSD: Organize sysctls #13756

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2022

Motivation and Context

FreeBSD had a few platform-specific ARC tunables in the wrong place:

vfs.zfs.arc_free_target
vfs.zfs.arc_no_grow_shift

instead of:

vfs.zfs.arc.free_target
vfs.zfs.arc.no_grow_shift

In fixing this I noticed that most sysctl handlers for ZFS module parameters were not marked mpsafe. All the parameter handlers implement their own locking if needed and do not require the global Giant lock.

I also did some general cleanup including organization of sections, not passing unneeded arguments to handlers, using the correct types for local temporaries, etc.

Description

  • Mark ZFS_MODULE_PARAM_CALL handlers as MPSAFE.
  • Move FreeBSD-specifc ARC tunables into the same vfs.zfs.arc node as
    the rest of the ARC tunables.
  • Move the handlers from arc_os.c to sysctl_os.c and add compat sysctls
    for the legacy names.
  • Most handlers are specific to a particular variable and don't need a
    pointer passed through the args.
  • Group blocks of related variables, handlers, and sysctl declarations
    into logical sections.
  • Match variable types for temporaries in handlers with the type of the
    global variable.
  • Remove leftover comments.

How Has This Been Tested?

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:

@ghost ghost requested a review from amotin August 9, 2022 15:52
@ghost
Copy link
Author

ghost commented Aug 9, 2022

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 9, 2022
module/os/freebsd/zfs/sysctl_os.c Outdated Show resolved Hide resolved
module/os/freebsd/zfs/sysctl_os.c Show resolved Hide resolved
module/os/freebsd/zfs/sysctl_os.c Outdated Show resolved Hide resolved
Ryan Moeller added 2 commits August 12, 2022 15:19
ZFS_MODULE_PARAM_CALL handlers implement their own locking if needed
and do not require Giant.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
FreeBSD had a few platform-specific ARC tunables in the wrong place:

- Move FreeBSD-specifc ARC tunables into the same vfs.zfs.arc node as
  the rest of the ARC tunables.
- Move the handlers from arc_os.c to sysctl_os.c and add compat sysctls
  for the legacy names.

While here, some additional clean up:

- Most handlers are specific to a particular variable and don't need a
  pointer passed through the args.
- Group blocks of related variables, handlers, and sysctl declarations
  into logical sections.
- Match variable types for temporaries in handlers with the type of the
  global variable.
- Remove leftover comments.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
@ghost
Copy link
Author

ghost commented Aug 12, 2022

I've also changed several legacy sysctls that were CTLFLAG_RW to now match the corresponding new sysctls as CTLFLAG_RWTUN.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 2, 2022
@behlendorf behlendorf closed this in 4723eba Sep 2, 2022
behlendorf pushed a commit that referenced this pull request Sep 2, 2022
FreeBSD had a few platform-specific ARC tunables in the wrong place:

- Move FreeBSD-specifc ARC tunables into the same vfs.zfs.arc node as
  the rest of the ARC tunables.
- Move the handlers from arc_os.c to sysctl_os.c and add compat sysctls
  for the legacy names.

While here, some additional clean up:

- Most handlers are specific to a particular variable and don't need a
  pointer passed through the args.
- Group blocks of related variables, handlers, and sysctl declarations
  into logical sections.
- Match variable types for temporaries in handlers with the type of the
  global variable.
- Remove leftover comments.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #13756
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2022
ZFS_MODULE_PARAM_CALL handlers implement their own locking if needed
and do not require Giant.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#13756
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
ZFS_MODULE_PARAM_CALL handlers implement their own locking if needed
and do not require Giant.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#13756
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
FreeBSD had a few platform-specific ARC tunables in the wrong place:

- Move FreeBSD-specifc ARC tunables into the same vfs.zfs.arc node as
  the rest of the ARC tunables.
- Move the handlers from arc_os.c to sysctl_os.c and add compat sysctls
  for the legacy names.

While here, some additional clean up:

- Most handlers are specific to a particular variable and don't need a
  pointer passed through the args.
- Group blocks of related variables, handlers, and sysctl declarations
  into logical sections.
- Match variable types for temporaries in handlers with the type of the
  global variable.
- Remove leftover comments.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#13756
@ixhamza ixhamza deleted the arc-tunables branch August 3, 2023 13:13
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.

2 participants