os/bluestore: fix SharedBlob unregistration #18805
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
ifed01
reviewed
Nov 8, 2017
| @@ -1670,7 +1670,7 @@ void BlueStore::SharedBlob::put() | |||
| << " removing self from set " << get_parent() | |||
| << dendl; | |||
| if (get_parent()) { | |||
| if (get_parent()->remove(this)) { | |||
| if (get_parent()->try_remove(this)) { | |||
There was a problem hiding this comment.
Funny thing but you can use NEW remove here instead and get rid off try_remove at all. See if (--nrerf == 0) above...
There was a problem hiding this comment.
We don't take the lock until inside try_remove(), and we're handling the race between one thread dropping the last reference and trying to remove from the man and another thread calling lookup() and bumping the ref from 0 -> 1.
ifed01
approved these changes
Nov 9, 2017
We use the SharedBlobSet remove() in three cases: - from SharedBlob::put(), we try to remove ourselves from the set, but have to deal with a racing lookup, so the removal is conditional on nref still being 0. - from split_cache(), we move the SharedBlob to another collection - from make_blob_unshared(), we remove the entry when we clear the sbid. The problem is that the condtiional remove() (for the first case) was being used for all three cases, and in the second two cases nref is always != 0, so it doesn't actually happen. This can lead to a crash during cache shutdown. Fix by making two variants: remove() that is unconditional, and try_remove() that is conditional. Set the sb->coll pointer after because remove() asserts the parent matches where we are unregistering. Fixes: http://tracker.ceph.com/issues/22039 Signed-off-by: Sage Weil <sage@redhat.com>
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.
We use the SharedBlobSet remove() in three cases:
have to deal with a racing lookup, so the removal is conditional on
nref still being 0.
The problem is that the condtiional remove() (for the first case) was being
used for all three cases, and in the second two cases nref is always != 0,
so it doesn't actually happen. This can lead to a crash during cache
shutdown.
Fix by making two variants: remove() that is unconditional, and
try_remove() that is conditional.
Fixes: http://tracker.ceph.com/issues/22039
Signed-off-by: Sage Weil sage@redhat.com