Update version to 8.8.1

Summary:
`CacheWithSecondaryAdapter` can distribute placeholder reservations across the primary and secondary caches. The current implementation of the accounting is quite complicated in order to avoid using a mutex. This may cause the accounting to be slightly off after changes to the cache capacity and ratio, resulting in assertion failures. There's also a bug in the unlikely event that the total reservation exceeds the cache capacity. Furthermore, the current implementation is difficult to reason about.

This PR simplifies it by doing the accounting while holding a mutex. The reservations are processed in 1MB chunks in order to avoid taking a lock too frequently. As a side effect, this also removes the restriction of not allowing to increase the compressed secondary cache capacity after decreasing it to 0.

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

Test Plan: Existing unit tests, and a new test for capacity increase from 0

Reviewed By: pdillinger

Differential Revision: D51278686

Pulled By: anand1976

fbshipit-source-id: 7e1ad2c50694772997072dd59cab35c93c12ba4f

.circleci/config.yml

@@ -69,7 +69,7 @@ commands:
           command: |
             mkdir build
             cd build
-            & $Env:CMAKE_BIN -G "$Env:CMAKE_GENERATOR" -DCMAKE_BUILD_TYPE=Debug -DOPTDBG=1 -DPORTABLE=1 -DSNAPPY=1 -DJNI=1 ..
+            & $Env:CMAKE_BIN -G "$Env:CMAKE_GENERATOR" -DCMAKE_BUILD_TYPE=Debug -DOPTDBG=1 -DPORTABLE="$Env:CMAKE_PORTABLE" -DSNAPPY=1 -DJNI=1 ..
             cd ..
             echo "Building with VS version: $Env:CMAKE_GENERATOR"
             msbuild.exe build/rocksdb.sln -maxCpuCount -property:Configuration=Debug -property:Platform=x64
@@ -227,6 +227,9 @@ executors:
       #  $ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined -it zjay437/rocksdb:0.5 bash
       # option `--cap-add=SYS_PTRACE --security-opt seccomp=unconfined` is used to enable gdb to attach an existing process
       - image: zjay437/rocksdb:0.6
+  linux-java-docker:
+    docker:
+      - image: evolvedbinary/rocksjava:centos6_x64-be
 
 jobs:
   build-macos:
@@ -563,6 +566,23 @@ jobs:
           no_output_timeout: 100m
       - post-steps
 
+  build-windows-vs2022-avx2:
+    executor:
+      name: win/server-2022
+      size: 2xlarge
+    environment:
+      THIRDPARTY_HOME: C:/Users/circleci/thirdparty
+      CMAKE_HOME: C:/Program Files/CMake
+      CMAKE_BIN: C:/Program Files/CMake/bin/cmake.exe
+      CTEST_BIN: C:/Program Files/CMake/bin/ctest.exe
+      SNAPPY_HOME: C:/Users/circleci/thirdparty/snappy-1.1.8
+      SNAPPY_INCLUDE: C:/Users/circleci/thirdparty/snappy-1.1.8;C:/Users/circleci/thirdparty/snappy-1.1.8/build
+      SNAPPY_LIB_DEBUG: C:/Users/circleci/thirdparty/snappy-1.1.8/build/Debug/snappy.lib
+      CMAKE_GENERATOR: Visual Studio 17 2022
+      CMAKE_PORTABLE: AVX2
+    steps:
+      - windows-build-steps
+
   build-windows-vs2022:
     executor:
       name: win/server-2022
@@ -576,6 +596,7 @@ jobs:
       SNAPPY_INCLUDE: C:/Users/circleci/thirdparty/snappy-1.1.8;C:/Users/circleci/thirdparty/snappy-1.1.8/build
       SNAPPY_LIB_DEBUG: C:/Users/circleci/thirdparty/snappy-1.1.8/build/Debug/snappy.lib
       CMAKE_GENERATOR: Visual Studio 17 2022
+      CMAKE_PORTABLE: 1
     steps:
       - windows-build-steps
 
@@ -592,6 +613,7 @@ jobs:
       SNAPPY_INCLUDE: C:/Users/circleci/thirdparty/snappy-1.1.8;C:/Users/circleci/thirdparty/snappy-1.1.8/build
       SNAPPY_LIB_DEBUG: C:/Users/circleci/thirdparty/snappy-1.1.8/build/Debug/snappy.lib
       CMAKE_GENERATOR: Visual Studio 16 2019
+      CMAKE_PORTABLE: 1
     steps:
       - windows-build-steps
 
@@ -613,7 +635,7 @@ jobs:
       - post-steps
 
   build-linux-java-pmd:
-    machine: 
+    machine:
       image: ubuntu-2004:202111-02
     resource_class: large
     environment:
@@ -634,7 +656,7 @@ jobs:
       - post-pmd-steps
 
   build-linux-java-static:
-    executor: linux-docker
+    executor: linux-java-docker
     resource_class: large
     steps:
       - pre-steps
@@ -647,7 +669,7 @@ jobs:
             which javac && javac -version
       - run:
           name: "Build RocksDBJava Static Library"
-          command: make V=1 J=8 -j8 rocksdbjavastatic
+          command: scl enable devtoolset-7 'make V=1 J=8 -j8 rocksdbjavastatic'
       - post-steps
 
   build-macos-java:
@@ -887,6 +909,7 @@ workflows:
       - build-linux-mini-crashtest
   jobs-windows:
     jobs:
+      - build-windows-vs2022-avx2
       - build-windows-vs2022
       - build-windows-vs2019
       - build-cmake-mingw

HISTORY.md

