periodic/daily/801.trim-zfs: Add a daily zfs trim script #956
Conversation
| if ! zpool status "${pool}" | grep -q '(trimming)'; then | ||
| echo " starting trim of pool '${pool}'" | ||
| zpool trim "${pool}" | ||
| [ $rc -eq 0 ] && rc=1 |
There was a problem hiding this comment.
I don't understand this line
There was a problem hiding this comment.
this is to indicate informational output from the script (exit 1) but only if we didn't already encounter a more serious error (exit 3 or 4). i copied this from 800.scrub-zfs, but it seems to be the method that most periodic scripts use to do this (or at least roughly similar).
|
|
||
| if ! zpool status "${pool}" | grep -q '(trimming)'; then | ||
| echo " starting trim of pool '${pool}'" | ||
| zpool trim "${pool}" |
There was a problem hiding this comment.
Generally, we have knobs to control how the trim happens. For example 130.clean-msgs has a parameter daily_clean_msgs_days which controls the number of days. Other things have flags that can be passed to this program or that.
There's only two options to zpool trim that might be relevant. One is -d/--secure which would allow secure erase of the trimmed data. I'm not sure we even support that. The other is -r/--rate which specifies the maximum rate. That we support. Based on that, I'd add a daily_trim_zfs_rate variable and pass it -r to the zpool trim. You an see what 130.clean-msgs does for an example. Had there been more options, I'd opt for the more generic daily_trim_zfs_flags to match the rc.d pattern, defaulting that to nothing and unconditionally passing that in.
There was a problem hiding this comment.
i'm inclined to do this with daily_trim_zfs_flags anyway, even if there's only a single useful option right now, since this means less work for users later if more flags are added - does that seem reasonable?
|
Generally I think this is a good idea, and the implementation is pretty close: just one item i'd be tempted to add. Also, I added the 'changes requested' label, which just means that we're talking about details and I should return to this once new replies come in. |
|
sorry, i accidentally pushed a commit to this PR that was meant for a different branch, i think i've successfully un-done that. |
If it fixes a PR, we should MFC it (to where the PR was reported in) |
As mentioned in zpoolprops(7), on some SSDs, it may not be desirable to use ZFS autotrim because a large number of trim requests can degrade disk performance; instead, the pool should be manually trimmed at regular intervals. Add a new daily periodic script for this purpose, 801.trim-zfs. If enabled (daily_trim_zfs_enable=YES; the default is NO), it will run a 'zpool trim' operation on all online pools, or on the pools listed in 'daily_trim_zfs_pools'. The trim is not started if the pool is degraded (which matches the behaviour of the existing 800.scrub-zfs script) or if a trim is already running on that pool. Having autotrim enabled does not inhibit the periodic trim; it's sometimes desirable to run periodic trims even with autotrim enabled, because autotrim can elide trims for very small regions. PR: 275965 MFC after: 2 weeks
|
i've added i also modified the script to not exit with rc 1 if a trim was started, because i think this is not "notable information" as described in periodic(8). it will still show the output, it just won't be included in the periodic mail if rebased on current main at the same time. |
As mentioned in zpoolprops(7), on some SSDs, it may not be desirable to use ZFS autotrim because a large number of trim requests can degrade disk performance; instead, the pool should be manually trimmed at regular intervals. Add a new daily periodic script for this purpose, 801.trim-zfs. If enabled (daily_trim_zfs_enable=YES; the default is NO), it will run a 'zpool trim' operation on all online pools, or on the pools listed in 'daily_trim_zfs_pools'. The trim is not started if the pool is degraded (which matches the behaviour of the existing 800.scrub-zfs script) or if a trim is already running on that pool. Having autotrim enabled does not inhibit the periodic trim; it's sometimes desirable to run periodic trims even with autotrim enabled, because autotrim can elide trims for very small regions. PR: 275965 MFC after: 1 week Reviewed by: imp Pull Request: #956
|
I pushed something bad trying to autoclose this |
|
and now that it's closed, things work better. |
As mentioned in zpoolprops(7), on some SSDs, it may not be desirable to use ZFS autotrim because a large number of trim requests can degrade disk performance; instead, the pool should be manually trimmed at regular intervals. Add a new daily periodic script for this purpose, 801.trim-zfs. If enabled (daily_trim_zfs_enable=YES; the default is NO), it will run a 'zpool trim' operation on all online pools, or on the pools listed in 'daily_trim_zfs_pools'. The trim is not started if the pool is degraded (which matches the behaviour of the existing 800.scrub-zfs script) or if a trim is already running on that pool. Having autotrim enabled does not inhibit the periodic trim; it's sometimes desirable to run periodic trims even with autotrim enabled, because autotrim can elide trims for very small regions. PR: 275965 MFC after: 1 week Reviewed by: imp Pull Request: #956 (cherry picked from commit 493908c)
As mentioned in zpoolprops(7), on some SSDs, it may not be desirable to use ZFS autotrim because a large number of trim requests can degrade disk performance; instead, the pool should be manually trimmed at regular intervals.
Add a new daily periodic script for this purpose, 801.trim-zfs. If enabled (daily_trim_zfs_enable=YES; the default is NO), it will run a 'zpool trim' operation on all online pools, or on the pools listed in 'daily_trim_zfs_pools'.
The trim is not started if the pool is degraded (which matches the behaviour of the existing 800.scrub-zfs script) or if a trim is already running on that pool. Having autotrim enabled does not inhibit the periodic trim; it's sometimes to desirable to run periodic trims even with autotrim enabled, because autotrim can elide trims for very small regions.
PR: 275965