Skip to content

Conversation

robn
Copy link
Member

@robn robn commented Apr 8, 2025

Motivation and Context

Replacing #16900, which was almost finished with review updates but has stalled. I've been asked to take it over.

See original PR for details.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@robn
Copy link
Member Author

robn commented Apr 8, 2025

Because there will be a little bouncing between two PRs, and because there's two different authors involved, I'll be pushing fixup commits to this branch. Once everyone is happy with review, I will squash them down for merge.

I'll close out the remaining review comments on #16900, and would like it if new comments could be added here. Thanks all for your patience; I know its a bit fiddly (it'd be nicer if Github would allow a PR to change branches, alas).

@robn robn mentioned this pull request Apr 8, 2025
13 tasks
@robn robn force-pushed the raidz-detect-slow-outlier branch from 905c466 to 4a1a3d3 Compare April 8, 2025 04:15
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Apr 8, 2025
@tonyhutter
Copy link
Contributor

I haven't looked into it, but I see all the FreeBSD runners have:

Tests with results other than PASS that are unexpected:
    SKIP events/setup (expected PASS)
    SKIP events/slow_vdev_sit_out (expected PASS)

@tonyhutter
Copy link
Contributor

tonyhutter commented May 15, 2025

@robn this fixed the FreeBSD CI errors for me: tonyhutter@a491397. Try squashing that in and rebasing.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up this work! It'd be great if we can refine this a bit more and get it integrated.

@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 2 times, most recently from 83d0de9 to 6fa275b Compare July 28, 2025 23:59
@pcd1193182
Copy link
Contributor

pcd1193182 commented Aug 5, 2025

I've updated this branch to fix a few things. First, I've significantly reduced (hopefully eliminated almost entirely) the unnecessary sit-outs Brian was reporting. We reproduced them internally during our performance testing and the updated version doesn't display them at all. The changes here are to use the latency histogram stats instead of the EWMA as a better source of data. We also decrease the check frequency dramatically to reduce noise, decrease the number of outliers to compensate (along with adding a facility for extreme events to increase their outlier count more rapidly), increase the fence value significantly, and add a decay mechanism to prevent random noise from eventually causing a sit-out of healthy disks.

Second, I've added an autosit property to vdevs. When set to on, it activates this code. This allows users to decide if they want this logic enabled or not. This is intended to work with the next change:

Third, I've made the sitout property writeable. This allows individual vdevs to be sat out from userland. This, in conjunction with the autosit property, allows the user to decide if they want no disk sit-outs, the kernel's automatic sit-outs, or to do something more complex. Using zpool iostat latency data, SMART stats, or any other data source they can think of, they could now create a userland daemon that monitors disk health and sits out disks that it feels are unhealthy.

Giving the capability to this in userland has a number of advantages: easier access to high-level languages and their rich libraries, more safe and rapid iteration of complex logic, and the ability to improve the logic using new developments and advanced approaches without requiring a kernel upgrade or downtime. The kernel functionality is left in place as a simple plug-and-play approach.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that histograms should indeed be a better source of data, comparing to EWMA, except as I mention below, we may take a closer look when we update the previous state. Same time, I wonder if histograms may actually give us even more statistical information about the distribution curves, so that we could better estimate the confidence interval on a small number of disks.

@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 2 times, most recently from e620961 to 9138823 Compare August 6, 2025 23:52
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the amount of churn in this PR it'd be nice to squash the commits the next time it's updated.

@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 2 times, most recently from b4ad263 to 7013cb6 Compare August 7, 2025 21:50
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch from 0e3ae43 to 56cc562 Compare August 26, 2025 16:38
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch from 713ac04 to afbd724 Compare August 27, 2025 23:45
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 3 times, most recently from 11e6837 to 611d87b Compare September 2, 2025 20:31
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 3 times, most recently from 397c375 to 4ed592e Compare September 3, 2025 23:06
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch 3 times, most recently from a33ff6c to 6b529ac Compare September 8, 2025 22:43
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 9, 2025
Paul Dagnelie added 2 commits September 9, 2025 13:57
A single slow responding disk can affect the overall read
performance of a raidz group.  When a raidz child disk is
determined to be a persistent slow outlier, then have it
sit out during reads for a period of time. The raidz group
can use parity to reconstruct the data that was skipped.

Each time a slow disk is placed into a sit out period, its
`vdev_stat.vs_slow_ios count` is incremented and a zevent
class `ereport.fs.zfs.delay` is posted.

The length of the sit out period can be changed using the
`raid_read_sit_out_secs` module parameter.  Setting it to
zero disables slow outlier detection.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Paul Dagnelie <[email protected]>
Contributions-by: Don Brady <[email protected]>
Contributions-by: Brian Behlendorf <[email protected]>
@pcd1193182 pcd1193182 force-pushed the raidz-detect-slow-outlier branch from 6b529ac to 97393b1 Compare September 9, 2025 21:10
@github-actions github-actions bot removed the Status: Accepted Ready to integrate (reviewed, tested) label Sep 9, 2025
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 10, 2025
@robn
Copy link
Member Author

robn commented Sep 11, 2025

@pcd1193182 thanks for picking up this often-dropped PR and getting it over the line. Thanks all for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants