Prevent unnecessary resilver restarts #9588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #9588 +/- ##
==========================================
- Coverage 79.19% 79.12% -0.08%
==========================================
Files 418 418
Lines 123531 123535 +4
==========================================
- Hits 97828 97743 -85
- Misses 25703 25792 +89
Continue to review full report at Codecov.
|
|
@jwpoduska |
jwpoduska
force-pushed
the
reresilver
branch
from
8202186 to
53fff3f
Compare
|
@Ornias1993 right you are, thanks. vdev_clear() still needs to remove spares, even if the resilver has finished, so I added the poorly named async task SPA_ASYNC_RESILVER_DONE to do it, if a resilver isn't in progress or scheduled. Previously, it would always kick off a new resilver or even a deferred resilver after the current one, which would do a resilver scan where nothing got updated. |
behlendorf
approved these changes
Nov 21, 2019
There was a problem hiding this comment.
Nice! This looks great, and works as intended in my local testing. My only suggestion would be to add a test which specifically tests the scenarios you manually tested. This particular issue has caused a fair bit of trouble and we want to make sure it stays fixed!
PrivatePuffin
approved these changes
Nov 21, 2019
jgallag88
approved these changes
Nov 22, 2019
module/zfs/vdev.c
Outdated
| * If a leaf vdev has a DTL, and seems healthy, then kick off a | ||
| * resilver. But don't do this if we are doing a reopen for a scrub, | ||
| * since this would just restart the scrub we are already doing. | ||
| * If this is a leaf vdev, assesss whether a resilver is needed. |
There was a problem hiding this comment.
[nit] One too many 's's in assesss
If a device is participating in an active resilver, then it will have a
non-empty DTL. Operations like vdev_{open,reopen,probe}() can cause the
resilver to be restarted (or deferred to be restarted later), which is
unnecessary if the DTL is still covered by the current scan range. This
is similar to the logic in vdev_dtl_should_excise() where the DTL can
only be excised if it's max txg is in the resilvered range.
Signed-off-by: John Poduska <[email protected]>
jwpoduska
force-pushed
the
reresilver
branch
from
53fff3f to
83b5d97
Compare
|
I added a test to check both with and without deferred resilvers. Both fail without this fix and succeed with it. |
behlendorf
added
Status: Accepted
|
@jwpoduska thanks for adding the test cases and getting to the bottom of this issue. Merged! |
This was referenced Dec 9, 2019
behlendorf
pushed a commit
that referenced
this pull request
Dec 10, 2019
The resilver restart test was reported as failing about 2% of the time. Two issues were found: - The event log wasn't large enough, so resilver events were missing - One 'zpool sync' wasn't enough for resilver to start after zinject Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: John Poduska <[email protected]> Issue #9588 Closes #9677 Closes #9703
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Jan 22, 2020
If a device is participating in an active resilver, then it will have a
non-empty DTL. Operations like vdev_{open,reopen,probe}() can cause the
resilver to be restarted (or deferred to be restarted later), which is
unnecessary if the DTL is still covered by the current scan range. This
is similar to the logic in vdev_dtl_should_excise() where the DTL can
only be excised if it's max txg is in the resilvered range.
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Gallagher <[email protected]>
Reviewed-by: Kjeld Schouten <[email protected]>
Signed-off-by: John Poduska <[email protected]>
Issue openzfs#840
Closes openzfs#9155
Closes openzfs#9378
Closes openzfs#9551
Closes openzfs#9588
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Jan 22, 2020
The resilver restart test was reported as failing about 2% of the time. Two issues were found: - The event log wasn't large enough, so resilver events were missing - One 'zpool sync' wasn't enough for resilver to start after zinject Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: John Poduska <[email protected]> Issue openzfs#9588 Closes openzfs#9677 Closes openzfs#9703
tonyhutter
pushed a commit
that referenced
this pull request
Jan 23, 2020
If a device is participating in an active resilver, then it will have a
non-empty DTL. Operations like vdev_{open,reopen,probe}() can cause the
resilver to be restarted (or deferred to be restarted later), which is
unnecessary if the DTL is still covered by the current scan range. This
is similar to the logic in vdev_dtl_should_excise() where the DTL can
only be excised if it's max txg is in the resilvered range.
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: John Gallagher <[email protected]>
Reviewed-by: Kjeld Schouten <[email protected]>
Signed-off-by: John Poduska <[email protected]>
Issue #840
Closes #9155
Closes #9378
Closes #9551
Closes #9588
tonyhutter
pushed a commit
that referenced
this pull request
Jan 23, 2020
The resilver restart test was reported as failing about 2% of the time. Two issues were found: - The event log wasn't large enough, so resilver events were missing - One 'zpool sync' wasn't enough for resilver to start after zinject Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: John Poduska <[email protected]> Issue #9588 Closes #9677 Closes #9703
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
ZFS is a little trigger happy about restarting resilvers. There are a number of issues around this, including:
9155, 9378 & 9551
Possibly even: 840
Description
If a device is participating in an active resilver, then it will have a non-empty DTL. Operations like vdev_{open,reopen,probe}() can cause the resilver to be restarted (or deferred to be restarted later), which is unnecessary if the DTL is still covered by the current scan range. This is similar to the logic in vdev_dtl_should_excise() where the DTL can only be excised if it's max txg is in the resilvered range.
The fix itself is really just the 'if' statement in dsl_scan_assess_vdev() to prevent requesting a resilver or deferred resilver when the max txg in the vdev's DTL is already participating in the current resilver.
This change includes some cleanup as well.
How Has This Been Tested?
Tested that incorrect re-resilvering was prevented by kicking off a resilver, and doing 'zpool reopen', 'zpool clean', export/import, zinject io errors, etc, and verifying that the resilver didn't restart.
Also, tested that resilver correctly happens by kicking off a resilver, offlining and onlining a vdev to introduce a DTL with a max that isn't covered by the current scan range, and verifying that the deferred resilver occurs after the current one finishes. Or, that a restart of resilver happens if the resilver_defer feature is not enabled.
Types of changes
Checklist:
Signed-off-by.