andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix valgrind error caused by FileMetaData as two level iterator's index block handle

Browse Source 
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
This commit is contained in:
sdong 2014-04-01 18:36:18 -07:00
parent 8c4a3bfa5b
commit 284c365b77
2 changed files with 35 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
db/table_cache.h
View File

@ -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,

45
db/version_set.cc
View File

@ -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;
}