Bug#986185: /lib/zfs-linux/trim: please provide an override to the new only-NVMe behaviour

[наб]

Package: zfsutils-linux
Version: 2.0.3-2
Severity: normal

Dear Maintainer,

The new autotrim script will fail for mixed HDD and NVMe pools,
but I assume you don't care about those, since you wrote it like this.

How-ever, I would still like to trim my pools that consist of SATA SSDs,
and that's now globally broken by default, on every machine.
A global "trim all pools" toggle or a list of pools to trim regardless
of their make-up would go a long way to fix this.

Well, that, or doing it the other way around and adding a list of pools
to /not/ trim, since that isn't a very common problem (and it's readily
apparent when it does manifest), and doesn't break every extant set-up.

Best,
наб

-- System Information:
Debian Release: bullseye/sid
APT prefers unstable
APT policy: (500, 'unstable')
Architecture: x32 (x86_64)
Foreign Architectures: amd64, i386

Kernel: Linux 5.10.0-5-amd64 (SMP w/2 CPU threads)
Kernel taint flags: TAINT_PROPRIETARY_MODULE, TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages zfsutils-linux depends on:
ii init-system-helpers 1.60
ii libblkid1 2.36.1-7
ii libc6 2.31-10
ii libnvpair3linux 2.0.3-2
ii libuuid1 2.36.1-7
ii libuutil3linux 2.0.3-2
ii libzfs4linux 2.0.3-2
ii libzpool4linux 2.0.3-2
ii python3 3.9.2-2

Versions of packages zfsutils-linux recommends:
ii lsb-base 11.1.0
ii zfs-dkms [zfs-modules] 2.0.3-2
ii zfs-zed 2.0.3-2

Versions of packages zfsutils-linux suggests:
pn nfs-kernel-server <none>
pn samba-common-bin <none>
pn zfs-initramfs | zfs-dracut <none>

-- Configuration Files:
/etc/sudoers.d/zfs [Errno 13] Permission denied: '/etc/sudoers.d/zfs'

-- no debconf information

[наб]

This looks great, thanks! You've definitely taken the right approach here.

Just a few nitpicks, see diff:
* no need to call modprobe, just check sysfs directly
(this is what modprobe does anyway)
* the proper name for this is "periodic-{scrub,trim}" ‒
"periodical" really doesn't work here in modern use
* get_property() was overly complex ‒
if the pool doesn't exist, zfs-get already exits with 1,
so just bubble it up through "|| return 1" to appease -e;
if the property isn't set, it just returns 0 and prints "-",
which we already check
* I also touched the comment up and linked to the zpool userprops PR

