From fd98b308e99a300dc67327879a71ff2ebd06efb4 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 10 May 2023 20:25:47 +0200 Subject: [PATCH 1/8] asyncify get_value_reconstruct_data (impls still use sync IO) --- pageserver/src/tenant/storage_layer.rs | 24 ++- .../src/tenant/storage_layer/delta_layer.rs | 150 +++++++++--------- .../src/tenant/storage_layer/image_layer.rs | 70 ++++---- .../tenant/storage_layer/inmemory_layer.rs | 82 +++++----- .../src/tenant/storage_layer/remote_layer.rs | 24 +-- pageserver/src/tenant/timeline.rs | 73 +++++---- 6 files changed, 231 insertions(+), 192 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 0af3d4ce39d3..8e9630a144d0 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -23,6 +23,7 @@ use pageserver_api::models::{ }; use std::ops::Range; use std::path::PathBuf; +use std::pin::Pin; use std::sync::{Arc, Mutex}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tracing::warn; @@ -337,6 +338,13 @@ impl LayerAccessStats { } } +pub(crate) type GetValueReconstructFuture = Pin< + Box< + dyn Send + + std::future::Future>, + >, +>; + /// Supertrait of the [`Layer`] trait that captures the bare minimum interface /// required by [`LayerMap`]. /// @@ -374,12 +382,12 @@ pub trait Layer: std::fmt::Debug + Send + Sync { /// the predecessor layer and call again with the same 'reconstruct_data' to /// collect more data. fn get_value_reconstruct_data( - &self, + self: Arc, key: Key, lsn_range: Range, - reconstruct_data: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> Result; + reconstruct_data: ValueReconstructState, + ctx: RequestContext, + ) -> GetValueReconstructFuture; /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. fn short_id(&self) -> String; @@ -513,12 +521,12 @@ impl Layer for LayerDescriptor { } fn get_value_reconstruct_data( - &self, + self: Arc, _key: Key, _lsn_range: Range, - _reconstruct_data: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> Result { + _reconstruct_data: ValueReconstructState, + _ctx: RequestContext, + ) -> GetValueReconstructFuture { todo!("This method shouldn't be part of the Layer trait") } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 6e146631216a..5a5d77108b56 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -47,6 +47,7 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; +use std::sync::Arc; use tracing::*; use utils::{ @@ -56,8 +57,8 @@ use utils::{ }; use super::{ - DeltaFileName, Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - PathOrConf, PersistentLayerDesc, + DeltaFileName, GetValueReconstructFuture, Layer, LayerAccessStats, LayerAccessStatsReset, + LayerIter, LayerKeyIter, PathOrConf, PersistentLayerDesc, }; /// @@ -295,89 +296,90 @@ impl Layer for DeltaLayer { } fn get_value_reconstruct_data( - &self, + self: Arc, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> anyhow::Result { - ensure!(lsn_range.start >= self.desc.lsn_range.start); - let mut need_image = true; - - ensure!(self.desc.key_range.contains(&key)); - - { - // Open the file and lock the metadata in memory - let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; - - // Scan the page versions backwards, starting from `lsn`. - let file = &inner.file; - let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( - inner.index_start_blk, - inner.index_root_blk, - file, - ); - let search_key = DeltaKey::from_key_lsn(&key, Lsn(lsn_range.end.0 - 1)); - - let mut offsets: Vec<(Lsn, u64)> = Vec::new(); - - tree_reader.visit(&search_key.0, VisitDirection::Backwards, |key, value| { - let blob_ref = BlobRef(value); - if key[..KEY_SIZE] != search_key.0[..KEY_SIZE] { - return false; - } - let entry_lsn = DeltaKey::extract_lsn_from_buf(key); - if entry_lsn < lsn_range.start { - return false; - } - offsets.push((entry_lsn, blob_ref.pos())); - - !blob_ref.will_init() - })?; + mut reconstruct_state: ValueReconstructState, + ctx: RequestContext, + ) -> GetValueReconstructFuture { + Box::pin(async move { + ensure!(lsn_range.start >= self.desc.lsn_range.start); + let mut need_image = true; + + ensure!(self.desc.key_range.contains(&key)); + { + // Open the file and lock the metadata in memory + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + + // Scan the page versions backwards, starting from `lsn`. + let file = &inner.file; + let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( + inner.index_start_blk, + inner.index_root_blk, + file, + ); + let search_key = DeltaKey::from_key_lsn(&key, Lsn(lsn_range.end.0 - 1)); + + let mut offsets: Vec<(Lsn, u64)> = Vec::new(); + + tree_reader.visit(&search_key.0, VisitDirection::Backwards, |key, value| { + let blob_ref = BlobRef(value); + if key[..KEY_SIZE] != search_key.0[..KEY_SIZE] { + return false; + } + let entry_lsn = DeltaKey::extract_lsn_from_buf(key); + if entry_lsn < lsn_range.start { + return false; + } + offsets.push((entry_lsn, blob_ref.pos())); - // Ok, 'offsets' now contains the offsets of all the entries we need to read - let mut cursor = file.block_cursor(); - let mut buf = Vec::new(); - for (entry_lsn, pos) in offsets { - cursor.read_blob_into_buf(pos, &mut buf).with_context(|| { - format!( - "Failed to read blob from virtual file {}", - file.file.path.display() - ) - })?; - let val = Value::des(&buf).with_context(|| { - format!( - "Failed to deserialize file blob from virtual file {}", - file.file.path.display() - ) + !blob_ref.will_init() })?; - match val { - Value::Image(img) => { - reconstruct_state.img = Some((entry_lsn, img)); - need_image = false; - break; - } - Value::WalRecord(rec) => { - let will_init = rec.will_init(); - reconstruct_state.records.push((entry_lsn, rec)); - if will_init { - // This WAL record initializes the page, so no need to go further back + + // Ok, 'offsets' now contains the offsets of all the entries we need to read + let mut cursor = file.block_cursor(); + let mut buf = Vec::new(); + for (entry_lsn, pos) in offsets { + cursor.read_blob_into_buf(pos, &mut buf).with_context(|| { + format!( + "Failed to read blob from virtual file {}", + file.file.path.display() + ) + })?; + let val = Value::des(&buf).with_context(|| { + format!( + "Failed to deserialize file blob from virtual file {}", + file.file.path.display() + ) + })?; + match val { + Value::Image(img) => { + reconstruct_state.img = Some((entry_lsn, img)); need_image = false; break; } + Value::WalRecord(rec) => { + let will_init = rec.will_init(); + reconstruct_state.records.push((entry_lsn, rec)); + if will_init { + // This WAL record initializes the page, so no need to go further back + need_image = false; + break; + } + } } } + // release metadata lock and close the file } - // release metadata lock and close the file - } - // If an older page image is needed to reconstruct the page, let the - // caller know. - if need_image { - Ok(ValueReconstructResult::Continue) - } else { - Ok(ValueReconstructResult::Complete) - } + // If an older page image is needed to reconstruct the page, let the + // caller know. + if need_image { + Ok((reconstruct_state, ValueReconstructResult::Continue)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } + }) } /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 07a16a7de2ff..a89ebec5ecec 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -43,7 +43,7 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::prelude::FileExt; use std::path::{Path, PathBuf}; -use std::sync::{RwLock, RwLockReadGuard}; +use std::sync::{Arc, RwLock, RwLockReadGuard}; use tracing::*; use utils::{ @@ -53,7 +53,10 @@ use utils::{ }; use super::filename::ImageFileName; -use super::{Layer, LayerAccessStatsReset, LayerIter, PathOrConf, PersistentLayerDesc}; +use super::{ + GetValueReconstructFuture, Layer, LayerAccessStatsReset, LayerIter, PathOrConf, + PersistentLayerDesc, +}; /// /// Header stored in the beginning of the file @@ -182,38 +185,41 @@ impl Layer for ImageLayer { /// Look up given page in the file fn get_value_reconstruct_data( - &self, + self: Arc, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> anyhow::Result { - assert!(self.desc.key_range.contains(&key)); - assert!(lsn_range.start >= self.lsn); - assert!(lsn_range.end >= self.lsn); - - let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; - - let file = inner.file.as_ref().unwrap(); - let tree_reader = DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); - - let mut keybuf: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; - key.write_to_byte_slice(&mut keybuf); - if let Some(offset) = tree_reader.get(&keybuf)? { - let blob = file.block_cursor().read_blob(offset).with_context(|| { - format!( - "failed to read value from data file {} at offset {}", - self.path().display(), - offset - ) - })?; - let value = Bytes::from(blob); - - reconstruct_state.img = Some((self.lsn, value)); - Ok(ValueReconstructResult::Complete) - } else { - Ok(ValueReconstructResult::Missing) - } + mut reconstruct_state: ValueReconstructState, + ctx: RequestContext, + ) -> GetValueReconstructFuture { + Box::pin(async move { + assert!(self.desc.key_range.contains(&key)); + assert!(lsn_range.start >= self.lsn); + assert!(lsn_range.end >= self.lsn); + + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + + let file = inner.file.as_ref().unwrap(); + let tree_reader = + DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); + + let mut keybuf: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; + key.write_to_byte_slice(&mut keybuf); + if let Some(offset) = tree_reader.get(&keybuf)? { + let blob = file.block_cursor().read_blob(offset).with_context(|| { + format!( + "failed to read value from data file {} at offset {}", + self.path().display(), + offset + ) + })?; + let value = Bytes::from(blob); + + reconstruct_state.img = Some((self.lsn, value)); + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Missing)) + } + }) } /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 78bcfdafc0d2..e33fc030899d 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -27,9 +27,9 @@ use utils::{ // while being able to use std::fmt::Write's methods use std::fmt::Write as _; use std::ops::Range; -use std::sync::RwLock; +use std::sync::{Arc, RwLock}; -use super::{DeltaLayer, DeltaLayerWriter, Layer}; +use super::{DeltaLayer, DeltaLayerWriter, GetValueReconstructFuture, Layer}; thread_local! { /// A buffer for serializing object during [`InMemoryLayer::put_value`]. @@ -191,52 +191,54 @@ impl Layer for InMemoryLayer { /// Look up given value in the layer. fn get_value_reconstruct_data( - &self, + self: Arc, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> anyhow::Result { - ensure!(lsn_range.start >= self.start_lsn); - let mut need_image = true; - - let inner = self.inner.read().unwrap(); - - let mut reader = inner.file.block_cursor(); - - // Scan the page versions backwards, starting from `lsn`. - if let Some(vec_map) = inner.index.get(&key) { - let slice = vec_map.slice_range(lsn_range); - for (entry_lsn, pos) in slice.iter().rev() { - let buf = reader.read_blob(*pos)?; - let value = Value::des(&buf)?; - match value { - Value::Image(img) => { - reconstruct_state.img = Some((*entry_lsn, img)); - return Ok(ValueReconstructResult::Complete); - } - Value::WalRecord(rec) => { - let will_init = rec.will_init(); - reconstruct_state.records.push((*entry_lsn, rec)); - if will_init { - // This WAL record initializes the page, so no need to go further back - need_image = false; - break; + mut reconstruct_state: ValueReconstructState, + _ctx: RequestContext, + ) -> GetValueReconstructFuture { + Box::pin(async move { + ensure!(lsn_range.start >= self.start_lsn); + let mut need_image = true; + + let inner = self.inner.read().unwrap(); + + let mut reader = inner.file.block_cursor(); + + // Scan the page versions backwards, starting from `lsn`. + if let Some(vec_map) = inner.index.get(&key) { + let slice = vec_map.slice_range(lsn_range); + for (entry_lsn, pos) in slice.iter().rev() { + let buf = reader.read_blob(*pos)?; + let value = Value::des(&buf)?; + match value { + Value::Image(img) => { + reconstruct_state.img = Some((*entry_lsn, img)); + return Ok((reconstruct_state, ValueReconstructResult::Complete)); + } + Value::WalRecord(rec) => { + let will_init = rec.will_init(); + reconstruct_state.records.push((*entry_lsn, rec)); + if will_init { + // This WAL record initializes the page, so no need to go further back + need_image = false; + break; + } } } } } - } - // release lock on 'inner' + // release lock on 'inner' - // If an older page image is needed to reconstruct the page, let the - // caller know. - if need_image { - Ok(ValueReconstructResult::Continue) - } else { - Ok(ValueReconstructResult::Complete) - } + // If an older page image is needed to reconstruct the page, let the + // caller know. + if need_image { + Ok((reconstruct_state, ValueReconstructResult::Continue)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } + }) } } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 387bae5b1f9c..6126bf3cf5b3 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -6,7 +6,7 @@ use crate::context::RequestContext; use crate::repository::Key; use crate::tenant::layer_map::BatchedUpdates; use crate::tenant::remote_timeline_client::index::LayerFileMetadata; -use crate::tenant::storage_layer::{Layer, ValueReconstructResult, ValueReconstructState}; +use crate::tenant::storage_layer::{Layer, ValueReconstructState}; use anyhow::{bail, Result}; use pageserver_api::models::HistoricLayerInfo; use std::ops::Range; @@ -20,8 +20,8 @@ use utils::{ use super::filename::{DeltaFileName, ImageFileName}; use super::{ - DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, + DeltaLayer, GetValueReconstructFuture, ImageLayer, LayerAccessStats, LayerAccessStatsReset, + LayerIter, LayerKeyIter, LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, }; /// RemoteLayer is a not yet downloaded [`ImageLayer`] or @@ -65,16 +65,18 @@ impl std::fmt::Debug for RemoteLayer { impl Layer for RemoteLayer { fn get_value_reconstruct_data( - &self, + self: Arc, _key: Key, _lsn_range: Range, - _reconstruct_state: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> Result { - bail!( - "layer {} needs to be downloaded", - self.filename().file_name() - ); + _reconstruct_state: ValueReconstructState, + _ctx: RequestContext, + ) -> GetValueReconstructFuture { + Box::pin(async move { + bail!( + "layer {} needs to be downloaded", + self.filename().file_name() + ); + }) } /// debugging function to print out the contents of the layer diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index d64209099602..aca925c88630 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -553,13 +553,14 @@ impl Timeline { None => None, }; - let mut reconstruct_state = ValueReconstructState { + let reconstruct_state = ValueReconstructState { records: Vec::new(), img: cached_page_img, }; let timer = self.metrics.get_reconstruct_data_time_histo.start_timer(); - self.get_reconstruct_data(key, lsn, &mut reconstruct_state, ctx) + let reconstruct_state = self + .get_reconstruct_data(key, lsn, reconstruct_state, ctx) .await?; timer.stop_and_record(); @@ -2351,9 +2352,9 @@ impl Timeline { &self, key: Key, request_lsn: Lsn, - reconstruct_state: &mut ValueReconstructState, + mut reconstruct_state: ValueReconstructState, ctx: &RequestContext, - ) -> Result<(), PageReconstructError> { + ) -> Result { // Start from the current timeline. let mut timeline_owned; let mut timeline = self; @@ -2383,12 +2384,12 @@ impl Timeline { // The function should have updated 'state' //info!("CALLED for {} at {}: {:?} with {} records, cached {}", key, cont_lsn, result, reconstruct_state.records.len(), cached_lsn); match result { - ValueReconstructResult::Complete => return Ok(()), + ValueReconstructResult::Complete => return Ok(reconstruct_state), ValueReconstructResult::Continue => { // If we reached an earlier cached page image, we're done. if cont_lsn == cached_lsn + 1 { self.metrics.materialized_page_cache_hit_counter.inc_by(1); - return Ok(()); + return Ok(reconstruct_state); } if prev_lsn <= cont_lsn { // Didn't make any progress in last iteration. Error out to avoid @@ -2492,13 +2493,19 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match open_layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { - Ok(result) => result, + result = match Arc::clone(open_layer) + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx.attached_child(), + ) + .await + { + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2519,13 +2526,19 @@ impl Timeline { if cont_lsn > start_lsn { //info!("CHECKING for {} at {} on frozen layer {}", key, cont_lsn, frozen_layer.filename().display()); let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match frozen_layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { - Ok(result) => result, + result = match Arc::clone(frozen_layer) + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx.attached_child(), + ) + .await + { + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2554,13 +2567,19 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, lsn_floor); - result = match layer.get_value_reconstruct_data( - key, - lsn_floor..cont_lsn, - reconstruct_state, - ctx, - ) { - Ok(result) => result, + result = match Arc::clone(&layer) + .get_value_reconstruct_data( + key, + lsn_floor..cont_lsn, + reconstruct_state, + ctx.attached_child(), + ) + .await + { + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; From d67a5b2f82c4de7007dcec0cf5b3771653394327 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 13 Jun 2023 14:10:40 +0200 Subject: [PATCH 2/8] switch to async_trait --- pageserver/src/tenant/storage_layer.rs | 18 +-- .../src/tenant/storage_layer/delta_layer.rs | 145 +++++++++--------- .../src/tenant/storage_layer/image_layer.rs | 65 ++++---- .../tenant/storage_layer/inmemory_layer.rs | 77 +++++----- .../src/tenant/storage_layer/remote_layer.rs | 19 ++- 5 files changed, 155 insertions(+), 169 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 8e9630a144d0..a6334b3f239a 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -23,7 +23,6 @@ use pageserver_api::models::{ }; use std::ops::Range; use std::path::PathBuf; -use std::pin::Pin; use std::sync::{Arc, Mutex}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tracing::warn; @@ -338,19 +337,13 @@ impl LayerAccessStats { } } -pub(crate) type GetValueReconstructFuture = Pin< - Box< - dyn Send - + std::future::Future>, - >, ->; - /// Supertrait of the [`Layer`] trait that captures the bare minimum interface /// required by [`LayerMap`]. /// /// All layers should implement a minimal `std::fmt::Debug` without tenant or /// timeline names, because those are known in the context of which the layers /// are used in (timeline). +#[async_trait::async_trait] pub trait Layer: std::fmt::Debug + Send + Sync { /// Range of keys that this layer covers fn get_key_range(&self) -> Range; @@ -381,13 +374,13 @@ pub trait Layer: std::fmt::Debug + Send + Sync { /// is available. If this returns ValueReconstructResult::Continue, look up /// the predecessor layer and call again with the same 'reconstruct_data' to /// collect more data. - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, key: Key, lsn_range: Range, reconstruct_data: ValueReconstructState, ctx: RequestContext, - ) -> GetValueReconstructFuture; + ) -> Result<(ValueReconstructState, ValueReconstructResult)>; /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. fn short_id(&self) -> String; @@ -507,6 +500,7 @@ impl LayerDescriptor { } } +#[async_trait::async_trait] impl Layer for LayerDescriptor { fn get_key_range(&self) -> Range { self.key.clone() @@ -520,13 +514,13 @@ impl Layer for LayerDescriptor { self.is_incremental } - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, _key: Key, _lsn_range: Range, _reconstruct_data: ValueReconstructState, _ctx: RequestContext, - ) -> GetValueReconstructFuture { + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { todo!("This method shouldn't be part of the Layer trait") } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 5a5d77108b56..25313edad28e 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -57,8 +57,8 @@ use utils::{ }; use super::{ - DeltaFileName, GetValueReconstructFuture, Layer, LayerAccessStats, LayerAccessStatsReset, - LayerIter, LayerKeyIter, PathOrConf, PersistentLayerDesc, + DeltaFileName, Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, + PathOrConf, PersistentLayerDesc, }; /// @@ -219,6 +219,7 @@ impl std::fmt::Debug for DeltaLayerInner { } } +#[async_trait::async_trait] impl Layer for DeltaLayer { /// debugging function to print out the contents of the layer fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { @@ -295,91 +296,89 @@ impl Layer for DeltaLayer { Ok(()) } - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, key: Key, lsn_range: Range, mut reconstruct_state: ValueReconstructState, ctx: RequestContext, - ) -> GetValueReconstructFuture { - Box::pin(async move { - ensure!(lsn_range.start >= self.desc.lsn_range.start); - let mut need_image = true; - - ensure!(self.desc.key_range.contains(&key)); - { - // Open the file and lock the metadata in memory - let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; - - // Scan the page versions backwards, starting from `lsn`. - let file = &inner.file; - let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( - inner.index_start_blk, - inner.index_root_blk, - file, - ); - let search_key = DeltaKey::from_key_lsn(&key, Lsn(lsn_range.end.0 - 1)); - - let mut offsets: Vec<(Lsn, u64)> = Vec::new(); - - tree_reader.visit(&search_key.0, VisitDirection::Backwards, |key, value| { - let blob_ref = BlobRef(value); - if key[..KEY_SIZE] != search_key.0[..KEY_SIZE] { - return false; - } - let entry_lsn = DeltaKey::extract_lsn_from_buf(key); - if entry_lsn < lsn_range.start { - return false; - } - offsets.push((entry_lsn, blob_ref.pos())); + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + ensure!(lsn_range.start >= self.desc.lsn_range.start); + let mut need_image = true; + + ensure!(self.desc.key_range.contains(&key)); + { + // Open the file and lock the metadata in memory + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + + // Scan the page versions backwards, starting from `lsn`. + let file = &inner.file; + let tree_reader = DiskBtreeReader::<_, DELTA_KEY_SIZE>::new( + inner.index_start_blk, + inner.index_root_blk, + file, + ); + let search_key = DeltaKey::from_key_lsn(&key, Lsn(lsn_range.end.0 - 1)); + + let mut offsets: Vec<(Lsn, u64)> = Vec::new(); + + tree_reader.visit(&search_key.0, VisitDirection::Backwards, |key, value| { + let blob_ref = BlobRef(value); + if key[..KEY_SIZE] != search_key.0[..KEY_SIZE] { + return false; + } + let entry_lsn = DeltaKey::extract_lsn_from_buf(key); + if entry_lsn < lsn_range.start { + return false; + } + offsets.push((entry_lsn, blob_ref.pos())); - !blob_ref.will_init() - })?; + !blob_ref.will_init() + })?; - // Ok, 'offsets' now contains the offsets of all the entries we need to read - let mut cursor = file.block_cursor(); - let mut buf = Vec::new(); - for (entry_lsn, pos) in offsets { - cursor.read_blob_into_buf(pos, &mut buf).with_context(|| { - format!( - "Failed to read blob from virtual file {}", - file.file.path.display() - ) - })?; - let val = Value::des(&buf).with_context(|| { - format!( - "Failed to deserialize file blob from virtual file {}", - file.file.path.display() - ) - })?; - match val { - Value::Image(img) => { - reconstruct_state.img = Some((entry_lsn, img)); + // Ok, 'offsets' now contains the offsets of all the entries we need to read + let mut cursor = file.block_cursor(); + let mut buf = Vec::new(); + for (entry_lsn, pos) in offsets { + cursor.read_blob_into_buf(pos, &mut buf).with_context(|| { + format!( + "Failed to read blob from virtual file {}", + file.file.path.display() + ) + })?; + let val = Value::des(&buf).with_context(|| { + format!( + "Failed to deserialize file blob from virtual file {}", + file.file.path.display() + ) + })?; + match val { + Value::Image(img) => { + reconstruct_state.img = Some((entry_lsn, img)); + need_image = false; + break; + } + Value::WalRecord(rec) => { + let will_init = rec.will_init(); + reconstruct_state.records.push((entry_lsn, rec)); + if will_init { + // This WAL record initializes the page, so no need to go further back need_image = false; break; } - Value::WalRecord(rec) => { - let will_init = rec.will_init(); - reconstruct_state.records.push((entry_lsn, rec)); - if will_init { - // This WAL record initializes the page, so no need to go further back - need_image = false; - break; - } - } } } - // release metadata lock and close the file } + // release metadata lock and close the file + } - // If an older page image is needed to reconstruct the page, let the - // caller know. - if need_image { - Ok((reconstruct_state, ValueReconstructResult::Continue)) - } else { - Ok((reconstruct_state, ValueReconstructResult::Complete)) - } - }) + // If an older page image is needed to reconstruct the page, let the + // caller know. + if need_image { + Ok((reconstruct_state, ValueReconstructResult::Continue)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } } /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index a89ebec5ecec..f2d09a1e5a34 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -53,10 +53,7 @@ use utils::{ }; use super::filename::ImageFileName; -use super::{ - GetValueReconstructFuture, Layer, LayerAccessStatsReset, LayerIter, PathOrConf, - PersistentLayerDesc, -}; +use super::{Layer, LayerAccessStatsReset, LayerIter, PathOrConf, PersistentLayerDesc}; /// /// Header stored in the beginning of the file @@ -152,6 +149,7 @@ impl std::fmt::Debug for ImageLayerInner { } } +#[async_trait::async_trait] impl Layer for ImageLayer { /// debugging function to print out the contents of the layer fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()> { @@ -184,42 +182,39 @@ impl Layer for ImageLayer { } /// Look up given page in the file - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, key: Key, lsn_range: Range, mut reconstruct_state: ValueReconstructState, ctx: RequestContext, - ) -> GetValueReconstructFuture { - Box::pin(async move { - assert!(self.desc.key_range.contains(&key)); - assert!(lsn_range.start >= self.lsn); - assert!(lsn_range.end >= self.lsn); - - let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; - - let file = inner.file.as_ref().unwrap(); - let tree_reader = - DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); - - let mut keybuf: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; - key.write_to_byte_slice(&mut keybuf); - if let Some(offset) = tree_reader.get(&keybuf)? { - let blob = file.block_cursor().read_blob(offset).with_context(|| { - format!( - "failed to read value from data file {} at offset {}", - self.path().display(), - offset - ) - })?; - let value = Bytes::from(blob); - - reconstruct_state.img = Some((self.lsn, value)); - Ok((reconstruct_state, ValueReconstructResult::Complete)) - } else { - Ok((reconstruct_state, ValueReconstructResult::Missing)) - } - }) + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + assert!(self.desc.key_range.contains(&key)); + assert!(lsn_range.start >= self.lsn); + assert!(lsn_range.end >= self.lsn); + + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + + let file = inner.file.as_ref().unwrap(); + let tree_reader = DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); + + let mut keybuf: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; + key.write_to_byte_slice(&mut keybuf); + if let Some(offset) = tree_reader.get(&keybuf)? { + let blob = file.block_cursor().read_blob(offset).with_context(|| { + format!( + "failed to read value from data file {} at offset {}", + self.path().display(), + offset + ) + })?; + let value = Bytes::from(blob); + + reconstruct_state.img = Some((self.lsn, value)); + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Missing)) + } } /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index e33fc030899d..e6e9e3ee7a3a 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -29,7 +29,7 @@ use std::fmt::Write as _; use std::ops::Range; use std::sync::{Arc, RwLock}; -use super::{DeltaLayer, DeltaLayerWriter, GetValueReconstructFuture, Layer}; +use super::{DeltaLayer, DeltaLayerWriter, Layer}; thread_local! { /// A buffer for serializing object during [`InMemoryLayer::put_value`]. @@ -110,6 +110,7 @@ impl InMemoryLayer { } } +#[async_trait::async_trait] impl Layer for InMemoryLayer { fn get_key_range(&self) -> Range { Key::MIN..Key::MAX @@ -190,55 +191,53 @@ impl Layer for InMemoryLayer { } /// Look up given value in the layer. - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, key: Key, lsn_range: Range, mut reconstruct_state: ValueReconstructState, _ctx: RequestContext, - ) -> GetValueReconstructFuture { - Box::pin(async move { - ensure!(lsn_range.start >= self.start_lsn); - let mut need_image = true; - - let inner = self.inner.read().unwrap(); - - let mut reader = inner.file.block_cursor(); - - // Scan the page versions backwards, starting from `lsn`. - if let Some(vec_map) = inner.index.get(&key) { - let slice = vec_map.slice_range(lsn_range); - for (entry_lsn, pos) in slice.iter().rev() { - let buf = reader.read_blob(*pos)?; - let value = Value::des(&buf)?; - match value { - Value::Image(img) => { - reconstruct_state.img = Some((*entry_lsn, img)); - return Ok((reconstruct_state, ValueReconstructResult::Complete)); - } - Value::WalRecord(rec) => { - let will_init = rec.will_init(); - reconstruct_state.records.push((*entry_lsn, rec)); - if will_init { - // This WAL record initializes the page, so no need to go further back - need_image = false; - break; - } + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + ensure!(lsn_range.start >= self.start_lsn); + let mut need_image = true; + + let inner = self.inner.read().unwrap(); + + let mut reader = inner.file.block_cursor(); + + // Scan the page versions backwards, starting from `lsn`. + if let Some(vec_map) = inner.index.get(&key) { + let slice = vec_map.slice_range(lsn_range); + for (entry_lsn, pos) in slice.iter().rev() { + let buf = reader.read_blob(*pos)?; + let value = Value::des(&buf)?; + match value { + Value::Image(img) => { + reconstruct_state.img = Some((*entry_lsn, img)); + return Ok((reconstruct_state, ValueReconstructResult::Complete)); + } + Value::WalRecord(rec) => { + let will_init = rec.will_init(); + reconstruct_state.records.push((*entry_lsn, rec)); + if will_init { + // This WAL record initializes the page, so no need to go further back + need_image = false; + break; } } } } + } - // release lock on 'inner' + // release lock on 'inner' - // If an older page image is needed to reconstruct the page, let the - // caller know. - if need_image { - Ok((reconstruct_state, ValueReconstructResult::Continue)) - } else { - Ok((reconstruct_state, ValueReconstructResult::Complete)) - } - }) + // If an older page image is needed to reconstruct the page, let the + // caller know. + if need_image { + Ok((reconstruct_state, ValueReconstructResult::Continue)) + } else { + Ok((reconstruct_state, ValueReconstructResult::Complete)) + } } } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 6126bf3cf5b3..713f056c6ed2 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -20,8 +20,8 @@ use utils::{ use super::filename::{DeltaFileName, ImageFileName}; use super::{ - DeltaLayer, GetValueReconstructFuture, ImageLayer, LayerAccessStats, LayerAccessStatsReset, - LayerIter, LayerKeyIter, LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, + DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, + LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, ValueReconstructResult, }; /// RemoteLayer is a not yet downloaded [`ImageLayer`] or @@ -63,20 +63,19 @@ impl std::fmt::Debug for RemoteLayer { } } +#[async_trait::async_trait] impl Layer for RemoteLayer { - fn get_value_reconstruct_data( + async fn get_value_reconstruct_data( self: Arc, _key: Key, _lsn_range: Range, _reconstruct_state: ValueReconstructState, _ctx: RequestContext, - ) -> GetValueReconstructFuture { - Box::pin(async move { - bail!( - "layer {} needs to be downloaded", - self.filename().file_name() - ); - }) + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + bail!( + "layer {} needs to be downloaded", + self.filename().file_name() + ); } /// debugging function to print out the contents of the layer From c9f6de9128af13e1c14880d2447d6eab8b4278b0 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 13 Jun 2023 14:16:26 +0200 Subject: [PATCH 3/8] undo the value_reconstruct_data passing, it's not needd with async_trait --- pageserver/src/tenant/storage_layer.rs | 16 ++++---- .../src/tenant/storage_layer/delta_layer.rs | 16 ++++---- .../src/tenant/storage_layer/image_layer.rs | 16 ++++---- .../tenant/storage_layer/inmemory_layer.rs | 16 ++++---- .../src/tenant/storage_layer/remote_layer.rs | 12 +++--- pageserver/src/tenant/timeline.rs | 40 +++++++------------ 6 files changed, 53 insertions(+), 63 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index a6334b3f239a..25c60ac1c7df 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -375,12 +375,12 @@ pub trait Layer: std::fmt::Debug + Send + Sync { /// the predecessor layer and call again with the same 'reconstruct_data' to /// collect more data. async fn get_value_reconstruct_data( - self: Arc, + &self, key: Key, lsn_range: Range, - reconstruct_data: ValueReconstructState, - ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)>; + reconstruct_data: &mut ValueReconstructState, + ctx: &RequestContext, + ) -> Result; /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. fn short_id(&self) -> String; @@ -515,12 +515,12 @@ impl Layer for LayerDescriptor { } async fn get_value_reconstruct_data( - self: Arc, + &self, _key: Key, _lsn_range: Range, - _reconstruct_data: ValueReconstructState, - _ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + _reconstruct_data: &mut ValueReconstructState, + _ctx: &RequestContext, + ) -> Result { todo!("This method shouldn't be part of the Layer trait") } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 25313edad28e..94b00f02a382 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -47,7 +47,6 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; -use std::sync::Arc; use tracing::*; use utils::{ @@ -297,19 +296,20 @@ impl Layer for DeltaLayer { } async fn get_value_reconstruct_data( - self: Arc, + &self, key: Key, lsn_range: Range, - mut reconstruct_state: ValueReconstructState, - ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + reconstruct_state: &mut ValueReconstructState, + ctx: &RequestContext, + ) -> anyhow::Result { ensure!(lsn_range.start >= self.desc.lsn_range.start); let mut need_image = true; ensure!(self.desc.key_range.contains(&key)); + { // Open the file and lock the metadata in memory - let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; // Scan the page versions backwards, starting from `lsn`. let file = &inner.file; @@ -375,9 +375,9 @@ impl Layer for DeltaLayer { // If an older page image is needed to reconstruct the page, let the // caller know. if need_image { - Ok((reconstruct_state, ValueReconstructResult::Continue)) + Ok(ValueReconstructResult::Continue) } else { - Ok((reconstruct_state, ValueReconstructResult::Complete)) + Ok(ValueReconstructResult::Complete) } } diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index f2d09a1e5a34..cfda2c69ec75 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -43,7 +43,7 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::prelude::FileExt; use std::path::{Path, PathBuf}; -use std::sync::{Arc, RwLock, RwLockReadGuard}; +use std::sync::{RwLock, RwLockReadGuard}; use tracing::*; use utils::{ @@ -183,17 +183,17 @@ impl Layer for ImageLayer { /// Look up given page in the file async fn get_value_reconstruct_data( - self: Arc, + &self, key: Key, lsn_range: Range, - mut reconstruct_state: ValueReconstructState, - ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + reconstruct_state: &mut ValueReconstructState, + ctx: &RequestContext, + ) -> anyhow::Result { assert!(self.desc.key_range.contains(&key)); assert!(lsn_range.start >= self.lsn); assert!(lsn_range.end >= self.lsn); - let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; + let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; let file = inner.file.as_ref().unwrap(); let tree_reader = DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); @@ -211,9 +211,9 @@ impl Layer for ImageLayer { let value = Bytes::from(blob); reconstruct_state.img = Some((self.lsn, value)); - Ok((reconstruct_state, ValueReconstructResult::Complete)) + Ok(ValueReconstructResult::Complete) } else { - Ok((reconstruct_state, ValueReconstructResult::Missing)) + Ok(ValueReconstructResult::Missing) } } diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index e6e9e3ee7a3a..e458a7e5ef8f 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -27,7 +27,7 @@ use utils::{ // while being able to use std::fmt::Write's methods use std::fmt::Write as _; use std::ops::Range; -use std::sync::{Arc, RwLock}; +use std::sync::RwLock; use super::{DeltaLayer, DeltaLayerWriter, Layer}; @@ -192,12 +192,12 @@ impl Layer for InMemoryLayer { /// Look up given value in the layer. async fn get_value_reconstruct_data( - self: Arc, + &self, key: Key, lsn_range: Range, - mut reconstruct_state: ValueReconstructState, - _ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + reconstruct_state: &mut ValueReconstructState, + _ctx: &RequestContext, + ) -> anyhow::Result { ensure!(lsn_range.start >= self.start_lsn); let mut need_image = true; @@ -214,7 +214,7 @@ impl Layer for InMemoryLayer { match value { Value::Image(img) => { reconstruct_state.img = Some((*entry_lsn, img)); - return Ok((reconstruct_state, ValueReconstructResult::Complete)); + return Ok(ValueReconstructResult::Complete); } Value::WalRecord(rec) => { let will_init = rec.will_init(); @@ -234,9 +234,9 @@ impl Layer for InMemoryLayer { // If an older page image is needed to reconstruct the page, let the // caller know. if need_image { - Ok((reconstruct_state, ValueReconstructResult::Continue)) + Ok(ValueReconstructResult::Continue) } else { - Ok((reconstruct_state, ValueReconstructResult::Complete)) + Ok(ValueReconstructResult::Complete) } } } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 713f056c6ed2..690b189e31bc 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -6,7 +6,7 @@ use crate::context::RequestContext; use crate::repository::Key; use crate::tenant::layer_map::BatchedUpdates; use crate::tenant::remote_timeline_client::index::LayerFileMetadata; -use crate::tenant::storage_layer::{Layer, ValueReconstructState}; +use crate::tenant::storage_layer::{Layer, ValueReconstructResult, ValueReconstructState}; use anyhow::{bail, Result}; use pageserver_api::models::HistoricLayerInfo; use std::ops::Range; @@ -21,7 +21,7 @@ use utils::{ use super::filename::{DeltaFileName, ImageFileName}; use super::{ DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, ValueReconstructResult, + LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, }; /// RemoteLayer is a not yet downloaded [`ImageLayer`] or @@ -66,12 +66,12 @@ impl std::fmt::Debug for RemoteLayer { #[async_trait::async_trait] impl Layer for RemoteLayer { async fn get_value_reconstruct_data( - self: Arc, + &self, _key: Key, _lsn_range: Range, - _reconstruct_state: ValueReconstructState, - _ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + _reconstruct_state: &mut ValueReconstructState, + _ctx: &RequestContext, + ) -> Result { bail!( "layer {} needs to be downloaded", self.filename().file_name() diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index aca925c88630..8a31cf3b944f 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -553,14 +553,13 @@ impl Timeline { None => None, }; - let reconstruct_state = ValueReconstructState { + let mut reconstruct_state = ValueReconstructState { records: Vec::new(), img: cached_page_img, }; let timer = self.metrics.get_reconstruct_data_time_histo.start_timer(); - let reconstruct_state = self - .get_reconstruct_data(key, lsn, reconstruct_state, ctx) + self.get_reconstruct_data(key, lsn, &mut reconstruct_state, ctx) .await?; timer.stop_and_record(); @@ -2352,9 +2351,9 @@ impl Timeline { &self, key: Key, request_lsn: Lsn, - mut reconstruct_state: ValueReconstructState, + reconstruct_state: &mut ValueReconstructState, ctx: &RequestContext, - ) -> Result { + ) -> Result<(), PageReconstructError> { // Start from the current timeline. let mut timeline_owned; let mut timeline = self; @@ -2384,12 +2383,12 @@ impl Timeline { // The function should have updated 'state' //info!("CALLED for {} at {}: {:?} with {} records, cached {}", key, cont_lsn, result, reconstruct_state.records.len(), cached_lsn); match result { - ValueReconstructResult::Complete => return Ok(reconstruct_state), + ValueReconstructResult::Complete => return Ok(()), ValueReconstructResult::Continue => { // If we reached an earlier cached page image, we're done. if cont_lsn == cached_lsn + 1 { self.metrics.materialized_page_cache_hit_counter.inc_by(1); - return Ok(reconstruct_state); + return Ok(()); } if prev_lsn <= cont_lsn { // Didn't make any progress in last iteration. Error out to avoid @@ -2493,19 +2492,16 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match Arc::clone(open_layer) + result = match open_layer .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx.attached_child(), + ctx, ) .await { - Ok((new_reconstruct_state, result)) => { - reconstruct_state = new_reconstruct_state; - result - } + Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2526,19 +2522,16 @@ impl Timeline { if cont_lsn > start_lsn { //info!("CHECKING for {} at {} on frozen layer {}", key, cont_lsn, frozen_layer.filename().display()); let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match Arc::clone(frozen_layer) + result = match frozen_layer .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx.attached_child(), + ctx, ) .await { - Ok((new_reconstruct_state, result)) => { - reconstruct_state = new_reconstruct_state; - result - } + Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2567,19 +2560,16 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, lsn_floor); - result = match Arc::clone(&layer) + result = match layer .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx.attached_child(), + ctx, ) .await { - Ok((new_reconstruct_state, result)) => { - reconstruct_state = new_reconstruct_state; - result - } + Ok(result) => result, Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; From 9d547b5ede920a966502ee62a026f84509a2e95e Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Thu, 11 May 2023 17:10:15 +0200 Subject: [PATCH 4/8] layer impls: run get_value_reconstruct_data in block_in_place not ideal but an easy fix --- pageserver/src/tenant/storage_layer.rs | 29 +++++++++++++++++-- .../src/tenant/storage_layer/delta_layer.rs | 4 +-- .../src/tenant/storage_layer/image_layer.rs | 2 +- .../tenant/storage_layer/inmemory_layer.rs | 2 +- .../src/tenant/storage_layer/remote_layer.rs | 2 +- 5 files changed, 32 insertions(+), 7 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 25c60ac1c7df..ce8c59778cb2 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -374,7 +374,7 @@ pub trait Layer: std::fmt::Debug + Send + Sync { /// is available. If this returns ValueReconstructResult::Continue, look up /// the predecessor layer and call again with the same 'reconstruct_data' to /// collect more data. - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, key: Key, lsn_range: Range, @@ -382,6 +382,31 @@ pub trait Layer: std::fmt::Debug + Send + Sync { ctx: &RequestContext, ) -> Result; + /// CANCEL SAFETY: if the returned future is dropped, + /// the wrapped closure still run to completion and the return value discarded. + /// For the case of get_value_reconstruct_data, we expect the closure to not + /// have any side effects, as it only attempts to read a layer (and stuff like + /// page cache isn't considered a real side effect). + /// But, ... + /// TRACING: + /// If the returned future is cancelled, the spawn_blocking span can outlive + /// the caller's span. + /// So, technically, we should be using `parent: None` and `follows_from: current` + /// instead. However, in practice, the advantage of maintaining the span stack + /// in logs outweighs the disadvantage of having a dangling span in a case that + /// is not expected to happen because in pageserver we generally don't drop pending futures. + async fn get_value_reconstruct_data( + &self, + key: Key, + lsn_range: Range, + reconstruct_data: &mut ValueReconstructState, + ctx: &RequestContext, + ) -> Result { + tokio::task::block_in_place(move || { + self.get_value_reconstruct_data_blocking(key, lsn_range, reconstruct_data, ctx) + }) + } + /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. fn short_id(&self) -> String; @@ -514,7 +539,7 @@ impl Layer for LayerDescriptor { self.is_incremental } - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, _key: Key, _lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 94b00f02a382..7db482456abe 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -295,7 +295,7 @@ impl Layer for DeltaLayer { Ok(()) } - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, key: Key, lsn_range: Range, @@ -366,7 +366,7 @@ impl Layer for DeltaLayer { need_image = false; break; } - } + } // release metadata lock and close the file } } // release metadata lock and close the file diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index cfda2c69ec75..947af170ae5c 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -182,7 +182,7 @@ impl Layer for ImageLayer { } /// Look up given page in the file - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, key: Key, lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index e458a7e5ef8f..44ded30e7526 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -191,7 +191,7 @@ impl Layer for InMemoryLayer { } /// Look up given value in the layer. - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, key: Key, lsn_range: Range, diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 690b189e31bc..f708f6d5861a 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -65,7 +65,7 @@ impl std::fmt::Debug for RemoteLayer { #[async_trait::async_trait] impl Layer for RemoteLayer { - async fn get_value_reconstruct_data( + fn get_value_reconstruct_data_blocking( &self, _key: Key, _lsn_range: Range, From f37e3b95a63ff7159f13b4c088500b366be2edc3 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 13 Jun 2023 19:50:21 +0200 Subject: [PATCH 5/8] switch to spawn_blocking --- pageserver/src/tenant/storage_layer.rs | 30 ++++++++------ .../src/tenant/storage_layer/delta_layer.rs | 14 +++---- .../src/tenant/storage_layer/image_layer.rs | 12 +++--- .../tenant/storage_layer/inmemory_layer.rs | 12 +++--- .../src/tenant/storage_layer/remote_layer.rs | 10 ++--- pageserver/src/tenant/timeline.rs | 40 ++++++++++++------- 6 files changed, 66 insertions(+), 52 deletions(-) diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index ce8c59778cb2..06b90decd2c1 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -12,7 +12,7 @@ use crate::context::RequestContext; use crate::repository::{Key, Value}; use crate::task_mgr::TaskKind; use crate::walrecord::NeonWalRecord; -use anyhow::Result; +use anyhow::{Context, Result}; use bytes::Bytes; use enum_map::EnumMap; use enumset::EnumSet; @@ -344,7 +344,7 @@ impl LayerAccessStats { /// timeline names, because those are known in the context of which the layers /// are used in (timeline). #[async_trait::async_trait] -pub trait Layer: std::fmt::Debug + Send + Sync { +pub trait Layer: std::fmt::Debug + Send + Sync + 'static { /// Range of keys that this layer covers fn get_key_range(&self) -> Range; @@ -378,9 +378,9 @@ pub trait Layer: std::fmt::Debug + Send + Sync { &self, key: Key, lsn_range: Range, - reconstruct_data: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> Result; + reconstruct_data: ValueReconstructState, + ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)>; /// CANCEL SAFETY: if the returned future is dropped, /// the wrapped closure still run to completion and the return value discarded. @@ -396,15 +396,19 @@ pub trait Layer: std::fmt::Debug + Send + Sync { /// in logs outweighs the disadvantage of having a dangling span in a case that /// is not expected to happen because in pageserver we generally don't drop pending futures. async fn get_value_reconstruct_data( - &self, + self: Arc, key: Key, lsn_range: Range, - reconstruct_data: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> Result { - tokio::task::block_in_place(move || { + reconstruct_data: ValueReconstructState, + ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + let span = tracing::info_span!("get_value_reconstruct_data_spawn_blocking"); + tokio::task::spawn_blocking(move || { + let _enter = span.enter(); self.get_value_reconstruct_data_blocking(key, lsn_range, reconstruct_data, ctx) }) + .await + .context("spawn_blocking")? } /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. @@ -543,9 +547,9 @@ impl Layer for LayerDescriptor { &self, _key: Key, _lsn_range: Range, - _reconstruct_data: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> Result { + _reconstruct_data: ValueReconstructState, + _ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { todo!("This method shouldn't be part of the Layer trait") } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 7db482456abe..4c26e3df1fcf 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -47,6 +47,7 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; + use tracing::*; use utils::{ @@ -299,17 +300,16 @@ impl Layer for DeltaLayer { &self, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> anyhow::Result { + mut reconstruct_state: ValueReconstructState, + ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { ensure!(lsn_range.start >= self.desc.lsn_range.start); let mut need_image = true; ensure!(self.desc.key_range.contains(&key)); - { // Open the file and lock the metadata in memory - let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; // Scan the page versions backwards, starting from `lsn`. let file = &inner.file; @@ -375,9 +375,9 @@ impl Layer for DeltaLayer { // If an older page image is needed to reconstruct the page, let the // caller know. if need_image { - Ok(ValueReconstructResult::Continue) + Ok((reconstruct_state, ValueReconstructResult::Continue)) } else { - Ok(ValueReconstructResult::Complete) + Ok((reconstruct_state, ValueReconstructResult::Complete)) } } diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 947af170ae5c..21935fdfa79b 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -186,14 +186,14 @@ impl Layer for ImageLayer { &self, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - ctx: &RequestContext, - ) -> anyhow::Result { + mut reconstruct_state: ValueReconstructState, + ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { assert!(self.desc.key_range.contains(&key)); assert!(lsn_range.start >= self.lsn); assert!(lsn_range.end >= self.lsn); - let inner = self.load(LayerAccessKind::GetValueReconstructData, ctx)?; + let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; let file = inner.file.as_ref().unwrap(); let tree_reader = DiskBtreeReader::new(inner.index_start_blk, inner.index_root_blk, file); @@ -211,9 +211,9 @@ impl Layer for ImageLayer { let value = Bytes::from(blob); reconstruct_state.img = Some((self.lsn, value)); - Ok(ValueReconstructResult::Complete) + Ok((reconstruct_state, ValueReconstructResult::Complete)) } else { - Ok(ValueReconstructResult::Missing) + Ok((reconstruct_state, ValueReconstructResult::Missing)) } } diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 44ded30e7526..3eeebcca9d73 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -195,9 +195,9 @@ impl Layer for InMemoryLayer { &self, key: Key, lsn_range: Range, - reconstruct_state: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> anyhow::Result { + mut reconstruct_state: ValueReconstructState, + _ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { ensure!(lsn_range.start >= self.start_lsn); let mut need_image = true; @@ -214,7 +214,7 @@ impl Layer for InMemoryLayer { match value { Value::Image(img) => { reconstruct_state.img = Some((*entry_lsn, img)); - return Ok(ValueReconstructResult::Complete); + return Ok((reconstruct_state, ValueReconstructResult::Complete)); } Value::WalRecord(rec) => { let will_init = rec.will_init(); @@ -234,9 +234,9 @@ impl Layer for InMemoryLayer { // If an older page image is needed to reconstruct the page, let the // caller know. if need_image { - Ok(ValueReconstructResult::Continue) + Ok((reconstruct_state, ValueReconstructResult::Continue)) } else { - Ok(ValueReconstructResult::Complete) + Ok((reconstruct_state, ValueReconstructResult::Complete)) } } } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index f708f6d5861a..a62334689a1e 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -6,7 +6,7 @@ use crate::context::RequestContext; use crate::repository::Key; use crate::tenant::layer_map::BatchedUpdates; use crate::tenant::remote_timeline_client::index::LayerFileMetadata; -use crate::tenant::storage_layer::{Layer, ValueReconstructResult, ValueReconstructState}; +use crate::tenant::storage_layer::{Layer, ValueReconstructState}; use anyhow::{bail, Result}; use pageserver_api::models::HistoricLayerInfo; use std::ops::Range; @@ -21,7 +21,7 @@ use utils::{ use super::filename::{DeltaFileName, ImageFileName}; use super::{ DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, + LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, ValueReconstructResult, }; /// RemoteLayer is a not yet downloaded [`ImageLayer`] or @@ -69,9 +69,9 @@ impl Layer for RemoteLayer { &self, _key: Key, _lsn_range: Range, - _reconstruct_state: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> Result { + _reconstruct_state: ValueReconstructState, + _ctx: RequestContext, + ) -> Result<(ValueReconstructState, ValueReconstructResult)> { bail!( "layer {} needs to be downloaded", self.filename().file_name() diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 8a31cf3b944f..aca925c88630 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -553,13 +553,14 @@ impl Timeline { None => None, }; - let mut reconstruct_state = ValueReconstructState { + let reconstruct_state = ValueReconstructState { records: Vec::new(), img: cached_page_img, }; let timer = self.metrics.get_reconstruct_data_time_histo.start_timer(); - self.get_reconstruct_data(key, lsn, &mut reconstruct_state, ctx) + let reconstruct_state = self + .get_reconstruct_data(key, lsn, reconstruct_state, ctx) .await?; timer.stop_and_record(); @@ -2351,9 +2352,9 @@ impl Timeline { &self, key: Key, request_lsn: Lsn, - reconstruct_state: &mut ValueReconstructState, + mut reconstruct_state: ValueReconstructState, ctx: &RequestContext, - ) -> Result<(), PageReconstructError> { + ) -> Result { // Start from the current timeline. let mut timeline_owned; let mut timeline = self; @@ -2383,12 +2384,12 @@ impl Timeline { // The function should have updated 'state' //info!("CALLED for {} at {}: {:?} with {} records, cached {}", key, cont_lsn, result, reconstruct_state.records.len(), cached_lsn); match result { - ValueReconstructResult::Complete => return Ok(()), + ValueReconstructResult::Complete => return Ok(reconstruct_state), ValueReconstructResult::Continue => { // If we reached an earlier cached page image, we're done. if cont_lsn == cached_lsn + 1 { self.metrics.materialized_page_cache_hit_counter.inc_by(1); - return Ok(()); + return Ok(reconstruct_state); } if prev_lsn <= cont_lsn { // Didn't make any progress in last iteration. Error out to avoid @@ -2492,16 +2493,19 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match open_layer + result = match Arc::clone(open_layer) .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx, + ctx.attached_child(), ) .await { - Ok(result) => result, + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2522,16 +2526,19 @@ impl Timeline { if cont_lsn > start_lsn { //info!("CHECKING for {} at {} on frozen layer {}", key, cont_lsn, frozen_layer.filename().display()); let lsn_floor = max(cached_lsn + 1, start_lsn); - result = match frozen_layer + result = match Arc::clone(frozen_layer) .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx, + ctx.attached_child(), ) .await { - Ok(result) => result, + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; @@ -2560,16 +2567,19 @@ impl Timeline { // Get all the data needed to reconstruct the page version from this layer. // But if we have an older cached page image, no need to go past that. let lsn_floor = max(cached_lsn + 1, lsn_floor); - result = match layer + result = match Arc::clone(&layer) .get_value_reconstruct_data( key, lsn_floor..cont_lsn, reconstruct_state, - ctx, + ctx.attached_child(), ) .await { - Ok(result) => result, + Ok((new_reconstruct_state, result)) => { + reconstruct_state = new_reconstruct_state; + result + } Err(e) => return Err(PageReconstructError::from(e)), }; cont_lsn = lsn_floor; From 042e706b204b7dd10eba15c4db2edc066e137f44 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 14 Jun 2023 18:55:21 +0200 Subject: [PATCH 6/8] minimize diff --- pageserver/src/tenant/storage_layer/delta_layer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 4c26e3df1fcf..e5c67aa76e59 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -47,7 +47,6 @@ use std::io::{Seek, SeekFrom}; use std::ops::Range; use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; - use tracing::*; use utils::{ @@ -307,6 +306,7 @@ impl Layer for DeltaLayer { let mut need_image = true; ensure!(self.desc.key_range.contains(&key)); + { // Open the file and lock the metadata in memory let inner = self.load(LayerAccessKind::GetValueReconstructData, &ctx)?; @@ -366,7 +366,7 @@ impl Layer for DeltaLayer { need_image = false; break; } - } // release metadata lock and close the file + } } } // release metadata lock and close the file From 40e9a6b6e31bf08797e6f1dd902f1cc4d3856a02 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 14 Jun 2023 18:55:55 +0200 Subject: [PATCH 7/8] use anyhow::Result instead of Result in return signatures of impls --- pageserver/src/tenant/storage_layer/delta_layer.rs | 2 +- pageserver/src/tenant/storage_layer/image_layer.rs | 2 +- pageserver/src/tenant/storage_layer/inmemory_layer.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index e5c67aa76e59..cec7a09eff76 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -301,7 +301,7 @@ impl Layer for DeltaLayer { lsn_range: Range, mut reconstruct_state: ValueReconstructState, ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + ) -> anyhow::Result<(ValueReconstructState, ValueReconstructResult)> { ensure!(lsn_range.start >= self.desc.lsn_range.start); let mut need_image = true; diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 21935fdfa79b..6019590db0a7 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -188,7 +188,7 @@ impl Layer for ImageLayer { lsn_range: Range, mut reconstruct_state: ValueReconstructState, ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + ) -> anyhow::Result<(ValueReconstructState, ValueReconstructResult)> { assert!(self.desc.key_range.contains(&key)); assert!(lsn_range.start >= self.lsn); assert!(lsn_range.end >= self.lsn); diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 3eeebcca9d73..4efd032ba93e 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -197,7 +197,7 @@ impl Layer for InMemoryLayer { lsn_range: Range, mut reconstruct_state: ValueReconstructState, _ctx: RequestContext, - ) -> Result<(ValueReconstructState, ValueReconstructResult)> { + ) -> anyhow::Result<(ValueReconstructState, ValueReconstructResult)> { ensure!(lsn_range.start >= self.start_lsn); let mut need_image = true; From 9afb5893dcf60adfdbb209dae27ea370e0fa04fd Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 23 Jun 2023 15:26:46 +0200 Subject: [PATCH 8/8] empty commit to trigger CI rerun without benchmarks