[SERVER-38828] Replace uses of UninterruptibleLockGuard and MODE_X collection locks with uses of the CSRLock and MODE_IX collection locks where necessary. Created: 03/Jan/19 Updated: 29/Oct/23 Resolved: 21/Feb/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Sharding |
| Affects Version/s: | 4.1.6 |
| Fix Version/s: | 4.1.9 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Blake Oler | Assignee: | Blake Oler |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | prepare_interruptibility, uninterruptibleLockGuardRemoval | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||
| Sprint: | Sharding 2019-01-14, Sharding 2019-01-28, Sharding 2019-02-11, Sharding 2019-02-25 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 0 | ||||||||||||||||
| Description |
|
Access to CollectionShardingRuntime state can be safeguarded by blocking on access to the mutex added to the CollectionShardingRuntime. This mutex is accessed by taking the new CSRLock. This will unblock these uses of the UninterruptibleLockGuard. https://github.com/mongodb/mongo/blob/dcf7e0dd89d34f58b592f1adb3d41e5edd6e2012/src/mongo/db/s/collection_sharding_runtime.cpp#L190 Functions that will now be protected by the CSRLock: CSS::enterCriticalSectionCatchUpPhase (Collection X Lock, CSR X Lock) *An exception will be made with a ..._withoutLock function for CSS::CSSMap::report, because we already hold the CSSMap mutex here, and don't want to take collection locks for the general reporting of the state of the CSSMap. Update 1/22/18After discussion with kaloian.manassiev and renctan we have decided to back out of some of the intended changes. Since the CollectionShardingRuntime's _metadataManager variable has an internal mutex, we do not need to take the CSRLock in cases where we are only interacting with that variable. We can simply downgrade the collection lock to MODE_IX in these cases. This includes: MigrationDestinationManager::_forgetPending In MigrationSourceManager::cleanup we will take the collection IX lock, and take the CSRLock in exclusive mode to protect changes to state variables (MigrationSourceManager, CriticalSection) on the CollectionShardingRuntime. We will still need to modify function signatures around the critical section to offer synchronization with modification of the critical section. These changes still apply: CSS::enterCriticalSectionCatchUpPhase (Collection X Lock, CSR X Lock) as well as a CSRLock in MODE_IS in a section in CollectionShardingState::checkShardVersionOrThrow where we check the state of the critical section variable. |
| Comments |
| Comment by Githook User [ 22/Feb/19 ] |
|
Author: {'name': 'Blake Oler', 'username': 'BlakeIsBlake', 'email': 'blake.oler@mongodb.com'}Message: |
| Comment by Githook User [ 21/Feb/19 ] |
|
Author: {'name': 'Blake Oler', 'email': 'blake.oler@mongodb.com', 'username': 'BlakeIsBlake'}Message: Allows us to reduce MODE_X collection locks to MODE_IX when under UninterruptibleLockGuards |
| Comment by Blake Oler [ 07/Feb/19 ] |
|
After discussion with kaloian.manassiev and renctan offline, we have decided the following:
Doing this will allow us to avoid using the CSRLock inside the cloner, which would be a layer violation. |
| Comment by Kaloian Manassiev [ 06/Feb/19 ] |
|
blake.oler, does this problem only occur in cases where the migration chunk cloner source is being uninstalled due to error? (i.e., the migration never entered the critical section). What I want to make sure is that upon leaving of the critical section there are never onCommit handlers registered, because that would imply there is a bug where we missed document changes or we are registering onCommit handlers for updates not just for the chunk being moved.
Does this mean that the migration chunk cloner source would know about the CSR lock? Because I think this is kind of a layering violation. How about adding a drain() method to the cloner (or even to its destructor) to ensure that all registered onCommit handlers have completed instead? |
| Comment by Blake Oler [ 02/Feb/19 ] |
DiscoveryAfter discussion with renctan, we discovered that a race condition exists (found in BF-11995 and the reason for the previous reversion). Problem ExplanationWhen we run a migration, we install a chunk cloner object on the source shard (in the MigrationSourceManager). This object will listen for and queue up any operations to be done to the collection while it's migrating. As part of this process, when writes/updates/deletes are observed, a callback is registered on the RecoveryUnit's commit handler. When the RecoveryUnit registers commit, it will call into the chunk cloner to add said operation to the list of queued operations. In this callback, the RecoveryUnit is handed a raw pointer to the chunk cloner object. This previous process was fine before the changes in this ticket. However, in this ticket, we changed the collection locking from X to IX when removing the chunk cloner object from the source shard's MigrationSourceManager. The exclusive lock would previously imply that we wait for all onCommit callback handlers to be called before removing the chunk cloner. The onCommit callback handlers take a lock on the collection, thus requiring the exclusive lock request to wait for all other users of the lock to finish before uninstalling the chunk cloner. With the change to the IX lock, it is possible to uninstall the chunk cloner before some onCommit callback handlers have run. In that case, it implies that the callback handler will have a stale (read: no longer existing) chunk cloner pointer. SolutionAs part of this ticket, we add more fine-grained synchronization with the CollectionShardingRuntimeLock (CSRLock). When we are uninstalling the chunk cloner, we require the CSRLock to be taken in exclusive mode. If we add use of the CSRLock to the onCommit callback handlers as well, we can prevent any memory issues. We can change the process to create two possible scenarios. New behavior will be marked in bold. Scenario One:
Scenario Two:
One might question the validity of backing out of observing an operation because the chunk cloner has been removed. However, this is perfectly fine for our use case. The scenario where the chunk cloner can be removed while writes are still coming into the collection is a scenario only ever found during error. In a successful migration, the MigrationSourceManager will wait for the exclusive lock in order to enter the critical section, ensuring all previous writes are seen and preventing new writes from entering that collection. If we are cleaning up in-memory variables due to an error during cloning, we don't care if we miss observing writes. If the migration has been cancelled, these writes still exist on the original shard. New DependenciesIntroducing this new area of locking gives us a new dependency. We will now need to pass the opCtx to the onCommit callback handlers. This is so the callbacks can access the CollectionShardingRuntime and the corresponding CSRLock. This creates issues during multi-document transaction writes. The opCtx variables during a given CRUD statement and then during commit will be different opCtx }}es completely – the former {{opCtx es will go out of scope before the onCommit callback can be run. As such, we need to pass in only the transaction-commit-time opCtx. This requires us to wait for |
| Comment by Githook User [ 31/Jan/19 ] |
|
Author: {'name': 'Blake Oler', 'email': 'blake.oler@mongodb.com', 'username': 'BlakeIsBlake'}Message: Revert " This reverts commit c2fb213da54e20a87a96816449e070623bdafe61. |
| Comment by Kelsey Schubert [ 31/Jan/19 ] |
|
Original fix: Author: {'username': 'BlakeIsBlake', 'email': 'blake.oler@mongodb.com', 'name': 'Blake Oler'}Message: Introduce CollectionShardingRuntimeLock usage to collection critical sections. Allows us to reduce MODE_X collection locks to MODE_IX when under |
| Comment by Githook User [ 31/Jan/19 ] |
|
Author: {'username': 'benety', 'email': 'benety@mongodb.com', 'name': 'Benety Goh'}Message: |
| Comment by Blake Oler [ 07/Jan/19 ] |
|
kaloian.manassiev outlined planned functions to safeguard with the CSRLock in this ticket. LGTY? |