Fix hash_lock / keystore.sk_dk_lock lock inversion #7115
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
tcaputi
approved these changes
Feb 2, 2018
module/zfs/dsl_crypt.c
Outdated
| /* Lookup the key in the tree of currently loaded keys */ | ||
| rw_enter(&spa->spa_keystore.sk_dk_lock, RW_READER); | ||
| ret = spa_keystore_dsl_key_hold_impl(spa, dckobj, tag, &dck_ks); | ||
| rw_exit(&spa->spa_keystore.sk_dk_lock); | ||
| if (!ret) { |
There was a problem hiding this comment.
probably should change this to ret == 0 while we're here.
The keystore.sk_dk_lock should not be held while performing I/O. Drop the lock when reading from disk and update the code so they the first successful caller adds the lock. Improve error handling in spa_keystore_create_mapping_impl(). Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> TEST_ZTEST_TIMEOUT=3600 Issue openzfs#7112
behlendorf
force-pushed
the
ztest-keystore-deadlock2
branch
from
1cacf7d to
b2d2ea0
Compare
|
@tcaputi I've refresh this can you look at it again. |
Codecov Report
@@ Coverage Diff @@
## master #7115 +/- ##
==========================================
- Coverage 75.43% 75.33% -0.11%
==========================================
Files 296 296
Lines 95912 95910 -2
==========================================
- Hits 72355 72253 -102
- Misses 23557 23657 +100
Continue to review full report at Codecov.
|
|
Zlooped for the last ~4h, no sudden death yet. |
|
is this going to be included in 0.7.6 release? |
Encryption is not going to be included at all until 0.8.0. |
Nasf-Fan
pushed a commit
to Nasf-Fan/zfs
that referenced
this pull request
Feb 13, 2018
The keystore.sk_dk_lock should not be held while performing I/O. Drop the lock when reading from disk and update the code so they the first successful caller adds the key. Improve error handling in spa_keystore_create_mapping_impl(). Reviewed by: Thomas Caputi <tcaputi@datto.com> Reviewed-by: RageLtMan <rageltman@sempervictus> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7112 Closes openzfs#7115
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.
Description
The keystore.sk_dk_lock should not be held while performing I/O. Drop the lock when reading from disk and update the code so they the first successful caller adds the key.
Motivation and Context
#7112
How Has This Been Tested?
Locally built, overnight ztest run underway.
Types of changes
Checklist:
Signed-off-by.