From 4cb7068c1e7534fa31dd6132f02f111c7f0988a4 Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Mon, 6 Aug 2018 16:45:02 -0700 Subject: [PATCH] BlobDB: Fix VisibleToActiveSnapshot() (#4236) 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 --- db/snapshot_impl.h | 3 +++ utilities/blob_db/blob_db_impl.cc | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/db/snapshot_impl.h b/db/snapshot_impl.h index c9ddabdec..edaabade7 100644 --- a/db/snapshot_impl.h +++ b/db/snapshot_impl.h @@ -56,6 +56,9 @@ class SnapshotList { count_ = 0; } + // No copy-construct. + SnapshotList(const SnapshotList&) = delete; + bool empty() const { return list_.next_ == &list_; } SnapshotImpl* oldest() const { assert(!empty()); return list_.next_; } SnapshotImpl* newest() const { assert(!empty()); return list_.prev_; } diff --git a/utilities/blob_db/blob_db_impl.cc b/utilities/blob_db/blob_db_impl.cc index da9a3d13e..7cbd5809f 100644 --- a/utilities/blob_db/blob_db_impl.cc +++ b/utilities/blob_db/blob_db_impl.cc @@ -1254,11 +1254,11 @@ bool BlobDBImpl::VisibleToActiveSnapshot( // [earliest_sequence, obsolete_sequence). But doing so will make the // implementation more complicated. SequenceNumber obsolete_sequence = bfile->GetObsoleteSequence(); - SequenceNumber oldest_snapshot = 0; + SequenceNumber oldest_snapshot = kMaxSequenceNumber; { // Need to lock DBImpl mutex before access snapshot list. InstrumentedMutexLock l(db_impl_->mutex()); - auto snapshots = db_impl_->snapshots(); + auto& snapshots = db_impl_->snapshots(); if (!snapshots.empty()) { oldest_snapshot = snapshots.oldest()->GetSequenceNumber(); }