Fix TSAN-reported data race with uncache_aggressiveness (#12753)

Summary:
Data race reported on
BlockBasedTableReader::Rep::uncache_aggressiveness because apparently a file can be marked obsolete through multiple table cache references in parallel. Using a relaxed atomic should resolve the race quite reasonably, especially considering this is a rare case and the racing writes should be storing the same value anyway.

Pull Request resolved: #12753

Test Plan: watch for TSAN crash test results

Reviewed By: ltamasi

Differential Revision: D58397473

Pulled By: pdillinger

fbshipit-source-id: 3e78b6adac4f7a7056790754bee42b3cb244f037

table/block_based/block_based_table_reader.cc

@@ -136,10 +136,10 @@ extern const std::string kHashIndexPrefixesBlock;
 extern const std::string kHashIndexPrefixesMetadataBlock;
 
 BlockBasedTable::~BlockBasedTable() {
-  if (rep_->uncache_aggressiveness > 0 && rep_->table_options.block_cache) {
+  auto ua = rep_->uncache_aggressiveness.LoadRelaxed();
+  if (ua > 0 && rep_->table_options.block_cache) {
     if (rep_->filter) {
-      rep_->filter->EraseFromCacheBeforeDestruction(
-          rep_->uncache_aggressiveness);
+      rep_->filter->EraseFromCacheBeforeDestruction(ua);
     }
     if (rep_->index_reader) {
       {
@@ -160,7 +160,7 @@ BlockBasedTable::~BlockBasedTable() {
         // without I/O. (NOTE: It's extremely unlikely that a data block
         // will be in block cache without the index block pointing to it
         // also in block cache.)
-        UncacheAggressivenessAdvisor advisor(rep_->uncache_aggressiveness);
+        UncacheAggressivenessAdvisor advisor(ua);
         for (iiter->SeekToFirst(); iiter->Valid() && advisor.ShouldContinue();
              iiter->Next()) {
           bool erased = EraseFromCache(iiter->value().handle);
@@ -170,8 +170,7 @@ BlockBasedTable::~BlockBasedTable() {
       }
 
       // Un-cache the index block(s)
-      rep_->index_reader->EraseFromCacheBeforeDestruction(
-          rep_->uncache_aggressiveness);
+      rep_->index_reader->EraseFromCacheBeforeDestruction(ua);
     }
   }
   delete rep_;
@@ -3296,7 +3295,7 @@ void BlockBasedTable::DumpKeyValue(const Slice& key, const Slice& value,
 }
 
 void BlockBasedTable::MarkObsolete(uint32_t uncache_aggressiveness) {
-  rep_->uncache_aggressiveness = uncache_aggressiveness;
+  rep_->uncache_aggressiveness.StoreRelaxed(uncache_aggressiveness);
 }
 
 }  // namespace ROCKSDB_NAMESPACE

table/block_based/block_based_table_reader.h

@@ -33,6 +33,7 @@
 #include "table/table_reader.h"
 #include "table/two_level_iterator.h"
 #include "trace_replay/block_cache_tracer.h"
+#include "util/atomic.h"
 #include "util/coro_utils.h"
 #include "util/hash_containers.h"
 
@@ -675,8 +676,11 @@ struct BlockBasedTable::Rep {
   const bool user_defined_timestamps_persisted;
 
   // Set to >0 when the file is known to be obsolete and should have its block
-  // cache entries evicted on close.
-  uint32_t uncache_aggressiveness = 0;
+  // cache entries evicted on close. NOTE: when the file becomes obsolete,
+  // there could be multiple table cache references that all mark this file as
+  // obsolete. An atomic resolves the race quite reasonably. Even in the rare
+  // case of such a race, they will most likely be storing the same value.
+  RelaxedAtomic<uint32_t> uncache_aggressiveness{0};
 
   std::unique_ptr<CacheReservationManager::CacheReservationHandle>
       table_reader_cache_res_handle = nullptr;

table/table_reader.h

@@ -191,7 +191,9 @@ class TableReader {
 
   // Tell the reader that the file should now be obsolete, e.g. as a hint
   // to delete relevant cache entries on destruction. (It might not be safe
-  // to "unpin" cache entries until destruction time.)
+  // to "unpin" cache entries until destruction time.) NOTE: must be thread
+  // safe because multiple table cache references might all mark this file as
+  // obsolete when they are released (the last of which destroys this reader).
   virtual void MarkObsolete(uint32_t /*uncache_aggressiveness*/) {
     // no-op as default
   }