From a0cbb694217cedf87e5ed10dd32f2053a18a2107 Mon Sep 17 00:00:00 2001
From: anand76 <anand76@devvm4702.ftw0.facebook.com>
Date: Wed, 30 Jun 2021 13:28:45 -0700
Subject: [PATCH] Fix assertion failure when releasing a handle after secondary
 cache lookup fails (#8470)

Summary:
When the secondary cache lookup fails, we may still allocate a handle and charge the cache for metadata usage. If the cache is full, this can cause the usage to go over capacity. Later, when a (unrelated) handle is released, it trips up an assertion that checks that usage is less than capacity. To prevent this assertion failure, don't charge the cache for a failed secondary cache lookup.

Tests:
Run crash_test

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

Reviewed By: zhichao-cao

Differential Revision: D29474713

Pulled By: anand1976

fbshipit-source-id: 27191969c95470a7b070d292b458efce71395bf2
---
 cache/lru_cache.cc | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/cache/lru_cache.cc b/cache/lru_cache.cc
index f7da46b69..c9f54f311 100644
--- a/cache/lru_cache.cc
+++ b/cache/lru_cache.cc
@@ -407,11 +407,10 @@ void LRUCacheShard::Promote(LRUHandle* e) {
     }
   } else {
     // Since the secondary cache lookup failed, mark the item as not in cache
-    // and charge the cache only for metadata usage, i.e handle, key etc
+    // Don't charge the cache as its only metadata that'll shortly be released
     MutexLock l(&mutex_);
     e->charge = 0;
     e->SetInCache(false);
-    usage_ += e->CalcTotalCharge(metadata_charge_policy_);
   }
 }
 
@@ -522,7 +521,12 @@ bool LRUCacheShard::Release(Cache::Handle* handle, bool force_erase) {
         last_reference = false;
       }
     }
-    if (last_reference) {
+    // If it was the last reference, and the entry is either not secondary
+    // cache compatible (i.e a dummy entry for accounting), or is secondary
+    // cache compatible and has a non-null value, then decrement the cache
+    // usage. If value is null in the latter case, taht means the lookup
+    // failed and we didn't charge the cache.
+    if (last_reference && (!e->IsSecondaryCacheCompatible() || e->value)) {
       size_t total_charge = e->CalcTotalCharge(metadata_charge_policy_);
       assert(usage_ >= total_charge);
       usage_ -= total_charge;