From b1423bd9e2235185fe8c6b4c1a9eb5e473651a66 Mon Sep 17 00:00:00 2001 From: Miguel Portilla Date: Wed, 8 Apr 2020 11:49:28 -0400 Subject: [PATCH 1/3] Improve online delete backend locking --- src/ripple/app/misc/SHAMapStoreImp.cpp | 20 ++-- src/ripple/nodestore/DatabaseRotating.h | 13 +-- .../nodestore/impl/DatabaseRotatingImp.cpp | 104 +++++++++++++++--- .../nodestore/impl/DatabaseRotatingImp.h | 62 ++--------- 4 files changed, 108 insertions(+), 91 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 46fdeeac019..c142d65deee 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -426,22 +426,16 @@ SHAMapStoreImp::run() default:; } - std::string nextArchiveDir = - dbRotating_->getWritableBackend()->getName(); lastRotated_ = validatedSeq; - std::shared_ptr oldBackend; - { - std::lock_guard lock(dbRotating_->peekMutex()); + state_db_.setState(SavedState { + newBackend->getName(), + dbRotating_->getName(), + lastRotated_}); - state_db_.setState(SavedState{ - newBackend->getName(), nextArchiveDir, lastRotated_}); - clearCaches(validatedSeq); - oldBackend = - dbRotating_->rotateBackends(std::move(newBackend), lock); - } - JLOG(journal_.warn()) << "finished rotation " << validatedSeq; + clearCaches(validatedSeq); + dbRotating_->rotateBackends(std::move(newBackend)); - oldBackend->setDeletePath(); + JLOG(journal_.warn()) << "finished rotation " << validatedSeq; } } } diff --git a/src/ripple/nodestore/DatabaseRotating.h b/src/ripple/nodestore/DatabaseRotating.h index caf5a9c4a9c..574b16d1348 100644 --- a/src/ripple/nodestore/DatabaseRotating.h +++ b/src/ripple/nodestore/DatabaseRotating.h @@ -47,16 +47,9 @@ class DatabaseRotating : public Database virtual TaggedCache const& getPositiveCache() = 0; - virtual std::mutex& - peekMutex() const = 0; - - virtual std::shared_ptr const& - getWritableBackend() const = 0; - - virtual std::shared_ptr - rotateBackends( - std::shared_ptr newBackend, - std::lock_guard const&) = 0; + virtual + void + rotateBackends(std::shared_ptr newBackend) = 0; }; } // namespace NodeStore diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp index f09229efde3..784c2bb4b6f 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp @@ -55,15 +55,54 @@ DatabaseRotatingImp::DatabaseRotatingImp( setParent(parent); } -std::shared_ptr -DatabaseRotatingImp::rotateBackends( - std::shared_ptr newBackend, - std::lock_guard const&) +void +DatabaseRotatingImp::rotateBackends(std::shared_ptr newBackend) { - auto oldBackend{std::move(archiveBackend_)}; + std::lock_guard lock(mutex_); + archiveBackend_->setDeletePath(); archiveBackend_ = std::move(writableBackend_); writableBackend_ = std::move(newBackend); - return oldBackend; +} + +std::string +DatabaseRotatingImp::getName() const +{ + std::lock_guard lock(mutex_); + return writableBackend_->getName(); +} + +std::int32_t +DatabaseRotatingImp::getWriteLoad() const +{ + std::lock_guard lock(mutex_); + return writableBackend_->getWriteLoad(); +} + +void +DatabaseRotatingImp::import(Database& source) +{ + auto const backend = [&] { + std::lock_guard lock(mutex_); + return writableBackend_; + }(); + + importInternal(*backend, source); +} + +bool +DatabaseRotatingImp::storeLedger(std::shared_ptr const& srcLedger) +{ + auto const backend = [&] { + std::lock_guard lock(mutex_); + return writableBackend_; + }(); + + return Database::storeLedger( + *srcLedger, + backend, + pCache_, + nCache_, + nullptr); } void @@ -75,7 +114,13 @@ DatabaseRotatingImp::store( { auto nObj = NodeObject::createObject(type, std::move(data), hash); pCache_->canonicalize_replace_cache(hash, nObj); - getWritableBackend()->store(nObj); + + auto const backend = [&] { + std::lock_guard lock(mutex_); + return writableBackend_; + }(); + backend->store(nObj); + nCache_->erase(hash); storeStats(nObj->getData().size()); } @@ -90,6 +135,7 @@ DatabaseRotatingImp::asyncFetch( object = pCache_->fetch(hash); if (object || nCache_->touch_if_exists(hash)) return true; + // Otherwise post a read Database::asyncFetch(hash, seq, pCache_, nCache_); return false; @@ -114,19 +160,49 @@ DatabaseRotatingImp::sweep() std::shared_ptr DatabaseRotatingImp::fetchFrom(uint256 const& hash, std::uint32_t seq) { - Backends b = getBackends(); - auto nObj = fetchInternal(hash, b.writableBackend); - if (!nObj) + auto backends = [&] { + std::lock_guard lock(mutex_); + return std::make_pair(writableBackend_, archiveBackend_); + }(); + + // Try to fetch from the writable backend + auto nObj = fetchInternal(hash, backends.first); + if (! nObj) { - nObj = fetchInternal(hash, b.archiveBackend); + // Otherwise try to fetch from the archive backend + nObj = fetchInternal(hash, backends.second); if (nObj) { - getWritableBackend()->store(nObj); + { + // Refresh the writable backend pointer + std::lock_guard lock(mutex_); + backends.first = writableBackend_; + } + + // Update writable backend with data from the archive backend + backends.first->store(nObj); nCache_->erase(hash); } } return nObj; } -} // namespace NodeStore -} // namespace ripple +void +DatabaseRotatingImp::for_each( + std::function )> f) +{ + auto const backends = [&] { + std::lock_guard lock(mutex_); + return std::make_pair(writableBackend_, archiveBackend_); + }(); + + // Iterate the writable backend + backends.first->for_each(f); + + // Iterate the archive backend + backends.second->for_each(f); +} + + +} // NodeStore +} // ripple diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.h b/src/ripple/nodestore/impl/DatabaseRotatingImp.h index 8a29d117a2b..6c2d1829fe2 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.h +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.h @@ -49,41 +49,17 @@ class DatabaseRotatingImp : public DatabaseRotating stopThreads(); } - std::shared_ptr const& - getWritableBackend() const override - { - std::lock_guard lock(rotateMutex_); - return writableBackend_; - } - - std::shared_ptr - rotateBackends( - std::shared_ptr newBackend, - std::lock_guard const&) override; - - std::mutex& - peekMutex() const override - { - return rotateMutex_; - } + void + rotateBackends(std::shared_ptr newBackend) override; std::string - getName() const override - { - return getWritableBackend()->getName(); - } + getName() const override; std::int32_t - getWriteLoad() const override - { - return getWritableBackend()->getWriteLoad(); - } + getWriteLoad() const override; void - import(Database& source) override - { - importInternal(*getWritableBackend(), source); - } + import(Database& source) override; void store( @@ -105,11 +81,7 @@ class DatabaseRotatingImp : public DatabaseRotating std::shared_ptr& object) override; bool - storeLedger(std::shared_ptr const& srcLedger) override - { - return Database::storeLedger( - *srcLedger, getWritableBackend(), pCache_, nCache_, nullptr); - } + storeLedger(std::shared_ptr const& srcLedger) override; int getDesiredAsyncReadCount(std::uint32_t seq) override @@ -147,31 +119,13 @@ class DatabaseRotatingImp : public DatabaseRotating std::shared_ptr writableBackend_; std::shared_ptr archiveBackend_; - mutable std::mutex rotateMutex_; - - struct Backends - { - std::shared_ptr const& writableBackend; - std::shared_ptr const& archiveBackend; - }; - - Backends - getBackends() const - { - std::lock_guard lock(rotateMutex_); - return Backends{writableBackend_, archiveBackend_}; - } + mutable std::mutex mutex_; std::shared_ptr fetchFrom(uint256 const& hash, std::uint32_t seq) override; void - for_each(std::function)> f) override - { - Backends b = getBackends(); - b.archiveBackend->for_each(f); - b.writableBackend->for_each(f); - } + for_each(std::function )> f) override; }; } // namespace NodeStore From c2291a6b756ecc248af169ef2111253c4736f627 Mon Sep 17 00:00:00 2001 From: Miguel Portilla Date: Fri, 17 Apr 2020 13:01:50 -0400 Subject: [PATCH 2/3] [FOLD] Address seelabs feedback --- src/ripple/app/misc/SHAMapStoreImp.cpp | 13 +++++++------ src/ripple/nodestore/DatabaseRotating.h | 4 +++- .../nodestore/impl/DatabaseRotatingImp.cpp | 19 ++++++++++--------- .../nodestore/impl/DatabaseRotatingImp.h | 2 +- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index c142d65deee..968bc2a84c4 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -427,13 +427,14 @@ SHAMapStoreImp::run() } lastRotated_ = validatedSeq; - state_db_.setState(SavedState { - newBackend->getName(), - dbRotating_->getName(), - lastRotated_}); - clearCaches(validatedSeq); - dbRotating_->rotateBackends(std::move(newBackend)); + + SavedState savedState; + savedState.writableDb = newBackend->getName(); + savedState.archiveDb = dbRotating_->rotateBackends( + std::move(newBackend)); + savedState.lastRotated = lastRotated_; + state_db_.setState(savedState); JLOG(journal_.warn()) << "finished rotation " << validatedSeq; } diff --git a/src/ripple/nodestore/DatabaseRotating.h b/src/ripple/nodestore/DatabaseRotating.h index 574b16d1348..e01520daaf8 100644 --- a/src/ripple/nodestore/DatabaseRotating.h +++ b/src/ripple/nodestore/DatabaseRotating.h @@ -47,8 +47,10 @@ class DatabaseRotating : public Database virtual TaggedCache const& getPositiveCache() = 0; + /** Returns the name of the rotated backend. + */ virtual - void + std::string rotateBackends(std::shared_ptr newBackend) = 0; }; diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp index 784c2bb4b6f..9a95a33003a 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp @@ -55,13 +55,14 @@ DatabaseRotatingImp::DatabaseRotatingImp( setParent(parent); } -void +std::string DatabaseRotatingImp::rotateBackends(std::shared_ptr newBackend) { std::lock_guard lock(mutex_); archiveBackend_->setDeletePath(); archiveBackend_ = std::move(writableBackend_); writableBackend_ = std::move(newBackend); + return archiveBackend_->getName(); } std::string @@ -160,27 +161,27 @@ DatabaseRotatingImp::sweep() std::shared_ptr DatabaseRotatingImp::fetchFrom(uint256 const& hash, std::uint32_t seq) { - auto backends = [&] { + auto [writable, archive] = [&] { std::lock_guard lock(mutex_); return std::make_pair(writableBackend_, archiveBackend_); }(); // Try to fetch from the writable backend - auto nObj = fetchInternal(hash, backends.first); + auto nObj = fetchInternal(hash, writable); if (! nObj) { // Otherwise try to fetch from the archive backend - nObj = fetchInternal(hash, backends.second); + nObj = fetchInternal(hash, archive); if (nObj) { { // Refresh the writable backend pointer std::lock_guard lock(mutex_); - backends.first = writableBackend_; + writable = writableBackend_; } // Update writable backend with data from the archive backend - backends.first->store(nObj); + writable->store(nObj); nCache_->erase(hash); } } @@ -191,16 +192,16 @@ void DatabaseRotatingImp::for_each( std::function )> f) { - auto const backends = [&] { + auto [writable, archive] = [&] { std::lock_guard lock(mutex_); return std::make_pair(writableBackend_, archiveBackend_); }(); // Iterate the writable backend - backends.first->for_each(f); + writable->for_each(f); // Iterate the archive backend - backends.second->for_each(f); + archive->for_each(f); } diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.h b/src/ripple/nodestore/impl/DatabaseRotatingImp.h index 6c2d1829fe2..654483bb9f6 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.h +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.h @@ -49,7 +49,7 @@ class DatabaseRotatingImp : public DatabaseRotating stopThreads(); } - void + std::string rotateBackends(std::shared_ptr newBackend) override; std::string From c2541adf2dda2833f38f346dd56c4e5e55e3e5d0 Mon Sep 17 00:00:00 2001 From: Miguel Portilla Date: Thu, 30 Apr 2020 13:12:35 -0400 Subject: [PATCH 3/3] [FOLD] Address mtrippled feedback --- src/ripple/app/misc/SHAMapStoreImp.cpp | 19 +++++++++------ src/ripple/nodestore/DatabaseRotating.h | 10 ++++---- .../nodestore/impl/DatabaseRotatingImp.cpp | 24 +++++++++---------- .../nodestore/impl/DatabaseRotatingImp.h | 8 ++++--- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 968bc2a84c4..3c9ccb94235 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -427,14 +427,19 @@ SHAMapStoreImp::run() } lastRotated_ = validatedSeq; - clearCaches(validatedSeq); - SavedState savedState; - savedState.writableDb = newBackend->getName(); - savedState.archiveDb = dbRotating_->rotateBackends( - std::move(newBackend)); - savedState.lastRotated = lastRotated_; - state_db_.setState(savedState); + dbRotating_->rotateWithLock( + [&](std::string const& writableBackendName) { + SavedState savedState; + savedState.writableDb = newBackend->getName(); + savedState.archiveDb = writableBackendName; + savedState.lastRotated = lastRotated_; + state_db_.setState(savedState); + + clearCaches(validatedSeq); + + return std::move(newBackend); + }); JLOG(journal_.warn()) << "finished rotation " << validatedSeq; } diff --git a/src/ripple/nodestore/DatabaseRotating.h b/src/ripple/nodestore/DatabaseRotating.h index e01520daaf8..b1feb257095 100644 --- a/src/ripple/nodestore/DatabaseRotating.h +++ b/src/ripple/nodestore/DatabaseRotating.h @@ -47,11 +47,13 @@ class DatabaseRotating : public Database virtual TaggedCache const& getPositiveCache() = 0; - /** Returns the name of the rotated backend. + /** Rotates the backends. + + @param f A function executed before the rotation and under the same lock */ - virtual - std::string - rotateBackends(std::shared_ptr newBackend) = 0; + virtual void + rotateWithLock(std::function( + std::string const& writableBackendName)> const& f) = 0; }; } // namespace NodeStore diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp index 9a95a33003a..9b9f966ae52 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.cpp @@ -55,14 +55,17 @@ DatabaseRotatingImp::DatabaseRotatingImp( setParent(parent); } -std::string -DatabaseRotatingImp::rotateBackends(std::shared_ptr newBackend) +void +DatabaseRotatingImp::rotateWithLock( + std::function( + std::string const& writableBackendName)> const& f) { std::lock_guard lock(mutex_); + + auto newBackend = f(writableBackend_->getName()); archiveBackend_->setDeletePath(); archiveBackend_ = std::move(writableBackend_); writableBackend_ = std::move(newBackend); - return archiveBackend_->getName(); } std::string @@ -99,11 +102,7 @@ DatabaseRotatingImp::storeLedger(std::shared_ptr const& srcLedger) }(); return Database::storeLedger( - *srcLedger, - backend, - pCache_, - nCache_, - nullptr); + *srcLedger, backend, pCache_, nCache_, nullptr); } void @@ -168,7 +167,7 @@ DatabaseRotatingImp::fetchFrom(uint256 const& hash, std::uint32_t seq) // Try to fetch from the writable backend auto nObj = fetchInternal(hash, writable); - if (! nObj) + if (!nObj) { // Otherwise try to fetch from the archive backend nObj = fetchInternal(hash, archive); @@ -190,7 +189,7 @@ DatabaseRotatingImp::fetchFrom(uint256 const& hash, std::uint32_t seq) void DatabaseRotatingImp::for_each( - std::function )> f) + std::function)> f) { auto [writable, archive] = [&] { std::lock_guard lock(mutex_); @@ -204,6 +203,5 @@ DatabaseRotatingImp::for_each( archive->for_each(f); } - -} // NodeStore -} // ripple +} // namespace NodeStore +} // namespace ripple diff --git a/src/ripple/nodestore/impl/DatabaseRotatingImp.h b/src/ripple/nodestore/impl/DatabaseRotatingImp.h index 654483bb9f6..ea6c92567ef 100644 --- a/src/ripple/nodestore/impl/DatabaseRotatingImp.h +++ b/src/ripple/nodestore/impl/DatabaseRotatingImp.h @@ -49,8 +49,10 @@ class DatabaseRotatingImp : public DatabaseRotating stopThreads(); } - std::string - rotateBackends(std::shared_ptr newBackend) override; + void + rotateWithLock( + std::function( + std::string const& writableBackendName)> const& f) override; std::string getName() const override; @@ -125,7 +127,7 @@ class DatabaseRotatingImp : public DatabaseRotating fetchFrom(uint256 const& hash, std::uint32_t seq) override; void - for_each(std::function )> f) override; + for_each(std::function)> f) override; }; } // namespace NodeStore