@@ -1,6 +1,38 @@
 # Rocksdb Change Log
 > NOTE: Entries for next release do not go here. Follow instructions in `unreleased_history/README.txt`
 
+## 8.8.1 (11/17/2023)
+### Bug fixes
+* Make the cache memory reservation accounting in Tiered cache (primary and compressed secondary cache) more accurate to avoid over/under charging the secondary cache.
+* Allow increasing the compressed_secondary_ratio in the Tiered cache after setting it to 0 to disable.
+
+## 8.8.0 (10/23/2023)
+### New Features
+* Introduce AttributeGroup by adding the first AttributeGroup support API, MultiGetEntity(). Through the use of Column Families, AttributeGroup enables users to logically group wide-column entities. More APIs to support AttributeGroup will come soon, including GetEntity, PutEntity, and others.
+* Added new tickers `rocksdb.fifo.{max.size|ttl}.compactions` to count FIFO compactions that drop files for different reasons
+* Add an experimental offpeak duration awareness by setting `DBOptions::daily_offpeak_time_utc` in "HH:mm-HH:mm" format. This information will be used for resource optimization in the future
+* Users can now change the max bytes granted in a single refill period (i.e, burst) during runtime by `SetSingleBurstBytes()` for RocksDB rate limiter
+
+### Public API Changes
+* The default value of `DBOptions::fail_if_options_file_error` changed from `false` to `true`. Operations that set in-memory options (e.g., `DB::Open*()`, `DB::SetOptions()`, `DB::CreateColumnFamily*()`, and `DB::DropColumnFamily()`) but fail to persist the change will now return a non-OK `Status` by default.
+* Add new Cache APIs GetSecondaryCacheCapacity() and GetSecondaryCachePinnedUsage() to return the configured capacity, and cache reservation charged to the secondary cache.
+
+### Behavior Changes
+* For non direct IO, eliminate the file system prefetching attempt for compaction read when `Options::compaction_readahead_size` is 0
+* During a write stop, writes now block on in-progress recovery attempts
+* Deleting stale files upon recovery are delegated to SstFileManger if available so they can be rate limited.
+
+### Bug Fixes
+* Fix a bug in auto_readahead_size where first_internal_key of index blocks wasn't copied properly resulting in corruption error when first_internal_key was used for comparison.
+* Fixed a bug where compaction read under non direct IO still falls back to RocksDB internal prefetching after file system's prefetching returns non-OK status other than `Status::NotSupported()`
+* Add bounds check in WBWIIteratorImpl and make BaseDeltaIterator, WriteUnpreparedTxn and WritePreparedTxn respect the upper bound and lower bound in ReadOption. See 11680.
+* Fixed the handling of wide-column base values in the `max_successive_merges` logic.
+* Fixed a rare race bug involving a concurrent combination of Create/DropColumnFamily and/or Set(DB)Options that could lead to inconsistency between (a) the DB's reported options state, (b) the DB options in effect, and (c) the latest persisted OPTIONS file.
+* Fixed a possible underflow when computing the compressed secondary cache share of memory reservations while updating the compressed secondary to total block cache ratio.
+
+### Performance Improvements
+* Improved the I/O efficiency of DB::Open a new DB with `create_missing_column_families=true` and many column families.
+
 ## 8.7.0 (09/22/2023)
 ### New Features
 * Added an experimental new "automatic" variant of HyperClockCache that does not require a prior estimate of the average size of cache entries. This variant is activated when HyperClockCacheOptions::estimated\_entry\_charge = 0 and has essentially the same concurrency benefits as the existing HyperClockCache.

Makefile

@@ -2107,7 +2107,7 @@ LZ4_VER ?= 1.9.4
 LZ4_SHA256 ?= 0b0e3aa07c8c063ddf40b082bdf7e37a1562bda40a0ff5272957f3e987e0e54b
 LZ4_DOWNLOAD_BASE ?= https://github.com/lz4/lz4/archive
 ZSTD_VER ?= 1.5.5
-ZSTD_SHA256 ?= 9c4396cc829cfae319a6e2615202e82aad41372073482fce286fac78646d3ee4
+ZSTD_SHA256 ?= 98e9c3d949d1b924e28e01eccb7deed865eefebf25c2f21c702e5cd5b63b85e1
 ZSTD_DOWNLOAD_BASE ?= https://github.com/facebook/zstd/archive
 CURL_SSL_OPTS ?= --tlsv1
 

cache/compressed_secondary_cache_test.cc

@@ -1224,13 +1224,11 @@ TEST_P(CompressedSecCacheTestWithTiered, DynamicUpdate) {
   ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
   ASSERT_EQ(sec_capacity, 0);
 
-  ASSERT_NOK(UpdateTieredCache(tiered_cache, -1, 0.3));
-  // Only check usage for LRU cache. HCC shows a 64KB usage for some reason
-  if (std::get<0>(GetParam()) == PrimaryCacheType::kCacheTypeLRU) {
-    ASSERT_EQ(GetCache()->GetUsage(), 0);
-  }
+  ASSERT_OK(UpdateTieredCache(tiered_cache, -1, 0.3));
+  EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (30 << 20),
+               GetPercent(30 << 20, 1));
   ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
