Fix a bug in LRUCacheShard::LRU_Insert (facebook#12429)

Summary:
we saw crash test fail with
```
lru_cache.cc:249: void rocksdb::lru_cache::LRUCacheShard::LRU_Remove(rocksdb::lru_cache::LRUHandle *): Assertion `high_pri_pool_usage_ >= e->total_charge' failed.
```
One cause for this is that `lru_low_pri_` pointer is not updated in `LRU_insert()` before we try to balance high pri and low pri pool in `MaintainPoolSize();`. A repro unit test is provided.

Pull Request resolved: facebook#12429

Test Plan:
Not able to reproduce the failure with db_stress yet.
`./lru_cache_test --gtest_filter="*InsertAfterReducingCapacity*`. It fails the assertion before this PR.

Reviewed By: pdillinger

Differential Revision: D54908919

Pulled By: cbi42

fbshipit-source-id: f485fdbc0ea61c8092a0be5fe561a59c15c78fd3

cache/lru_cache.cc

@@ -277,8 +277,8 @@ void LRUCacheShard::LRU_Insert(LRUHandle* e) {
     e->SetInHighPriPool(false);
     e->SetInLowPriPool(true);
     low_pri_pool_usage_ += e->total_charge;
-    MaintainPoolSize();
     lru_low_pri_ = e;
+    MaintainPoolSize();
   } else {
     // Insert "e" to the head of bottom-pri pool.
     e->next = lru_bottom_pri_->next;
@@ -301,6 +301,7 @@ void LRUCacheShard::MaintainPoolSize() {
     // Overflow last entry in high-pri pool to low-pri pool.
     lru_low_pri_ = lru_low_pri_->next;
     assert(lru_low_pri_ != &lru_);
+    assert(lru_low_pri_->InHighPriPool());
     lru_low_pri_->SetInHighPriPool(false);
     lru_low_pri_->SetInLowPriPool(true);
     assert(high_pri_pool_usage_ >= lru_low_pri_->total_charge);
@@ -312,6 +313,7 @@ void LRUCacheShard::MaintainPoolSize() {
     // Overflow last entry in low-pri pool to bottom-pri pool.
     lru_bottom_pri_ = lru_bottom_pri_->next;
     assert(lru_bottom_pri_ != &lru_);
+    assert(lru_bottom_pri_->InLowPriPool());
     lru_bottom_pri_->SetInHighPriPool(false);
     lru_bottom_pri_->SetInLowPriPool(false);
     assert(low_pri_pool_usage_ >= lru_bottom_pri_->total_charge);

cache/lru_cache_test.cc

@@ -57,10 +57,11 @@ class LRUCacheTest : public testing::Test {
   }
 
   void Insert(const std::string& key,
-              Cache::Priority priority = Cache::Priority::LOW) {
+              Cache::Priority priority = Cache::Priority::LOW,
+              size_t charge = 1) {
     EXPECT_OK(cache_->Insert(key, 0 /*hash*/, nullptr /*value*/,
-                             &kNoopCacheItemHelper, 1 /*charge*/,
-                             nullptr /*handle*/, priority));
+                             &kNoopCacheItemHelper, charge, nullptr /*handle*/,
+                             priority));
   }
 
   void Insert(char key, Cache::Priority priority = Cache::Priority::LOW) {
@@ -144,8 +145,10 @@ class LRUCacheTest : public testing::Test {
     ASSERT_EQ(num_bottom_pri_pool_keys, bottom_pri_pool_keys);
   }
 
- private:
+ protected:
   LRUCacheShard* cache_ = nullptr;
+
+ private:
   Cache::EvictionCallback eviction_callback_;
 };
 
@@ -2703,6 +2706,23 @@ TEST_P(DBSecondaryCacheTest, TestSecondaryCacheOptionTwoDB) {
   ASSERT_OK(DestroyDB(dbname2, options));
 }
 
+TEST_F(LRUCacheTest, InsertAfterReducingCapacity) {
+  // Fix a bug in LRU cache where it may try to remove a low pri entry's
+  // charge from high pri pool. It causes
+  // Assertion failed: (high_pri_pool_usage_ >= lru_low_pri_->total_charge),
+  // function MaintainPoolSize, file lru_cache.cc
+  NewCache(/*capacity=*/10, /*high_pri_pool_ratio=*/0.2,
+           /*low_pri_pool_ratio=*/0.8);
+  // high pri pool size and usage are both 2
+  Insert("x", Cache::Priority::HIGH);
+  Insert("y", Cache::Priority::HIGH);
+  cache_->SetCapacity(5);
+  // high_pri_pool_size is 1, the next time we try to maintain pool size,
+  // we will move entries from high pri pool to low pri pool
+  // The bug was deducting this entry's charge from high pri pool usage.
+  Insert("aaa", Cache::Priority::LOW, /*charge=*/3);
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {