This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.1
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.1 by this push:
new 8dc7b8a6e17 branch-4.1: [fix](cloud) Avoid lock inversion in delete
bitmap checks #69892 (#69968)
8dc7b8a6e17 is described below
commit 8dc7b8a6e173faad5e38e4f3c82ecdfd91349f58
Author: bobhan1 <[email protected]>
AuthorDate: Wed Nov 4 10:38:42 2026 +0800
branch-4.1: [fix](cloud) Avoid lock inversion in delete bitmap checks
#69892 (#69968)
### What problem does this PR solve?
Related PR: #69892
Backport #69892 to `branch-4.1` from commit
`b2164ff1bacd3e467fd29f443ef9c03faa899658`.
Background delete bitmap checks acquire the bitmap read lock before the
tablet header lock, while compaction acquires them in the opposite
order. The checks also run under the tablet map mutex, so a blocked
tablet can stall unrelated tablet initialization, Rowsets scans,
synchronization, and compaction scheduling.
Read the sequence-column flag before bitmap traversal and run checks
over the existing tablet snapshot outside the map mutex. Preserve the
active and stale rowset version checks for sequence and non-sequence
tables.
Test context conflicts were adapted to the existing 4.1 fixture, and all
five new upstream test instances are preserved. Explicitly include
`cpp/sync_point.h` in `base_tablet.cpp` so production builds have the
definition of `TEST_SYNC_POINT_CALLBACK`; the original unit-test
configuration did not expose the missing include.
### Validation
- ASAN BE unit tests after the include fix: **47 passed, 0 failures, 0
errors**, verified in the GTest XML report.
- Production ASAN BE build with `MAKE_TEST=OFF`, `ENABLE_PCH=OFF`, and
`ENABLE_INJECTION_POINT=OFF`: **passed**, including compilation,
linking, and packaging via `./build.sh --be -j100`.
```bash
./run-be-ut.sh --run
--filter='SequenceModes/CloudTabletDeleteBitmapCheckTest.*:CloudTabletMgrTest.*:CloudTabletDeleteBitmapTest.*:TabletMetaTest.*:CloudCompactionTest.*'
-j100
```
- Changed-line clang-format 16 and `git diff --check
upstream/branch-4.1...HEAD` passed.
### Release note
Fix BE deadlocks and cascading write and metadata stalls when background
delete bitmap checks are enabled.
### Check List (For Author)
- Test
- [x] Unit Test
- Behavior changed:
- [x] Yes. Delete bitmap checks avoid reversed lock ordering and run
outside the tablet map mutex.
- Does this need documentation?
- [x] No. No new setting or syntax.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
---
be/src/cloud/cloud_tablet_mgr.cpp | 6 +-
be/src/storage/tablet/base_tablet.cpp | 10 ++-
be/test/cloud/cloud_tablet_mgr_test.cpp | 48 ++++++++++++++
be/test/cloud/cloud_tablet_test.cpp | 111 ++++++++++++++++++++++++++++++++
4 files changed, 172 insertions(+), 3 deletions(-)
diff --git a/be/src/cloud/cloud_tablet_mgr.cpp
b/be/src/cloud/cloud_tablet_mgr.cpp
index 9b021c6b7dd..1e10cb243fd 100644
--- a/be/src/cloud/cloud_tablet_mgr.cpp
+++ b/be/src/cloud/cloud_tablet_mgr.cpp
@@ -349,7 +349,9 @@ void CloudTabletMgr::vacuum_stale_rowsets(const
CountDownLatch& stop_latch) {
int64_t max_useless_rowset_version_count = 0;
int64_t tablet_id_with_max_useless_rowset_version_count = 0;
OlapStopWatch watch;
- _tablet_map->traverse([&](auto&& tablet) {
+ // Reuse the snapshot outside TabletMap's mutex: a check waiting
on one tablet's
+ // header lock must not block loading or enumerating unrelated
tablets.
+ for (const auto& tablet : tablets_to_remove_unused_rowsets) {
int64_t useless_rowset_count = 0;
int64_t useless_rowset_version_count = 0;
tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowset_count,
@@ -362,7 +364,7 @@ void CloudTabletMgr::vacuum_stale_rowsets(const
CountDownLatch& stop_latch) {
max_useless_rowset_version_count =
useless_rowset_version_count;
tablet_id_with_max_useless_rowset_version_count =
tablet->tablet_id();
}
- });
+ }
g_max_rowsets_with_useless_delete_bitmap.set_value(max_useless_rowset_count);
g_max_rowsets_with_useless_delete_bitmap_version.set_value(
max_useless_rowset_version_count);
diff --git a/be/src/storage/tablet/base_tablet.cpp
b/be/src/storage/tablet/base_tablet.cpp
index af494004bbc..5c6667087c3 100644
--- a/be/src/storage/tablet/base_tablet.cpp
+++ b/be/src/storage/tablet/base_tablet.cpp
@@ -37,6 +37,7 @@
#include "common/status.h"
#include "core/assert_cast.h"
#include "core/data_type/data_type_factory.hpp"
+#include "cpp/sync_point.h"
#include "load/memtable/memtable.h"
#include "service/point_query_executor.h"
#include "storage/compaction/cumulative_compaction_time_series_policy.h"
@@ -2074,6 +2075,13 @@ void
BaseTablet::check_agg_delete_bitmap_for_stale_rowsets(int64_t& useless_rows
},
true);
+ // Compaction holds the header lock while merging the delete bitmap. Read
the schema
+ // before taking the bitmap lock so the traversal cannot acquire these
locks in reverse.
+ const bool has_sequence_col = tablet_schema()->has_sequence_col();
+ TEST_SYNC_POINT_CALLBACK(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ this);
+
std::set<RowsetId> useless_rowsets;
std::map<RowsetId, std::vector<int64_t>> useless_rowset_versions;
{
@@ -2090,7 +2098,7 @@ void
BaseTablet::check_agg_delete_bitmap_for_stale_rowsets(int64_t& useless_rows
return -1;
}
if (end_versions.find(version) == end_versions.end()) {
- if (tablet_schema()->has_sequence_col()) {
+ if (has_sequence_col) {
auto rowset_version = rowset_it->second;
if (version >= rowset_version.first &&
version <= rowset_version.second) {
diff --git a/be/test/cloud/cloud_tablet_mgr_test.cpp
b/be/test/cloud/cloud_tablet_mgr_test.cpp
index ac956dbc19a..1838cc9c544 100644
--- a/be/test/cloud/cloud_tablet_mgr_test.cpp
+++ b/be/test/cloud/cloud_tablet_mgr_test.cpp
@@ -22,7 +22,9 @@
#include <gtest/gtest.h>
#include <atomic>
+#include <chrono>
#include <condition_variable>
+#include <future>
#include <memory>
#include <mutex>
#include <thread>
@@ -92,6 +94,52 @@ TEST_F(CloudTabletMgrTest,
VacuumReclaimsWarmupStatesWithoutStaleOrUnusedRowsets
EXPECT_EQ(tablet->get_rowset_warmup_state(rowset.rowset_id()).progress,
WarmUpProgress::NONE);
}
+TEST_F(CloudTabletMgrTest, DeleteBitmapCheckDoesNotHoldTabletMapLock) {
+ const bool old_check =
config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap;
+ auto sp = SyncPoint::get_instance();
+ sp->clear_all_call_backs();
+ Defer cleanup([&] {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap =
old_check;
+ });
+ config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap = true;
+
+ CloudTabletMgr mgr(_engine);
+ auto tablet = std::make_shared<CloudTablet>(_engine, _tablet_meta);
+ mgr.put_tablet_for_UT(tablet);
+ TabletMetaSharedPtr other_meta(new TabletMeta(
+ 1, 2, _tablet_meta->tablet_id() + 1, 15674, 4, 5, TTabletSchema(),
6, {{7, 8}},
+ UniqueId(9, 10), TTabletType::TABLET_TYPE_DISK,
TCompressionType::LZ4F));
+ auto other_tablet = std::make_shared<CloudTablet>(_engine, other_meta);
+
+ CountDownLatch check_entered(1);
+ CountDownLatch resume_check(1);
+ sp->set_call_back(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ [&](auto&&) {
+ check_entered.count_down();
+ resume_check.wait();
+ });
+ sp->enable_processing();
+
+ CountDownLatch stop_latch(1);
+ auto vacuum = std::async(std::launch::async, [&] {
mgr.vacuum_stale_rowsets(stop_latch); });
+ Defer unblock_check([&] { resume_check.count_down(); });
+ ASSERT_TRUE(check_entered.wait_for(std::chrono::seconds(10)));
+
+ auto map_access = std::async(std::launch::async, [&] {
+ mgr.put_tablet_for_UT(other_tablet);
+ return mgr.get_weak_tablets().size();
+ });
+ const auto map_status = map_access.wait_for(std::chrono::seconds(10));
+ // Release the check before joining, so the old locking behavior fails
without hanging UT.
+ resume_check.count_down();
+ vacuum.get();
+ EXPECT_EQ(map_status, std::future_status::ready);
+ EXPECT_EQ(map_access.get(), 2);
+}
+
// Test concurrent get_tablet calls for the same tablet_id.
// Reproduces bug where tablet ends up in cache but not in _tablet_map.
//
diff --git a/be/test/cloud/cloud_tablet_test.cpp
b/be/test/cloud/cloud_tablet_test.cpp
index 913d98e794b..533e39ca1b4 100644
--- a/be/test/cloud/cloud_tablet_test.cpp
+++ b/be/test/cloud/cloud_tablet_test.cpp
@@ -39,6 +39,8 @@
#include "storage/rowset/rowset_factory.h"
#include "storage/rowset/rowset_meta.h"
#include "storage/tablet/tablet_meta.h"
+#include "util/countdown_latch.h"
+#include "util/defer_op.h"
#include "util/uid_util.h"
namespace doris {
@@ -337,6 +339,115 @@ TEST_F(CloudTabletDeleteBitmapTest,
AggDeleteBitmapForCompactionReturnsPreRowset
EXPECT_EQ(rowset_versions_without_stats, pre_rowset_to_versions);
}
+class CloudTabletDeleteBitmapCheckTest : public CloudTabletWarmUpStateTest,
+ public
testing::WithParamInterface<bool> {
+public:
+ void SetUp() override {
+ CloudTabletWarmUpStateTest::SetUp();
+ TColumn key;
+ key.__set_column_name("k");
+ key.__set_is_key(true);
+ key.column_type.type = TPrimitiveType::BIGINT;
+ TColumn value;
+ value.__set_column_name(GetParam() ? SEQUENCE_COL : "v");
+ value.__set_is_key(false);
+ value.__set_aggregation_type(TAggregationType::REPLACE);
+ value.column_type.type = TPrimitiveType::BIGINT;
+ TTabletSchema schema;
+ schema.__set_short_key_column_count(1);
+ schema.__set_schema_hash(4);
+ schema.__set_keys_type(TKeysType::UNIQUE_KEYS);
+ schema.__set_columns({key, value});
+ schema.__set_sequence_col_idx(GetParam() ? 1 : -1);
+ _tablet_meta.reset(new TabletMeta(1, 2, 15673, 15674, 4, 5, schema, 2,
{{0, 0}, {1, 1}},
+ UniqueId(9, 10),
TTabletType::TABLET_TYPE_DISK,
+ TCompressionType::LZ4F, 0, true));
+ _tablet = std::make_shared<CloudTablet>(_engine, _tablet_meta);
+ }
+};
+
+TEST_P(CloudTabletDeleteBitmapCheckTest,
DoesNotAcquireHeaderLockDuringBitmapTraversal) {
+ auto rowset = create_rowset(Version(2, 4));
+ ASSERT_NE(rowset, nullptr);
+ _tablet->_rs_version_map.emplace(rowset->version(), rowset);
+ auto& delete_bitmap = _tablet->tablet_meta()->delete_bitmap();
+ roaring::Roaring bitmap;
+ bitmap.add(1);
+ // Version 3 is not a rowset end version, so both schema variants exercise
the sequence check.
+ delete_bitmap.set({rowset->rowset_id(), 0, 3}, bitmap);
+
+ auto sp = SyncPoint::get_instance();
+ sp->clear_all_call_backs();
+ Defer cleanup([sp] {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ });
+ CountDownLatch snapshot_ready(1);
+ CountDownLatch resume_check(1);
+ sp->set_call_back(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ [&](auto&&) {
+ snapshot_ready.count_down();
+ resume_check.wait();
+ });
+ sp->enable_processing();
+
+ int64_t useless_rowsets = 0;
+ int64_t useless_versions = 0;
+ auto check = std::async(std::launch::async, [&] {
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ });
+ Defer unblock_check([&] { resume_check.count_down(); });
+ ASSERT_TRUE(snapshot_ready.wait_for(seconds(10)));
+
+ // Model compaction's exclusive header ownership after the check has
captured its rowsets.
+ std::unique_lock header_lock(_tablet->get_header_lock());
+ resume_check.count_down();
+ const auto check_status = check.wait_for(seconds(10));
+ if (check_status == std::future_status::ready) {
+ // Even an empty compaction bitmap takes the DBM write lock while
holding the header lock.
+ DeleteBitmap empty_bitmap(_tablet->tablet_id());
+ delete_bitmap.merge(empty_bitmap);
+ }
+ // Let the old callback finish after a timeout instead of leaving a
deadlocked test thread.
+ header_lock.unlock();
+ check.get();
+ EXPECT_EQ(check_status, std::future_status::ready);
+ EXPECT_EQ(useless_rowsets, 0);
+ EXPECT_EQ(useless_versions, GetParam() ? 0 : 1);
+}
+
+TEST_P(CloudTabletDeleteBitmapCheckTest,
PreservesActiveAndStaleRowsetVersionChecks) {
+ auto active = create_rowset(Version(2, 4));
+ auto stale = create_rowset(Version(5, 7));
+ auto orphan = create_rowset(Version(8, 8));
+ ASSERT_TRUE(active && stale && orphan);
+ _tablet->_rs_version_map.emplace(active->version(), active);
+ _tablet->_stale_rs_version_map.emplace(stale->version(), stale);
+ auto& delete_bitmap = _tablet->tablet_meta()->delete_bitmap();
+ roaring::Roaring bitmap;
+ bitmap.add(1);
+ delete_bitmap.set({active->rowset_id(), 0, 3}, bitmap);
+ delete_bitmap.set({active->rowset_id(), 0, 4}, bitmap);
+ delete_bitmap.set({stale->rowset_id(), 0, 6}, bitmap);
+ delete_bitmap.set({stale->rowset_id(), 0, 7}, bitmap);
+ delete_bitmap.set({orphan->rowset_id(), 0, 8}, bitmap);
+
+ int64_t useless_rowsets = 0;
+ int64_t useless_versions = 0;
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ EXPECT_EQ(useless_rowsets, 1);
+ EXPECT_EQ(useless_versions, GetParam() ? 0 : 2);
+
+ // A version outside the rowset's range is still useless for a sequence
table.
+ delete_bitmap.set({active->rowset_id(), 0, 1}, bitmap);
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ EXPECT_EQ(useless_rowsets, 1);
+ EXPECT_EQ(useless_versions, GetParam() ? 1 : 2);
+}
+
+INSTANTIATE_TEST_SUITE_P(SequenceModes, CloudTabletDeleteBitmapCheckTest,
testing::Bool());
+
// Test get_rowset_warmup_state for non-existent rowset
TEST_F(CloudTabletWarmUpStateTest, TestGetRowsetWarmupStateNonExistent) {
auto rowset = create_rowset(Version(1, 1));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
