andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Work around falsely reported data race on LRUHandle::flags (#8539)

Browse Source 
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
This commit is contained in:
Peter Dillinger 2021-07-15 16:08:19 -07:00 committed by Facebook GitHub Bot
parent 31193a73a4
commit 5ad3227650
2 changed files with 30 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
cache/lru_cache.cc vendored
View File

@ -557,6 +557,9 @@ Status LRUCacheShard::Insert(const Slice& key, uint32_t hash, void* value,
e->SetSecondaryCacheCompatible(true); e->SetSecondaryCacheCompatible(true);
e->info_.helper = helper; e->info_.helper = helper;
} else { } else {
#ifdef __SANITIZE_THREAD__
e->is_secondary_cache_compatible_for_tsan = false;
#endif // __SANITIZE_THREAD__
e->info_.deleter = deleter; e->info_.deleter = deleter;
} }
e->charge = charge; e->charge = charge;

34
cache/lru_cache.h vendored
View File

@ -80,8 +80,8 @@ struct LRUHandle {
IN_HIGH_PRI_POOL = (1 << 2), IN_HIGH_PRI_POOL = (1 << 2),
// Whether this entry has had any lookups (hits). // Whether this entry has had any lookups (hits).
HAS_HIT = (1 << 3), HAS_HIT = (1 << 3),
// Can this be inserted into the tiered cache // Can this be inserted into the secondary cache
IS_TIERED_CACHE_COMPATIBLE = (1 << 4), IS_SECONDARY_CACHE_COMPATIBLE = (1 << 4),
// Is the handle still being read from a lower tier // Is the handle still being read from a lower tier
IS_PENDING = (1 << 5), IS_PENDING = (1 << 5),
// Has the item been promoted from a lower tier // Has the item been promoted from a lower tier
@ -90,6 +90,14 @@ struct LRUHandle {
uint8_t flags; 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!) // Beginning of the key (MUST BE THE LAST FIELD IN THIS STRUCT!)
char key_data[1]; char key_data[1];
@ -113,7 +121,11 @@ struct LRUHandle {
bool InHighPriPool() const { return flags & IN_HIGH_PRI_POOL; } bool InHighPriPool() const { return flags & IN_HIGH_PRI_POOL; }
bool HasHit() const { return flags & HAS_HIT; } bool HasHit() const { return flags & HAS_HIT; }
bool IsSecondaryCacheCompatible() const { 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 IsPending() const { return flags & IS_PENDING; }
bool IsPromoted() const { return flags & IS_PROMOTED; } bool IsPromoted() const { return flags & IS_PROMOTED; }
@ -144,12 +156,15 @@ struct LRUHandle {
void SetHit() { flags |= HAS_HIT; } void SetHit() { flags |= HAS_HIT; }
void SetSecondaryCacheCompatible(bool tiered) { void SetSecondaryCacheCompatible(bool compat) {
if (tiered) { if (compat) {
flags |= IS_TIERED_CACHE_COMPATIBLE; flags |= IS_SECONDARY_CACHE_COMPATIBLE;
} else { } 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) { void SetIncomplete(bool incomp) {
@ -170,6 +185,11 @@ struct LRUHandle {
void Free() { void Free() {
assert(refs == 0); 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) { if (!IsSecondaryCacheCompatible() && info_.deleter) {
(*info_.deleter)(key(), value); (*info_.deleter)(key(), value);
} else if (IsSecondaryCacheCompatible()) { } else if (IsSecondaryCacheCompatible()) {