From ec9f52ece6f40072fcdfc0d2497806b247fb2b48 Mon Sep 17 00:00:00 2001 From: anand76 Date: Mon, 30 Aug 2021 19:09:43 -0700 Subject: [PATCH] Fix a race in LRUCacheShard::Promote (#8717) Summary: In ```LRUCacheShard::Promote```, a reference is released outside the LRU mutex. Fix the race condition. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8717 Reviewed By: zhichao-cao Differential Revision: D30649206 Pulled By: anand1976 fbshipit-source-id: 09c0af05b2294a7fe2c02876a61b0bad6e3ada61 --- HISTORY.md | 1 + cache/lru_cache.cc | 11 +++++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 238a4ac7c..94ee05da3 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ * Fixed a bug of secondary instance's last_sequence going backward, and reads on the secondary fail to see recent updates from the primary. * Fixed a bug that could lead to duplicate DB ID or DB session ID in POSIX environments without /proc/sys/kernel/random/uuid. * Fix a race in DumpStats() with column family destruction due to not taking a Ref on each entry while iterating the ColumnFamilySet. +* Fix a race in item ref counting in LRUCache when promoting an item from the SecondaryCache. ### New Features * RemoteCompaction's interface now includes `db_name`, `db_id`, `session_id`, which could help the user uniquely identify compaction job between db instances and sessions. diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc index 689242ca9..bab58a2a2 100644 --- a/cache/lru_cache.cc +++ b/cache/lru_cache.cc @@ -360,7 +360,10 @@ Status LRUCacheShard::InsertItem(LRUHandle* e, Cache::Handle** handle, if (handle == nullptr) { LRU_Insert(e); } else { - e->Ref(); + // If caller already holds a ref, no need to take one here + if (!e->HasRefs()) { + e->Ref(); + } *handle = reinterpret_cast(e); } } @@ -398,11 +401,7 @@ void LRUCacheShard::Promote(LRUHandle* e) { if (e->value) { Cache::Handle* handle = reinterpret_cast(e); Status s = InsertItem(e, &handle, /*free_handle_on_fail=*/false); - if (s.ok()) { - // InsertItem would have taken a reference on the item, so decrement it - // here as we expect the caller to already hold a reference - e->Unref(); - } else { + if (!s.ok()) { // Item is in memory, but not accounted against the cache capacity. // When the handle is released, the item should get deleted assert(!e->InCache());