dnode_sync is careless with range tree #10823
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
Because dnode_sync_free_range() must drop dn_mtx during its processing, using it as a callback to range_tree_vacate() is not safe. No other operations (besides destroy) are allowed once range_tree_vacate() has begun, and dropping dn_mtx would leave a window open for another thread to observe that invalid (and unsafe) state via dnode_block_freed(). Signed-off-by: Patrick Mooney <pmooney@oxide.computer> Closes openzfs#10708
ahrens
approved these changes
Aug 26, 2020
behlendorf
approved these changes
Aug 26, 2020
behlendorf
added
Status: Accepted
ikozhukhov
approved these changes
Aug 26, 2020
|
i hope it'll fix my one old known panic too |
Codecov Report
@@ Coverage Diff @@
## master #10823 +/- ##
==========================================
+ Coverage 79.69% 79.78% +0.09%
==========================================
Files 395 395
Lines 125044 125045 +1
==========================================
+ Hits 99654 99773 +119
+ Misses 25390 25272 -118
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
behlendorf
pushed a commit
that referenced
this pull request
Aug 27, 2020
Because dnode_sync_free_range() must drop dn_mtx during its processing, using it as a callback to range_tree_vacate() is not safe. No other operations (besides destroy) are allowed once range_tree_vacate() has begun, and dropping dn_mtx would leave a window open for another thread to observe that invalid (and unsafe) state via dnode_block_freed(). Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Igor Kozhukhov <igor@dilos.org> Signed-off-by: Patrick Mooney <pmooney@oxide.computer> Closes #10708 Closes #10823
behlendorf
moved this from Backport for OpenZFS 2.0
to FreeBSD Features (In Progress)
in OpenZFS 2.0
jsai20
pushed a commit
to jsai20/zfs
that referenced
this pull request
Mar 30, 2021
Because dnode_sync_free_range() must drop dn_mtx during its processing, using it as a callback to range_tree_vacate() is not safe. No other operations (besides destroy) are allowed once range_tree_vacate() has begun, and dropping dn_mtx would leave a window open for another thread to observe that invalid (and unsafe) state via dnode_block_freed(). Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Igor Kozhukhov <igor@dilos.org> Signed-off-by: Patrick Mooney <pmooney@oxide.computer> Closes openzfs#10708 Closes openzfs#10823
sempervictus
pushed a commit
to sempervictus/zfs
that referenced
this pull request
May 31, 2021
Because dnode_sync_free_range() must drop dn_mtx during its processing, using it as a callback to range_tree_vacate() is not safe. No other operations (besides destroy) are allowed once range_tree_vacate() has begun, and dropping dn_mtx would leave a window open for another thread to observe that invalid (and unsafe) state via dnode_block_freed(). Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Igor Kozhukhov <igor@dilos.org> Signed-off-by: Patrick Mooney <pmooney@oxide.computer> Closes openzfs#10708 Closes openzfs#10823
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.
Because dnode_sync_free_range() must drop dn_mtx during its processing,
using it as a callback to range_tree_vacate() is not safe. No other
operations (besides destroy) are allowed once range_tree_vacate() has
begun, and dropping dn_mtx would leave a window open for another thread
to observe that invalid (and unsafe) state via dnode_block_freed().
Signed-off-by: Patrick Mooney pmooney@oxide.computer
Closes #10708
Motivation and Context
While running a specific workload under a DEBUG illumos kernel, we observed panics in
dnode_block_freed()as it accessed what was then an invalid zfs btree. This btree was undergoing arange_tree_vacate()operation from another thread, which had droppeddn_mtxon the dnode while it synced the free state. This is written up in #10708 and illumos #13034.Description
Since the underlying implementation of
range_tree_vacate()states that while the vacate is under way, no other operations are valid on the data structure, using a callback to perform the sync as part of vacate is impossible to do safely. Instead a separaterange_tree_walk()call can perform the syncs (which drop thedn_mtx) followed by a callback-lessrange_tree_vacate()call which empties that element ofdn_free_ranges[]under the safety ofdn_mtx.How Has This Been Tested?
This bug was only observed under very specific circumstances (a HVM workload atop a zvol, while running a DEBUG kernel). Re-running the workload was a fairly reliable reproducer, so the absence of a crash when running a DEBUG kernel featuring the fix was taken as a positive result.
Types of changes
Checklist:
Signed-off-by.