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.
This commit is contained in:
Christian Schwarz
2023-08-28 17:49:16 +02:00
parent 529f8b5016
commit 6db427edc3
5 changed files with 26 additions and 3 deletions
+1
View File
@@ -99,6 +99,7 @@ async fn get_holes(path: &Path, max_holes: usize) -> Result<Vec<Hole>> {
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,
+22 -2
View File
@@ -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: _ }
+1 -1
View File
@@ -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]>),
@@ -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
@@ -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