From baf395983f0b27d318309a83e07e7f75f582ab2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Mon, 14 Aug 2023 18:48:09 +0200 Subject: [PATCH] Turn BlockLease associated type into an enum (#4982) ## Problem The `BlockReader` trait is not ready to be asyncified, as associated types are not supported by asyncification strategies like via the `async_trait` macro, or via adopting enums. ## Summary of changes Remove the `BlockLease` associated type from the `BlockReader` trait and turn it into an enum instead, bearing the same name. The enum has two variants, one of which is gated by `#[cfg(test)]`. Therefore, outside of test settings, the enum has zero overhead over just having the `PageReadGuard`. Using the enum allows us to impl `BlockReader` without needing the page cache. Part of https://github.com/neondatabase/neon/issues/4743 --- pageserver/src/tenant/block_io.rs | 56 ++++++++++++++----- pageserver/src/tenant/disk_btree.rs | 7 +-- pageserver/src/tenant/ephemeral_file.rs | 8 +-- .../src/tenant/storage_layer/delta_layer.rs | 8 +-- 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/pageserver/src/tenant/block_io.rs b/pageserver/src/tenant/block_io.rs index e6ebebe594d6..f25df324f4dc 100644 --- a/pageserver/src/tenant/block_io.rs +++ b/pageserver/src/tenant/block_io.rs @@ -2,8 +2,7 @@ //! Low-level Block-oriented I/O functions //! -use crate::page_cache; -use crate::page_cache::{ReadBufResult, PAGE_SZ}; +use crate::page_cache::{self, PageReadGuard, ReadBufResult, PAGE_SZ}; use bytes::Bytes; use std::ops::{Deref, DerefMut}; use std::os::unix::fs::FileExt; @@ -15,14 +14,12 @@ use std::sync::atomic::AtomicU64; /// There are currently two implementations: EphemeralFile, and FileBlockReader /// below. pub trait BlockReader { - type BlockLease: Deref + 'static; - /// /// Read a block. Returns a "lease" object that can be used to /// access to the contents of the page. (For the page cache, the /// lease object represents a lock on the buffer.) /// - fn read_blk(&self, blknum: u32) -> Result; + fn read_blk(&self, blknum: u32) -> Result; /// /// Create a new "cursor" for reading from this reader. @@ -41,13 +38,48 @@ impl BlockReader for &B where B: BlockReader, { - type BlockLease = B::BlockLease; - - fn read_blk(&self, blknum: u32) -> Result { + fn read_blk(&self, blknum: u32) -> Result { (*self).read_blk(blknum) } } +/// A block accessible for reading +/// +/// During builds with `#[cfg(test)]`, this is a proper enum +/// with two variants to support testing code. During normal +/// builds, it just has one variant and is thus a cheap newtype +/// wrapper of [`PageReadGuard`] +pub enum BlockLease { + PageReadGuard(PageReadGuard<'static>), + #[cfg(test)] + Rc(std::rc::Rc<[u8; PAGE_SZ]>), +} + +impl From> for BlockLease { + fn from(value: PageReadGuard<'static>) -> Self { + BlockLease::PageReadGuard(value) + } +} + +#[cfg(test)] +impl From> for BlockLease { + fn from(value: std::rc::Rc<[u8; PAGE_SZ]>) -> Self { + BlockLease::Rc(value) + } +} + +impl Deref for BlockLease { + type Target = [u8; PAGE_SZ]; + + fn deref(&self) -> &Self::Target { + match self { + BlockLease::PageReadGuard(v) => v.deref(), + #[cfg(test)] + BlockLease::Rc(v) => v.deref(), + } + } +} + /// /// A "cursor" for efficiently reading multiple pages from a BlockReader /// @@ -80,7 +112,7 @@ where BlockCursor { reader } } - pub fn read_blk(&self, blknum: u32) -> Result { + pub fn read_blk(&self, blknum: u32) -> Result { self.reader.read_blk(blknum) } } @@ -118,9 +150,7 @@ impl BlockReader for FileBlockReader where F: FileExt, { - type BlockLease = page_cache::PageReadGuard<'static>; - - fn read_blk(&self, blknum: u32) -> Result { + fn read_blk(&self, blknum: u32) -> Result { // Look up the right page let cache = page_cache::get(); loop { @@ -132,7 +162,7 @@ where format!("Failed to read immutable buf: {e:#}"), ) })? { - ReadBufResult::Found(guard) => break Ok(guard), + ReadBufResult::Found(guard) => break Ok(guard.into()), ReadBufResult::NotFound(mut write_guard) => { // Read the page from disk into the buffer self.fill_buffer(write_guard.deref_mut(), blknum)?; diff --git a/pageserver/src/tenant/disk_btree.rs b/pageserver/src/tenant/disk_btree.rs index d0ebd3eb4e69..2064a62c5607 100644 --- a/pageserver/src/tenant/disk_btree.rs +++ b/pageserver/src/tenant/disk_btree.rs @@ -685,6 +685,7 @@ impl BuildNode { #[cfg(test)] mod tests { use super::*; + use crate::tenant::block_io::BlockLease; use rand::Rng; use std::collections::BTreeMap; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -699,12 +700,10 @@ mod tests { } } impl BlockReader for TestDisk { - type BlockLease = std::rc::Rc<[u8; PAGE_SZ]>; - - fn read_blk(&self, blknum: u32) -> io::Result { + fn read_blk(&self, blknum: u32) -> io::Result { let mut buf = [0u8; PAGE_SZ]; buf.copy_from_slice(&self.blocks[blknum as usize]); - Ok(std::rc::Rc::new(buf)) + Ok(std::rc::Rc::new(buf).into()) } } impl BlockWriter for &mut TestDisk { diff --git a/pageserver/src/tenant/ephemeral_file.rs b/pageserver/src/tenant/ephemeral_file.rs index 38a491f931c6..51841b8715a0 100644 --- a/pageserver/src/tenant/ephemeral_file.rs +++ b/pageserver/src/tenant/ephemeral_file.rs @@ -4,7 +4,7 @@ use crate::config::PageServerConf; use crate::page_cache::{self, ReadBufResult, WriteBufResult, PAGE_SZ}; use crate::tenant::blob_io::BlobWriter; -use crate::tenant::block_io::BlockReader; +use crate::tenant::block_io::{BlockLease, BlockReader}; use crate::virtual_file::VirtualFile; use once_cell::sync::Lazy; use std::cmp::min; @@ -303,9 +303,7 @@ pub fn writeback(file_id: u64, blkno: u32, buf: &[u8]) -> Result<(), io::Error> } impl BlockReader for EphemeralFile { - type BlockLease = page_cache::PageReadGuard<'static>; - - fn read_blk(&self, blknum: u32) -> Result { + fn read_blk(&self, blknum: u32) -> Result { // Look up the right page let cache = page_cache::get(); loop { @@ -313,7 +311,7 @@ impl BlockReader for EphemeralFile { .read_ephemeral_buf(self.file_id, blknum) .map_err(|e| to_io_error(e, "Failed to read ephemeral buf"))? { - ReadBufResult::Found(guard) => return Ok(guard), + ReadBufResult::Found(guard) => return Ok(guard.into()), ReadBufResult::NotFound(mut write_guard) => { // Read the page from disk into the buffer self.fill_buffer(write_guard.deref_mut(), blknum)?; diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index a0562ddc1089..bd77d177b054 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -29,10 +29,10 @@ //! use crate::config::PageServerConf; use crate::context::RequestContext; -use crate::page_cache::{PageReadGuard, PAGE_SZ}; +use crate::page_cache::PAGE_SZ; use crate::repository::{Key, Value, KEY_SIZE}; use crate::tenant::blob_io::{BlobWriter, WriteBlobWriter}; -use crate::tenant::block_io::{BlockBuf, BlockCursor, BlockReader, FileBlockReader}; +use crate::tenant::block_io::{BlockBuf, BlockCursor, BlockLease, BlockReader, FileBlockReader}; use crate::tenant::disk_btree::{DiskBtreeBuilder, DiskBtreeReader, VisitDirection}; use crate::tenant::storage_layer::{ PersistentLayer, ValueReconstructResult, ValueReconstructState, @@ -1041,9 +1041,7 @@ impl> ValueRef { struct Adapter>(T); impl> BlockReader for Adapter { - type BlockLease = PageReadGuard<'static>; - - fn read_blk(&self, blknum: u32) -> Result { + fn read_blk(&self, blknum: u32) -> Result { self.0.as_ref().file.read_blk(blknum) } }