[Pkg-zfsonlinux-devel] Bug#986185: closed by Debian FTP Masters <ftpmaster at ftp-master.debian.org> (reply to Mo Zhou <lumin at debian.org>) (Bug#986185: fixed in zfs-linux 2.0.3-3)

[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.

On Fri, 2021-04-02 at 12:20 +0200, наб wrote:
> 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 () {