From 9c24a5cb4dfa6963fe0be05a62eafbb30f6ed980 Mon Sep 17 00:00:00 2001 From: Zhen Li Date: Sat, 13 Jun 2020 13:26:03 -0700 Subject: [PATCH] Fix persistent cache on windows (#6932) Summary: Persistent cache feature caused rocks db crash on windows. I posted a issue for it, https://github.com/facebook/rocksdb/issues/6919. I found this is because no "persistent_cache_key_prefix" is generated for persistent cache. Looking repo history, "GetUniqueIdFromFile" is not implemented on Windows. So my fix is adding "NewId()" function in "persistent_cache" and using it to generate prefix for persistent cache. In this PR, i also re-enable related test cases defined in "db_test2" and "persistent_cache_test" for windows. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6932 Test Plan: 1. run related test cases in "db_test2" and "persistent_cache_test" on windows and see it passed. 2. manually run db_bench.exe with "read_cache_path" and verified. Reviewed By: riversand963 Differential Revision: D21911608 Pulled By: cheng-chang fbshipit-source-id: cdfd938d54a385edbb2836b13aaa1d39b0a6f1c2 --- HISTORY.md | 1 + db/db_test2.cc | 12 ++--- include/rocksdb/persistent_cache.h | 6 +++ .../block_based/block_based_table_builder.cc | 2 +- table/block_based/block_based_table_reader.cc | 45 +++++-------------- table/block_based/block_based_table_reader.h | 17 +++++-- .../persistent_cache/persistent_cache_test.cc | 9 ++-- .../persistent_cache/persistent_cache_tier.cc | 4 ++ .../persistent_cache/persistent_cache_tier.h | 3 ++ 9 files changed, 49 insertions(+), 50 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 4e1db503a..fc811d00d 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -12,6 +12,7 @@ * Fix false negative from the VerifyChecksum() API when there is a checksum mismatch in an index partition block in a BlockBasedTable format table file (index_type is kTwoLevelIndexSearch). * Fix sst_dump to return non-zero exit code if the specified file is not a recognized SST file or fails requested checks. * Fix incorrect results from batched MultiGet for duplicate keys, when the duplicate key matches the largest key of an SST file and the value type for the key in the file is a merge value. +* Fix "bad block type" error from persistent cache on Windows. ### Public API Change * Flush(..., column_family) may return Status::ColumnFamilyDropped() instead of Status::InvalidArgument() if column_family is dropped while processing the flush request. diff --git a/db/db_test2.cc b/db/db_test2.cc index 467492155..52370b136 100644 --- a/db/db_test2.cc +++ b/db/db_test2.cc @@ -2010,6 +2010,10 @@ class MockPersistentCache : public PersistentCache { return PersistentCache::StatsType(); } + uint64_t NewId() override { + return last_id_.fetch_add(1, std::memory_order_relaxed); + } + Status Insert(const Slice& page_key, const char* data, const size_t size) override { MutexLock _(&lock_); @@ -2050,6 +2054,7 @@ class MockPersistentCache : public PersistentCache { const bool is_compressed_ = true; size_t size_ = 0; const size_t max_size_ = 10 * 1024; // 10KiB + std::atomic last_id_{1}; }; #ifdef OS_LINUX @@ -2173,10 +2178,7 @@ TEST_F(DBTest2, TestPerfContextIterCpuTime) { } #endif // OS_LINUX -// GetUniqueIdFromFile is not implemented on these platforms. Persistent cache -// breaks when that function is not implemented and no regular block cache is -// provided. -#if !defined(OS_SOLARIS) && !defined(OS_WIN) +#if !defined OS_SOLARIS TEST_F(DBTest2, PersistentCache) { int num_iter = 80; @@ -2240,7 +2242,7 @@ TEST_F(DBTest2, PersistentCache) { } } } -#endif // !defined(OS_SOLARIS) && !defined(OS_WIN) +#endif // !defined OS_SOLARIS namespace { void CountSyncPoint() { diff --git a/include/rocksdb/persistent_cache.h b/include/rocksdb/persistent_cache.h index 9651812c8..e2dcfcac3 100644 --- a/include/rocksdb/persistent_cache.h +++ b/include/rocksdb/persistent_cache.h @@ -56,6 +56,12 @@ class PersistentCache { virtual StatsType Stats() = 0; virtual std::string GetPrintableOptions() const = 0; + + // Return a new numeric id. May be used by multiple clients who are + // sharding the same persistent cache to partition the key space. Typically + // the client will allocate a new id at startup and prepend the id to its + // cache keys. + virtual uint64_t NewId() = 0; }; // Factor method to create a new persistent cache diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index bc1b16ec2..70078a8c9 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -722,7 +722,7 @@ BlockBasedTableBuilder::BlockBasedTableBuilder( rep_->filter_builder->StartBlock(0); } if (table_options.block_cache_compressed.get() != nullptr) { - BlockBasedTable::GenerateCachePrefix( + BlockBasedTable::GenerateCachePrefix( table_options.block_cache_compressed.get(), file->writable_file(), &rep_->compressed_cache_key_prefix[0], &rep_->compressed_cache_key_prefix_size); diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 7ce7ca55e..bb3bfd1d2 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -452,44 +452,21 @@ void BlockBasedTable::SetupCacheKeyPrefix(Rep* rep) { rep->cache_key_prefix_size = 0; rep->compressed_cache_key_prefix_size = 0; if (rep->table_options.block_cache != nullptr) { - GenerateCachePrefix(rep->table_options.block_cache.get(), rep->file->file(), - &rep->cache_key_prefix[0], &rep->cache_key_prefix_size); + GenerateCachePrefix( + rep->table_options.block_cache.get(), rep->file->file(), + &rep->cache_key_prefix[0], &rep->cache_key_prefix_size); } if (rep->table_options.persistent_cache != nullptr) { - GenerateCachePrefix(/*cache=*/nullptr, rep->file->file(), - &rep->persistent_cache_key_prefix[0], - &rep->persistent_cache_key_prefix_size); + GenerateCachePrefix( + rep->table_options.persistent_cache.get(), rep->file->file(), + &rep->persistent_cache_key_prefix[0], + &rep->persistent_cache_key_prefix_size); } if (rep->table_options.block_cache_compressed != nullptr) { - GenerateCachePrefix(rep->table_options.block_cache_compressed.get(), - rep->file->file(), &rep->compressed_cache_key_prefix[0], - &rep->compressed_cache_key_prefix_size); - } -} - -void BlockBasedTable::GenerateCachePrefix(Cache* cc, FSRandomAccessFile* file, - char* buffer, size_t* size) { - // generate an id from the file - *size = file->GetUniqueId(buffer, kMaxCacheKeyPrefixSize); - - // If the prefix wasn't generated or was too long, - // create one from the cache. - if (cc != nullptr && *size == 0) { - char* end = EncodeVarint64(buffer, cc->NewId()); - *size = static_cast(end - buffer); - } -} - -void BlockBasedTable::GenerateCachePrefix(Cache* cc, FSWritableFile* file, - char* buffer, size_t* size) { - // generate an id from the file - *size = file->GetUniqueId(buffer, kMaxCacheKeyPrefixSize); - - // If the prefix wasn't generated or was too long, - // create one from the cache. - if (cc != nullptr && *size == 0) { - char* end = EncodeVarint64(buffer, cc->NewId()); - *size = static_cast(end - buffer); + GenerateCachePrefix( + rep->table_options.block_cache_compressed.get(), rep->file->file(), + &rep->compressed_cache_key_prefix[0], + &rep->compressed_cache_key_prefix_size); } } diff --git a/table/block_based/block_based_table_reader.h b/table/block_based/block_based_table_reader.h index 94f659530..87bf82e53 100644 --- a/table/block_based/block_based_table_reader.h +++ b/table/block_based/block_based_table_reader.h @@ -439,10 +439,19 @@ class BlockBasedTable : public TableReader { static void SetupCacheKeyPrefix(Rep* rep); // Generate a cache key prefix from the file - static void GenerateCachePrefix(Cache* cc, FSRandomAccessFile* file, - char* buffer, size_t* size); - static void GenerateCachePrefix(Cache* cc, FSWritableFile* file, char* buffer, - size_t* size); + template + static void GenerateCachePrefix(TCache* cc, TFile* file, char* buffer, + size_t* size) { + // generate an id from the file + *size = file->GetUniqueId(buffer, kMaxCacheKeyPrefixSize); + + // If the prefix wasn't generated or was too long, + // create one from the cache. + if (cc != nullptr && *size == 0) { + char* end = EncodeVarint64(buffer, cc->NewId()); + *size = static_cast(end - buffer); + } + } // Size of all data blocks, maybe approximate uint64_t GetApproximateDataSize(); diff --git a/utilities/persistent_cache/persistent_cache_test.cc b/utilities/persistent_cache/persistent_cache_test.cc index e7d6a313b..92b39307d 100644 --- a/utilities/persistent_cache/persistent_cache_test.cc +++ b/utilities/persistent_cache/persistent_cache_test.cc @@ -6,10 +6,7 @@ // Copyright (c) 2011 The LevelDB Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. - -// GetUniqueIdFromFile is not implemented on Windows. Persistent cache -// breaks when that function is not implemented -#if !defined(ROCKSDB_LITE) && !defined(OS_WIN) +#if !defined ROCKSDB_LITE #include "utilities/persistent_cache/persistent_cache_test.h" @@ -472,6 +469,6 @@ int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); } -#else // !defined(ROCKSDB_LITE) && !defined(OS_WIN) +#else // !defined ROCKSDB_LITE int main() { return 0; } -#endif // !defined(ROCKSDB_LITE) && !defined(OS_WIN) +#endif // !defined ROCKSDB_LITE diff --git a/utilities/persistent_cache/persistent_cache_tier.cc b/utilities/persistent_cache/persistent_cache_tier.cc index 3847a4ee9..54cbce8f7 100644 --- a/utilities/persistent_cache/persistent_cache_tier.cc +++ b/utilities/persistent_cache/persistent_cache_tier.cc @@ -99,6 +99,10 @@ PersistentCache::StatsType PersistentCacheTier::Stats() { return PersistentCache::StatsType{}; } +uint64_t PersistentCacheTier::NewId() { + return last_id_.fetch_add(1, std::memory_order_relaxed); +} + // // PersistentTieredCache implementation // diff --git a/utilities/persistent_cache/persistent_cache_tier.h b/utilities/persistent_cache/persistent_cache_tier.h index 3905957de..f8fa88c34 100644 --- a/utilities/persistent_cache/persistent_cache_tier.h +++ b/utilities/persistent_cache/persistent_cache_tier.h @@ -266,6 +266,8 @@ class PersistentCacheTier : public PersistentCache { virtual std::string GetPrintableOptions() const override = 0; + virtual uint64_t NewId() override; + // Return a reference to next tier virtual Tier& next_tier() { return next_tier_; } @@ -283,6 +285,7 @@ class PersistentCacheTier : public PersistentCache { private: Tier next_tier_; // next tier + std::atomic last_id_{1}; }; // PersistentTieredCache