diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 5b649a420c..45b485f5ee 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -1271,11 +1271,12 @@ impl RemoteTimelineClient { let upload_result: anyhow::Result<()> = match &task.op { UploadOp::UploadLayer(ref layer, ref layer_metadata) => { - let path = layer.local_path(); + let path = layer.build_local_path(&self.tenant_shard_id, &self.timeline_id); + upload::upload_timeline_layer( self.conf, &self.storage_impl, - path, + &path, layer_metadata, self.generation, ) diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 4d9b0a9838..232e2a15ca 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -498,7 +498,7 @@ impl DeltaLayerWriterInner { let layer = Layer::finish_creating(self.conf, timeline, desc, &self.path)?; - trace!("created delta layer {}", layer.local_path()); + trace!("created delta layer {}", self.path); Ok(layer) } diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 07182ea82c..94ed53647c 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -572,7 +572,7 @@ impl ImageLayerWriterInner { // FIXME: why not carry the virtualfile here, it supports renaming? let layer = Layer::finish_creating(self.conf, timeline, desc, &self.path)?; - trace!("created image layer {}", layer.local_path()); + trace!("created image layer {}", self.path); Ok(layer) } diff --git a/pageserver/src/tenant/storage_layer/layer.rs b/pageserver/src/tenant/storage_layer/layer.rs index e01034face..65851a166d 100644 --- a/pageserver/src/tenant/storage_layer/layer.rs +++ b/pageserver/src/tenant/storage_layer/layer.rs @@ -3,12 +3,13 @@ use camino::{Utf8Path, Utf8PathBuf}; use pageserver_api::models::{ HistoricLayerInfo, LayerAccessKind, LayerResidenceEventReason, LayerResidenceStatus, }; -use pageserver_api::shard::ShardIndex; +use pageserver_api::shard::{ShardIndex, TenantShardId}; use std::ops::Range; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{Arc, Weak}; use std::time::SystemTime; use tracing::Instrument; +use utils::id::TimelineId; use utils::lsn::Lsn; use utils::sync::heavier_once_cell; @@ -179,7 +180,7 @@ impl Layer { let downloaded = resident.expect("just initialized"); // if the rename works, the path is as expected - std::fs::rename(temp_path, owner.local_path()) + std::fs::rename(temp_path, owner.local_path()?) .with_context(|| format!("rename temporary file as correct path for {owner}"))?; Ok(ResidentLayer { downloaded, owner }) @@ -299,8 +300,12 @@ impl Layer { &self.0.access_stats } - pub(crate) fn local_path(&self) -> &Utf8Path { - &self.0.path + fn local_path(&self) -> Result { + self.0.local_path() + } + + pub(crate) fn filename(&self) -> LayerFileName { + self.0.desc.filename() } pub(crate) fn metadata(&self) -> LayerFileMetadata { @@ -392,13 +397,9 @@ impl ResidentOrWantedEvicted { } struct LayerInner { - /// Only needed to check ondemand_download_behavior_treat_error_as_warn and creation of - /// [`Self::path`]. + /// Only needed to check ondemand_download_behavior_treat_error_as_warn and in [`Self::build_local_path`] conf: &'static PageServerConf, - /// Full path to the file; unclear if this should exist anymore. - path: Utf8PathBuf, - desc: PersistentLayerDesc, /// Timeline access is needed for remote timeline client and metrics. @@ -480,12 +481,17 @@ impl Drop for LayerInner { return; } + // We will only do I/O on drop if our Timeline still exists. Otherwise, we may safely + // leave garbage layers behind to be cleaned up the next time this Timeline is instantiated. + let timeline = match self.timeline.upgrade() { + Some(t) => t, + None => return, + }; + // If timeline is alive, we can construct a span with IDs for this function. - let span = self.timeline.upgrade().map(|timeline| { - tracing::info_span!(parent: None, "layer_gc", tenant_id = %timeline.tenant_shard_id.tenant_id, shard_id=%timeline.tenant_shard_id.shard_slug(), timeline_id = %timeline.timeline_id) - }); + let span = tracing::info_span!(parent: None, "layer_gc", tenant_id = %timeline.tenant_shard_id.tenant_id, shard_id=%timeline.tenant_shard_id.shard_slug(), timeline_id = %timeline.timeline_id); + let path = self.build_local_path(&timeline.tenant_shard_id, &timeline.timeline_id); - let path = std::mem::take(&mut self.path); let file_name = self.layer_desc().filename(); let file_size = self.layer_desc().file_size; let timeline = self.timeline.clone(); @@ -493,7 +499,7 @@ impl Drop for LayerInner { let status = self.status.clone(); crate::task_mgr::BACKGROUND_RUNTIME.spawn_blocking(move || { - let _g = span.map(|s| s.entered()); + let _g = span.entered(); // carry this until we are finished for [`Layer::wait_drop`] support let _status = status; @@ -557,10 +563,6 @@ impl LayerInner { generation: Generation, shard: ShardIndex, ) -> Self { - let path = conf - .timeline_path(&timeline.tenant_shard_id, &timeline.timeline_id) - .join(desc.filename().to_string()); - let (inner, version) = if let Some(inner) = downloaded { let version = inner.version; let resident = ResidentOrWantedEvicted::Resident(inner); @@ -571,7 +573,6 @@ impl LayerInner { LayerInner { conf, - path, desc, timeline: Arc::downgrade(timeline), have_remote_client: timeline.remote_client.is_some(), @@ -587,6 +588,29 @@ impl LayerInner { } } + /// Use our weak ref to Timeline to build a local file path: this will + /// fail if our parent Timeline has been destroyed: layers should not + /// do any I/O after this point, and therefore not require knowledge of path. + pub(crate) fn local_path(&self) -> Result { + let timeline = self + .timeline + .upgrade() + .ok_or_else(|| DownloadError::TimelineShutdown)?; + + Ok(self.build_local_path(&timeline.tenant_shard_id, &timeline.timeline_id)) + } + + /// Use this instead of `local_path` if you already have a Timeline to hand. + pub(crate) fn build_local_path( + &self, + tenant_shard_id: &TenantShardId, + timeline_id: &TimelineId, + ) -> Utf8PathBuf { + self.conf + .timeline_path(tenant_shard_id, timeline_id) + .join(self.desc.filename().to_string()) + } + fn garbage_collect_on_drop(&self) { let res = self.wanted_garbage_collected.compare_exchange( false, @@ -923,7 +947,11 @@ impl LayerInner { } async fn needs_download(&self) -> Result, std::io::Error> { - match tokio::fs::metadata(&self.path).await { + let path = self + .local_path() + .map_err(|_| std::io::Error::new(std::io::ErrorKind::Other, "Timeline destroyed"))?; + + match tokio::fs::metadata(path).await { Ok(m) => Ok(self.is_file_present_and_good_size(&m).err()), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Some(NeedsDownload::NotFound)), Err(e) => Err(e), @@ -931,7 +959,11 @@ impl LayerInner { } fn needs_download_blocking(&self) -> Result, std::io::Error> { - match self.path.metadata() { + let path = self + .local_path() + .map_err(|_| std::io::Error::new(std::io::ErrorKind::Other, "Timeline destroyed"))?; + + match path.metadata() { Ok(m) => Ok(self.is_file_present_and_good_size(&m).err()), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Some(NeedsDownload::NotFound)), Err(e) => Err(e), @@ -1068,7 +1100,11 @@ impl LayerInner { LayerResidenceEventReason::ResidenceChange, ); - let res = match capture_mtime_and_remove(&self.path) { + let local_path = self + .local_path() + .map_err(|_| EvictionCancelled::TimelineGone)?; + + let res = match capture_mtime_and_remove(&local_path) { Ok(local_layer_mtime) => { let duration = SystemTime::now().duration_since(local_layer_mtime); match duration { @@ -1239,7 +1275,7 @@ impl DownloadedLayer { owner.desc.key_range.clone(), owner.desc.lsn_range.clone(), )); - delta_layer::DeltaLayerInner::load(&owner.path, summary, ctx) + delta_layer::DeltaLayerInner::load(&owner.local_path()?, summary, ctx) .await .map(|res| res.map(LayerKind::Delta)) } else { @@ -1250,7 +1286,7 @@ impl DownloadedLayer { owner.desc.key_range.clone(), lsn, )); - image_layer::ImageLayerInner::load(&owner.path, lsn, summary, ctx) + image_layer::ImageLayerInner::load(&owner.local_path()?, lsn, summary, ctx) .await .map(|res| res.map(LayerKind::Image)) }; @@ -1374,8 +1410,12 @@ impl ResidentLayer { } } - pub(crate) fn local_path(&self) -> &Utf8Path { - &self.owner.0.path + pub(crate) fn build_local_path( + &self, + tenant_shard_id: &TenantShardId, + timeline_id: &TimelineId, + ) -> Utf8PathBuf { + self.owner.0.build_local_path(tenant_shard_id, timeline_id) } pub(crate) fn access_stats(&self) -> &LayerAccessStats { diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index f666f1049f..e9f47bbf23 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -2176,7 +2176,7 @@ trait TraversalLayerExt { impl TraversalLayerExt for Layer { fn traversal_id(&self) -> TraversalId { - self.local_path().to_string() + self.filename().to_string() } } @@ -2890,7 +2890,8 @@ impl Timeline { let _g = span.entered(); let new_delta = Handle::current().block_on(frozen_layer.write_to_disk(&self_clone, &ctx))?; - let new_delta_path = new_delta.local_path().to_owned(); + let new_delta_path = new_delta + .build_local_path(&self_clone.tenant_shard_id, &self_clone.timeline_id); // Sync it to disk. // @@ -3134,7 +3135,7 @@ impl Timeline { // and fsync them all in parallel. let all_paths = image_layers .iter() - .map(|layer| layer.local_path().to_owned()) + .map(|layer| layer.build_local_path(&self.tenant_shard_id, &self.timeline_id)) .collect::>(); par_fsync::par_fsync_async(&all_paths) @@ -3683,7 +3684,7 @@ impl Timeline { // FIXME: the writer already fsyncs all data, only rename needs to be fsynced here let layer_paths: Vec = new_layers .iter() - .map(|l| l.local_path().to_owned()) + .map(|l| l.build_local_path(&self.tenant_shard_id, &self.timeline_id)) .collect(); // Fsync all the layer files and directory using multiple threads to