-  ASSERT_EQ(sec_capacity, 0);
+  ASSERT_EQ(sec_capacity, (30 << 20));
 }
 
 TEST_P(CompressedSecCacheTestWithTiered, DynamicUpdateWithReservation) {
@@ -1276,6 +1274,13 @@ TEST_P(CompressedSecCacheTestWithTiered, DynamicUpdateWithReservation) {
   ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
   ASSERT_EQ(sec_capacity, (30 << 20));
 
+  ASSERT_OK(tiered_cache->GetSecondaryCacheCapacity(sec_capacity));
+  ASSERT_EQ(sec_capacity, 30 << 20);
+  size_t sec_usage;
+  ASSERT_OK(tiered_cache->GetSecondaryCachePinnedUsage(sec_usage));
+  EXPECT_PRED3(CacheUsageWithinBounds, sec_usage, 3 << 20,
+               GetPercent(3 << 20, 1));
+
   ASSERT_OK(UpdateTieredCache(tiered_cache, -1, 0.39));
   EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (45 << 20),
                GetPercent(45 << 20, 1));
@@ -1309,28 +1314,50 @@ TEST_P(CompressedSecCacheTestWithTiered, DynamicUpdateWithReservation) {
   ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
   ASSERT_EQ(sec_capacity, 0);
 
+  ASSERT_OK(UpdateTieredCache(tiered_cache, -1, 0.3));
+  EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (37 << 20),
+               GetPercent(37 << 20, 1));
+  EXPECT_PRED3(CacheUsageWithinBounds, sec_cache->TEST_GetUsage(), (3 << 20),
+               GetPercent(3 << 20, 1));
+  ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
+  ASSERT_EQ(sec_capacity, 30 << 20);
+
   ASSERT_OK(cache_res_mgr()->UpdateCacheReservation(0));
 }
 
