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

set autotrim default to 'off' everywhere #15079

Merged
merged 1 commit into from Jul 20, 2023
Merged

set autotrim default to 'off' everywhere #15079

merged 1 commit into from Jul 20, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 18, 2023

Re-do this correctly by simply removing the logic for FreeBSD that didn't really work and default to 'off' everywhere.

Motivation and Context

As it turns out having autotrim default to 'on' on FreeBSD never really worked due to mess with defines where userland and kernel module were getting different default values (userland was defaulting to 'off', module was thinking it's 'on').

Description

Remove SPA_AUTOTRIM_DEFAULT, simply use SPA_AUTOTRIM_OFF when initializing property.

How Has This Been Tested?

Creating the pool on FreeBSD with and without explicitly specifying the autotrim option, and checking that the source reported by zpool get autotrim is correct.

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:

As it turns out having autotrim default to 'on' on FreeBSD never really
worked due to mess with defines where userland and kernel module were
getting different default values (userland was defaulting to 'off',
module was thinking it's 'on').

Signed-off-by: Yuri Pankov <[email protected]>
@ghost
Copy link
Author

ghost commented Jul 18, 2023

@amotin please review here as well

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I can confirm that on my FreeBSD main system with built-in ZFS autotrim on newly created pool is indeed disabled. I'd really like to see it enabled everywhere, but once it already got disabled due to some bug, lets remove the unneeded divergence.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jul 20, 2023
@behlendorf behlendorf merged commit 8beabfd into openzfs:master Jul 20, 2023
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jul 21, 2023
As it turns out having autotrim default to 'on' on FreeBSD never really
worked due to mess with defines where userland and kernel module were
getting different default values (userland was defaulting to 'off',
module was thinking it's 'on').

Reviewed-by: Tino Reichardt <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Yuri Pankov <[email protected]>
Closes openzfs#15079
behlendorf pushed a commit that referenced this pull request Jul 21, 2023
As it turns out having autotrim default to 'on' on FreeBSD never really
worked due to mess with defines where userland and kernel module were
getting different default values (userland was defaulting to 'off',
module was thinking it's 'on').

Reviewed-by: Tino Reichardt <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Yuri Pankov <[email protected]>
Closes #15079
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
As it turns out having autotrim default to 'on' on FreeBSD never really
worked due to mess with defines where userland and kernel module were
getting different default values (userland was defaulting to 'off',
module was thinking it's 'on').

Reviewed-by: Tino Reichardt <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Yuri Pankov <[email protected]>
Closes openzfs#15079
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