From 6db427edc33ab828e91e51a459d72cd8bc6b5574 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Mon, 28 Aug 2023 11:47:41 +0200 Subject: [PATCH] page_cache: use try_write in drop_buffers_for_immutable 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. --- pageserver/ctl/src/layer_map_analyzer.rs | 1 + pageserver/src/page_cache.rs | 24 +++++++++++++++++-- pageserver/src/tenant/block_io.rs | 2 +- .../src/tenant/storage_layer/delta_layer.rs | 1 + .../src/tenant/storage_layer/image_layer.rs | 1 + 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/pageserver/ctl/src/layer_map_analyzer.rs b/pageserver/ctl/src/layer_map_analyzer.rs index 883762a273..5f4da7e474 100644 --- a/pageserver/ctl/src/layer_map_analyzer.rs +++ b/pageserver/ctl/src/layer_map_analyzer.rs @@ -99,6 +99,7 @@ async fn get_holes(path: &Path, max_holes: usize) -> Result> { let file = FileBlockReader::new(VirtualFile::open(path)?); let summary_blk = file.read_blk(0)?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; + drop(summary_blk); let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( actual_summary.index_start_blk, actual_summary.index_root_blk, diff --git a/pageserver/src/page_cache.rs b/pageserver/src/page_cache.rs index e1e696ddad..b7aba96189 100644 --- a/pageserver/src/page_cache.rs +++ b/pageserver/src/page_cache.rs @@ -77,7 +77,7 @@ use std::{ convert::TryInto, sync::{ atomic::{AtomicU64, AtomicU8, AtomicUsize, Ordering}, - RwLock, RwLockReadGuard, RwLockWriteGuard, TryLockError, + RwLock, RwLockReadGuard, RwLockWriteGuard, TryLockError, TryLockResult, }, }; @@ -430,7 +430,27 @@ impl PageCache { for slot_idx in 0..self.slots.len() { let slot = &self.slots[slot_idx]; - let mut inner = slot.inner.write().unwrap(); + let mut inner = match slot.inner.try_write() { + TryLockResult::Ok(guard) => guard, + TryLockResult::Err(e @ TryLockError::Poisoned(_)) => { + panic!("slot lock {slot_idx} poisoned: {e:?}") + } + TryLockResult::Err(TryLockError::WouldBlock) => { + // This function is only called from the `Drop` impl of EphemeralFile. + // So, there shouldn't be any page cache users that are reading from + // that EphemeralFile anymore, because, + // 1. EphemeralFile doesn't hand out page cache read guards and + // 2. there can't be any concurrent `EphemeralFile::reads`s because it's being dropped. + // + // So, the only reason why the page is locked right now is `find_victim` + // trying to claim this page. + // + // Avoid waiting and keep the page in the cache. + // Sooner or later it'll become a victim and get evicted. + continue; + } + }; + if let Some(key) = &inner.key { match key { CacheKey::ImmutableFilePage { file_id, blkno: _ } diff --git a/pageserver/src/tenant/block_io.rs b/pageserver/src/tenant/block_io.rs index 3a6806357b..2e0382015e 100644 --- a/pageserver/src/tenant/block_io.rs +++ b/pageserver/src/tenant/block_io.rs @@ -36,7 +36,7 @@ where /// Reference to an in-memory copy of an immutable on-disk block. pub enum BlockLease<'a> { - PageReadGuard(PageReadGuard<'static>), + PageReadGuard(PageReadGuard<'a>), EphemeralFileMutableTail(&'a [u8; PAGE_SZ]), #[cfg(test)] Rc(std::rc::Rc<[u8; PAGE_SZ]>), diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 7a482706e5..56c2c54b4f 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -848,6 +848,7 @@ impl DeltaLayerInner { let summary_blk = file.read_blk(0)?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; + drop(summary_blk); if let Some(mut expected_summary) = summary { // production code path diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index cf4f40008a..b743daafd3 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -442,6 +442,7 @@ impl ImageLayerInner { let file = FileBlockReader::new(file); let summary_blk = file.read_blk(0)?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; + drop(summary_blk); if let Some(mut expected_summary) = summary { // production code path