fix(layer): remove the need to repair internal state #7030

koivunej · 2024-03-06T10:45:56Z

Problem

The current implementation of struct Layer supports canceled read requests, but those will leave the internal state such that a following Layer::keep_resident call will need to repair the state. In pathological cases seen during generation numbers resetting in staging or with too many in-progress on-demand downloads, this repair activity will need to wait for the download to complete, which stalls disk usage-based eviction. Similar stalls have been observed in staging near disk-full situations, where downloads failed because the disk was full.

Fixes #6028 or the "layer is present on filesystem but not evictable" problems by:

not canceling pending evictions by a canceled LayerInner::get_or_maybe_download
completing post-download initialization of the LayerInner::inner from the download task

Not canceling evictions above case (1) and always initializing (2) lead to plain LayerInner::inner always having the up-to-date information, which leads to the old Layer::keep_resident never having to wait for downloads to complete. Finally, the Layer::keep_resident is replaced with Layer::is_likely_resident. These fix #7145.

Summary of changes

add a new test showing that a canceled get_or_maybe_download should not cancel the eviction
switch to using a watch internally rather than a broadcast to avoid hanging eviction while a download is ongoing
doc changes for new semantics and cleanup
fix Layer::keep_resident to use just self.0.inner.get() as truth as Layer::is_likely_resident
remove LayerInner::wanted_evicted boolean as no longer needed

Builds upon: #7185. Cc: #5331.

pageserver/src/tenant/storage_layer/layer/failpoints.rs

github-actions · 2024-03-06T11:56:13Z

2706 tests run: 2576 passed, 0 failed, 130 skipped (full report)

Flaky tests (3)

Postgres 15

test_vm_bit_clear_on_heap_lock: debug
test_wal_restore: release
test_wal_restore_initdb: release

Code coverage* (full report)

functions: 28.4% (7181 of 25277 functions)
lines: 47.1% (44154 of 93761 lines)

* collected from Rust tests only

_{The comment gets automatically updated with the latest test results
d265126 at 2024-03-20T23:57:57.268Z :recycle:}

koivunej · 2024-03-14T08:37:41Z

Rebased away the conflict, handled the #[cfg(test)] mess in failpoints.

Split off from #7030: - each early exit is counted as canceled init, even though it most likely was just `LayerInner::keep_resident` doing the no-download repair check - `downloaded_after` could had been accounted for multiple times, and also when repairing to match on-disk state Cc: #5331

Aiming for the design where `heavier_once_cell::OnceCell` is initialized by a future factory lead to awkwardness with how `LayerInner::get_or_maybe_download` looks right now with the `loop`. The loop helps with two situations: - an eviction has been scheduled but has not yet happened, and a read access should cancel the eviction - a previous `LayerInner::get_or_maybe_download` that canceled a pending eviction was canceled leaving the `heavier_once_cell::OnceCell` uninitialized but needing repair by the next `LayerInner::get_or_maybe_download` By instead supporting detached initialization in `heavier_once_cell::OnceCell` via an `OnceCell::get_or_detached_init`, we can fix what the monolithic #7030 does: - spawned off download task initializes the `heavier_once_cell::OnceCell` regardless of the download starter being canceled - a canceled `LayerInner::get_or_maybe_download` no longer stops eviction but can win it if not canceled Split off from #7030. Cc: #5331

pageserver/src/tenant/storage_layer/layer/tests.rs

pageserver/src/tenant/storage_layer/layer.rs

Since #6115 with more often used get_value_reconstruct_data and friends, we should not have needless INFO level span creation near hot paths. In our prod configuration, INFO spans are always created, but in practice, very rarely anything at INFO level is logged underneath. `ResidentLayer::load_keys` is only used during compaction so it is not that hot, but this aligns the access paths and their span usage. PR changes the span level to debug to align with others, and adds the layer name to the error which was missing. Split off from #7030.

at best, it was always the same as the internal state in LayerInner::inner. At worst, it added at least one possible bad state we should care: what if we get to the drop with a strong LayerInner reference but the wanted_evicted was false? luckily we don't seem to have hit this case ever.

it was added very early, but was never used.

revert it while keeping the log message -- do not mention the metrics. because there is now a re-check against status being Evicted, this is no longer expected.

introduced in #7175, and we focused on other things in the review.

#7030 introduced an annoying papercut, deeming a failure to acquire a strong reference to `LayerInner` from `DownloadedLayer::drop` as a canceled eviction. Most of the time, it wasn't that, but just timeline deletion or tenant detach with the layer not wanting to be deleted or evicted. When a Layer is dropped as part of a normal shutdown, the `Layer` is dropped first, and the `DownloadedLayer` the second. Because of this, we cannot detect eviction being canceled from the `DownloadedLayer::drop`. We can detect it from `LayerInner::drop`, which this PR adds. Test case is added which before had 1 started eviction, 2 canceled. Now it accurately finds 1 started, 1 canceled.

koivunej force-pushed the joonas/layer-cancellation-safety branch from d95f733 to 42b37c4 Compare March 6, 2024 11:06

koivunej commented Mar 6, 2024

View reviewed changes

pageserver/src/tenant/storage_layer/layer/failpoints.rs Outdated Show resolved Hide resolved

koivunej mentioned this pull request Mar 7, 2024

