From 5ad3227650cf0b67aa3e451a45da8bf940333b51 Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Thu, 15 Jul 2021 16:08:19 -0700
Subject: [PATCH] Work around falsely reported data race on LRUHandle::flags
 (#8539)

Summary:
Some bits are mutated and read while holding a lock, other
immutable bits (esp. secondary cache compatibility) can be read by
arbitrary threads without holding a lock. AFAIK, this doesn't cause an
issue on any architecture we care about, because you will get some
legitimate version of the value that includes the initialization, as
long as synchronization guarantees the initialization happens before the
read.

I've only seen this in https://github.com/facebook/rocksdb/issues/8538 so far, but it should be fixed regardless.
Otherwise, we'll surely get these false reports again some time.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/8539

Test Plan: some local TSAN test runs and in CircleCI

Reviewed By: zhichao-cao

Differential Revision: D29720262

Pulled By: pdillinger

fbshipit-source-id: 365fd7e565577c648815161f71b339bcb5ce12d5
---
 cache/lru_cache.cc |  3 +++
 cache/lru_cache.h  | 34 +++++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc
index c9f54f311..db60828c0 100644
--- a/cache/lru_cache.cc
+++ b/cache/lru_cache.cc
@@ -557,6 +557,9 @@ Status LRUCacheShard::Insert(const Slice& key, uint32_t hash, void* value,
     e->SetSecondaryCacheCompatible(true);
     e->info_.helper = helper;
   } else {
+#ifdef __SANITIZE_THREAD__
+    e->is_secondary_cache_compatible_for_tsan = false;
+#endif  // __SANITIZE_THREAD__
     e->info_.deleter = deleter;
   }
   e->charge = charge;
diff --git a/cache/lru_cache.h b/cache/lru_cache.h
index af0155ad9..d2e21489c 100644
--- a/cache/lru_cache.h
+++ b/cache/lru_cache.h
@@ -80,8 +80,8 @@ struct LRUHandle {
     IN_HIGH_PRI_POOL = (1 << 2),
     // Whether this entry has had any lookups (hits).
     HAS_HIT = (1 << 3),
-    // Can this be inserted into the tiered cache
-    IS_TIERED_CACHE_COMPATIBLE = (1 << 4),
+    // Can this be inserted into the secondary cache
+    IS_SECONDARY_CACHE_COMPATIBLE = (1 << 4),
     // Is the handle still being read from a lower tier
     IS_PENDING = (1 << 5),
     // Has the item been promoted from a lower tier
@@ -90,6 +90,14 @@ struct LRUHandle {
 
   uint8_t flags;
 
+#ifdef __SANITIZE_THREAD__
+  // TSAN can report a false data race on flags, where one thread is writing
+  // to one of the mutable bits and another thread is reading this immutable
+  // bit. So precisely suppress that TSAN warning, we separate out this bit
+  // during TSAN runs.
+  bool is_secondary_cache_compatible_for_tsan;
+#endif  // __SANITIZE_THREAD__
+
   // Beginning of the key (MUST BE THE LAST FIELD IN THIS STRUCT!)
   char key_data[1];
 
@@ -113,7 +121,11 @@ struct LRUHandle {
   bool InHighPriPool() const { return flags & IN_HIGH_PRI_POOL; }
   bool HasHit() const { return flags & HAS_HIT; }
   bool IsSecondaryCacheCompatible() const {
-    return flags & IS_TIERED_CACHE_COMPATIBLE;
+#ifdef __SANITIZE_THREAD__
+    return is_secondary_cache_compatible_for_tsan;
+#else
+    return flags & IS_SECONDARY_CACHE_COMPATIBLE;
+#endif  // __SANITIZE_THREAD__
   }
   bool IsPending() const { return flags & IS_PENDING; }
   bool IsPromoted() const { return flags & IS_PROMOTED; }
@@ -144,12 +156,15 @@ struct LRUHandle {
 
   void SetHit() { flags |= HAS_HIT; }
 
-  void SetSecondaryCacheCompatible(bool tiered) {
-    if (tiered) {
-      flags |= IS_TIERED_CACHE_COMPATIBLE;
+  void SetSecondaryCacheCompatible(bool compat) {
+    if (compat) {
+      flags |= IS_SECONDARY_CACHE_COMPATIBLE;
     } else {
-      flags &= ~IS_TIERED_CACHE_COMPATIBLE;
+      flags &= ~IS_SECONDARY_CACHE_COMPATIBLE;
     }
+#ifdef __SANITIZE_THREAD__
+    is_secondary_cache_compatible_for_tsan = compat;
+#endif  // __SANITIZE_THREAD__
   }
 
   void SetIncomplete(bool incomp) {
@@ -170,6 +185,11 @@ struct LRUHandle {
 
   void Free() {
     assert(refs == 0);
+#ifdef __SANITIZE_THREAD__
+    // Here we can safely assert they are the same without a data race reported
+    assert(((flags & IS_SECONDARY_CACHE_COMPATIBLE) != 0) ==
+           is_secondary_cache_compatible_for_tsan);
+#endif  // __SANITIZE_THREAD__
     if (!IsSecondaryCacheCompatible() && info_.deleter) {
       (*info_.deleter)(key(), value);
     } else if (IsSecondaryCacheCompatible()) {