Make page cache and read_blk async #5023

arpad-m · 2023-08-17T13:16:29Z

Problem

read_blk does I/O and thus we would like to make it async. We can't make the function async as long as the PageReadGuard returned by read_blk isn't Send. The page cache is called by read_blk, and thus it can't be async without read_blk being async. Thus, we have a circular dependency.

Summary of changes

Due to the circular dependency, we convert both the page cache and read_blk to async at the same time:

We make the page cache use tokio::sync synchronization primitives as those are Send. This makes all the places that acquire a lock require async though, which we then also do. This includes also asyncification of the read_blk function.

Builds upon #4994, #5015, #5056, and #5129.

Part of #4743.

github-actions · 2023-08-17T13:22:03Z

1624 tests run: 1549 passed, 0 failed, 75 skipped (full report)

_{The comment gets automatically updated with the latest test results
80b99f8 at 2023-08-29T08:54:06.506Z :recycle:}

pageserver/src/page_cache.rs

pageserver/src/tenant/ephemeral_file.rs

koivunej

I don't think we can do Handle::block_on in drop.

pageserver/src/tenant/ephemeral_file.rs

pageserver/src/page_cache.rs

## Problem The `EphemeralFile::write_blob` function accesses the page cache internally. We want to require `async` for these accesses in #5023. ## Summary of changes This removes the implementaiton of the `BlobWriter` trait for `EphemeralFile` and turns the `write_blob` function into an inherent function. We can then make it async as well as the `push_bytes` function. We move the `SER_BUFFER` thread-local into the `InMemoryLayerInner` so that the same buffer can be accessed by different threads as the async is (potentially) moved between threads. Part of #4743, preparation for #5023.

In #5023, we want to make the slot locks async. The `drop_buffers_for_immutable` is used by `EphemeralFile::drop` and locks the slot locks. Drop impls can't be async, so, we must find a workaround. Some background on drop_buffers_for_immutable: it's really just a nice courtesy to proactively give back the slots. After dropping the EphemeralFile, we're not going to use them in the future and so, `find_victim` will repurpose them eventually. The only part that is important is `remove_mapping`, because if we don't do that, we'll never get back to removing the materialized_page_map / immutable_page_map, thereby effectively leaking memory. So, how to work around the lack of async destructors: the simple way would be to push the work into a background queue. This has an obvious perf penalty. So, this PR takes another approach, based on the insight that the BlockLease/PageReadGuards handed out by EphemeralFile barely outlive the EphemeralFile. This PR changes the lifetime of the PageReadGuard returned by EphemeralFile to _ensure_ this in the type system. With that, we can switch to try_write(), and be quite certain that there are no outstanding PageReadGuards for the EphemeralFile that is being dropped. So, we know we're doing the slot clean-up work. Note: this PR does not debate whether the slot cleanup work is really necessary. In my opinion, we can just not do it and let the pages get find_victim'ed a little later than now. But, let's have that discussion another day.

Before this patch, when dropping an EphemeralFile, we'd scan the entire `slots` to proactively evict its pages (`drop_buffers_for_immutable`). This was _necessary_ before #4994 because the page cache was a write-back cache: we'd be deleting the EphemeralFile from disk after, so, if we hadn't evicted its pages before that, write-back in `find_victim` wouldhave failed. But, since #4994, the page cache is a read-only cache, so, it's safe to keep read-only data cached. It's never going to get accessed again and eventually, `find_victim` will evict it. The only remaining advantage of `drop_buffers_for_immutable` over relying on `find_victim` is that `find_victim` has to do the clock page replacement iterations until the count reaches 0, whereas `drop_buffers_for_immutable` can kick the page out right away. However, weigh that against the cost of `drop_buffers_for_immutable`, which currently scans the entire `slots` array to find the EphemeralFile's pages. Alternatives have been proposed in #5122 and #5128, but, they come with their own overheads & trade-offs. Also, the real reason why we're looking into this piece of code is that we want to make the slots rwlock async in #5023. Since `drop_buffers_for_immutable` is called from drop, and there is no async drop, it would be nice to not have to deal with this. So, let's just stop doing `drop_buffers_for_immutable` and observe the performance impact in benchmarks.

The returned PageReadGuard is not Send so we change the locks used for the SlotInner's in the page cache to the ones from tokio. Also, make read_blk async.

jcsp

This makes sense as our next step in async-izing things.

problame

Pushed some fixups as arpad is OOO.

## Problem `block_in_place` is a quite expensive operation, and if it is used, we should explicitly have to opt into it by allowing the `clippy::disallowed_methods` lint. For more, see #5023 (comment). Similar arguments exist for `Handle::block_on`, but we don't do this yet as there is still usages. ## Summary of changes Adds a clippy.toml file, configuring the [`disallowed_methods` lint](https://rust-lang.github.io/rust-clippy/master/#/disallowed_method).

arpad-m mentioned this pull request Aug 17, 2023

Epic: convert remaining IO stack for Timeline::get to async fn #4743

Closed

arpad-m changed the title ~~Make page cace and read_blk async~~ Make page cache and read_blk async Aug 17, 2023

arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from 983fe79 to 523a54e Compare August 17, 2023 15:37

arpad-m mentioned this pull request Aug 17, 2023

