This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push: new 1c69ec608 [tablet] fix race in RowSetMetadata::id() 1c69ec608 is described below commit 1c69ec6081a88dc531ebaa60b19abd4c0e54ae47 Author: Alexey Serbin <ale...@apache.org> AuthorDate: Tue Sep 10 09:42:55 2024 -0700 [tablet] fix race in RowSetMetadata::id() This patch fixes a race in access to the RowSetMetadata::id_ field in the rollback scenario in the MajorCompactDeltaStoresWithColumnIds() method of the DiskRowSet class. Before this patch, TSAN would report warnings like below when running the MultiThreadedHybridClockTabletTest.UpdateNoMergeCompaction scenario: of the mt-tablet-test: Read of size 8 at 0x7b3400014780 by thread T30 (mutexes: write M76293278759445 9152, write M7098002): #0 kudu::tablet::RowSetMetadata::id() const src/kudu/tablet/rowset_metadata.h:100:31 (libtablet.so+0x346faa) #1 kudu::tablet::RowSetTree::Reset(...) src/kudu/tablet/rowset_tree.cc:190:48 (libtablet.so+0x4bf666) #2 kudu::tablet::Tablet::ModifyRowSetTree(...) src/kudu/tablet/tablet.cc:1490:3 (libtablet.so+0x323755) #3 kudu::tablet::Tablet::AtomicSwapRowSetsUnlocked(...) src/kudu/tablet/tablet.cc:1504:3 (libtablet.so+0x3239bc) #4 kudu::tablet::Tablet::AtomicSwapRowSets(...) src/kudu/tablet/tablet.cc:1496:3 (libtablet.so+0x3238f9) ... Previous write of size 8 at 0x7b3400014780 by thread T12 (mutexes: write M625572878699880144, write M530715863088620288, write M525367769810683784): #0 kudu::tablet::RowSetMetadata::LoadFromPB(...) src/kudu/tablet/rowset_metadata.cc:77:7 (libtablet.so+0x4f9f03) #1 kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0::operator()() const src/kudu/tablet/diskrowset.cc:603:23 (libtablet.so+0x46eddf) #2 kudu::ScopedCleanup<kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...)::$_0>::~ScopedCleanup() src/kudu/util/scoped_cleanup.h:51:7 (libtablet.so+0x46cc5a) #3 kudu::tablet::DiskRowSet::MajorCompactDeltaStoresWithColumnIds(...) src/kudu/tablet/diskrowset.cc:636:1 (libtablet.so+0x46c5c9) #4 kudu::tablet::DiskRowSet::MajorCompactDeltaStores(...) src/kudu/tablet/diskrowset.cc:570:10 (libtablet.so+0x46c013) ... SUMMARY: ThreadSanitizer: data race src/kudu/tablet/rowset_metadata.h:100:31 in kudu::tablet::RowSetMetadata::id() const Change-Id: I4b09575616e754b7dbb24586293f128e361b9360 Reviewed-on: http://gerrit.cloudera.org:8080/21779 Reviewed-by: Mahesh Reddy <mre...@cloudera.com> Tested-by: Alexey Serbin <ale...@apache.org> Reviewed-by: Yingchun Lai <laiyingc...@apache.org> --- src/kudu/tablet/cfile_set.cc | 10 ++++------ src/kudu/tablet/rowset_metadata.cc | 12 ++++++++---- src/kudu/tablet/rowset_metadata.h | 10 ++++++++-- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/kudu/tablet/cfile_set.cc b/src/kudu/tablet/cfile_set.cc index 48ed9e26f..1bcdfaaea 100644 --- a/src/kudu/tablet/cfile_set.cc +++ b/src/kudu/tablet/cfile_set.cc @@ -137,18 +137,16 @@ Status CFileSet::DoOpen(const IOContext* io_context) { // Lazily open the column data cfiles. Each one will be fully opened // later, when the first iterator seeks for the first time. - RowSetMetadata::ColumnIdToBlockIdMap block_map = rowset_metadata_->GetColumnBlocksById(); - for (const RowSetMetadata::ColumnIdToBlockIdMap::value_type& e : block_map) { - ColumnId col_id = e.first; - DCHECK(!ContainsKey(readers_by_col_id_, col_id)) << "already open"; - + const RowSetMetadata::ColumnIdToBlockIdMap block_map = rowset_metadata_->GetColumnBlocksById(); + for (const auto& [col_id, _] : block_map) { unique_ptr<CFileReader> reader; RETURN_NOT_OK(OpenReader(rowset_metadata_->fs_manager(), cfile_reader_tracker_, rowset_metadata_->column_data_block_for_col_id(col_id), io_context, &reader)); - readers_by_col_id_[col_id] = std::move(reader); + const auto& ins_info = readers_by_col_id_.emplace(col_id, std::move(reader)); + DCHECK(ins_info.second) << "already open"; VLOG(1) << "Successfully opened cfile for column id " << col_id << " in " << rowset_metadata_->ToString(); } diff --git a/src/kudu/tablet/rowset_metadata.cc b/src/kudu/tablet/rowset_metadata.cc index c14d2ca97..463c9489f 100644 --- a/src/kudu/tablet/rowset_metadata.cc +++ b/src/kudu/tablet/rowset_metadata.cc @@ -64,7 +64,7 @@ Status RowSetMetadata::Flush() { } Status RowSetMetadata::InitFromPB(const RowSetDataPB& pb) { - CHECK(!initted_); + DCHECK(!initted_); LoadFromPB(pb); @@ -125,9 +125,8 @@ void RowSetMetadata::LoadFromPB(const RowSetDataPB& pb) { } void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) { - pb->set_id(id_); - std::lock_guard l(lock_); + pb->set_id(id_); // Write Column Files for (const ColumnIdToBlockIdMap::value_type& e : blocks_by_col_id_) { @@ -175,7 +174,12 @@ void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) { } const std::string RowSetMetadata::ToString() const { - return Substitute("RowSet($0)", id_); + int64_t id = 0; + { + std::lock_guard l(lock_); + id = id_; + } + return Substitute("RowSet($0)", id); } void RowSetMetadata::SetColumnDataBlocks(const std::map<ColumnId, BlockId>& blocks_by_col_id) { diff --git a/src/kudu/tablet/rowset_metadata.h b/src/kudu/tablet/rowset_metadata.h index 4e33cfb0f..922270cac 100644 --- a/src/kudu/tablet/rowset_metadata.h +++ b/src/kudu/tablet/rowset_metadata.h @@ -97,7 +97,10 @@ class RowSetMetadata { const std::string ToString() const; - int64_t id() const { return id_; } + int64_t id() const { + std::lock_guard l(lock_); + return id_; + } const SchemaPtr tablet_schema() const { return tablet_metadata_->schema(); @@ -266,11 +269,14 @@ class RowSetMetadata { TabletMetadata* const tablet_metadata_; bool initted_; - int64_t id_; // Protects the below mutable fields. mutable LockType lock_; + // Rowset identifier. Set in one of the constructors, and may be also set + // via LoadFromPB(). + int64_t id_; + // The min and max keys of the rowset. std::optional<std::string> min_encoded_key_; std::optional<std::string> max_encoded_key_;