diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 45b485f5ee..df4efbb922 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -1271,7 +1271,7 @@ impl RemoteTimelineClient { let upload_result: anyhow::Result<()> = match &task.op { UploadOp::UploadLayer(ref layer, ref layer_metadata) => { - let path = layer.build_local_path(&self.tenant_shard_id, &self.timeline_id); + let path = layer.local_path_from_id(&self.tenant_shard_id, &self.timeline_id); upload::upload_timeline_layer( self.conf, diff --git a/pageserver/src/tenant/storage_layer/layer.rs b/pageserver/src/tenant/storage_layer/layer.rs index de29ad4dcd..fca07e9674 100644 --- a/pageserver/src/tenant/storage_layer/layer.rs +++ b/pageserver/src/tenant/storage_layer/layer.rs @@ -97,7 +97,7 @@ impl Layer { metadata.shard, ))); - debug_assert!(owner.0.needs_download_blocking().unwrap().is_some()); + debug_assert!(owner.0.needs_download_blocking(timeline).unwrap().is_some()); owner } @@ -136,7 +136,7 @@ impl Layer { let downloaded = resident.expect("just initialized"); - debug_assert!(owner.0.needs_download_blocking().unwrap().is_none()); + debug_assert!(owner.0.needs_download_blocking(timeline).unwrap().is_none()); timeline .metrics @@ -181,7 +181,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(timeline)) .with_context(|| format!("rename temporary file as correct path for {owner}"))?; Ok(ResidentLayer { downloaded, owner }) @@ -301,8 +301,8 @@ impl Layer { &self.0.access_stats } - fn local_path(&self) -> Result { - self.0.local_path() + fn local_path(&self, timeline: &Timeline) -> Utf8PathBuf { + self.0.local_path(timeline) } pub(crate) fn filename(&self) -> LayerFileName { @@ -482,7 +482,12 @@ impl Drop for LayerInner { // 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, + None => { + // no need to nag that timeline is gone: under normal situation on + // task_mgr::remove_tenant_from_memory the timeline is gone before we get dropped. + LAYER_IMPL_METRICS.inc_deletes_failed(DeleteFailed::TimelineGone); + return; + } }; // We will only do I/O during drop if our Timeline's layer_gate is open: this avoids @@ -490,16 +495,18 @@ impl Drop for LayerInner { // path for which the Timeline object has been torn down already. let _gate_guard = match timeline.layer_gate.enter() { Ok(g) => g, - Err(GateError::GateClosed) => return, + Err(GateError::GateClosed) => { + LAYER_IMPL_METRICS.inc_deletes_failed(DeleteFailed::TimelineGone); + return; + } }; // If timeline is alive, we can construct a span with IDs for this function. let span = tracing::info_span!(parent: None, "layer_delete", 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 = self.local_path(&timeline); let file_name = self.layer_desc().filename(); let file_size = self.layer_desc().file_size; - let timeline = self.timeline.clone(); let meta = self.metadata(); let status = self.status.clone(); @@ -527,32 +534,26 @@ impl Drop for LayerInner { } }; - if let Some(timeline) = timeline.upgrade() { - if removed { - timeline.metrics.resident_physical_size_sub(file_size); - } - if let Some(remote_client) = timeline.remote_client.as_ref() { - let res = remote_client.schedule_deletion_of_unlinked(vec![(file_name, meta)]); + if removed { + timeline.metrics.resident_physical_size_sub(file_size); + } + if let Some(remote_client) = timeline.remote_client.as_ref() { + let res = remote_client.schedule_deletion_of_unlinked(vec![(file_name, meta)]); - if let Err(e) = res { - // test_timeline_deletion_with_files_stuck_in_upload_queue is good at - // demonstrating this deadlock (without spawn_blocking): stop will drop - // queued items, which will have ResidentLayer's, and those drops would try - // to re-entrantly lock the RemoteTimelineClient inner state. - if !timeline.is_active() { - tracing::info!("scheduling deletion on drop failed: {e:#}"); - } else { - tracing::warn!("scheduling deletion on drop failed: {e:#}"); - } - LAYER_IMPL_METRICS.inc_deletes_failed(DeleteFailed::DeleteSchedulingFailed); + if let Err(e) = res { + // test_timeline_deletion_with_files_stuck_in_upload_queue is good at + // demonstrating this deadlock (without spawn_blocking): stop will drop + // queued items, which will have ResidentLayer's, and those drops would try + // to re-entrantly lock the RemoteTimelineClient inner state. + if !timeline.is_active() { + tracing::info!("scheduling deletion on drop failed: {e:#}"); } else { - LAYER_IMPL_METRICS.inc_completed_deletes(); + tracing::warn!("scheduling deletion on drop failed: {e:#}"); } + LAYER_IMPL_METRICS.inc_deletes_failed(DeleteFailed::DeleteSchedulingFailed); + } else { + LAYER_IMPL_METRICS.inc_completed_deletes(); } - } else { - // no need to nag that timeline is gone: under normal situation on - // task_mgr::remove_tenant_from_memory the timeline is gone before we get dropped. - LAYER_IMPL_METRICS.inc_deletes_failed(DeleteFailed::TimelineGone); } }); } @@ -593,20 +594,16 @@ 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)) + /// All call sites that need this function should already have a Timeline (e.g. from + /// upgrading the Self::timeline weak pointer) -- it doesn't make sense to try and + /// do anything with the local file if the Timeline isn't still alive. + fn local_path(&self, timeline: &Timeline) -> Utf8PathBuf { + self.local_path_from_id(&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( + /// Use this instead of `local_path` if you don't have a Timeline but do have its ID: this + /// is used by external callers such as [`crate::tenant::RemoteTimelineClient`] + pub(crate) fn local_path_from_id( &self, tenant_shard_id: &TenantShardId, timeline_id: &TimelineId, @@ -709,7 +706,7 @@ impl LayerInner { // check if we really need to be downloaded; could have been already downloaded by a // cancelled previous attempt. let needs_download = self - .needs_download() + .needs_download(&timeline) .await .map_err(DownloadError::PreStatFailed)?; @@ -859,6 +856,7 @@ impl LayerInner { // block tenant::mgr::remove_tenant_from_memory. let this: Arc = self.clone(); + let timeline_clone = timeline.clone(); crate::task_mgr::spawn( &tokio::runtime::Handle::current(), @@ -920,7 +918,7 @@ impl LayerInner { match rx.await { Ok((Ok(()), permit)) => { if let Some(reason) = self - .needs_download() + .needs_download(&timeline_clone) .await .map_err(DownloadError::PostStatFailed)? { @@ -955,10 +953,11 @@ impl LayerInner { } } - async fn needs_download(&self) -> Result, std::io::Error> { - let path = self - .local_path() - .map_err(|_| std::io::Error::new(std::io::ErrorKind::Other, "Timeline destroyed"))?; + async fn needs_download( + &self, + timeline: &Timeline, + ) -> Result, std::io::Error> { + let path = self.local_path(timeline); match tokio::fs::metadata(path).await { Ok(m) => Ok(self.is_file_present_and_good_size(&m).err()), @@ -967,10 +966,11 @@ impl LayerInner { } } - fn needs_download_blocking(&self) -> Result, std::io::Error> { - let path = self - .local_path() - .map_err(|_| std::io::Error::new(std::io::ErrorKind::Other, "Timeline destroyed"))?; + fn needs_download_blocking( + &self, + timeline: &Timeline, + ) -> Result, std::io::Error> { + let path = self.local_path(timeline); match path.metadata() { Ok(m) => Ok(self.is_file_present_and_good_size(&m).err()), @@ -1112,9 +1112,7 @@ impl LayerInner { LayerResidenceEventReason::ResidenceChange, ); - let local_path = self - .local_path() - .map_err(|_| EvictionCancelled::TimelineGone)?; + let local_path = self.local_path(&timeline); let res = match capture_mtime_and_remove(&local_path) { Ok(local_layer_mtime) => { @@ -1287,7 +1285,7 @@ impl DownloadedLayer { owner.desc.key_range.clone(), owner.desc.lsn_range.clone(), )); - delta_layer::DeltaLayerInner::load(&owner.local_path()?, summary, ctx) + delta_layer::DeltaLayerInner::load(&owner.local_path(&timeline), summary, ctx) .await .map(|res| res.map(LayerKind::Delta)) } else { @@ -1298,7 +1296,7 @@ impl DownloadedLayer { owner.desc.key_range.clone(), lsn, )); - image_layer::ImageLayerInner::load(&owner.local_path()?, lsn, summary, ctx) + image_layer::ImageLayerInner::load(&owner.local_path(&timeline), lsn, summary, ctx) .await .map(|res| res.map(LayerKind::Image)) }; @@ -1422,12 +1420,14 @@ impl ResidentLayer { } } - pub(crate) fn build_local_path( + pub(crate) fn local_path_from_id( &self, tenant_shard_id: &TenantShardId, timeline_id: &TimelineId, ) -> Utf8PathBuf { - self.owner.0.build_local_path(tenant_shard_id, timeline_id) + self.owner + .0 + .local_path_from_id(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 f472bae7e6..bf3e99b0ae 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -2903,7 +2903,7 @@ impl Timeline { let new_delta = Handle::current().block_on(frozen_layer.write_to_disk(&self_clone, &ctx))?; let new_delta_path = new_delta - .build_local_path(&self_clone.tenant_shard_id, &self_clone.timeline_id); + .local_path_from_id(&self_clone.tenant_shard_id, &self_clone.timeline_id); // Sync it to disk. // @@ -3147,7 +3147,7 @@ impl Timeline { // and fsync them all in parallel. let all_paths = image_layers .iter() - .map(|layer| layer.build_local_path(&self.tenant_shard_id, &self.timeline_id)) + .map(|layer| layer.local_path_from_id(&self.tenant_shard_id, &self.timeline_id)) .collect::>(); par_fsync::par_fsync_async(&all_paths) @@ -3696,7 +3696,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.build_local_path(&self.tenant_shard_id, &self.timeline_id)) + .map(|l| l.local_path_from_id(&self.tenant_shard_id, &self.timeline_id)) .collect(); // Fsync all the layer files and directory using multiple threads to