simplify page-caching of EphemeralFile #4994

Merged

arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch 2 times, most recently from 7c106f7 to 8d3b0d3 Compare August 18, 2023 19:18

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from f1062a2 to fcbd58a Compare August 18, 2023 19:38

koivunej reviewed Aug 18, 2023

View reviewed changes

pageserver/src/page_cache.rs Outdated Show resolved Hide resolved

koivunej reviewed Aug 18, 2023

View reviewed changes

pageserver/src/tenant/ephemeral_file.rs Outdated Show resolved Hide resolved

koivunej requested changes Aug 18, 2023

View reviewed changes

koivunej reviewed Aug 18, 2023

View reviewed changes

pageserver/src/tenant/ephemeral_file.rs Outdated Show resolved Hide resolved

arpad-m mentioned this pull request Aug 21, 2023

Make EphemeralFile::write_blob function async #5056

Merged

5 tasks

arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from 8d3b0d3 to d63f31d Compare August 23, 2023 15:47

arpad-m force-pushed the arpad/pageserver_async_read_blk branch 2 times, most recently from 4cdb800 to 4620505 Compare August 23, 2023 18:11

jcsp reviewed Aug 24, 2023

View reviewed changes

pageserver/src/page_cache.rs Outdated Show resolved Hide resolved

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from 4620505 to 09146c0 Compare August 24, 2023 21:00

arpad-m force-pushed the arpad/pageserver_cursor_read_blk branch from d63f31d to 3719b16 Compare August 24, 2023 21:00

arpad-m mentioned this pull request Aug 25, 2023

Disallow block_in_place and Handle::block_on #5101

Merged

Base automatically changed from arpad/pageserver_cursor_read_blk to main August 25, 2023 10:28

arpad-m mentioned this pull request Aug 25, 2023

Remove mappings in page cache lazily #5111

Closed

5 tasks

arpad-m changed the base branch from main to arpad/page_cache_refactor August 25, 2023 14:30

arpad-m force-pushed the arpad/pageserver_async_read_blk branch 2 times, most recently from 823a414 to fb37dfb Compare August 28, 2023 08:53

arpad-m mentioned this pull request Aug 28, 2023

Add async functions for VirtualFile reading #5121

Closed

5 tasks

problame mentioned this pull request Aug 28, 2023

page_cache: use try_write in drop_buffers_for_immutable #5122

Closed

arpad-m mentioned this pull request Aug 28, 2023

page_cache: eliminate PageWriteGuard #5119

Closed

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from fb37dfb to 02381a2 Compare August 28, 2023 13:42

arpad-m changed the base branch from arpad/page_cache_refactor to problame/page-cache-drop-buffers-immutable August 28, 2023 14:02

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from 02381a2 to 2f4cf35 Compare August 28, 2023 14:32

problame force-pushed the problame/page-cache-drop-buffers-immutable branch from 1f3d275 to 7e19960 Compare August 28, 2023 15:49

problame mentioned this pull request Aug 28, 2023

page cache: don't proactively evict EphemeralFile pages #5129

Merged

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from 2f4cf35 to 213e62d Compare August 28, 2023 20:34

arpad-m changed the base branch from problame/page-cache-drop-buffers-immutable to main August 28, 2023 20:35

arpad-m marked this pull request as ready for review August 28, 2023 20:35

arpad-m requested review from a team as code owners August 28, 2023 20:35

arpad-m requested review from lubennikovaav and problame and removed request for a team August 28, 2023 20:35

Make read_blk and parts of the page cache async

0cfc9ed

The returned PageReadGuard is not Send so we change the locks used for the SlotInner's in the page cache to the ones from tokio. Also, make read_blk async.

arpad-m force-pushed the arpad/pageserver_async_read_blk branch from 213e62d to 0cfc9ed Compare August 28, 2023 20:43

jcsp approved these changes Aug 29, 2023

View reviewed changes

problame added 3 commits August 29, 2023 10:25

switch to qualified use for rwlock types

1e71579

find_victim and search_mapping need not be async

81fde13

remove comment line likely resurrected by rebase

80b99f8

problame approved these changes Aug 29, 2023

View reviewed changes

problame removed the request for review from lubennikovaav August 30, 2023 06:17

koivunej approved these changes Aug 30, 2023

View reviewed changes

problame merged commit eb0a698 into main Aug 30, 2023

problame deleted the arpad/pageserver_async_read_blk branch August 30, 2023 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make page cache and read_blk async #5023

Make page cache and read_blk async #5023

arpad-m commented Aug 17, 2023 •

edited by problame

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

koivunej left a comment •

edited

Loading

jcsp left a comment

problame left a comment

Make page cache and read_blk async #5023

Make page cache and read_blk async #5023

Conversation

arpad-m commented Aug 17, 2023 • edited by problame Loading

Problem

Summary of changes

github-actions bot commented Aug 17, 2023 • edited Loading

1624 tests run: 1549 passed, 0 failed, 75 skipped (full report)

koivunej left a comment • edited Loading

Choose a reason for hiding this comment

jcsp left a comment

Choose a reason for hiding this comment

problame left a comment

Choose a reason for hiding this comment

arpad-m commented Aug 17, 2023 •

edited by problame

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

koivunej left a comment •

edited

Loading