pageserver: panic with download while detaching #7054

Closed

koivunej force-pushed the joonas/layer-cancellation-safety branch from 42b37c4 to 77ca39d Compare March 11, 2024 09:22

koivunej mentioned this pull request Mar 11, 2024

Epic: improved eviction #5331

Open

koivunej force-pushed the joonas/layer-cancellation-safety branch from 77ca39d to 91c9e69 Compare March 14, 2024 08:36

This was referenced Mar 15, 2024

heavier_once_cell: add detached init support #7135

Merged

fix(layer): metric miscalculations #7137

Merged

This was referenced Mar 15, 2024

Layer::keep_resident will wait for layer download blocking disk usage based eviction #7145

Closed

refactor(layer): use detached init #7152

Merged

koivunej force-pushed the joonas/layer-cancellation-safety branch 3 times, most recently from 16cdd9c to f2aec12 Compare March 19, 2024 10:56

This was referenced Mar 19, 2024

fix: ResidentLayer::load_keys should not create INFO level span #7174

Merged

fix(Layer): always init after downloading in the spawned task #7175

Merged

koivunej force-pushed the joonas/layer-cancellation-safety branch from f2aec12 to c1c92b0 Compare March 19, 2024 13:45

koivunej changed the base branch from main to joonas/layer-always-init-on-download March 19, 2024 15:03

koivunej force-pushed the joonas/layer-cancellation-safety branch from c1c92b0 to 23a9aae Compare March 20, 2024 11:40

koivunej changed the title ~~draft: layer enhancements~~ fix(layer): make internal state need no repairs Mar 20, 2024

koivunej changed the title ~~fix(layer): make internal state need no repairs~~ feat(layer): make internal state need no repairs Mar 20, 2024

koivunej mentioned this pull request Mar 20, 2024

fix(heavier_once_cell): take_and_deinit should take ownership #7185

Merged

koivunej linked an issue Mar 20, 2024 that may be closed by this pull request

Layer::keep_resident will wait for layer download blocking disk usage based eviction #7145

Closed

koivunej changed the base branch from joonas/layer-always-init-on-download to joonas/heavier-once-cell-small-fix March 20, 2024 13:21

koivunej commented Mar 20, 2024

View reviewed changes

pageserver/src/tenant/storage_layer/layer/tests.rs Outdated Show resolved Hide resolved

koivunej commented Mar 20, 2024

View reviewed changes

pageserver/src/tenant/storage_layer/layer.rs Outdated Show resolved Hide resolved

koivunej added 19 commits March 20, 2024 22:45

doc: LayerInner::inner

fbc5750

doc: obv wrong LayerInner::wanted_evicted

838db72

doc: more LayerInner field comment changes

24db957

doc: remove noisy comments

c7cf9af

doc: noisier comments from code review

cd85b72

doc: remove ideas about Evicted state in inner

4f0e99d

doc: replace one FIXME with explanation

5d57414

metrics: on shutdown, increment LayerGone cancellation

fcd9f4f

it was added very early, but was never used.

test: move is_likely_resident over outdated comment

bfefdb9

doc: remove bogus get_or_maybe_download comment

5b07a44

revert "fix: evict_blocking: FileNotFound is now accepted"

a82e700

revert it while keeping the log message -- do not mention the metrics. because there is now a re-check against status being Evicted, this is no longer expected.

code review: should probably abort

9a04c25

test: bogus synchronize comments

b416f50

test: refactor needless failpoints:: prefix

00c4213

doc: note not doing all status updates while holding InitPermit

99d6e2d

doc: add missing easy comment

dc00e9d

test: case for evict_and_wait not waiting for download

1d164db

fix: another cancellation tracking failure

76fee8c

introduced in #7175, and we focused on other things in the review.

koivunej force-pushed the joonas/layer-cancellation-safety branch from befd60e to 76fee8c Compare March 20, 2024 22:46

koivunej marked this pull request as ready for review March 20, 2024 22:46

koivunej requested a review from a team as a code owner March 20, 2024 22:46

koivunej requested a review from jcsp March 20, 2024 22:46

fix: replace logging of long evictions with histogram

d265126

koivunej changed the title ~~feat(layer): make internal state need no repair~~ fix(layer): make internal state need no repair Mar 20, 2024

koivunej changed the title ~~fix(layer): make internal state need no repair~~ fix(layer): remove the need to repair internal state Mar 20, 2024

koivunej merged commit 2206e14 into main Mar 21, 2024
53 checks passed

koivunej deleted the joonas/layer-cancellation-safety branch March 21, 2024 01:19

koivunej mentioned this pull request Apr 11, 2024

fix(Layer): metric regression with too many canceled evictions #7363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(layer): remove the need to repair internal state #7030

fix(layer): remove the need to repair internal state #7030

koivunej commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading

Postgres 15

koivunej commented Mar 14, 2024

fix(layer): remove the need to repair internal state #7030

fix(layer): remove the need to repair internal state #7030

Conversation

koivunej commented Mar 6, 2024 • edited Loading

Problem

Summary of changes

github-actions bot commented Mar 6, 2024 • edited Loading

2706 tests run: 2576 passed, 0 failed, 130 skipped (full report)

Postgres 15

Code coverage* (full report)

koivunej commented Mar 14, 2024

koivunej commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading