Differential D41056

zfs: set autotrim default to 'off'
ClosedPublic
Actions

Authored by yuripv on Jul 17 2023, 9:28 AM.

Details

Reviewers

mav

Group Reviewers

ZFS

Commits

rGb36f469a15ec: zfs: set autotrim default to 'off'

Summary

Coming back to this with what seems to be complete solution -- simply
default autotrim to 'off' and remove any FreeBSD-specific defines here.

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').

PR: 264234

Test Plan

Build, install, check that autotrim value of 'off' says 'default' in source field, 'on' - 'local'.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

yuripv created this revision.Jul 17 2023, 9:28 AM

Herald added subscribers: delphij, imp. · View Herald TranscriptJul 17 2023, 9:28 AM

yuripv requested review of this revision.Jul 17 2023, 9:28 AM

Harbormaster completed remote builds in B52672: Diff 124759.Jul 17 2023, 9:28 AM

yuripv edited the summary of this revision. (Show Details)Jul 17 2023, 9:31 AM

yuripv edited the test plan for this revision. (Show Details)

yuripv added a reviewer: ZFS.

I like removal of IN_FREEBSD_BASE. I would be happy if autotrim was enabled by default, but if it wasn't for some time, then I see no problem to clean this up. I just wonder how long it was broken. Respective part of the patch will need to be upstreamed to OpenZFS also, may be in parallel to make sure more people are in sync about it.

This revision is now accepted and ready to land.Jul 17 2023, 1:47 PM

In D41056#934733, @mav wrote:

I like removal of IN_FREEBSD_BASE. I would be happy if autotrim was enabled by default, but if it wasn't for some time, then I see no problem to clean this up. I just wonder how long it was broken. Respective part of the patch will need to be upstreamed to OpenZFS also, may be in parallel to make sure more people are in sync about it.

It's been that way since the switch to openzfs (so 3 years?), and nobody seems to have complained, other than the PR mentioned and only about inconsistent source information.

To make sure I'm not imagining things (yet again), I have added a printf to the end of spa_load_impl(), and creating/exporting/importing the pool without this change gets me the following (where the pools with explicitly enabled autotrim show 1):

Solaris: NOTICE: autotrim=0

I will open PR against openzfs, with a note that it will NOT change anything there, both linux build and openzfs build in ports default to autotrim being 0 already.

Closed by commit rGb36f469a15ec: zfs: set autotrim default to 'off' (authored by yuripv). · Explain WhyJul 18 2023, 9:21 AM

This revision was automatically updated to reflect the committed changes.

yuripv added a commit: rGb36f469a15ec: zfs: set autotrim default to 'off'.

des added a reverting change: D41483: openzfs: Revert to last known good state..Aug 16 2023, 6:31 PM