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

BlobDB: Fix VisibleToActiveSnapshot() (#4236)

Browse Source 
Summary:
There are two issues with `VisibleToActiveSnapshot`:
1. If there are no snapshots, `oldest_snapshot` will be 0 and `VisibleToActiveSnapshot` will always return true. Since the method is used to decide whether it is safe to delete obsolete files, obsolete file won't be able to delete in this case.
2. The `auto` keyword of `auto snapshots = db_impl_->snapshots()` translate to a copy of `const SnapshotList` instead of a reference. Since copy constructor of `SnapshotList` is not defined, using the copy may yield unexpected result.

Issue 2 actually hide issue 1 from being catch by tests. During test `snapshots.empty()` can return false while it should actually be empty, and `snapshots.oldest()` return an invalid address, making `oldest_snapshot` being some random large number.

The issue was originally reported by BlobDB early adopter at Kuaishou.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/4236

Differential Revision: D9188706

Pulled By: yiwu-arbug

fbshipit-source-id: a0f2624b927cf9bf28c1bb534784fee5d106f5ea
This commit is contained in:
Yi Wu 2018-08-06 16:45:02 -07:00
parent c1d8c1e7dc
commit 4bd3bc5c4f
2 changed files with 5 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
db/snapshot_impl.h
View File

@ -56,6 +56,9 @@ class SnapshotList {
count_ = 0; count_ = 0;
} }
// No copy-construct.
SnapshotList(const SnapshotList&) = delete;
bool empty() const { return list_.next_ == &list_; } bool empty() const { return list_.next_ == &list_; }
SnapshotImpl* oldest() const { assert(!empty()); return list_.next_; } SnapshotImpl* oldest() const { assert(!empty()); return list_.next_; }
SnapshotImpl* newest() const { assert(!empty()); return list_.prev_; } SnapshotImpl* newest() const { assert(!empty()); return list_.prev_; }

4
utilities/blob_db/blob_db_impl.cc
View File

@ -1254,11 +1254,11 @@ bool BlobDBImpl::VisibleToActiveSnapshot(
// [earliest_sequence, obsolete_sequence). But doing so will make the // [earliest_sequence, obsolete_sequence). But doing so will make the
// implementation more complicated. // implementation more complicated.
SequenceNumber obsolete_sequence = bfile->GetObsoleteSequence(); SequenceNumber obsolete_sequence = bfile->GetObsoleteSequence();
SequenceNumber oldest_snapshot = 0; SequenceNumber oldest_snapshot = kMaxSequenceNumber;
{ {
// Need to lock DBImpl mutex before access snapshot list. // Need to lock DBImpl mutex before access snapshot list.
InstrumentedMutexLock l(db_impl_->mutex()); InstrumentedMutexLock l(db_impl_->mutex());
auto snapshots = db_impl_->snapshots(); auto& snapshots = db_impl_->snapshots();
if (!snapshots.empty()) { if (!snapshots.empty()) {
oldest_snapshot = snapshots.oldest()->GetSequenceNumber(); oldest_snapshot = snapshots.oldest()->GetSequenceNumber();
} }