Skip to content

Revert scoped page-state locks #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adsharma merged 1 commit into from
Apr 26, 2026
Merged

Revert scoped page-state locks #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 0 additions & 76 deletions src/include/storage/buffer_manager/page_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
#pragma once

#include <atomic>
#include <optional>
#include <utility>

#include "common/assert.h"

Expand All @@ -23,68 +21,6 @@ class PageState {
static constexpr uint64_t NUM_BITS_TO_SHIFT_FOR_STATE = 56;

public:
class ScopedLock {
public:
ScopedLock() = default;
explicit ScopedLock(PageState* pageState) : pageState{pageState} {}
ScopedLock(const ScopedLock&) = delete;
ScopedLock& operator=(const ScopedLock&) = delete;
ScopedLock(ScopedLock&& other) noexcept
: pageState{std::exchange(other.pageState, nullptr)}, action{other.action} {}
ScopedLock& operator=(ScopedLock&& other) noexcept {
if (this != &other) {
release();
pageState = std::exchange(other.pageState, nullptr);
action = other.action;
}
return *this;
}
~ScopedLock() { release(); }

explicit operator bool() const { return pageState != nullptr; }

void unlock() {
action = Action::UNLOCK;
release();
}
void unlockUnchanged() {
action = Action::UNLOCK_UNCHANGED;
release();
}
void resetToEvicted() {
action = Action::RESET_TO_EVICTED;
release();
}
void releaseWithoutUnlock() { pageState = nullptr; }

private:
enum class Action : uint8_t { UNLOCK, UNLOCK_UNCHANGED, RESET_TO_EVICTED };

void release() {
if (!pageState) {
return;
}
switch (action) {
case Action::UNLOCK:
pageState->unlock();
break;
case Action::UNLOCK_UNCHANGED:
pageState->unlockUnchanged();
break;
case Action::RESET_TO_EVICTED:
pageState->resetToEvicted();
break;
default:
UNREACHABLE_CODE;
}
pageState = nullptr;
}

private:
PageState* pageState = nullptr;
Action action = Action::UNLOCK_UNCHANGED;
};

static constexpr uint64_t UNLOCKED = 0;
static constexpr uint64_t LOCKED = 1;
static constexpr uint64_t MARKED = 2;
Expand All @@ -111,22 +47,10 @@ class PageState {
}
}
}
// Prefer a scoped lock wrapper at call sites where possible. The raw tryLock/unlock APIs are
// easy to misuse on early-return paths.
bool tryLock(uint64_t oldStateAndVersion) {
return stateAndVersion.compare_exchange_strong(oldStateAndVersion,
updateStateWithSameVersion(oldStateAndVersion, LOCKED));
}
std::optional<ScopedLock> tryScopedLock(uint64_t oldStateAndVersion) {
if (!tryLock(oldStateAndVersion)) {
return std::nullopt;
}
return ScopedLock{this};
}
ScopedLock spinScopedLock(uint64_t oldStateAndVersion) {
spinLock(oldStateAndVersion);
return ScopedLock{this};
}
void unlock() {
// TODO(Keenan / Guodong): Track down this rare bug and re-enable the assert. Ref #2289.
// DASSERT(getState(stateAndVersion.load()) == LOCKED);
Expand Down
32 changes: 14 additions & 18 deletions src/storage/buffer_manager/buffer_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,17 +118,16 @@ uint8_t* BufferManager::pin(FileHandle& fileHandle, page_idx_t pageIdx,
auto currStateAndVersion = pageState->getStateAndVersion();
switch (PageState::getState(currStateAndVersion)) {
case PageState::EVICTED: {
if (auto lock = pageState->tryScopedLock(currStateAndVersion)) {
if (pageState->tryLock(currStateAndVersion)) {
if (!claimAFrame(fileHandle, pageIdx, pageReadPolicy)) {
lock->resetToEvicted();
pageState->resetToEvicted();
throw BufferManagerException("Unable to allocate memory! The buffer pool is "
"full and no memory could be freed!");
}
if (!evictionQueue.insert(fileHandle.getFileIndex(), pageIdx)) {
throw BufferManagerException(
"Eviction queue is full! This should be impossible.");
}
lock->releaseWithoutUnlock();
#if BM_MALLOC
DASSERT(pageState->getPage());
return pageState->getPage();
Expand All @@ -139,8 +138,7 @@ uint8_t* BufferManager::pin(FileHandle& fileHandle, page_idx_t pageIdx,
} break;
case PageState::UNLOCKED:
case PageState::MARKED: {
if (auto lock = pageState->tryScopedLock(currStateAndVersion)) {
lock->releaseWithoutUnlock();
if (pageState->tryLock(currStateAndVersion)) {
return getFrame(fileHandle, pageIdx);
}
} break;
Expand Down Expand Up @@ -289,9 +287,9 @@ uint64_t BufferManager::evictPages() {
} else if (evictionCandidate.isEvicted(pageStateAndVersion)) {
// Remove evicted candidate from queue. Lock page before clearing to avoid
// data races with other threads that might re-add or evict the same slot.
if (auto lock = pageState->tryScopedLock(pageStateAndVersion)) {
if (pageState->tryLock(pageStateAndVersion)) {
evictionQueue.clear(candidate);
lock->unlock();
pageState->unlock();
}
}
continue;
Expand Down Expand Up @@ -420,11 +418,7 @@ uint64_t BufferManager::tryEvictPage(std::atomic<EvictionCandidate>& _candidate)
auto currStateAndVersion = pageState.getStateAndVersion();
// We check if the page is evictable again. Note that if the page's state or version has
// changed after the check, `tryLock` will fail, and we will abort the eviction of this page.
if (!candidate.isEvictable(currStateAndVersion)) {
return 0;
}
auto lock = pageState.tryScopedLock(currStateAndVersion);
if (!lock) {
if (!candidate.isEvictable(currStateAndVersion) || !pageState.tryLock(currStateAndVersion)) {
return 0;
}
// The pageState was locked, but another thread already evicted this candidate and unlocked it
Expand All @@ -436,10 +430,12 @@ uint64_t BufferManager::tryEvictPage(std::atomic<EvictionCandidate>& _candidate)
|| pageState.getReaderCount() > 0
#endif
) {
pageState.unlockUnchanged();
return 0;
}
if (fileHandles[candidate.fileIdx]->isInMemoryMode()) {
// Cannot flush pages under in memory mode.
pageState.unlockUnchanged();
return 0;
}
// At this point, the page is LOCKED, and we have exclusive access to the eviction candidate.
Expand All @@ -449,7 +445,7 @@ uint64_t BufferManager::tryEvictPage(std::atomic<EvictionCandidate>& _candidate)
fileHandle.flushPageIfDirtyWithoutLock(candidate.pageIdx);
auto numBytesFreed = fileHandle.getPageSize();
releaseFrameForPage(fileHandle, candidate.pageIdx);
lock->resetToEvicted();
pageState.resetToEvicted();
evictionQueue.clear(_candidate);
return numBytesFreed;
}
Expand Down Expand Up @@ -501,12 +497,12 @@ void BufferManager::updateFrameIfPageIsInFrame(file_idx_t fileIdx, const uint8_t
if (PageState::getState(currentStateAndVersion) == PageState::EVICTED) {
return;
}
if (auto lock = pageState->tryScopedLock(currentStateAndVersion)) {
memcpy(getFrame(fileHandle, pageIdx), newPage, LBUG_PAGE_SIZE);
lock->unlock();
if (pageState->tryLock(currentStateAndVersion)) {
break;
}
}
memcpy(getFrame(fileHandle, pageIdx), newPage, LBUG_PAGE_SIZE);
pageState->unlock();
}

void BufferManager::removePageFromFrameIfNecessary(FileHandle& fileHandle, page_idx_t pageIdx) {
Expand All @@ -523,13 +519,13 @@ void BufferManager::removePageFromFrame(FileHandle& fileHandle, page_idx_t pageI
if (PageState::getState(pageState->getStateAndVersion()) == PageState::EVICTED) {
return;
}
auto lock = pageState->spinScopedLock(pageState->getStateAndVersion());
pageState->spinLock(pageState->getStateAndVersion());
if (shouldFlush) {
fileHandle.flushPageIfDirtyWithoutLock(pageIdx);
}
releaseFrameForPage(fileHandle, pageIdx);
freeUsedMemory(fileHandle.getPageSize());
lock.resetToEvicted();
pageState->resetToEvicted();
}

uint64_t BufferManager::freeUsedMemory(uint64_t size) {
Expand Down
Loading