-TEST_P(CompressedSecCacheTestWithTiered,
-       DynamicUpdateWithReservationUnderflow) {
+TEST_P(CompressedSecCacheTestWithTiered, ReservationOverCapacity) {
+  CompressedSecondaryCache* sec_cache =
+      reinterpret_cast<CompressedSecondaryCache*>(GetSecondaryCache());
   std::shared_ptr<Cache> tiered_cache = GetTieredCache();
-  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency(
-      {{"CacheWithSecondaryAdapter::Release:ChargeSecCache1",
-        "CacheWithSecondaryAdapter::UpdateCacheReservationRatio:Begin"},
-       {"CacheWithSecondaryAdapter::UpdateCacheReservationRatio:End",
-        "CacheWithSecondaryAdapter::Release:ChargeSecCache2"}});
-  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
-  port::Thread reserve_release_thread([&]() {
-    EXPECT_EQ(cache_res_mgr()->UpdateCacheReservation(50), Status::OK());
-    EXPECT_EQ(cache_res_mgr()->UpdateCacheReservation(0), Status::OK());
-  });
-  ASSERT_OK(UpdateTieredCache(tiered_cache, 100 << 20, 0.01));
-  reserve_release_thread.join();
-  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
+  ASSERT_OK(cache_res_mgr()->UpdateCacheReservation(110 << 20));
+  // Use EXPECT_PRED3 instead of EXPECT_NEAR to void too many size_t to
+  // double explicit casts
+  EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (110 << 20),
+               GetPercent(110 << 20, 1));
+  EXPECT_PRED3(CacheUsageWithinBounds, sec_cache->TEST_GetUsage(), (30 << 20),
+               GetPercent(30 << 20, 1));
+  size_t sec_capacity;
+  ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
+  ASSERT_EQ(sec_capacity, (30 << 20));
 
-  ASSERT_OK(UpdateTieredCache(tiered_cache, 100 << 20, 0.3));
+  ASSERT_OK(UpdateTieredCache(tiered_cache, -1, 0.39));
+  EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (110 << 20),
+               GetPercent(110 << 20, 1));
+  EXPECT_PRED3(CacheUsageWithinBounds, sec_cache->TEST_GetUsage(), (39 << 20),
+               GetPercent(39 << 20, 1));
+  ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
+  ASSERT_EQ(sec_capacity, (39 << 20));
+
+  ASSERT_OK(cache_res_mgr()->UpdateCacheReservation(90 << 20));
+  EXPECT_PRED3(CacheUsageWithinBounds, GetCache()->GetUsage(), (94 << 20),
+               GetPercent(94 << 20, 1));
+  EXPECT_PRED3(CacheUsageWithinBounds, sec_cache->TEST_GetUsage(), (35 << 20),
+               GetPercent(35 << 20, 1));
+  ASSERT_OK(sec_cache->GetCapacity(sec_capacity));
+  ASSERT_EQ(sec_capacity, (39 << 20));
+
+  ASSERT_OK(cache_res_mgr()->UpdateCacheReservation(0));
 }
 
 INSTANTIATE_TEST_CASE_P(

cache/secondary_cache_adapter.cc

@@ -83,7 +83,10 @@ CacheWithSecondaryAdapter::CacheWithSecondaryAdapter(
     : CacheWrapper(std::move(target)),
       secondary_cache_(std::move(secondary_cache)),
       adm_policy_(adm_policy),
-      distribute_cache_res_(distribute_cache_res) {
+      distribute_cache_res_(distribute_cache_res),
+      placeholder_usage_(0),
+      reserved_usage_(0),
+      sec_reserved_(0) {
   target_->SetEvictionCallback(
       [this](const Slice& key, Handle* handle, bool was_hit) {
         return EvictionHandler(key, handle, was_hit);
@@ -103,8 +106,7 @@ CacheWithSecondaryAdapter::CacheWithSecondaryAdapter(
     // secondary cache is freed from the reservation.
     s = pri_cache_res_->UpdateCacheReservation(sec_capacity);
     assert(s.ok());
-    sec_cache_res_ratio_.store((double)sec_capacity / target_->GetCapacity(),
-                               std::memory_order_relaxed);
+    sec_cache_res_ratio_ = (double)sec_capacity / target_->GetCapacity();
   }
 }
 
@@ -113,7 +115,7 @@ CacheWithSecondaryAdapter::~CacheWithSecondaryAdapter() {
   // use after free
   target_->SetEvictionCallback({});
 #ifndef NDEBUG
-  if (distribute_cache_res_ && !ratio_changed_) {
+  if (distribute_cache_res_) {
     size_t sec_capacity = 0;
     Status s = secondary_cache_->GetCapacity(sec_capacity);
     assert(s.ok());
@@ -236,13 +238,31 @@ Status CacheWithSecondaryAdapter::Insert(const Slice& key, ObjectPtr value,
                                          const Slice& compressed_value,
                                          CompressionType type) {
   Status s = target_->Insert(key, value, helper, charge, handle, priority);
-  if (s.ok() && value == nullptr && distribute_cache_res_) {
-    size_t sec_charge = static_cast<size_t>(
-        charge * (sec_cache_res_ratio_.load(std::memory_order_relaxed)));
-    s = secondary_cache_->Deflate(sec_charge);
-    assert(s.ok());
-    s = pri_cache_res_->UpdateCacheReservation(sec_charge, /*increase=*/false);
-    assert(s.ok());
+  if (s.ok() && value == nullptr && distribute_cache_res_ && handle) {
+    charge = target_->GetCharge(*handle);
+
+    MutexLock l(&cache_res_mutex_);
+    placeholder_usage_ += charge;
+    // Check if total placeholder reservation is more than the overall
+    // cache capacity. If it is, then we don't try to charge the
+    // secondary cache because we don't want to overcharge it (beyond
+    // its capacity).
+    // In order to make this a bit more lightweight, we also check if
+    // the difference between placeholder_usage_ and reserved_usage_ is
+    // atleast kReservationChunkSize and avoid any adjustments if not.
+    if ((placeholder_usage_ <= target_->GetCapacity()) &&
+        ((placeholder_usage_ - reserved_usage_) >= kReservationChunkSize)) {
+      reserved_usage_ = placeholder_usage_ & ~(kReservationChunkSize - 1);
+      size_t new_sec_reserved =
+          static_cast<size_t>(reserved_usage_ * sec_cache_res_ratio_);
+      size_t sec_charge = new_sec_reserved - sec_reserved_;
+      s = secondary_cache_->Deflate(sec_charge);
+      assert(s.ok());
+      s = pri_cache_res_->UpdateCacheReservation(sec_charge,
+                                                 /*increase=*/false);
+      assert(s.ok());
+      sec_reserved_ += sec_charge;
+    }
   }
   // Warm up the secondary cache with the compressed block. The secondary
   // cache may choose to ignore it based on the admission policy.
@@ -287,14 +307,27 @@ bool CacheWithSecondaryAdapter::Release(Handle* handle,
     ObjectPtr v = target_->Value(handle);
     if (v == nullptr && distribute_cache_res_) {
       size_t charge = target_->GetCharge(handle);
-      size_t sec_charge = static_cast<size_t>(
-          charge * (sec_cache_res_ratio_.load(std::memory_order_relaxed)));
-      TEST_SYNC_POINT("CacheWithSecondaryAdapter::Release:ChargeSecCache1");
-      TEST_SYNC_POINT("CacheWithSecondaryAdapter::Release:ChargeSecCache2");
-      Status s = secondary_cache_->Inflate(sec_charge);
-      assert(s.ok());
-      s = pri_cache_res_->UpdateCacheReservation(sec_charge, /*increase=*/true);
-      assert(s.ok());
+
+      MutexLock l(&cache_res_mutex_);
+      placeholder_usage_ -= charge;
+      // Check if total placeholder reservation is more than the overall
+      // cache capacity. If it is, then we do nothing as reserved_usage_ must
+      // be already maxed out
+      if ((placeholder_usage_ <= target_->GetCapacity()) &&
+          (placeholder_usage_ < reserved_usage_)) {
+        // Adjust reserved_usage_ in chunks of kReservationChunkSize, so
+        // we don't hit this slow path too often.
+        reserved_usage_ = placeholder_usage_ & ~(kReservationChunkSize - 1);
+        size_t new_sec_reserved =
+            static_cast<size_t>(reserved_usage_ * sec_cache_res_ratio_);
+        size_t sec_charge = sec_reserved_ - new_sec_reserved;
+        Status s = secondary_cache_->Inflate(sec_charge);
+        assert(s.ok());
+        s = pri_cache_res_->UpdateCacheReservation(sec_charge,
+                                                   /*increase=*/true);
+        assert(s.ok());
+        sec_reserved_ -= sec_charge;
+      }
     }
   }
   return target_->Release(handle, erase_if_last_ref);
@@ -441,13 +474,11 @@ const char* CacheWithSecondaryAdapter::Name() const {
 // where the new capacity < total cache reservations.
 void CacheWithSecondaryAdapter::SetCapacity(size_t capacity) {
   size_t sec_capacity = static_cast<size_t>(
-      capacity * (distribute_cache_res_
-                      ? sec_cache_res_ratio_.load(std::memory_order_relaxed)
-                      : 0.0));
+      capacity * (distribute_cache_res_ ? sec_cache_res_ratio_ : 0.0));
   size_t old_sec_capacity = 0;
 
   if (distribute_cache_res_) {
-    MutexLock m(&mutex_);
+    MutexLock m(&cache_res_mutex_);
 
     Status s = secondary_cache_->GetCapacity(old_sec_capacity);
     if (!s.ok()) {
@@ -462,9 +493,17 @@ void CacheWithSecondaryAdapter::SetCapacity(size_t capacity) {
       // 3. Decrease the primary cache capacity to the total budget
       s = secondary_cache_->SetCapacity(sec_capacity);
       if (s.ok()) {
+        if (placeholder_usage_ > capacity) {
+          // Adjust reserved_usage_ down
+          reserved_usage_ = capacity & ~(kReservationChunkSize - 1);
+        }
+        size_t new_sec_reserved =
+            static_cast<size_t>(reserved_usage_ * sec_cache_res_ratio_);
         s = pri_cache_res_->UpdateCacheReservation(
-            old_sec_capacity - sec_capacity,
+            (old_sec_capacity - sec_capacity) -
+                (sec_reserved_ - new_sec_reserved),
             /*increase=*/false);
+        sec_reserved_ = new_sec_reserved;
         assert(s.ok());
         target_->SetCapacity(capacity);
       }
@@ -489,6 +528,29 @@ void CacheWithSecondaryAdapter::SetCapacity(size_t capacity) {
   }
 }
 
+Status CacheWithSecondaryAdapter::GetSecondaryCacheCapacity(
+    size_t& size) const {
+  return secondary_cache_->GetCapacity(size);
+}
+
+Status CacheWithSecondaryAdapter::GetSecondaryCachePinnedUsage(
+    size_t& size) const {
+  Status s;
+  if (distribute_cache_res_) {
+    MutexLock m(&cache_res_mutex_);
+    size_t capacity = 0;
+    s = secondary_cache_->GetCapacity(capacity);
+    if (s.ok()) {
+      size = capacity - pri_cache_res_->GetTotalMemoryUsed();
+    } else {
+      size = 0;
+    }
+  } else {
+    size = 0;
+  }
+  return s;
+}
+
 // Update the secondary/primary allocation ratio (remember, the primary
 // capacity is the total memory budget when distribute_cache_res_ is true).
 // When the ratio changes, we may accumulate some error in the calculations
@@ -503,12 +565,11 @@ void CacheWithSecondaryAdapter::SetCapacity(size_t capacity) {
 // in the future.
 Status CacheWithSecondaryAdapter::UpdateCacheReservationRatio(
     double compressed_secondary_ratio) {
-  if (!distribute_cache_res_ ||
-      sec_cache_res_ratio_.load(std::memory_order_relaxed) == 0.0) {
+  if (!distribute_cache_res_) {
     return Status::NotSupported();
   }
 
-  MutexLock m(&mutex_);
+  MutexLock m(&cache_res_mutex_);
   size_t pri_capacity = target_->GetCapacity();
   size_t sec_capacity =
       static_cast<size_t>(pri_capacity * compressed_secondary_ratio);
@@ -518,38 +579,17 @@ Status CacheWithSecondaryAdapter::UpdateCacheReservationRatio(
     return s;
   }
 
-  TEST_SYNC_POINT(
-      "CacheWithSecondaryAdapter::UpdateCacheReservationRatio:Begin");
-
-  // There's a possible race condition here. Since the read of pri_cache_res_
-  // memory used (secondary cache usage charged to primary cache), and the
-  // change to sec_cache_res_ratio_ are not guarded by a mutex, its possible
-  // that an Insert/Release in another thread might decrease/increase the
-  // pri_cache_res_ reservation by the wrong amount. This should not be a
-  // problem because updating the sec/pri ratio is a rare operation, and
-  // the worst that can happen is we may over/under charge the secondary
-  // cache usage by a little bit. But we do need to protect against
-  // underflow of old_sec_reserved.
-  // TODO: Make the accounting more accurate by tracking the total memory
-  // reservation on the primary cache. This will also allow us to remove
-  // the restriction of not being able to change the sec/pri ratio from
-  // 0.0 to higher.
-  size_t sec_charge_to_pri = pri_cache_res_->GetTotalMemoryUsed();
-  size_t old_sec_reserved = (old_sec_capacity > sec_charge_to_pri)
-                                ? (old_sec_capacity - sec_charge_to_pri)
-                                : 0;
   // Calculate the new secondary cache reservation
-  size_t sec_reserved = static_cast<size_t>(
-      old_sec_reserved *
-      (double)(compressed_secondary_ratio /
-               sec_cache_res_ratio_.load(std::memory_order_relaxed)));
-  sec_cache_res_ratio_.store(compressed_secondary_ratio,
-                             std::memory_order_relaxed);
+  // reserved_usage_ will never be > the cache capacity, so we don't
+  // have to worry about adjusting it here.
+  sec_cache_res_ratio_ = compressed_secondary_ratio;
+  size_t new_sec_reserved =
+      static_cast<size_t>(reserved_usage_ * sec_cache_res_ratio_);
   if (sec_capacity > old_sec_capacity) {
     // We're increasing the ratio, thus ending up with a larger secondary
     // cache and a smaller usable primary cache capacity. Similar to
     // SetCapacity(), we try to avoid a temporary increase in total usage
-    // beyond teh configured capacity -
+    // beyond the configured capacity -
     // 1. A higher secondary cache ratio means it gets a higher share of
     //    cache reservations. So first account for that by deflating the
     //    secondary cache
@@ -557,12 +597,13 @@ Status CacheWithSecondaryAdapter::UpdateCacheReservationRatio(
     //    cache utilization (increase in capacity - increase in share of cache
     //    reservation)
     // 3. Increase secondary cache capacity
-    s = secondary_cache_->Deflate(sec_reserved - old_sec_reserved);
+    s = secondary_cache_->Deflate(new_sec_reserved - sec_reserved_);
     assert(s.ok());
     s = pri_cache_res_->UpdateCacheReservation(
-        (sec_capacity - old_sec_capacity) - (sec_reserved - old_sec_reserved),
+        (sec_capacity - old_sec_capacity) - (new_sec_reserved - sec_reserved_),
         /*increase=*/true);
     assert(s.ok());
+    sec_reserved_ = new_sec_reserved;
     s = secondary_cache_->SetCapacity(sec_capacity);
     assert(s.ok());
   } else {
@@ -576,21 +617,16 @@ Status CacheWithSecondaryAdapter::UpdateCacheReservationRatio(
     s = secondary_cache_->SetCapacity(sec_capacity);
     if (s.ok()) {
       s = pri_cache_res_->UpdateCacheReservation(
-          (old_sec_capacity - sec_capacity) - (old_sec_reserved - sec_reserved),
+          (old_sec_capacity - sec_capacity) -
+              (sec_reserved_ - new_sec_reserved),
           /*increase=*/false);
       assert(s.ok());
-      s = secondary_cache_->Inflate(old_sec_reserved - sec_reserved);
+      s = secondary_cache_->Inflate(sec_reserved_ - new_sec_reserved);
       assert(s.ok());
+      sec_reserved_ = new_sec_reserved;
     }
   }
 
-  TEST_SYNC_POINT("CacheWithSecondaryAdapter::UpdateCacheReservationRatio:End");
-#ifndef NDEBUG
-  // As mentioned in the function comments, we may accumulate some erros when
-  // the ratio is changed. We set a flag here which disables some assertions
-  // in the destructor
-  ratio_changed_ = true;
-#endif
   return s;
 }
 

cache/secondary_cache_adapter.h

@@ -47,6 +47,10 @@ class CacheWithSecondaryAdapter : public CacheWrapper {
 
   void SetCapacity(size_t capacity) override;
 
+  Status GetSecondaryCacheCapacity(size_t& size) const override;
+
+  Status GetSecondaryCachePinnedUsage(size_t& size) const override;
+
   Status UpdateCacheReservationRatio(double ratio);
 
   Status UpdateAdmissionPolicy(TieredAdmissionPolicy adm_policy);
@@ -56,6 +60,8 @@ class CacheWithSecondaryAdapter : public CacheWrapper {
   SecondaryCache* TEST_GetSecondaryCache() { return secondary_cache_.get(); }
 
  private:
+  static constexpr size_t kReservationChunkSize = 1 << 20;
+
   bool EvictionHandler(const Slice& key, Handle* handle, bool was_hit);
 
   void StartAsyncLookupOnMySecondary(AsyncLookupHandle& async_handle);
@@ -80,11 +86,18 @@ class CacheWithSecondaryAdapter : public CacheWrapper {
   std::shared_ptr<ConcurrentCacheReservationManager> pri_cache_res_;
   // Fraction of a cache memory reservation to be assigned to the secondary
   // cache
-  std::atomic<double> sec_cache_res_ratio_;
-  port::Mutex mutex_;
-#ifndef NDEBUG
-  bool ratio_changed_ = false;
-#endif
+  double sec_cache_res_ratio_;
+  // Mutex for use when managing cache memory reservations. Should not be used
+  // for other purposes, as it may risk causing deadlocks.
+  mutable port::Mutex cache_res_mutex_;
+  // Total memory reserved by placeholder entriesin the cache
+  size_t placeholder_usage_;
+  // Total placeholoder memory charged to both the primary and secondary
+  // caches. Will be <= placeholder_usage_.
+  size_t reserved_usage_;
+  // Amount of memory reserved in the secondary cache. This should be
+  // reserved_usage_ * sec_cache_res_ratio_ in steady state.
+  size_t sec_reserved_;
 };
 
 }  // namespace ROCKSDB_NAMESPACE

cache/sharded_cache.cc

@@ -83,6 +83,16 @@ size_t ShardedCacheBase::GetCapacity() const {
   return capacity_;
 }
 
+Status ShardedCacheBase::GetSecondaryCacheCapacity(size_t& size) const {
+  size = 0;
+  return Status::OK();
+}
+
+Status ShardedCacheBase::GetSecondaryCachePinnedUsage(size_t& size) const {
+  size = 0;
+  return Status::OK();
+}
+
 bool ShardedCacheBase::HasStrictCapacityLimit() const {
   MutexLock l(&config_mutex_);
   return strict_capacity_limit_;

cache/sharded_cache.h

@@ -99,6 +99,8 @@ class ShardedCacheBase : public Cache {
 
   bool HasStrictCapacityLimit() const override;
   size_t GetCapacity() const override;
+  Status GetSecondaryCacheCapacity(size_t& size) const override;
+  Status GetSecondaryCachePinnedUsage(size_t& size) const override;
 
   using Cache::GetUsage;
   size_t GetUsage(Handle* handle) const override;

db/db_impl/db_impl.h

@@ -1393,8 +1393,9 @@ class DBImpl : public DB {
     autovector<ColumnFamilyData*> cfds_;
     autovector<const MutableCFOptions*> mutable_cf_opts_;
     autovector<autovector<VersionEdit*>> edit_lists_;
-    // files_to_delete_ contains sst files
-    std::unordered_set<std::string> files_to_delete_;
+    // Stale SST files to delete found upon recovery. This stores a mapping from
+    // such a file's absolute path to its parent directory.
+    std::unordered_map<std::string, std::string> files_to_delete_;
     bool is_new_db_ = false;
   };
 

db/db_impl/db_impl_files.cc

@@ -1003,7 +1003,7 @@ Status DBImpl::DeleteUnreferencedSstFiles(RecoveryContext* recovery_ctx) {
       if (type == kTableFile && number >= next_file_number &&
           recovery_ctx->files_to_delete_.find(normalized_fpath) ==
               recovery_ctx->files_to_delete_.end()) {
-        recovery_ctx->files_to_delete_.emplace(normalized_fpath);
+        recovery_ctx->files_to_delete_.emplace(normalized_fpath, path);
       }
     }
   }

db/db_impl/db_impl_open.cc

@@ -942,8 +942,11 @@ Status DBImpl::LogAndApplyForRecovery(const RecoveryContext& recovery_ctx) {
       recovery_ctx.edit_lists_, &mutex_, directories_.GetDbDir());
   if (s.ok() && !(recovery_ctx.files_to_delete_.empty())) {
     mutex_.Unlock();
-    for (const auto& fname : recovery_ctx.files_to_delete_) {
-      s = env_->DeleteFile(fname);
+    for (const auto& stale_sst_file : recovery_ctx.files_to_delete_) {
+      s = DeleteDBFile(&immutable_db_options_, stale_sst_file.first,
+                       stale_sst_file.second,
+                       /*force_bg=*/false,
+                       /*force_fg=*/false);
       if (!s.ok()) {
         break;
       }

db/db_sst_test.cc

@@ -937,12 +937,21 @@ INSTANTIATE_TEST_CASE_P(DBWALTestWithParam, DBWALTestWithParam,
                         ::testing::Values(std::make_tuple("", true),
                                           std::make_tuple("_wal_dir", false)));
 
-TEST_F(DBSSTTest, OpenDBWithExistingTrash) {
+TEST_F(DBSSTTest, OpenDBWithExistingTrashAndObsoleteSstFile) {
   Options options = CurrentOptions();
-
   options.sst_file_manager.reset(
       NewSstFileManager(env_, nullptr, "", 1024 * 1024 /* 1 MB/sec */));
   auto sfm = static_cast<SstFileManagerImpl*>(options.sst_file_manager.get());
+  // Set an extra high trash ratio to prevent immediate/non-rate limited
+  // deletions
+  sfm->SetDeleteRateBytesPerSecond(1024 * 1024);
+  sfm->delete_scheduler()->SetMaxTrashDBRatio(1000.0);
+
+  int bg_delete_file = 0;
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
+      "DeleteScheduler::DeleteTrashFile:DeleteFile",
+      [&](void* /*arg*/) { bg_delete_file++; });
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
 
   Destroy(last_options_);
 
@@ -951,13 +960,22 @@ TEST_F(DBSSTTest, OpenDBWithExistingTrash) {
   ASSERT_OK(WriteStringToFile(env_, "abc", dbname_ + "/" + "001.sst.trash"));
   ASSERT_OK(WriteStringToFile(env_, "abc", dbname_ + "/" + "002.sst.trash"));
   ASSERT_OK(WriteStringToFile(env_, "abc", dbname_ + "/" + "003.sst.trash"));
+  // Manually add an obsolete sst file. Obsolete SST files are discovered and
+  // deleted upon recovery.
+  constexpr uint64_t kSstFileNumber = 100;
+  const std::string kObsoleteSstFile =
+      MakeTableFileName(dbname_, kSstFileNumber);
+  ASSERT_OK(WriteStringToFile(env_, "abc", kObsoleteSstFile));
 
-  // Reopen the DB and verify that it deletes existing trash files
+  // Reopen the DB and verify that it deletes existing trash files and obsolete
+  // SST files with rate limiting.
   Reopen(options);
   sfm->WaitForEmptyTrash();
   ASSERT_NOK(env_->FileExists(dbname_ + "/" + "001.sst.trash"));
   ASSERT_NOK(env_->FileExists(dbname_ + "/" + "002.sst.trash"));
   ASSERT_NOK(env_->FileExists(dbname_ + "/" + "003.sst.trash"));
+  ASSERT_NOK(env_->FileExists(kObsoleteSstFile));
+  ASSERT_EQ(bg_delete_file, 4);
 }
 
 // Create a DB with 2 db_paths, and generate multiple files in the 2

db_stress_tool/db_stress_common.cc

@@ -189,7 +189,7 @@ void CompressedCacheSetCapacityThread(void* v) {
                 s.ToString().c_str());
       }
     } else if (FLAGS_compressed_secondary_cache_ratio > 0.0) {
-      if (thread->rand.OneIn(2)) {
+      if (thread->rand.OneIn(2)) {  // if (thread->rand.OneIn(2)) {
         size_t capacity = block_cache->GetCapacity();
         size_t adjustment;
         if (FLAGS_use_write_buffer_manager && FLAGS_db_write_buffer_size > 0) {

env/unique_id_gen.cc

@@ -21,6 +21,7 @@
 #ifdef __SSE4_2__
 #ifdef _WIN32
 #include <intrin.h>
+#define _rdtsc() __rdtsc()
 #else
 #include <x86intrin.h>
 #endif

include/rocksdb/advanced_cache.h

@@ -375,6 +375,14 @@ class Cache {
   // Returns the helper for the specified entry.
   virtual const CacheItemHelper* GetCacheItemHelper(Handle* handle) const = 0;
 
+  virtual Status GetSecondaryCacheCapacity(size_t& /*size*/) const {
+    return Status::NotSupported();
+  }
+
+  virtual Status GetSecondaryCachePinnedUsage(size_t& /*size*/) const {
+    return Status::NotSupported();
+  }
+
   // Call this on shutdown if you want to speed it up. Cache will disown
   // any underlying data and will not free it on delete. This call will leak
   // memory - call this only if you're shutting down the process.

include/rocksdb/env.h

@@ -1001,7 +1001,7 @@ class WritableFile {
   /*
    * Get the size of valid data in the file.
    */
-  virtual uint64_t GetFileSize() { return 0; };
+  virtual uint64_t GetFileSize() { return 0; }
 
   /*
    * Get and set the default pre-allocation block size for writes to

include/rocksdb/version.h

@@ -13,7 +13,7 @@
 // minor or major version number planned for release.
 #define ROCKSDB_MAJOR 8
 #define ROCKSDB_MINOR 8
-#define ROCKSDB_PATCH 0
+#define ROCKSDB_PATCH 1
 
 // Do not use these. We made the mistake of declaring macros starting with
 // double underscore. Now we have to live with our choice. We'll deprecate these

unreleased_history/behavior_changes/no_fs_prefetch_on_zero_compaction_readahead.md

@@ -1 +0,0 @@
-For non direct IO, eliminate the file system prefetching attempt for compaction read when `Options::compaction_readahead_size` is 0

unreleased_history/behavior_changes/stopped_writes_wait_for_recovery.md

@@ -1 +0,0 @@
-* During a write stop, writes now block on in-progress recovery attempts

unreleased_history/bug_fixes/auto_readahead.md

@@ -1 +0,0 @@
-Fix a bug in auto_readahead_size where first_internal_key of index blocks wasn't copied properly resulting in corruption error when first_internal_key was used for comparison.

unreleased_history/bug_fixes/fallback_only_unsupported.md

@@ -1 +0,0 @@
-Fixed a bug where compaction read under non direct IO still falls back to RocksDB internal prefetching after file system's prefetching returns non-OK status other than `Status::NotSupported()`

unreleased_history/bug_fixes/fix_bounds_check_in_BaseDeltaIterator_and_Write(Un)PreparedTxn.md

@@ -1 +0,0 @@
-Add bounds check in WBWIIteratorImpl and make BaseDeltaIterator, WriteUnpreparedTxn and WritePreparedTxn respect the upper bound and lower bound in ReadOption. See 11680.

unreleased_history/bug_fixes/max_successive_merges_wide_columns.md

@@ -1 +0,0 @@
-Fixed the handling of wide-column base values in the `max_successive_merges` logic.

unreleased_history/bug_fixes/options_race.md

@@ -1 +0,0 @@
-* Fixed a rare race bug involving a concurrent combination of Create/DropColumnFamily and/or Set(DB)Options that could lead to inconsistency between (a) the DB's reported options state, (b) the DB options in effect, and (c) the latest persisted OPTIONS file.

unreleased_history/bug_fixes/sec_cache_reservation_underflow.md

@@ -1 +0,0 @@
-Fixed a possible underflow when computing the compressed secondary cache share of memory reservations while updating the compressed secondary to total block cache ratio.

unreleased_history/new_features/attribute_group_multiget.md

@@ -1 +0,0 @@
-Introduce AttributeGroup by adding the first AttributeGroup support API, MultiGetEntity(). Through the use of Column Families, AttributeGroup enables users to logically group wide-column entities. More APIs to support AttributeGroup will come soon, including GetEntity, PutEntity, and others.

unreleased_history/new_features/fifo_drop_file_new_stats.md

@@ -1 +0,0 @@
-Added new tickers `rocksdb.fifo.{max.size|ttl}.compactions` to count FIFO compactions that drop files for different reasons

unreleased_history/new_features/offpeak_db_option.md

@@ -1 +0,0 @@
-Add an experimental offpeak duration awareness by setting `DBOptions::daily_offpeak_time_utc` in "HH:mm-HH:mm" format. This information will be used for resource optimization in the future

unreleased_history/new_features/runtime_burst_bytes.md

@@ -1 +0,0 @@
-Users can now change the max bytes granted in a single refill period (i.e, burst) during runtime by `SetSingleBurstBytes()` for RocksDB rate limiter

unreleased_history/performance_improvements/options_files_on_open.md

@@ -1 +0,0 @@
-Improved the I/O efficiency of DB::Open a new DB with `create_missing_column_families=true` and many column families.

unreleased_history/public_api_changes/fail_if_options_file_error_default_change.md

@@ -1 +0,0 @@
-* The default value of `DBOptions::fail_if_options_file_error` changed from `false` to `true`. Operations that set in-memory options (e.g., `DB::Open*()`, `DB::SetOptions()`, `DB::CreateColumnFamily*()`, and `DB::DropColumnFamily()`) but fail to persist the change will now return a non-OK `Status` by default.

util/crc32c.cc

@@ -1117,7 +1117,13 @@ static inline Function Choose_Extend() {
   }
 #elif defined(__SSE4_2__) && defined(__PCLMUL__) && !defined NO_THREEWAY_CRC32C
   // NOTE: runtime detection no longer supported on x86
-  (void)ExtendImpl<DefaultCRC32>;  // suppress unused warning
+#ifdef _MSC_VER
+#pragma warning(disable: 4551)
+#endif
+  (void)ExtendImpl<DefaultCRC32>; // suppress unused warning
+#ifdef _MSC_VER
+#pragma warning(default: 4551)
+#endif
   return crc32c_3way;
 #else
   return ExtendImpl<DefaultCRC32>;