From 0850d28059ad1b1def6667e67fbad8e56559d7ea Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Wed, 1 Feb 2023 18:01:19 +0100 Subject: [PATCH] cleanup API + add flag to residence status to distinguish loading from creating --- libs/pageserver_api/src/models.rs | 13 +++-- pageserver/src/tenant/storage_layer.rs | 58 ++++++++++++++----- .../src/tenant/storage_layer/delta_layer.rs | 14 +++-- .../src/tenant/storage_layer/image_layer.rs | 14 +++-- .../src/tenant/storage_layer/remote_layer.rs | 25 ++++---- pageserver/src/tenant/timeline.rs | 32 +++++----- 6 files changed, 91 insertions(+), 65 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 5e52fac064..7788c86c50 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -256,8 +256,13 @@ pub struct LayerAccessStatFullDetails { #[derive(Debug, Copy, Clone, Serialize, Deserialize)] #[serde(tag = "kind")] pub enum LayerResidenceStatus { - Resident { timestamp_millis_since_epoch: u128 }, - Evicted { timestamp_millis_since_epoch: u128 }, + Resident { + timestamp_millis_since_epoch: u128, + creating: bool, + }, + Evicted { + timestamp_millis_since_epoch: u128, + }, } #[derive(Debug, Clone, Serialize)] @@ -300,7 +305,7 @@ pub enum HistoricLayerInfo { #[serde_as(as = "DisplayFromStr")] lsn_end: Lsn, remote: bool, - access_stats: Option, + access_stats: LayerAccessStats, }, Image { layer_file_name: String, @@ -311,7 +316,7 @@ pub enum HistoricLayerInfo { #[serde_as(as = "DisplayFromStr")] lsn_start: Lsn, remote: bool, - access_stats: Option, + access_stats: LayerAccessStats, }, } diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 66cef4618c..fb64b2a199 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -88,7 +88,7 @@ pub enum ValueReconstructResult { Missing, } -#[derive(Default, Debug)] +#[derive(Debug)] pub struct LayerAccessStats(Mutex); #[derive(Debug, Clone)] @@ -109,8 +109,13 @@ struct LayerAccessStatsInner { #[derive(Debug, Clone)] pub enum LayerResidenceStatus { - Resident { timestamp: SystemTime }, - Evicted { timestamp: SystemTime }, + Resident { + timestamp: SystemTime, + creating: bool, + }, + Evicted { + timestamp: SystemTime, + }, } #[derive(Clone, Copy, strum_macros::EnumString)] @@ -147,19 +152,22 @@ impl LayerResidenceStatus { } } - pub fn resident() -> Self { + pub fn resident(creating: bool) -> Self { LayerResidenceStatus::Resident { timestamp: SystemTime::now(), + creating, } } fn to_api_model(&self) -> pageserver_api::models::LayerResidenceStatus { match self { - LayerResidenceStatus::Resident { timestamp } => { - pageserver_api::models::LayerResidenceStatus::Resident { - timestamp_millis_since_epoch: system_time_to_millis_since_epoch(timestamp), - } - } + LayerResidenceStatus::Resident { + timestamp, + creating, + } => pageserver_api::models::LayerResidenceStatus::Resident { + timestamp_millis_since_epoch: system_time_to_millis_since_epoch(timestamp), + creating: *creating, + }, LayerResidenceStatus::Evicted { timestamp } => { pageserver_api::models::LayerResidenceStatus::Evicted { timestamp_millis_since_epoch: system_time_to_millis_since_epoch(timestamp), @@ -189,24 +197,44 @@ impl LayerAccessStats { const LAST_ACCESSES_MAX_LEN: usize = 16; const LAST_RESIDENCE_CHANGES_MAX_LEN: usize = 16; + pub(crate) fn new_for_loading_layer(residence_status: LayerResidenceStatus) -> Self { + let new = LayerAccessStats(Mutex::new(LayerAccessStatsInner::default())); + new.record_residence_change(residence_status); + new + } + + pub(crate) fn new_for_new_layer_file() -> Self { + let new = LayerAccessStats(Mutex::new(LayerAccessStatsInner::default())); + new.record_residence_change(LayerResidenceStatus::resident(true)); + new + } + /// Creates a clone of `self` and records `new_status` in the clone. /// The `new_status` is not recorded in `self` pub(crate) fn clone_for_residence_change( &self, new_status: LayerResidenceStatus, ) -> LayerAccessStats { - let mut clone = { + let clone = { let inner = self.0.lock().unwrap(); inner.clone() }; + let new = LayerAccessStats(Mutex::new(clone)); + new.record_residence_change(new_status); + new + } + + fn record_residence_change(&self, new_status: LayerResidenceStatus) { + if LAYER_ACCESS_STATS_KILLSWITCH.load(atomic::Ordering::SeqCst) { + return; + } + let mut inner = self.0.lock().unwrap(); // make room first to avoid reallocs - while clone.last_residence_changes.len() >= Self::LAST_RESIDENCE_CHANGES_MAX_LEN { - clone.last_residence_changes.pop_back(); + while inner.last_residence_changes.len() >= Self::LAST_RESIDENCE_CHANGES_MAX_LEN { + inner.last_residence_changes.pop_back(); } - clone.last_residence_changes.push_front(new_status.clone()); - - LayerAccessStats(Mutex::new(clone)) + inner.last_residence_changes.push_front(new_status); } fn record_access(&self, access_kind: LayerAccessKind, task_kind: TaskKind) { diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index e28bad3683..f10f3b33c5 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -57,7 +57,7 @@ use utils::{ use super::{ DeltaFileName, Layer, LayerAccessStats, LayerAccessStatsReset, LayerFileName, LayerIter, - LayerKeyIter, PathOrConf, + LayerKeyIter, LayerResidenceStatus, PathOrConf, }; /// @@ -438,7 +438,7 @@ impl PersistentLayer for DeltaLayer { lsn_start: lsn_range.start, lsn_end: lsn_range.end, remote: false, - access_stats: Some(self.access_stats.to_api_model()), + access_stats: self.access_stats.to_api_model(), }; if let Some(reset) = reset { @@ -582,7 +582,7 @@ impl DeltaLayer { tenant_id: TenantId, filename: &DeltaFileName, file_size: u64, - existing_access_stats: Option, + access_stats: LayerAccessStats, ) -> DeltaLayer { DeltaLayer { path_or_conf: PathOrConf::Conf(conf), @@ -591,7 +591,7 @@ impl DeltaLayer { key_range: filename.key_range.clone(), lsn_range: filename.lsn_range.clone(), file_size, - access_stats: existing_access_stats.unwrap_or_default(), + access_stats, inner: RwLock::new(DeltaLayerInner { loaded: false, file: None, @@ -621,7 +621,9 @@ impl DeltaLayer { key_range: summary.key_range, lsn_range: summary.lsn_range, file_size: metadata.len(), - access_stats: LayerAccessStats::default(), + access_stats: LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::resident( + false, + )), inner: RwLock::new(DeltaLayerInner { loaded: false, file: None, @@ -792,7 +794,7 @@ impl DeltaLayerWriterInner { key_range: self.key_start..key_end, lsn_range: self.lsn_range.clone(), file_size: metadata.len(), - access_stats: LayerAccessStats::default(), + access_stats: LayerAccessStats::new_for_new_layer_file(), inner: RwLock::new(DeltaLayerInner { loaded: false, file: None, diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 2ce3a4bf63..40a8bd597a 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -52,7 +52,7 @@ use utils::{ }; use super::filename::{ImageFileName, LayerFileName, PathOrConf}; -use super::{Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter}; +use super::{Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerResidenceStatus}; /// /// Header stored in the beginning of the file @@ -249,7 +249,7 @@ impl PersistentLayer for ImageLayer { key_end: key_range.end, lsn_start: lsn_range.start, remote: false, - access_stats: Some(self.access_stats.to_api_model()), + access_stats: self.access_stats.to_api_model(), }; if let Some(reset) = reset { @@ -385,7 +385,7 @@ impl ImageLayer { tenant_id: TenantId, filename: &ImageFileName, file_size: u64, - existing_access_stats: Option, + access_stats: LayerAccessStats, ) -> ImageLayer { ImageLayer { path_or_conf: PathOrConf::Conf(conf), @@ -394,7 +394,7 @@ impl ImageLayer { key_range: filename.key_range.clone(), lsn: filename.lsn, file_size, - access_stats: existing_access_stats.unwrap_or_default(), + access_stats, inner: RwLock::new(ImageLayerInner { loaded: false, file: None, @@ -422,7 +422,9 @@ impl ImageLayer { key_range: summary.key_range, lsn: summary.lsn, file_size: metadata.len(), - access_stats: LayerAccessStats::default(), + access_stats: LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::resident( + false, + )), inner: RwLock::new(ImageLayerInner { file: None, loaded: false, @@ -582,7 +584,7 @@ impl ImageLayerWriterInner { key_range: self.key_range.clone(), lsn: self.lsn, file_size: metadata.len(), - access_stats: LayerAccessStats::default(), + access_stats: LayerAccessStats::new_for_new_layer_file(), inner: RwLock::new(ImageLayerInner { loaded: false, file: None, diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 3b55ce6c70..9e2204d4d7 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -155,7 +155,7 @@ impl PersistentLayer for RemoteLayer { lsn_start: lsn_range.start, lsn_end: lsn_range.end, remote: true, - access_stats: Some(self.access_stats.to_api_model()), + access_stats: self.access_stats.to_api_model(), } } else { HistoricLayerInfo::Image { @@ -164,7 +164,7 @@ impl PersistentLayer for RemoteLayer { key_end: key_range.end, lsn_start: lsn_range.start, remote: true, - access_stats: Some(self.access_stats.to_api_model()), + access_stats: self.access_stats.to_api_model(), } } } @@ -180,7 +180,7 @@ impl RemoteLayer { timelineid: TimelineId, fname: &ImageFileName, layer_metadata: &LayerFileMetadata, - existing_access_stats: Option, + access_stats: LayerAccessStats, ) -> RemoteLayer { RemoteLayer { tenantid, @@ -192,7 +192,7 @@ impl RemoteLayer { file_name: fname.to_owned().into(), layer_metadata: layer_metadata.clone(), ongoing_download: Arc::new(tokio::sync::Semaphore::new(1)), - access_stats: existing_access_stats.unwrap_or_default(), + access_stats, } } @@ -201,8 +201,7 @@ impl RemoteLayer { timelineid: TimelineId, fname: &DeltaFileName, layer_metadata: &LayerFileMetadata, - - existing_access_stats: Option, + access_stats: LayerAccessStats, ) -> RemoteLayer { RemoteLayer { tenantid, @@ -214,7 +213,7 @@ impl RemoteLayer { file_name: fname.to_owned().into(), layer_metadata: layer_metadata.clone(), ongoing_download: Arc::new(tokio::sync::Semaphore::new(1)), - access_stats: existing_access_stats.unwrap_or_default(), + access_stats, } } @@ -235,10 +234,8 @@ impl RemoteLayer { self.tenantid, &fname, file_size, - Some( - self.access_stats - .clone_for_residence_change(LayerResidenceStatus::resident()), - ), + self.access_stats + .clone_for_residence_change(LayerResidenceStatus::resident(false)), )) } else { let fname = ImageFileName { @@ -251,10 +248,8 @@ impl RemoteLayer { self.tenantid, &fname, file_size, - Some( - self.access_stats - .clone_for_residence_change(LayerResidenceStatus::resident()), - ), + self.access_stats + .clone_for_residence_change(LayerResidenceStatus::resident(false)), )) } } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 4a8927fca5..c578c436bc 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -29,8 +29,8 @@ use crate::broker_client::is_broker_client_initialized; use crate::context::{DownloadBehavior, RequestContext}; use crate::tenant::remote_timeline_client::{self, index::LayerFileMetadata}; use crate::tenant::storage_layer::{ - DeltaFileName, DeltaLayerWriter, ImageFileName, ImageLayerWriter, InMemoryLayer, LayerFileName, - RemoteLayer, + DeltaFileName, DeltaLayerWriter, ImageFileName, ImageLayerWriter, InMemoryLayer, + LayerAccessStats, LayerFileName, LayerResidenceStatus, RemoteLayer, }; use crate::tenant::{ ephemeral_file::is_ephemeral_file, @@ -71,9 +71,7 @@ use walreceiver::spawn_connection_manager_task; use super::layer_map::BatchedUpdates; use super::remote_timeline_client::index::IndexPart; use super::remote_timeline_client::RemoteTimelineClient; -use super::storage_layer::{ - DeltaLayer, ImageLayer, Layer, LayerAccessStatsReset, LayerResidenceStatus, -}; +use super::storage_layer::{DeltaLayer, ImageLayer, Layer, LayerAccessStatsReset}; #[derive(Debug, PartialEq, Eq, Clone, Copy)] enum FlushLoopState { @@ -879,22 +877,18 @@ impl Timeline { self.timeline_id, &image_name, &layer_metadata, - Some( - local_layer - .access_stats() - .clone_for_residence_change(LayerResidenceStatus::evicted()), - ), + local_layer + .access_stats() + .clone_for_residence_change(LayerResidenceStatus::evicted()), ), LayerFileName::Delta(delta_name) => RemoteLayer::new_delta( self.tenant_id, self.timeline_id, &delta_name, &layer_metadata, - Some( - local_layer - .access_stats() - .clone_for_residence_change(LayerResidenceStatus::evicted()), - ), + local_layer + .access_stats() + .clone_for_residence_change(LayerResidenceStatus::evicted()), ), #[cfg(test)] LayerFileName::Test(_) => unreachable!(), @@ -1171,7 +1165,7 @@ impl Timeline { self.tenant_id, &imgfilename, file_size, - None, + LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::resident(false)), ); trace!("found layer {}", layer.path().display()); @@ -1203,7 +1197,7 @@ impl Timeline { self.tenant_id, &deltafilename, file_size, - None, + LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::resident(false)), ); trace!("found layer {}", layer.path().display()); @@ -1341,7 +1335,7 @@ impl Timeline { self.timeline_id, imgfilename, &remote_layer_metadata, - None, + LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::evicted()), ); let remote_layer = Arc::new(remote_layer); @@ -1366,7 +1360,7 @@ impl Timeline { self.timeline_id, deltafilename, &remote_layer_metadata, - None, + LayerAccessStats::new_for_loading_layer(LayerResidenceStatus::evicted()), ); let remote_layer = Arc::new(remote_layer); updates.insert_historic(remote_layer);