From f557790969dd6aa0d551777cf4cd3886db99a132 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Fri, 9 Jun 2023 17:02:23 +0200 Subject: [PATCH] EphemeralFile: panic if removal fails This requires us to be disciplined about dropping all EphemeralFile objects (=> InMemoryLayer objects) before removing the timeline / tenant dir on disk. If we don't do that, we'll panic inside the EphemeralFile::drop. We know that detach doesn't honor this, so, we cannot ship this patch just yet. But it's good to have it as an aspirational goal. --- pageserver/src/tenant/ephemeral_file.rs | 59 +++++++++++++++---- .../src/tenant/storage_layer/delta_layer.rs | 2 +- .../src/tenant/storage_layer/image_layer.rs | 2 +- pageserver/src/virtual_file.rs | 32 ++++++---- 4 files changed, 72 insertions(+), 23 deletions(-) diff --git a/pageserver/src/tenant/ephemeral_file.rs b/pageserver/src/tenant/ephemeral_file.rs index 81faf601ae..90e86a7607 100644 --- a/pageserver/src/tenant/ephemeral_file.rs +++ b/pageserver/src/tenant/ephemeral_file.rs @@ -39,7 +39,7 @@ pub struct EphemeralFile { file_id: u64, _tenant_id: TenantId, _timeline_id: TimelineId, - file: Arc, + file: Option>, pub size: u64, } @@ -73,11 +73,20 @@ impl EphemeralFile { let file_rc = Arc::new(file); l.files.insert(file_id, file_rc.clone()); + #[cfg(debug_assertions)] + debug!( + "created ephemeral file {}\n{}", + filename.display(), + std::backtrace::Backtrace::force_capture() + ); + #[cfg(not(debug_assertions))] + debug!("created ephemeral file {}", filename.display()); + Ok(EphemeralFile { file_id, _tenant_id: tenant_id, _timeline_id: timeline_id, - file: file_rc, + file: Some(file_rc), size: 0, }) } @@ -87,6 +96,8 @@ impl EphemeralFile { while off < PAGE_SZ { let n = self .file + .as_ref() + .unwrap() .read_at(&mut buf[off..], blkno as u64 * PAGE_SZ as u64 + off as u64)?; if n == 0 { @@ -269,17 +280,43 @@ impl Drop for EphemeralFile { cache.drop_buffers_for_ephemeral(self.file_id); // remove entry from the hash map - EPHEMERAL_FILES.write().unwrap().files.remove(&self.file_id); + let virtual_file = EPHEMERAL_FILES + .write() + .unwrap() + .files + .remove(&self.file_id) + .unwrap(); + + // remove file from self + let self_file = self.file.take().unwrap(); + + assert_eq!( + Arc::as_ptr(&virtual_file) as *const (), + Arc::as_ptr(&self_file) as *const () + ); + drop(self_file); + + // XXX once we upgrade to Rust 1.70, use Arc::into_inner. + // It does the following checks atomically. + assert_eq!(Arc::weak_count(&virtual_file), 0); + let virtual_file = Arc::try_unwrap(virtual_file).expect( + "we are being dropped and EPHEMERAL_FILES is the only other place where we put the Arc", + ); // unlink the file - let res = std::fs::remove_file(&self.file.path); - if let Err(e) = res { - warn!( - "could not remove ephemeral file '{}': {}", - self.file.path.display(), - e - ); - } + // TODO: we should be able to unwrap here, but, timeline delete and tenant detach do + // std::fs::remove_dir_all without dropping all InMemoryLayer => EphemeralFile + // of the tenant => need to fix that first. + match virtual_file.remove() { + Ok(()) => (), + Err((virtual_file, e)) => { + warn!( + "could not remove ephemeral file '{}': {}", + virtual_file.path.display(), + e + ); + } + }; } } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 624fe8dac4..80aed96933 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -917,7 +917,7 @@ impl Drop for DeltaLayerWriter { fn drop(&mut self) { if let Some(inner) = self.inner.take() { match inner.blob_writer.into_inner().into_inner() { - Ok(vfile) => vfile.remove(), + Ok(vfile) => vfile.remove().unwrap(), Err(err) => warn!( "error while flushing buffer of image layer temporary file: {}", err diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 07a16a7de2..a615411b12 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -709,7 +709,7 @@ impl ImageLayerWriter { impl Drop for ImageLayerWriter { fn drop(&mut self) { if let Some(inner) = self.inner.take() { - inner.blob_writer.into_inner().remove(); + inner.blob_writer.into_inner().remove().unwrap(); } } } diff --git a/pageserver/src/virtual_file.rs b/pageserver/src/virtual_file.rs index fb216123c1..6873e19120 100644 --- a/pageserver/src/virtual_file.rs +++ b/pageserver/src/virtual_file.rs @@ -324,16 +324,8 @@ impl VirtualFile { Ok(result) } - pub fn remove(self) { - let path = self.path.clone(); - drop(self); - std::fs::remove_file(path).expect("failed to remove the virtual file"); - } -} - -impl Drop for VirtualFile { - /// If a VirtualFile is dropped, close the underlying file if it was open. - fn drop(&mut self) { + /// Idempotently close the file descriptor we might have or have not open for this VirtualFile. + pub fn close(&mut self) { let handle = self.handle.get_mut().unwrap(); // We could check with a read-lock first, to avoid waiting on an @@ -351,6 +343,26 @@ impl Drop for VirtualFile { .observe_closure_duration(|| slot_guard.file.take()); } } + + /// Caller can retry if we return an `Err`. + #[allow(clippy::result_large_err)] + pub fn remove(mut self) -> Result<(), (Self, std::io::Error)> { + // close our fd before unlink system call, so that the unlink actually performs the removal + self.close(); + // Try to remove file on disk. + // If it fails, we idempotently closed the fd, but the caller can choose to retry. + match std::fs::remove_file(&self.path) { + Ok(()) => Ok(()), + Err(e) => Err((self, e)), + } + } +} + +impl Drop for VirtualFile { + /// If a VirtualFile is dropped, close the underlying file if it was open. + fn drop(&mut self) { + self.close(); + } } impl Read for VirtualFile {