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

Add define to enable autotrim to default to on #10719

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

mattmacy
Copy link
Contributor

In FreeBSD trim has defaulted to on for several
years. In order to minimize POLA violations on
import it's important to maintain this default
when importing vendored openzfs in to FreeBSD
base.

Signed-off-by: Matt Macy [email protected]

Motivation and Context

Description

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux 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.

@codecov
Copy link

codecov bot commented Aug 16, 2020

Codecov Report

Merging #10719 into master will decrease coverage by 14.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #10719       +/-   ##
===========================================
- Coverage   79.72%   65.17%   -14.56%     
===========================================
  Files         394      312       -82     
  Lines      124665   107467    -17198     
===========================================
- Hits        99390    70040    -29350     
- Misses      25275    37427    +12152     
Flag Coverage Δ
#kernel ?
#user 65.17% <ø> (-0.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/zcommon/zpool_prop.c 93.84% <ø> (ø)
module/zfs/objlist.c 0.00% <0.00%> (-100.00%) ⬇️
module/zfs/pathname.c 0.00% <0.00%> (-100.00%) ⬇️
include/sys/dmu_redact.h 0.00% <0.00%> (-100.00%) ⬇️
include/sys/dmu_traverse.h 0.00% <0.00%> (-100.00%) ⬇️
module/lua/ltablib.c 2.34% <0.00%> (-95.32%) ⬇️
module/zfs/bqueue.c 0.00% <0.00%> (-94.45%) ⬇️
module/zfs/zfs_rlock.c 0.00% <0.00%> (-94.21%) ⬇️
module/zcommon/zfs_deleg.c 0.00% <0.00%> (-92.46%) ⬇️
module/zfs/dmu_diff.c 0.00% <0.00%> (-87.88%) ⬇️
... and 239 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc34dfb...b93ce0a. Read the comment docs.

In FreeBSD trim has defaulted to on for several
years. In order to minimize POLA violations on
import it's important to maintain this default
when importing vendored openzfs in to FreeBSD
base.

Signed-off-by: Matt Macy <[email protected]>
@mattmacy mattmacy force-pushed the projects/autotrim_defaults branch from cb6037e to b93ce0a Compare August 17, 2020 22:33
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 17, 2020
Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

Having SPA_AUTOTRIM_DEFAULT as another value in this enum feels a little weird to me. That doesn't necessarily make it wrong, but that's why I'm asking these questions.

Does that property value get written to disk where a 2 would be unexpected? Why did you take this approach rather than a #define SPA_AUTOTRIM_DEFAULT SPA_AUTOTRIM_ON or putting the #ifdef in zpool_prop.c?

@ghost
Copy link

ghost commented Aug 18, 2020

Does that property value get written to disk where a 2 would be unexpected?

There isn't a 2, just 0 and 1.

@rlaager
Copy link
Member

rlaager commented Aug 18, 2020

Right. I see now. Thanks!

@behlendorf behlendorf merged commit 5e7eaf8 into openzfs:master Aug 18, 2020
@mattmacy mattmacy deleted the projects/autotrim_defaults branch August 28, 2020 19:08
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
In FreeBSD trim has defaulted to on for several
years. In order to minimize POLA violations on
import it's important to maintain this default
when importing vendored openzfs in to FreeBSD
base.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10719
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
In FreeBSD trim has defaulted to on for several
years. In order to minimize POLA violations on
import it's important to maintain this default
when importing vendored openzfs in to FreeBSD
base.

Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10719
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