From 284c365b77834cd250b3c73cc525d6f05aecd9ab Mon Sep 17 00:00:00 2001
From: sdong <siying.d@fb.com>
Date: Tue, 1 Apr 2014 18:36:18 -0700
Subject: [PATCH] Fix valgrind error caused by FileMetaData as two level
 iterator's index block handle

Summary: It is a regression valgrind bug caused by using FileMetaData as index block handle. One of the fields of FileMetaData is not initialized after being contructed and copied, but I'm not able to find which one. Also, I realized that it's not a good idea to use FileMetaData as in TwoLevelIterator::InitDataBlock(), a copied FileMetaData can be compared with the one in version set byte by byte, but the refs can be changed. Also comparing such a large structure is slightly more expensive. Use a simpler structure instead

Test Plan:
Run the failing valgrind test (Harness.RandomizedLongDB)
make all check

Reviewers: igor, haobo, ljin

Reviewed By: igor

CC: yhchiang, leveldb

Differential Revision: https://reviews.facebook.net/D17409
---
 db/table_cache.h  |  2 ++
 db/version_set.cc | 45 +++++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/db/table_cache.h b/db/table_cache.h
index 42dee2f0f..5f1c29ea5 100644
--- a/db/table_cache.h
+++ b/db/table_cache.h
@@ -25,6 +25,8 @@ namespace rocksdb {
 class Env;
 struct FileMetaData;
 
+// TODO(sdong): try to come up with a better API to pass the file information
+//              other than simply passing FileMetaData.
 class TableCache {
  public:
   TableCache(const std::string& dbname, const Options* options,
diff --git a/db/version_set.cc b/db/version_set.cc
index 77275bdd8..2057d6dd4 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -140,6 +140,18 @@ bool SomeFileOverlapsRange(
   return !BeforeFile(ucmp, largest_user_key, files[index]);
 }
 
+namespace {
+// Used for LevelFileNumIterator to pass "block handle" value,
+// which actually means file information in this iterator.
+// It contains subset of fields of FileMetaData, that is sufficient
+// for table cache to use.
+struct EncodedFileMetaData {
+  uint64_t number;   // file number
+  uint64_t file_size;   // file size
+  Cache::Handle* table_reader_handle;   // cached table reader's handler
+};
+}  // namespace
+
 // An internal iterator.  For a given version/level pair, yields
 // information about the files in the level.  For a given entry, key()
 // is the largest key that occurs in the file, and value() is an
@@ -181,14 +193,19 @@ class Version::LevelFileNumIterator : public Iterator {
   }
   Slice value() const {
     assert(Valid());
-    return Slice(reinterpret_cast<const char*>((*flist_)[index_]),
-                 sizeof(FileMetaData));
+    auto* file_meta = (*flist_)[index_];
+    current_value_.number = file_meta->number;
+    current_value_.file_size = file_meta->file_size;
+    current_value_.table_reader_handle = file_meta->table_reader_handle;
+    return Slice(reinterpret_cast<const char*>(&current_value_),
+                 sizeof(EncodedFileMetaData));
   }
   virtual Status status() const { return Status::OK(); }
  private:
   const InternalKeyComparator icmp_;
   const std::vector<FileMetaData*>* const flist_;
   uint32_t index_;
+  mutable EncodedFileMetaData current_value_;
 };
 
 static Iterator* GetFileIterator(void* arg, const ReadOptions& options,
@@ -196,7 +213,7 @@ static Iterator* GetFileIterator(void* arg, const ReadOptions& options,
                                  const InternalKeyComparator& icomparator,
                                  const Slice& file_value, bool for_compaction) {
   TableCache* cache = reinterpret_cast<TableCache*>(arg);
-  if (file_value.size() != sizeof(FileMetaData)) {
+  if (file_value.size() != sizeof(EncodedFileMetaData)) {
     return NewErrorIterator(
         Status::Corruption("FileReader invoked with unexpected value"));
   } else {
@@ -208,11 +225,13 @@ static Iterator* GetFileIterator(void* arg, const ReadOptions& options,
       options_copy.prefix = nullptr;
     }
 
-    const FileMetaData* meta_file =
-        reinterpret_cast<const FileMetaData*>(file_value.data());
+    const EncodedFileMetaData* encoded_meta =
+        reinterpret_cast<const EncodedFileMetaData*>(file_value.data());
+    FileMetaData meta(encoded_meta->number, encoded_meta->file_size);
+    meta.table_reader_handle = encoded_meta->table_reader_handle;
     return cache->NewIterator(
-        options.prefix ? options_copy : options, soptions, icomparator,
-        *meta_file, nullptr /* don't need reference to table*/, for_compaction);
+        options.prefix ? options_copy : options, soptions, icomparator, meta,
+        nullptr /* don't need reference to table*/, for_compaction);
   }
 }
 
@@ -231,11 +250,13 @@ bool Version::PrefixMayMatch(const ReadOptions& options,
     // key() will always be the biggest value for this SST?
     may_match = true;
   } else {
-    const FileMetaData* meta_file =
-        reinterpret_cast<const FileMetaData*>(level_iter->value().data());
-
-    may_match = vset_->table_cache_->PrefixMayMatch(
-        options, vset_->icmp_, *meta_file, internal_prefix, nullptr);
+    const EncodedFileMetaData* encoded_meta =
+        reinterpret_cast<const EncodedFileMetaData*>(
+            level_iter->value().data());
+    FileMetaData meta(encoded_meta->number, encoded_meta->file_size);
+    meta.table_reader_handle = encoded_meta->table_reader_handle;
+    may_match = vset_->table_cache_->PrefixMayMatch(options, vset_->icmp_, meta,
+                                                    internal_prefix, nullptr);
   }
   return may_match;
 }