Skip to content

Commit

Permalink
[BACKPORT 2.8][#10304] docdb: fix deadlock in ProcessTabletReportBatch
Browse files Browse the repository at this point in the history
Summary:
Since commit b14485a ([#8229] backup:
repartition table if needed on YSQL restore), there is a rare deadlock
issue between ProcessTabletReportBatch and RepartitionTable.  It can be
hit when

- thread 1 (RepartitionTable): import a YSQL snapshot where the number
  of tablets for a table mismatch between the cluster and the external
  snapshot
- thread 2 (ProcessTabletReportBatch): process tablets of that table
  from heartbeat

A more in-depth sequence of steps follows:

1. t1: table->LockForWrite (WriteLock)
1. t2: table->LockForRead (ReadLock)
1. t1: tablet->StartMutation (WriteLock)
2. t1: table_lock.Commit (UpgradeToCommitLock; blocks on t2 ReadLock)
3. t2: tablet->LockForWrite (WriteLock; blocks on t1 WriteLock)

To fix, for ProcessTabletReportBatch, take table write lock instead of
read lock.  The table metadata isn't mutated, so this is purely for
deadlock avoidance reasons (since only one writer is allowed at a time).

Bogdan thinks we should expect table write lock to be taken whenever
tablet write lock is taken.

Original Commit: cc90f01

Original Differential Revision: https://phabricator.dev.yugabyte.com/D13459

Test Plan:
    ./yb_build.sh \
      --cxx-test tools_yb-backup-test_ent \
      --gtest_filter YBBackupTest.TestYSQLChangeDefaultNumTablets \
      -n 1000 \
      --tp 1 \
      fastdebug

Reviewers: nicolas

Reviewed By: nicolas

Subscribers: bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D13730
  • Loading branch information
jaki committed Nov 1, 2021
1 parent 64ec460 commit ef35333
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
18 changes: 10 additions & 8 deletions src/yb/master/catalog_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5554,7 +5554,7 @@ bool CatalogManager::ProcessCommittedConsensusState(
TSDescriptor* ts_desc,
bool is_incremental,
const ReportedTabletPB& report,
const TableInfo::ReadLock& table_lock,
const TableInfo::WriteLock& table_lock,
const TabletInfoPtr& tablet,
const TabletInfo::WriteLock& tablet_lock,
std::vector<RetryingTSRpcTaskPtr>* rpcs) {
Expand Down Expand Up @@ -5753,13 +5753,15 @@ Status CatalogManager::ProcessTabletReportBatch(
ReportedTablets::const_iterator end,
TabletReportUpdatesPB* full_report_update,
std::vector<RetryingTSRpcTaskPtr>* rpcs) {
// 1. First Pass. Iterate in TabletId Order to discover all Table locks we'll need.
// 1. First Pass. Iterate in TabletId Order to discover all Table locks we'll need. Even though
// read locks are sufficient here, take write locks since we'll be writing to the tablet while
// holding this.
// Need to acquire both types of locks in Id order to prevent deadlock.
std::map<TableId, TableInfo::ReadLock> table_read_locks;
std::map<TableId, TableInfo::WriteLock> table_write_locks;
for (auto it = begin; it != end; ++it) {
auto& lock = table_read_locks[it->info->table()->id()];
auto& lock = table_write_locks[it->info->table()->id()];
if (!lock.locked()) {
lock = it->info->table()->LockForRead();
lock = it->info->table()->LockForWrite();
}
}

Expand All @@ -5780,7 +5782,7 @@ Status CatalogManager::ProcessTabletReportBatch(

// Get tablet lock on demand. This works in the batch case because the loop is ordered.
tablet_write_locks[tablet_id] = tablet->LockForWrite();
auto& table_lock = table_read_locks[table->id()];
auto& table_lock = table_write_locks[table->id()];
auto& tablet_lock = tablet_write_locks[tablet_id];

TRACE_EVENT1("master", "HandleReportedTablet", "tablet_id", report.tablet_id());
Expand Down Expand Up @@ -5887,10 +5889,10 @@ Status CatalogManager::ProcessTabletReportBatch(
} // Finished one round of batch processing.

// 7. Unlock the tables; we no longer need to access their state.
for (auto& l : table_read_locks) {
for (auto& l : table_write_locks) {
l.second.Unlock();
}
table_read_locks.clear();
table_write_locks.clear();

// 8. Write all tablet mutations to the catalog table.
//
Expand Down
2 changes: 1 addition & 1 deletion src/yb/master/catalog_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -1551,7 +1551,7 @@ class CatalogManager :
TSDescriptor* ts_desc,
bool is_incremental,
const ReportedTabletPB& report,
const TableInfo::ReadLock& table_lock,
const TableInfo::WriteLock& table_lock,
const TabletInfoPtr& tablet,
const TabletInfo::WriteLock& tablet_lock,
std::vector<RetryingTSRpcTaskPtr>* rpcs);
Expand Down

0 comments on commit ef35333

Please sign in to comment.