-
Notifications
You must be signed in to change notification settings - Fork 979
fix: gracefully handle out of disk space failures (#3425) #3908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,11 +68,46 @@ pub fn u64_to_key(prefix: u8, val: u64) -> Vec<u8> { | |
| pub use crate::lmdb::*; | ||
|
|
||
| use std::ffi::OsStr; | ||
| use std::fs::{remove_file, rename, File}; | ||
| use std::fs::{remove_file, rename, File, OpenOptions}; | ||
| use std::path::Path; | ||
|
|
||
| /// Returns true if the I/O error indicates the filesystem is out of space. | ||
| /// | ||
| /// Covers `ErrorKind::StorageFull` and platform-specific `ENOSPC` (28 on most Unix). | ||
| pub fn is_out_of_disk_space(err: &io::Error) -> bool { | ||
| if err.kind() == io::ErrorKind::StorageFull { | ||
| return true; | ||
| } | ||
| // ENOSPC on Unix; also match common string forms from lower layers. | ||
| if err.raw_os_error() == Some(28) { | ||
| return true; | ||
| } | ||
| let msg = err.to_string().to_lowercase(); | ||
| msg.contains("no space left") || msg.contains("disk full") || msg.contains("not enough space") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can no handle all possible messages here, so need to check disc space exactly to decide if it was a reason |
||
| } | ||
|
|
||
| /// Map an I/O error into a clear StorageFull error when out of disk space. | ||
| pub fn map_io_err(err: io::Error) -> io::Error { | ||
| if is_out_of_disk_space(&err) { | ||
| io::Error::new( | ||
| io::ErrorKind::StorageFull, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. map_io_err can wrap an existing StorageFull error repeatedly, so the final message may contain the same guidance several times. Could we keep it unchanged once it is already mapped? |
||
| format!( | ||
| "out of disk space: {}. Free disk space and restart; avoid wiping chain data unless recovery fails.", | ||
| err | ||
| ), | ||
| ) | ||
| } else { | ||
| err | ||
| } | ||
| } | ||
|
|
||
| /// Creates temporary file with name created by adding `temp_suffix` to `path`. | ||
| /// Applies writer function to it and renames temporary file into original specified by `path`. | ||
| /// | ||
| /// Durability notes (see also #3352 / #3425): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| /// - Writer errors leave the original file untouched (temp is removed). | ||
| /// - Temp file is fsync'd before rename. | ||
| /// - Parent directory is fsync'd after rename so the rename itself is durable. | ||
| pub fn save_via_temp_file<F, P, E>(path: P, temp_suffix: E, mut writer: F) -> Result<(), io::Error> | ||
| where | ||
| F: FnMut(&mut File) -> Result<(), io::Error>, | ||
|
|
@@ -83,23 +118,40 @@ where | |
| assert!(!temp_suffix.is_empty()); | ||
|
|
||
| let original = path.as_ref(); | ||
| let mut _original = original.as_os_str().to_os_string(); | ||
| _original.push(temp_suffix); | ||
| // Write temporary file | ||
| let temp_path = Path::new(&_original); | ||
| let mut temp_os = original.as_os_str().to_os_string(); | ||
| temp_os.push(temp_suffix); | ||
| let temp_path = Path::new(&temp_os); | ||
|
|
||
| if temp_path.exists() { | ||
| remove_file(&temp_path)?; | ||
| } | ||
|
|
||
| let mut temp_file = File::create(&temp_path)?; | ||
|
|
||
| // write the new data to the temp file | ||
| writer(&mut temp_file)?; | ||
|
|
||
| // force an fsync on the temp file to ensure bytes are on disk | ||
| temp_file.sync_all()?; | ||
|
|
||
| rename(&temp_path, &original)?; | ||
| let write_result = (|| { | ||
| let mut temp_file = File::create(&temp_path)?; | ||
| writer(&mut temp_file).map_err(map_io_err)?; | ||
| // force an fsync on the temp file to ensure bytes are on disk | ||
| temp_file.sync_all().map_err(map_io_err)?; | ||
| // drop file handle before rename (important on Windows) | ||
| drop(temp_file); | ||
| rename(&temp_path, &original).map_err(map_io_err)?; | ||
|
|
||
| // Fsync the parent directory so the rename is durable. | ||
| if let Some(parent) = original.parent() { | ||
| // On some platforms (e.g. certain Windows configs) directory sync | ||
| // may not be supported; treat that as best-effort. | ||
| if let Ok(dir) = OpenOptions::new().read(true).open(parent) { | ||
| let _ = dir.sync_all(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment promises a durable rename, but parent directory open and sync errors are ignored. Could we handle real I/O failures explicitly, or document this as best effort? |
||
| } | ||
| } | ||
| Ok(()) | ||
| })(); | ||
|
|
||
| if let Err(e) = write_result { | ||
| // Best-effort cleanup of the temp file so a failed write cannot | ||
| // leave a partial .tmp that confuses a later retry. | ||
| let _ = remove_file(&temp_path); | ||
| return Err(e); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,11 @@ pub enum Error { | |
| /// Wraps an error originating from LMDB | ||
| #[error("LMDB error: {0}")] | ||
| LmdbErr(String), | ||
| /// Filesystem is out of space (ENOSPC / StorageFull) | ||
| #[error( | ||
| "Out of disk space: {0}. Free disk space and restart; do not wipe chain data unless recovery fails." | ||
| )] | ||
| DiskFull(String), | ||
| /// Wraps a serialization error for Writeable or Readable | ||
| #[error("Serialization Error: {0}")] | ||
| SerErr(ser::Error), | ||
|
|
@@ -62,7 +67,29 @@ pub enum Error { | |
|
|
||
| impl From<heed::Error> for Error { | ||
| fn from(e: heed::Error) -> Error { | ||
| Error::LmdbErr(e.to_string()) | ||
| let msg = e.to_string(); | ||
| // heed/lmdb may surface ENOSPC as a map full / I/O error string. | ||
| let lower = msg.to_lowercase(); | ||
| if lower.contains("map full") | ||
| || lower.contains("mdb_map_full") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| || lower.contains("no space") | ||
| || lower.contains("enospc") | ||
| || lower.contains("disk full") | ||
| { | ||
| Error::DiskFull(msg) | ||
| } else { | ||
| Error::LmdbErr(msg) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<std::io::Error> for Error { | ||
| fn from(e: std::io::Error) -> Error { | ||
| if crate::is_out_of_disk_space(&e) { | ||
| Error::DiskFull(e.to_string()) | ||
| } else { | ||
| Error::FileErr(e.to_string()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -372,16 +372,24 @@ impl<T: PMMRable> PMMRBackend<T> { | |
|
|
||
| /// Syncs all files to disk. A call to sync is required to ensure all the | ||
| /// data has been successfully written to disk. | ||
| /// | ||
| /// On failure the error is mapped to a clear out-of-disk-space message when | ||
| /// appropriate (#3425). Callers should discard the extension and not commit | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same #3425 rationale is repeated across the chain, PMMR, file backend, and tests. Could we keep the background in the PR description and leave only comments that explain a non obvious invariant? |
||
| /// the corresponding DB batch. | ||
| pub fn sync(&mut self) -> io::Result<()> { | ||
| Ok(()) | ||
| .and(self.hash_file.flush()) | ||
| .and(self.data_file.flush()) | ||
| .and(self.sync_leaf_set()) | ||
| .and(self.prune_list.flush()) | ||
| self.hash_file | ||
| .flush() | ||
| .and_then(|_| self.data_file.flush()) | ||
| .and_then(|_| self.sync_leaf_set()) | ||
| .and_then(|_| self.prune_list.flush()) | ||
| .map_err(|e| { | ||
| let e = crate::map_io_err(e); | ||
| if crate::is_out_of_disk_space(&e) { | ||
| error!("PMMR sync failed — out of disk space: {}", e); | ||
| } | ||
| io::Error::new( | ||
| io::ErrorKind::Interrupted, | ||
| format!("Could not sync pmmr to disk: {:?}", e), | ||
| e.kind(), | ||
| format!("Could not sync pmmr to disk: {}", e), | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -353,10 +353,14 @@ where | |
|
|
||
| /// Syncs all writes (fsync), reallocating the memory map to make the newly | ||
| /// written data accessible. | ||
| /// | ||
| /// On failure (including out-of-disk-space), the in-memory buffer and rewind | ||
| /// backup are preserved so the caller can `discard()` the extension rather | ||
| /// than treating a half-written file as committed. See #3425. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same #3425 |
||
| pub fn flush(&mut self) -> io::Result<()> { | ||
| if let SizeInfo::VariableSize(ref mut size_file) = &mut self.size_info { | ||
| // Flush the associated size_file if we have one. | ||
| size_file.flush()? | ||
| size_file.flush().map_err(crate::map_io_err)? | ||
| } | ||
|
|
||
| if self.buffer_start_pos_bak > 0 { | ||
|
|
@@ -369,14 +373,16 @@ where | |
| .read(true) | ||
| .create(true) | ||
| .write(true) | ||
| .open(&self.path)?; | ||
| .open(&self.path) | ||
| .map_err(crate::map_io_err)?; | ||
|
|
||
| // Set length of the file to truncate it as necessary. | ||
| if self.buffer_start_pos == 0 { | ||
| file.set_len(0)?; | ||
| file.set_len(0).map_err(crate::map_io_err)?; | ||
| } else { | ||
| let (offset, size) = self.offset_and_size(self.buffer_start_pos - 1)?; | ||
| file.set_len(offset + size as u64)?; | ||
| file.set_len(offset + size as u64) | ||
| .map_err(crate::map_io_err)?; | ||
| }; | ||
| } | ||
| } | ||
|
|
@@ -386,14 +392,39 @@ where | |
| .read(true) | ||
| .create(true) | ||
| .append(true) | ||
| .open(&self.path)?; | ||
| .open(&self.path) | ||
| .map_err(crate::map_io_err)?; | ||
| self.file = Some(file); | ||
| self.buffer_start_pos_bak = 0; | ||
| // Intentionally do *not* clear buffer_start_pos_bak until write+sync | ||
| // succeed, so discard()/retry still know we were in a rewound state. | ||
| } | ||
|
|
||
| self.file.as_mut().unwrap().write_all(&self.buffer[..])?; | ||
| self.file.as_mut().unwrap().sync_all()?; | ||
| let file = self | ||
| .file | ||
| .as_mut() | ||
| .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "append-only file not open"))?; | ||
|
|
||
| if let Err(e) = file.write_all(&self.buffer[..]).and_then(|_| file.sync_all()) { | ||
| let e = crate::map_io_err(e); | ||
| if crate::is_out_of_disk_space(&e) { | ||
| error!( | ||
| "Out of disk space writing {}: free space and restart. \ | ||
| In-memory changes were not committed.", | ||
| self.path.display() | ||
| ); | ||
| } else { | ||
| error!( | ||
| "Failed writing append-only file {}: {}. In-memory changes were not committed.", | ||
| self.path.display(), | ||
| e | ||
| ); | ||
| } | ||
| // Keep buffer + buffer_start_pos_bak so discard() can roll back cleanly. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discard() only restores the in memory state here. set_len() may already have truncated the old tail, and write_all() may have written part of the new buffer. Could we make the original file state recoverable and test this partial write and restart path? |
||
| return Err(e); | ||
| } | ||
|
|
||
| // Write durable — only now clear rewind backup and buffer. | ||
| self.buffer_start_pos_bak = 0; | ||
| self.buffer.clear(); | ||
| self.buffer_start_pos = self.size_in_elmts()?; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These syncs are not atomic. If output succeeds and rangeproof, kernel, or the later DB commit fails, discard() cannot undo the already flushed output. Could we add a real recovery and reopen path and fault injection tests at each commit boundary?