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

Restore FreeBSD sysctl processing for arc.min and arc.max #12161

Merged
merged 1 commit into from
Aug 16, 2021

Conversation

allanjude
Copy link
Contributor

Motivation and Context

Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Description

How Has This Been Tested?

BEFORE:

loader.conf:

vfs.zfs.arc.max=402653184

After boot, the values on the running system (notice c_max is higher than requested by the user):

vfs.zfs.arc.max: 402653184
vfs.zfs.arc.min: 0
kstat.zfs.misc.arcstats.c_max: 33122279424
kstat.zfs.misc.arcstats.c_min: 1068625664

Attempting to set the arc.max to less than arc.min:
# sysctl vfs.zfs.arc.max=402653184

vfs.zfs.arc.max: 402653184 -> 402653184
[on the console only] kernel: Solaris: WARNING: ignoring tunable zfs_arc_max (using 33122279424 instead)

Resulting values (again, notice that the users request was ignored, and outside of inspecting dmesg, they have no way of knowing):

vfs.zfs.arc.max: 402653184
kstat.zfs.misc.arcstats.c_max: 33122279424

AFTER:

loader.conf:

vfs.zfs.arc.max=402653184

After boot, the values on the running system:

vfs.zfs.arc.max: 402653184
vfs.zfs.arc.min: 0
kstat.zfs.misc.arcstats.c_max: 402653184
kstat.zfs.misc.arcstats.c_min: 201326592

Attempting to set the arc.max to less or equal to arc.min:
# sysctl vfs.zfs.arc.max=201326592

vfs.zfs.arc.max: 402653184
sysctl: vfs.zfs.arc.max=201326592: Invalid argument

Resulting values:

vfs.zfs.arc.max: 402653184
kstat.zfs.misc.arcstats.c_max: 402653184

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:

@allanjude allanjude force-pushed the freebsd_arc_sysctl branch 2 times, most recently from 6831d75 to 1062b3b Compare May 29, 2021 23:22
@behlendorf behlendorf requested a review from a user May 30, 2021 04:24
@allanjude
Copy link
Contributor Author

This is causing arcstats_runtime_tuning to fail, as it tries to set vfs.zfs.arc.min back to 0, which is less than the minimum, 64MB.

Do we want to support 0 as a special value? what would it mean, currently, it would just be ignored by arc_tuning_update() because it fails the first condition

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

This is causing arcstats_runtime_tuning to fail, as it tries to set vfs.zfs.arc.min back to 0, which is less than the minimum, 64MB.

Do we want to support 0 as a special value? what would it mean, currently, it would just be ignored by arc_tuning_update() because it fails the first condition

I believe the current behavior is that a 0 value implies tunable is not set and arc_c_min remains as-is.

I think removing the below sysctl update (for both param_set_arc_min and param_set_arc_max) would restore the behavior.

	/* Update the sysctl to the tuned value */
	zfs_arc_min = arc_c_min;

@allanjude
Copy link
Contributor Author

I believe the current behavior is that a 0 value implies tunable is not set and arc_c_min remains as-is.

I think removing the below sysctl update (for both param_set_arc_min and param_set_arc_max) would restore the behavior.

	/* Update the sysctl to the tuned value */
	zfs_arc_min = arc_c_min;

I think we still want the update, to restore the older illumos ZFS FreeBSD behaviour, but, we can make it conditional on 'val' not being 0, to resolve the test and the expected '0' functionality.

@allanjude allanjude force-pushed the freebsd_arc_sysctl branch 2 times, most recently from 6656b41 to 5e3971a Compare June 3, 2021 13:25
@tonynguien
Copy link
Contributor

I believe the current behavior is that a 0 value implies tunable is not set and arc_c_min remains as-is.
I think removing the below sysctl update (for both param_set_arc_min and param_set_arc_max) would restore the behavior.

	/* Update the sysctl to the tuned value */
	zfs_arc_min = arc_c_min;

I think we still want the update, to restore the older illumos ZFS FreeBSD behaviour, but, we can make it conditional on 'val' not being 0, to resolve the test and the expected '0' functionality.

Ah, OK. That works.

Copy link
Contributor

@tonynguien tonynguien left a comment

Choose a reason for hiding this comment

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

LGTM.

module/os/freebsd/zfs/sysctl_os.c Outdated Show resolved Hide resolved
@behlendorf behlendorf added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Code Review Needed Ready for review and testing labels Jun 8, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Signed-off-by: Allan Jude <allan@klarasystems.com>
@ghost ghost force-pushed the freebsd_arc_sysctl branch from 5e3971a to 6bfc5f0 Compare August 3, 2021 10:41
@ghost
Copy link

ghost commented Aug 3, 2021

  • defined MIN_ARC_MAX
  • rebased

@ghost ghost added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Revision Needed Changes are required for the PR to be accepted labels Aug 3, 2021
@tonynguien tonynguien merged commit e945e8d into openzfs:master Aug 16, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 23, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Aug 24, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
behlendorf pushed a commit that referenced this pull request Aug 31, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes #12161
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 15, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
rincebrain pushed a commit to rincebrain/zfs that referenced this pull request Sep 22, 2021
Before OpenZFS 2.0, trying to set the FreeBSD sysctl vfs.zfs.arc_max
to a disallowed value would return an error.
Since the switch, it instead only generates WARN_IF_TUNING_IGNORED

Keep the ability to set the sysctl's specifically to 0, even though
that is less than the minimum, because some tests depend on this.

Also lost, was the ability to set vfs.zfs.arc_max to a value less
than the default vfs.zfs.arc_min at boot time. Restore this as well.

Reviewed-by: Tony Nguyen <tony.nguyen@delphix.com>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Allan Jude <allan@klarasystems.com>
Closes openzfs#12161
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.

3 participants