Skip to content
13 changes: 13 additions & 0 deletions include/xrpl/nodestore/DatabaseRotating.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ class DatabaseRotating : public Database
std::unique_ptr<NodeStore::Backend>&& newBackend,
std::function<void(std::string const& writableName, std::string const& archiveName)> const&
f) = 0;

/**
* Marks an online-delete rotation as in progress (or completed).
*
* While in flight, a read served by the archive backend is copied
* forward into the writable backend even for ordinary
* (duplicate == false) fetches: the archive is about to be deleted,
* and a node body canonicalized into caches during the rotation
* window would otherwise survive only in RAM once the archive is
* dropped.
*/
virtual void
setRotationInFlight(bool inFlight) = 0;
};

} // namespace xrpl::NodeStore
12 changes: 12 additions & 0 deletions include/xrpl/nodestore/detail/DatabaseRotatingImp.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <xrpl/nodestore/NodeObject.h>
#include <xrpl/nodestore/Scheduler.h>

#include <atomic>
#include <cstdint>
#include <functional>
#include <memory>
Expand Down Expand Up @@ -69,11 +70,22 @@ class DatabaseRotatingImp : public DatabaseRotating
void
sweep() override;

void
setRotationInFlight(bool inFlight) override;

private:
std::shared_ptr<Backend> writableBackend_;
std::shared_ptr<Backend> archiveBackend_;
mutable std::mutex mutex_;

// True between SHAMapStore starting the cache-freshen phase and the
// completion of rotate(). While true, archive hits on ordinary
// (duplicate == false) fetches are copied forward into the writable
// backend; copyForwardCount_ tallies them per rotation for the
// summary line logged at swap.
std::atomic<bool> rotationInFlight_{false};
std::atomic<std::uint64_t> copyForwardCount_{0};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] copyForwardCount_ can use std::memory_order_relaxed on both the increment (fetch_add) and the exchange(0). This counter is stats-only — its value feeds a single log line and gates no other memory access — so atomicity alone is sufficient. operator++ in particular currently defaults to seq_cst, which is the strongest (and most expensive) ordering. Switching to explicit fetch_add(1, std::memory_order_relaxed) is a small performance win.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I made the change though AI says: "Performance: real but negligible here. On x86-64, a seq_cst and a relaxed fetch_add compile to the identical lock xadd instruction — zero difference. On ARM (Apple Silicon, Graviton) there is a genuine difference ( ldadd vs ldaddal with acquire-release semantics). But context matters: this increment executes only on archive-backend hits during a rotation window, on a path that just completed a backend disk fetch and is about to do a backend store. A saved fence is unmeasurable next to the I/O on either side of it. The exchange(0) runs once per rotation (roughly hourly) — irrelevant either way."


std::shared_ptr<NodeObject>
fetchNodeObject(uint256 const& hash, std::uint32_t, FetchReport& fetchReport, bool duplicate)
override;
Expand Down
32 changes: 30 additions & 2 deletions src/libxrpl/nodestore/DatabaseRotatingImp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <xrpl/nodestore/Scheduler.h>
#include <xrpl/nodestore/Types.h>

#include <atomic>
#include <cstdint>
#include <exception>
#include <functional>
Expand Down Expand Up @@ -52,6 +53,7 @@
// callback finishes. Only then will the archive directory be
// deleted.
std::shared_ptr<NodeStore::Backend> oldArchiveBackend;
std::uint64_t copyForwards = 0;
{
std::scoped_lock const lock(mutex_);

Expand All @@ -62,11 +64,28 @@
newArchiveBackendName = archiveBackend_->getName();

writableBackend_ = std::move(newBackend);

copyForwards = copyForwardCount_.exchange(0, std::memory_order_relaxed);
}

if (copyForwards > 0)
{
JLOG(j_.warn()) << "Rotating: copied forward " << copyForwards
<< " archive-served reads into the writable backend "
"during the rotation window";
}

f(newWritableBackendName, newArchiveBackendName);
}

void
DatabaseRotatingImp::setRotationInFlight(bool inFlight)
{
rotationInFlight_.store(inFlight, std::memory_order_release);
JLOG(j_.debug()) << "Rotating: copy-forward on archive reads "
<< (inFlight ? "enabled" : "disabled");
}

