Fix memory leak for ColumnFamily drop with live iterator #7749
Conversation
Summary: Uncommon bug seen by ASAN with ColumnFamilyTest.LiveIteratorWithDroppedColumnFamily, if the last two references to a ColumnFamilyData are both SuperVersions (during InstallSuperVersion). The fix is to use UnrefAndTryDelete even in SuperVersion::Cleanup but with a parameter to avoid re-entering Cleanup on the same SuperVersion being cleaned up. Test Plan: ./column_family_test --gtest_filter=*LiveIter* --gtest_repeat=100
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Hi @pdillinger 👋 |
Dropping a column family is not normally done in response to service user data, but for maintenance or internal data migrations. |
| if (cfd->Unref()) { | ||
| delete cfd; | ||
| } | ||
| cfd->UnrefAndTryDelete(this); |
There was a problem hiding this comment.
Just a question: when does ColumnFamilyData have different SuperVersion?
I thought they're always point to each other for the same CF.
There was a problem hiding this comment.
From new comment
// (But while installing a new SuperVersion, this
// cfd could be referenced only by two SuperVersions.)
There was a problem hiding this comment.
oh, I see, one is the new SV, another one is the old SV.
| // If called under SuperVersion::Cleanup, we should not re-enter Cleanup on | ||
| // the same SuperVersion. (But while installing a new SuperVersion, this | ||
| // cfd could be referenced only by two SuperVersions.) | ||
| if (old_refs == 2 && super_version_ != nullptr && | ||
| super_version_ != sv_under_cleanup) { |
There was a problem hiding this comment.
If sv_under_cleanup is only used by SuperVersion::Cleanup(), should this logic also move to there? I feel having a SuperVersion a parameter for UnrefAndTryDelete() is a little bit confusing.
There was a problem hiding this comment.
When sv_under_cleanup is non-null, we might exclude this code. We might need the code below from any caller. If you have a specific proposal to simplify, let me know. This code is not as simple or clear as I would prefer.
There was a problem hiding this comment.
My suggestion would be changing SuperVersion::Cleanup() to like:
if (cfd->GetSuperVersion() == this) {
if (cfd->Unref()) {
delete cfd;
}
} else {
cfd->UnrefAndTryDelete();
}
At least, it won't make UnrefAndTryDelete more complicate and checking the SuperVersion within SuperVersion::cleanup.
There was a problem hiding this comment.
We would either need to keep the Unref() function around, which I fear invites further bugs, or make SuperVersion a friend of ColumnFamilyData. Making a friend suggests we would be violating the natural encapsulation for ColumnFamilyData.
|
@pdillinger merged this pull request in b1ee191. |
Summary: Uncommon bug seen by ASAN with ColumnFamilyTest.LiveIteratorWithDroppedColumnFamily, if the last two references to a ColumnFamilyData are both SuperVersions (during InstallSuperVersion). The fix is to use UnrefAndTryDelete even in SuperVersion::Cleanup but with a parameter to avoid re-entering Cleanup on the same SuperVersion being cleaned up. ColumnFamilyData::Unref is considered unsafe so removed. Pull Request resolved: facebook#7749 Test Plan: ./column_family_test --gtest_filter=*LiveIter* --gtest_repeat=100 Reviewed By: jay-zhuang Differential Revision: D25354304 Pulled By: pdillinger fbshipit-source-id: e78f3a3f67c40013b8432f31d0da8bec55c5321c
Summary: The `ColumnFamilyData::UnrefAndTryDelete` code currently on the trunk unlocks the DB mutex before destroying the `ThreadLocalPtr` holding the per-thread `SuperVersion` pointers when the only remaining reference is the back reference from `super_version_`. The idea behind this was to break the circular dependency between `ColumnFamilyData` and `SuperVersion`: when the penultimate reference goes away, `ColumnFamilyData` can clean up the `SuperVersion`, which can in turn clean up `ColumnFamilyData`. (Assuming there is a `SuperVersion` and it is not referenced by anything else.) However, unlocking the mutex throws a wrench in this plan by making it possible for another thread to jump in and take another reference to the `ColumnFamilyData`, keeping the object alive in a zombie `ThreadLocalPtr`-less state. This can cause issues like #8440 , #8382 , and might also explain the `was_last_ref` assertion failures from the `ColumnFamilySet` destructor we sometimes observe during close in our stress tests. Digging through the archives, this unlocking goes way back to 2014 (or earlier). The original rationale was that `SuperVersionUnrefHandle` used to lock the mutex so it can call `SuperVersion::Cleanup`; however, this logic turned out to be deadlock-prone. #3510 fixed the deadlock but left the unlocking in place. #6147 then introduced the circular dependency and associated cleanup logic described above (in order to enable iterators to keep the `ColumnFamilyData` for dropped column families alive), and moved the unlocking-relocking snippet to its present location in `UnrefAndTryDelete`. Finally, #7749 fixed a memory leak but apparently exacerbated the race by (otherwise correctly) switching to `UnrefAndTryDelete` in `SuperVersion::Cleanup`. The patch simply eliminates the unlocking and relocking, which has been unnecessary ever since #3510 made `SuperVersionUnrefHandle` lock-free. This closes the window during which another thread could increase the reference count, and hopefully fixes the issues above. Pull Request resolved: #8605 Test Plan: Ran `make check` and stress tests locally. Reviewed By: pdillinger Differential Revision: D30051035 Pulled By: ltamasi fbshipit-source-id: 8fe559e4b4ad69fc142579f8bc393ef525918528
Summary: The `ColumnFamilyData::UnrefAndTryDelete` code currently on the trunk unlocks the DB mutex before destroying the `ThreadLocalPtr` holding the per-thread `SuperVersion` pointers when the only remaining reference is the back reference from `super_version_`. The idea behind this was to break the circular dependency between `ColumnFamilyData` and `SuperVersion`: when the penultimate reference goes away, `ColumnFamilyData` can clean up the `SuperVersion`, which can in turn clean up `ColumnFamilyData`. (Assuming there is a `SuperVersion` and it is not referenced by anything else.) However, unlocking the mutex throws a wrench in this plan by making it possible for another thread to jump in and take another reference to the `ColumnFamilyData`, keeping the object alive in a zombie `ThreadLocalPtr`-less state. This can cause issues like #8440 , #8382 , and might also explain the `was_last_ref` assertion failures from the `ColumnFamilySet` destructor we sometimes observe during close in our stress tests. Digging through the archives, this unlocking goes way back to 2014 (or earlier). The original rationale was that `SuperVersionUnrefHandle` used to lock the mutex so it can call `SuperVersion::Cleanup`; however, this logic turned out to be deadlock-prone. #3510 fixed the deadlock but left the unlocking in place. #6147 then introduced the circular dependency and associated cleanup logic described above (in order to enable iterators to keep the `ColumnFamilyData` for dropped column families alive), and moved the unlocking-relocking snippet to its present location in `UnrefAndTryDelete`. Finally, #7749 fixed a memory leak but apparently exacerbated the race by (otherwise correctly) switching to `UnrefAndTryDelete` in `SuperVersion::Cleanup`. The patch simply eliminates the unlocking and relocking, which has been unnecessary ever since #3510 made `SuperVersionUnrefHandle` lock-free. This closes the window during which another thread could increase the reference count, and hopefully fixes the issues above. Pull Request resolved: #8605 Test Plan: Ran `make check` and stress tests locally. Reviewed By: pdillinger Differential Revision: D30051035 Pulled By: ltamasi fbshipit-source-id: 8fe559e4b4ad69fc142579f8bc393ef525918528
Summary: Uncommon bug seen by ASAN with
ColumnFamilyTest.LiveIteratorWithDroppedColumnFamily, if the last two
references to a ColumnFamilyData are both SuperVersions (during
InstallSuperVersion). The fix is to use UnrefAndTryDelete even in
SuperVersion::Cleanup but with a parameter to avoid re-entering Cleanup
on the same SuperVersion being cleaned up.
ColumnFamilyData::Unref is considered unsafe so removed.
Test Plan: ./column_family_test --gtest_filter=LiveIter --gtest_repeat=100