Skip to content

Commit

Permalink
page cache: don't proactively evict EphemeralFile pages
Browse files Browse the repository at this point in the history
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.

So, let's just stop doing `drop_buffers_for_immutable` and observe
the performance impact in benchmarks.
  • Loading branch information
problame committed Aug 28, 2023
1 parent 529f8b5 commit cbd8567
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 24 deletions.
21 changes: 0 additions & 21 deletions pageserver/src/page_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,27 +425,6 @@ impl PageCache {
self.lock_for_read(&mut cache_key)
}

/// Immediately drop all buffers belonging to given file
pub fn drop_buffers_for_immutable(&self, drop_file_id: FileId) {
for slot_idx in 0..self.slots.len() {
let slot = &self.slots[slot_idx];

let mut inner = slot.inner.write().unwrap();
if let Some(key) = &inner.key {
match key {
CacheKey::ImmutableFilePage { file_id, blkno: _ }
if *file_id == drop_file_id =>
{
// remove mapping for old buffer
self.remove_mapping(key);
inner.key = None;
}
_ => {}
}
}
}
}

//
// Section 2: Internal interface functions for lookup/update.
//
Expand Down
5 changes: 2 additions & 3 deletions pageserver/src/tenant/ephemeral_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,8 @@ pub fn is_ephemeral_file(filename: &str) -> bool {

impl Drop for EphemeralFile {
fn drop(&mut self) {
// drop all pages from page cache
let cache = page_cache::get();
cache.drop_buffers_for_immutable(self.page_cache_file_id);
// There might still be pages in the [`crate::page_cache`] for this file.
// We leave them there, [`crate::page_cache::PageCache::find_victim`] will evict them when needed.

// unlink the file
let res = std::fs::remove_file(&self.file.path);
Expand Down

0 comments on commit cbd8567

Please sign in to comment.