std::string
DatabaseRotatingImp::getName() const
{
Expand Down Expand Up @@ -177,9 +196,18 @@
writable = writableBackend_;
}

// Update writable backend with data from the archive backend
if (duplicate)
// Update writable backend with data from the archive backend.
// While a rotation is in flight, ordinary (duplicate == false)
// reads served by the archive are copied forward too: the
// archive is about to be deleted, and a body canonicalized
// into the cache after the freshen getKeys() snapshot would
// otherwise survive only in RAM once the archive is dropped.
if (duplicate || rotationInFlight_.load(std::memory_order_acquire))
{
if (!duplicate)
copyForwardCount_.fetch_add(1, std::memory_order_relaxed);

Check warning on line 208 in src/libxrpl/nodestore/DatabaseRotatingImp.cpp

View check run for this annotation

Codecov / codecov/patch

src/libxrpl/nodestore/DatabaseRotatingImp.cpp#L208

Added line #L208 was not covered by tests
writable->store(nodeObject);
}
Comment thread
ximinez marked this conversation as resolved.
}
}

Expand Down
37 changes: 36 additions & 1 deletion src/xrpld/app/misc/SHAMapStoreImp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
#include <xrpl/ledger/Ledger.h>
#include <xrpl/nodestore/Database.h>
#include <xrpl/nodestore/Manager.h>
#include <xrpl/nodestore/NodeObject.h>
#include <xrpl/nodestore/Scheduler.h>
#include <xrpl/nodestore/detail/DatabaseRotatingImp.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/server/NetworkOPs.h>
#include <xrpl/server/State.h>
#include <xrpl/shamap/SHAMapMissingNode.h>
Expand Down Expand Up @@ -254,8 +256,24 @@
SHAMapStoreImp::copyNode(std::uint64_t& nodeCount, SHAMapTreeNode const& node)
{
// Copy a single record from node to dbRotating_
dbRotating_->fetchNodeObject(
auto obj = dbRotating_->fetchNodeObject(
node.getHash().asUInt256(), 0, NodeStore::FetchType::Synchronous, true);
if (!obj)
{
XRPL_ASSERT(node.cowid() == 0, "SHAMapStoreImp::copyNode : rescued node must be clean");
// Reachable from the validated state map in memory, but present in
// neither backend: its only on-disk copy lived in a backend removed by
// an earlier rotation, and it was never rewritten because it is clean
// (cowid == 0, so flushDirty skips it). Persist the in-memory body
// directly into the writable backend so it survives this rotation
// instead of later surfacing as an unresolvable SHAMapMissingNode.
auto const hash = node.getHash().asUInt256();

Check warning on line 270 in src/xrpld/app/misc/SHAMapStoreImp.cpp

View check run for this annotation

Codecov / codecov/patch

src/xrpld/app/misc/SHAMapStoreImp.cpp#L270

Added line #L270 was not covered by tests
Serializer s;
node.serializeWithPrefix(s);
dbRotating_->store(NodeObjectType::AccountNode, std::move(s.modData()), hash, 0);
Comment thread
ximinez marked this conversation as resolved.
JLOG(journal_.warn()) << "copyNode: re-stored node missing from both backends, hash="
<< hash << " type=" << static_cast<int>(node.getType());
}
if ((++nodeCount % checkHealthInterval_) == 0u)
{
if (healthWait() == HealthResult::Stopping)
Expand Down Expand Up @@ -348,6 +366,23 @@
JLOG(journal_.debug())
<< "copied ledger " << validatedSeq << " nodecount " << nodeCount;

// Close the getKeys()->swap exposure window: from here until
// rotate() completes, an ordinary read served by the archive is
// copied forward into the writable backend, so a node fetched
// from the doomed archive cannot be left RAM-only when the
// archive is deleted. RAII so the early returns below (and any
// exception) also clear the flag.
struct RotationExposureGuard
{
NodeStore::DatabaseRotating& db;
~RotationExposureGuard()
{
db.setRotationInFlight(false);
}
};
RotationExposureGuard const rotationExposureGuard{*dbRotating_};
dbRotating_->setRotationInFlight(true);

JLOG(journal_.debug()) << "freshening caches";
freshenCaches();
if (healthWait() == HealthResult::Stopping)
Expand Down
Loading