Bug 264234 - "zpool get autotrim" displays propety as default regardless of value
Summary: "zpool get autotrim" displays propety as default regardless of value
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Yuri Pankov
URL: https://reviews.freebsd.org/D41056
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-25 13:21 UTC by Anton Saietskii
Modified: 2024-10-30 15:10 UTC (History)
4 users (show)

See Also:
linimon: mfc-stable13?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anton Saietskii 2022-05-25 13:21:29 UTC
root@localhost:~ # uname -a
FreeBSD localhost.localdomain 13.1-RELEASE FreeBSD 13.1-RELEASE releng/13.1-n250148-fc952ac2212 GENERIC amd64
root@localhost:~ # zpool create test da1 && zpool get autotrim test && zpool destroy test && zpool create -o autotrim=on test da1 && zpool get autotrim test
NAME  PROPERTY  VALUE     SOURCE
test  autotrim  off       default
NAME  PROPERTY  VALUE     SOURCE
test  autotrim  on        default

On the 2nd run, when autotrim enabled, property should be listed al "local" instead of default.
Comment 1 Yuri Pankov freebsd_committer freebsd_triage 2023-05-12 00:12:33 UTC
Note that default value is "on", so the pool created without passing autotrim option should report "on default", and the pool created with autotrim=off should report "off local" (same for values set after creation).
Comment 2 Anton Saietskii 2023-06-04 14:01:43 UTC
(In reply to Yuri Pankov from comment #1)
>> Note that default value is "on"
Interesting, zpoolprops(7) in releng/13.2 says the opposite:
> $ man zpoolprops | col -bx | grep -A6 autotrim
>     autotrim=on|off
>             When set to on space which has been recently freed, and is no
>             longer allocated by the pool, will be periodically trimmed.  This
>             allows block device vdevs which support BLKDISCARD, such as SSDs,
>             or file vdevs on which the underlying file system supports hole-
>             punching, to reclaim unused blocks.  The default value for this
>             property is off.
Comment 3 Yuri Pankov freebsd_committer freebsd_triage 2023-06-04 15:06:46 UTC
(In reply to Anton Saietskii from comment #2)
Yes, it is a bug in documentation.  Please see the following commit:

https://github.com/openzfs/zfs/commit/5e7eaf8fbda348db9878ce013ecb4ab5e9b5cf5a
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-06-29 20:18:17 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d2a45e9e817ad68f3e163d13404744b8ea9c675b

commit d2a45e9e817ad68f3e163d13404744b8ea9c675b
Author:     Yuri Pankov <yuripv@FreeBSD.org>
AuthorDate: 2023-06-29 18:22:50 +0000
Commit:     Yuri Pankov <yuripv@FreeBSD.org>
CommitDate: 2023-06-29 20:14:18 +0000

    openzfs: use IN_BASE instead of IN_FREEBSD_BASE in spa.h

    Consistently use IN_BASE to allow libzfs to get the same default
    autotrim value as kernel does.

    Note that this does not change the default value itself, rather
    fixing the source of value and the value itself in e.g. zpool get
    output if it was not set explicitly.  (And as a reminder, default
    value of autotrim on FreeBSD is 'on', despite what zpoolprops(7)
    says currently.)

    PR:             264234
    Reviewed by:    imp
    Differential Revision:  https://reviews.freebsd.org/D40075

 sys/contrib/openzfs/include/sys/spa.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 5 Anton Saietskii 2023-06-30 13:14:41 UTC
(In reply to commit-hook from comment #4)
Few questions here:
1. Is there a plan for MFC? (I would like to see MFS as well, but understand that change may be not important enough.)
2. Should the patch apply to releng/13.2 right away? Perhaps I'll decide to fix this issue immediately.
3. Is there a plan to fix zpoolprops(7)?
Comment 6 Yuri Pankov freebsd_committer freebsd_triage 2023-07-01 06:56:38 UTC
(In reply to Anton Saietskii from comment #5)
Sorry, I had to revert the change for the moment, as I clearly didn't think it through (got confused by defines naming).
Comment 7 commit-hook freebsd_committer freebsd_triage 2023-07-18 09:22:35 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b36f469a15ecf508a2fdc2b58a76f0f2b4a0b88d

commit b36f469a15ecf508a2fdc2b58a76f0f2b4a0b88d
Author:     Yuri Pankov <yuripv@FreeBSD.org>
AuthorDate: 2023-07-17 09:12:53 +0000
Commit:     Yuri Pankov <yuripv@FreeBSD.org>
CommitDate: 2023-07-18 09:20:11 +0000

    zfs: set autotrim default to 'off'

    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
    Reviewed by:    mav (zfs)
    Differential Revision: https://reviews.freebsd.org/D41056

 sys/conf/kern.pre.mk                            | 3 +--
 sys/contrib/openzfs/include/sys/spa.h           | 6 ------
 sys/contrib/openzfs/module/zcommon/zpool_prop.c | 2 +-
 sys/modules/zfs/Makefile                        | 2 +-
 4 files changed, 3 insertions(+), 10 deletions(-)
Comment 8 Yuri Pankov freebsd_committer freebsd_triage 2023-07-18 09:24:58 UTC
Just a note here: autotrim was really defaulting to 'off' (due to confusion with defines) despite the attempt to keep it as 'on' when switching to openzfs, so man page is correct (even if by coincidence).
Comment 9 Mark Johnston freebsd_committer freebsd_triage 2024-10-30 15:00:47 UTC
On main:

# zpool create test md0 && zpool get autotrim test && zpool destroy test && zpool create -o autotrim=on test md0 && zpool get autotrim test
NAME  PROPERTY  VALUE     SOURCE
test  autotrim  off       default
NAME  PROPERTY  VALUE     SOURCE
test  autotrim  on        local

Is there anything left to do?
Comment 10 Anton Saietskii 2024-10-30 15:10:09 UTC
(In reply to Mark Johnston from comment #9)

MFC maybe?