Also, please add note of this to the NEWS file, something like
-- >8 --
Starting with this version, the auto-scrub and auto-trim jobs will use
the "org.debian:periodic-{scrub,trim}" user properties on the pool's
root dataset to determine if they should do anything; accepted values
are:
* "auto" ‒ same as unset, use default checks
* "enable" ‒ always scrub/trim automatically
* "disable" ‒ never scrub/trim automatically
.
The default for auto-scrub is to scrub, as before,
but the default for auto-trim has changed: it will now only trim
if the pool consists of /only/ NVMe drives, since some SATA 2
and SATA 3.0 SSDs will hang or crash during large TRIMs (#983086) ‒
if your pools with SATA SSDs had no problems trimming before,
you will need to run
zfs set org.debian:periodic-trim=enable sata-pool
to restore previous behaviour.
-- >8 --
would be great, feel free to just steal this.

Best,
наб

[M. Zhou]

Hi,

Thanks for the super awesome feedback!

I indeed forgot to write a README.Debian [1] recording this
change and its usage. Let me prepare a revision and upload shortly.

[1] instead of NEWS, in order to avoid being too abruptive.

> diff --git a/debian/tree/zfsutils-linux/usr/lib/zfs-linux/scrub
> b/debian/tree/zfsutils-linux/usr/lib/zfs-linux/scrub
> index 91631cb18..cb4e3c07e 100755
> --- a/debian/tree/zfsutils-linux/usr/lib/zfs-linux/scrub
> +++ b/debian/tree/zfsutils-linux/usr/lib/zfs-linux/scrub
> @@ -1,27 +1,19 @@
>  #!/bin/sh -eu
>  
>  # directly exit successfully when zfs module is not loaded
> -if modprobe -n -q --first-time zfs; then
> +if ! [ -d /sys/module/zfs ]; then
>         exit 0
>  fi
>  
>  # [auto] / enable / disable
> -PROPERTY_NAME="org.debian:periodical-scrub"
> +PROPERTY_NAME="org.debian:periodic-scrub"
>  
>  get_property () {
> -       # Detect the ${PROPERTY_NAME} property from a given zpool
> -       # Note, we are abusing user-defined property on zpool root
> dataset
> -       # as "zpool user-defined property".
> +       # Detect the ${PROPERTY_NAME} property on a given pool
> +       # We are abusing user-defined properties on the root dataset,
> +       # since they're not available on pools
> https://github.com/openzfs/zfs/pull/11680
>         pool=$1
> -       if ! zfs list -H -o name "${pool}" 1>/dev/null 2>/dev/null;
> then
> -               return 1  # failed to find the root dataset
> -       fi
> -       if ! zfs get -H -o value "${PROPERTY_NAME}" "${pool}"
> 1>/dev/null 2>/dev/null; then
> -               return 1  # no such property
> -       else
> -               # has such property
> -               zfs get -H -o value "${PROPERTY_NAME}" "${pool}"
> -       fi
> +       zfs get -H -o value "${PROPERTY_NAME}" "${pool}" 2>/dev/null
> || return 1
>  }
>  
>  scrub_if_not_scrub_in_progress () {
> diff --git a/debian/tree/zfsutils-linux/usr/lib/zfs-linux/trim
> b/debian/tree/zfsutils-linux/usr/lib/zfs-linux/trim
> index 585a58baf..5a0216507 100755
> --- a/debian/tree/zfsutils-linux/usr/lib/zfs-linux/trim
> +++ b/debian/tree/zfsutils-linux/usr/lib/zfs-linux/trim
> @@ -1,27 +1,19 @@
> -#!/bin/sh -e
> +#!/bin/sh -eu
>  
>  # directly exit successfully when zfs module is not loaded
> -if modprobe -n -q --first-time zfs; then
> +if ! [ -d /sys/module/zfs ]; then
>         exit 0
>  fi
>  
>  # [auto] / enable / disable
> -PROPERTY_NAME="org.debian:periodical-trim"
> +PROPERTY_NAME="org.debian:periodic-trim"
>  
>  get_property () {
> -       # Detect the ${PROPERTY_NAME} property from a given zpool
> -       # Note, we are abusing user-defined property on zpool root
> dataset
> -       # as "zpool user-defined property".
> -       pool=$1
> -       if ! zfs list -H -o name "${pool}" 1>/dev/null 2>/dev/null;
> then
> -               return 1  # failed to find the root dataset
> -       fi
> -       if ! zfs get -H -o value "${PROPERTY_NAME}" "${pool}"
> 1>/dev/null 2>/dev/null; then
> -               return 1  # no such property
> -       else
> -               # has such property
> -               zfs get -H -o value "${PROPERTY_NAME}" "${pool}"
> -       fi
> +    # Detect the ${PROPERTY_NAME} property on a given pool
> +    # We are abusing user-defined properties on the root dataset,
> +    # since they're not available on pools
> https://github.com/openzfs/zfs/pull/11680
> +    pool=$1
> +    zfs get -H -o value "${PROPERTY_NAME}" "${pool}" 2>/dev/null ||
> return 1
>  }
>  
>  trim_if_not_already_trimming () {

[наб]

On Fri, Apr 02, 2021 at 12:05:16PM +0000, M. Zhou wrote:
> [1] instead of NEWS, in order to avoid being too abruptive.

I do think this is definitely NEWS-worthy ‒ you're affecting existing
set-ups in ways that are hard to notice and could potentially lead
to premature media failure or, at the very least, significant
performance degradation.

I mean, I'm a maniac and I do read all changelogs, but the /vast/
majority of people don't, and this feels like the prime use of NEWS.

But I agree, having this in README.Debian would be good as well.

Best,
наб