[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:
Depends
depends on SERVER-39021 Switch migrations to observe multi-st... Closed
Related
related to SERVER-33577 Remove UninterruptibleLockGuards in s... Closed
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
https://github.com/mongodb/mongo/blob/dcf7e0dd89d34f58b592f1adb3d41e5edd6e2012/src/mongo/db/s/migration_destination_manager.cpp#L1150
https://github.com/mongodb/mongo/blob/dcf7e0dd89d34f58b592f1adb3d41e5edd6e2012/src/mongo/db/s/migration_source_manager.cpp#L444
https://github.com/mongodb/mongo/blob/dcf7e0dd89d34f58b592f1adb3d41e5edd6e2012/src/mongo/db/s/migration_source_manager.cpp#L480
https://github.com/mongodb/mongo/blob/dcf7e0dd89d34f58b592f1adb3d41e5edd6e2012/src/mongo/db/s/migration_source_manager.cpp#L679

Functions that will now be protected by the CSRLock:

CSS::enterCriticalSectionCatchUpPhase (Collection X Lock, CSR X Lock)
CSS::enterCriticalSectionCommitPhase (Collection X Lock, CSR X Lock)
CSS::exitCriticalSection (Collection X IX Lock, CSR X Lock)
CSS::getCurrentMetadataIfKnown (Collection X IS or IX Lock?, CSR IS Lock)* Need further clarification on the collection locks necessary to be used here. Multiple cases use either IX or IS.
CSR::setFilteringMetadata (Collection X Lock, CSR X Lock)
CSR::clearFilteringMetadata (Collection X IX Lock, CSR X Lock)
CollectionCriticalSection::constructor (Collection X Lock, CSR X Lock)
CollectionCriticalSection::destructor (Collection X IX Lock, CSR X Lock)
CollectionCriticalSection::enterCommitPhase (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/18

After 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
MigrationSourceManager::commitChunkMetadataOnConfig error handling section 1
MigrationSourceManager::commitChunkMetadataOnConfig error handling section 2

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)
CSS::enterCriticalSectionCommitPhase (Collection X Lock, CSR X Lock)
CSS::exitCriticalSection (Collection X IX Lock, CSR X Lock)
CollectionCriticalSection::constructor (Collection X Lock, CSR X Lock)
CollectionCriticalSection::destructor (Collection X IX Lock, CSR X Lock)
CollectionCriticalSection::enterCommitPhase (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: SERVER-38828 Use condition_variable::wait with a predicate to safeguard against spurious wakeups
Branch: master
https://github.com/mongodb/mongo/commit/5c25d74c596fcba4a3dee6a6f4294718bbd1cbd6

Comment by Githook User [ 21/Feb/19 ]

Author:

{'name': 'Blake Oler', 'email': 'blake.oler@mongodb.com', 'username': 'BlakeIsBlake'}

Message: SERVER-38828 Introduce CollectionShardingRuntimeLock usage to collection critical sections.

Allows us to reduce MODE_X collection locks to MODE_IX when under UninterruptibleLockGuards
Branch: master
https://github.com/mongodb/mongo/commit/5cbcc18f3c1f100deb7124d3d665901e473134b1

Comment by Blake Oler [ 07/Feb/19 ]

After discussion with kaloian.manassiev and renctan offline, we have decided the following:

  1. Use a counter to track the outstanding onCommit handlers in the chunk cloner.
  2. Wait on a condition variable in the chunk cloner's destructor (or in a separate drain() method) so that we will block on the completion of commit handlers before we remove the cloner's memory.

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.

If we add use of the CSRLock to the onCommit callback handlers as well ...

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 ]

Discovery

After discussion with renctan, we discovered that a race condition exists (found in BF-11995 and the reason for the previous reversion).

Problem Explanation

When 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.

Solution

As 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:

  1. The chunk cloner object still exists.
  2. An onCommit callback takes the CSRLock in intent shared mode.
  3. The MigrationSourceManager attempts to uninstall the chunk cloner. Because it needs the CSRLock in exclusive mode, it must wait for the onCommit callback to complete.
  4. The onCommit callback checks if the chunk cloner still exists (it does) and calls into the cloner to observe the operation. The onCommit callback completes.
  5. The MigrationSourceManager takes the CSRLock in exclusive mode and successfully uninstalls the chunk cloner.

Scenario Two:

  1. The chunk cloner object has been removed.
  2. An onCommit callback takes the CSRLock in intent shared mode.
  3. The onCommit callback sees that the chunk cloner does not exist, and backs out of the operation.

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 Dependencies

Introducing 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 SERVER-39021 to go in. With SERVER-39021, we will be able to observe transactions, and pass in opCtx es, at transaction commit time instead of CRUD operation time.

Comment by Githook User [ 31/Jan/19 ]

Author:

{'name': 'Blake Oler', 'email': 'blake.oler@mongodb.com', 'username': 'BlakeIsBlake'}

Message: Revert "SERVER-38828 Introduce CollectionShardingRuntimeLock usage to collection critical sections."

This reverts commit c2fb213da54e20a87a96816449e070623bdafe61.
Branch: master
https://github.com/mongodb/mongo/commit/c15a40aa4eaee67e060ac63256998232deb97c38

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
UninterruptibleLockGuards.
Branch: master
https://github.com/mongodb/mongo/commit/c2fb213da54e20a87a96816449e070623bdafe61

Comment by Githook User [ 31/Jan/19 ]

Author:

{'username': 'benety', 'email': 'benety@mongodb.com', 'name': 'Benety Goh'}

Message: SERVER-38828 fix lint
Branch: master
https://github.com/mongodb/mongo/commit/d3e18b2aa9105c2bba0f70f2fa4d446d63965ea2

Comment by Blake Oler [ 07/Jan/19 ]

kaloian.manassiev outlined planned functions to safeguard with the CSRLock in this ticket. LGTY?

Generated at Thu Feb 08 04:50:10 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.