Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict raidz faulted vdev count #16569

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

don-brady
Copy link
Contributor

Motivation and Context

In a ZFS pool, leaf vdevs can be faulted after tripping a diagnostic failure (ZED). However ZFS prevents faulting a drive if it would cause data loss. For example, in a raidz2, faulting more than 2 disks would cause read failures.

In determining if a vdev can be faulted, the check in vdev_dtl_required() doesn't consider the case where a marginal drive is being resilvered. If the marginal disk is considered valid during the checks, then zfs can start encountering read errors when the faulted vdev count matches the parity count.

Description

When a drive replacement is active, the replacing vdev should not be considered when checking if a peer can be faulted. I.e., treat it as if it is offline and cannot contribute. Specifically, a child in a replacing vdev won't count when assessing the dtl during a vdev_fault().

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

How Has This Been Tested?

  1. Added a new test: functional/fault/fault_limits
  2. Manual testing

Example result from fault_limits showing that the third request to fault a leaf vdev in a raidz3 group was denied and turned instead to a degrade due to presence of a replacing vdev.

  pool: fault-test-pool
 state: DEGRADED
status: One or more devices are faulted in response to persistent errors.
        Sufficient replicas exist for the pool to continue functioning in a
        degraded state.
action: Replace the faulted device, or use 'zpool clear' to mark the device
        repaired.
  scan: resilvered 654K in 00:00:02 with 0 errors on Wed Sep 25 17:27:33 2024
config:

        NAME                  STATE     READ WRITE CKSUM
        fault-test-pool       DEGRADED     0     0     0
          raidz3-0            DEGRADED     0     0     0
            /var/tmp/dev-0    FAULTED      0     0     0  external device fault
            /var/tmp/dev-1    FAULTED      0     0     0  external device fault
            /var/tmp/dev-2    DEGRADED     0     0     0  external device fault
            /var/tmp/dev-3    ONLINE       0     0     0
            /var/tmp/dev-4    ONLINE       0     0     0
            /var/tmp/dev-5    ONLINE       0     0     0
            /var/tmp/dev-6    ONLINE       0     0     0
            /var/tmp/dev-7    ONLINE       0     0     0
            replacing-8       DEGRADED     0     0     0
              /var/tmp/dev-8  ONLINE       0     0     0
              /var/tmp/dev-9  OFFLINE      0     0     0

errors: No known data errors

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:

@behlendorf
Copy link
Contributor

@don-brady can you rebase this on the latest master code. We cut over to using GitHub Actions fully for the CI and this change needs to include those commits so it can run the tests.

@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing Component: ZED ZFS Event Daemon labels Sep 25, 2024
module/zfs/vdev.c Outdated Show resolved Hide resolved
tests/zfs-tests/tests/functional/fault/fault_limits.ksh Outdated Show resolved Hide resolved
tests/zfs-tests/tests/functional/fault/fault_limits.ksh Outdated Show resolved Hide resolved
@don-brady
Copy link
Contributor Author

Rebased to latest master branch

Copy link
Contributor

@mcmilk mcmilk left a comment

Choose a reason for hiding this comment

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

It looked right before the ZTS system was running, but afterwards - sth. does not work as intended I think :/

tests/zfs-tests/tests/functional/fault/fault_limits.ksh Outdated Show resolved Hide resolved
@behlendorf behlendorf self-requested a review September 28, 2024 16:24
@don-brady don-brady force-pushed the vdev-fault-limits branch 2 times, most recently from 31a4ab5 to 2d5059b Compare September 30, 2024 17:05
@don-brady
Copy link
Contributor Author

Fixed the failing test cases. Turns out that the original fix was breaking vdev replace.

@allanjude
Copy link
Contributor

Fixed the failing test cases. Turns out that the original fix was breaking vdev replace.

Thanks Don

Specifically, a child in a replacing vdev won't count when assessing
the dtl during a vdev_fault()

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Don Brady <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 1, 2024
@behlendorf behlendorf merged commit 141368a into openzfs:master Oct 1, 2024
19 of 21 checks passed
ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 11, 2024
Specifically, a child in a replacing vdev won't count when assessing
the dtl during a vdev_fault()

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tino Reichardt <[email protected]>
Signed-off-by: Don Brady <[email protected]>
Closes openzfs#16569
don-brady added a commit to don-brady/openzfs that referenced this pull request Dec 13, 2024
Similar to what we saw in openzfs#16569, we need to consider that a
replacing vdev should not be considered as fully contributing
to the redundancy of a raidz vdev even though current IO has
enough redundancy.

When a failed vdev_probe() is faulting a disk, it now checks
if that disk is required, and if so it suspends the pool until
the admin can return the missing disks.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Signed-off-by: Don Brady <[email protected]>
don-brady added a commit to don-brady/openzfs that referenced this pull request Dec 14, 2024
Similar to what we saw in openzfs#16569, we need to consider that a
replacing vdev should not be considered as fully contributing
to the redundancy of a raidz vdev even though current IO has
enough redundancy.

When a failed vdev_probe() is faulting a disk, it now checks
if that disk is required, and if so it suspends the pool until
the admin can return the missing disks.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Signed-off-by: Don Brady <[email protected]>
don-brady added a commit to don-brady/openzfs that referenced this pull request Dec 17, 2024
Similar to what we saw in openzfs#16569, we need to consider that a
replacing vdev should not be considered as fully contributing
to the redundancy of a raidz vdev even though current IO has
enough redundancy.

When a failed vdev_probe() is faulting a disk, it now checks
if that disk is required, and if so it suspends the pool until
the admin can return the missing disks.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Signed-off-by: Don Brady <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ZED ZFS Event Daemon Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants