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,
наб
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 () {
signature.asc
Description: PGP signature
