gsadmin/rustfs
1
0
Fork 0
mirror of https://github.com/rustfs/rustfs.git synced 2026-09-02 02:08:41 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(ecstore): make store-to-disk error narrowing named and fallible (#6626)

Browse Source 
refactor(ecstore): make store-to-disk error narrowing a named fallible operation

Backlog#1845 step 4. The blanket impl From<StorageError> for DiskError let ? silently push store-only errors (locks, buckets, quotas) across the disk boundary into DiskError::other, where the rendered message fragments reduce_errs quorum buckets. Same story for the blanket From<StorageError> for rustfs_filemeta::Error and its other() catch-all.

Both impls are replaced by named, fallible methods: StorageError::narrow_to_disk() and StorageError::narrow_to_filemeta(). Variants with an identity on the far side map across unchanged - including the two documented lossy collapses (SlowDown -> TooManyOpenFiles, StorageFull -> DiskFull) that the round-trip tests pin - and everything else returns Err(self) so the call site decides what crossing the boundary means. Removing the impls let the compiler enumerate every conversion site; the census that scoped this issue had found 5, the compiler found 33.

Call sites keep their existing behavior: the io identity bridge and the generic sites fold Err into the io-backed other() exactly as the old catch-all did (identity still recoverable by downcast), listing paths use one shared to_filemeta_err helper, and the two sites that relied on the SlowDown collapse now construct DiskError::TooManyOpenFiles directly so the loss is visible where it happens. No behavior change intended anywhere; the io::Error bridge itself is untouched by design.

Ref rustfs/backlog#1845
This commit is contained in:
Zhengchao An
2026-08-26 12:32:37 +08:00
committed by GitHub
parent 1590d9107b
commit aa56d4b847
11 changed files with 155 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/ecstore/src/error/conversion_roundtrip_tests.rs
+22 -8
View File
@@ -49,7 +49,7 @@ fn heal_matched_disk_variants() -> Vec<DiskError> {
fn disk_to_storage_to_disk_preserves_heal_matched_variants() {
for disk_err in heal_matched_disk_variants() {
let storage: StorageError = disk_err.clone().into();
let back: DiskError = storage.into();
let back: DiskError = storage.narrow_to_disk().expect("heal-matched variants must narrow");
assert_eq!(back, disk_err, "DiskError → StorageError → DiskError must be identity for {disk_err:?}");
}
}
@@ -63,7 +63,10 @@ fn storage_to_disk_to_storage_preserves_heal_matched_variants() {
StorageError::ErasureWriteQuorum,
];
for storage_err in variants {
let disk: DiskError = storage_err.clone().into();
let disk: DiskError = storage_err
.clone()
.narrow_to_disk()
.expect("heal-matched variants must narrow");
let back: StorageError = disk.into();
assert_eq!(
back, storage_err,
@@ -77,11 +80,11 @@ fn storage_to_disk_to_storage_preserves_heal_matched_variants() {
/// A classifier on the far side of the disk boundary can no longer tell
/// backpressure ("please slow down") apart from fd exhaustion.
///
/// Pinned as-is for backlog#1845; PR4's fallible `narrow_to_disk()` is the
/// planned place to surface this loss explicitly.
/// The fallible `narrow_to_disk()` keeps this collapse as a documented arm;
/// only variants with no disk-layer identity at all narrow to `Err`.
#[test]
fn slowdown_collapses_to_too_many_open_files_across_disk_boundary() {
let disk: DiskError = StorageError::SlowDown.into();
let disk: DiskError = StorageError::SlowDown.narrow_to_disk().expect("SlowDown narrows, lossily");
assert_eq!(disk, DiskError::TooManyOpenFiles);
let back: StorageError = disk.into();
@@ -96,7 +99,9 @@ fn slowdown_collapses_to_too_many_open_files_across_disk_boundary() {
/// and comes back as `DiskFull`.
#[test]
fn storage_full_collapses_to_disk_full_across_disk_boundary() {
let disk: DiskError = StorageError::StorageFull.into();
let disk: DiskError = StorageError::StorageFull
.narrow_to_disk()
.expect("StorageFull narrows, lossily");
assert_eq!(disk, DiskError::DiskFull);
let back: StorageError = disk.into();
@@ -204,7 +209,10 @@ fn storage_to_filemeta_to_storage_preserves_matched_variants() {
StorageError::Unexpected,
];
for storage_err in variants {
let filemeta: rustfs_filemeta::Error = storage_err.clone().into();
let filemeta: rustfs_filemeta::Error = storage_err
.clone()
.narrow_to_filemeta()
.expect("matched variants must narrow");
let back: StorageError = filemeta.into();
assert_eq!(
back, storage_err,
@@ -219,7 +227,13 @@ fn storage_to_filemeta_to_storage_preserves_matched_variants() {
/// recovers identity" property of the by-design bridge stays load-bearing.
#[test]
fn storage_to_filemeta_other_recovers_identity_via_io_bridge() {
let filemeta: rustfs_filemeta::Error = StorageError::SlowDown.into();
// A variant with no filemeta identity narrows to Err; callers that need a
// total conversion fold it into the io-backed other(), and the identity
// bridge recovers the boxed StorageError on the way back.
let refused = StorageError::SlowDown
.narrow_to_filemeta()
.expect_err("SlowDown has no filemeta identity");
let filemeta = rustfs_filemeta::Error::other(refused);
assert!(
matches!(filemeta, rustfs_filemeta::Error::Io(_)),
"unmatched variants fold into the io-backed other()"
crates/ecstore/src/error/mod.rs
+45 -25
View File
@@ -327,9 +327,20 @@ impl From<DiskError> for StorageError {
}
}
impl From<StorageError> for DiskError {
fn from(val: StorageError) -> Self {
match val {
impl StorageError {
/// Narrow a store-layer error to the disk-layer vocabulary.
///
/// This used to be a blanket `impl From<StorageError> for DiskError`, which
/// let `?` silently push store-only errors across the disk boundary into
/// `DiskError::other`, fragmenting `reduce_errs` quorum buckets
/// (backlog#1845). Narrowing is now a named, fallible operation: variants
/// with a disk-layer identity map across (including two documented lossy
/// collapses kept for compatibility — `SlowDown` → `TooManyOpenFiles` and
/// `StorageFull` → `DiskFull`, pinned by conversion_roundtrip_tests), and
/// everything else comes back as `Err(self)` so the caller decides what
/// crossing the boundary means for it.
pub fn narrow_to_disk(self) -> core::result::Result<DiskError, StorageError> {
Ok(match self {
StorageError::Io(io_error) => io_error.into(),
StorageError::Unexpected => DiskError::Unexpected,
StorageError::FileNotFound => DiskError::FileNotFound,
@@ -375,8 +386,26 @@ impl From<StorageError> for DiskError {
StorageError::VolumeAccessDenied => DiskError::VolumeAccessDenied,
StorageError::FileAccessDenied => DiskError::FileAccessDenied,
StorageError::RemoteClientUnavailable(detail) => DiskError::RemoteClientUnavailable(detail),
_ => DiskError::other(val),
}
val => return Err(val),
})
}
/// Same contract as [`StorageError::narrow_to_disk`], for the
/// `rustfs_filemeta::Error` vocabulary (formerly a blanket `From` impl
/// with an `other()` catch-all). No production path currently narrows in
/// this direction; the named form keeps future callers deliberate.
pub fn narrow_to_filemeta(self) -> core::result::Result<rustfs_filemeta::Error, StorageError> {
Ok(match self {
StorageError::Unexpected => rustfs_filemeta::Error::Unexpected,
StorageError::FileNotFound => rustfs_filemeta::Error::FileNotFound,
StorageError::FileVersionNotFound => rustfs_filemeta::Error::FileVersionNotFound,
StorageError::FileCorrupt => rustfs_filemeta::Error::FileCorrupt,
StorageError::DoneForNow => rustfs_filemeta::Error::DoneForNow,
StorageError::MethodNotAllowed => rustfs_filemeta::Error::MethodNotAllowed,
StorageError::VolumeNotFound => rustfs_filemeta::Error::VolumeNotFound,
StorageError::Io(io_error) => io_error.into(),
val => return Err(val),
})
}
}
@@ -433,22 +462,6 @@ impl From<rustfs_filemeta::Error> for StorageError {
}
}
impl From<StorageError> for rustfs_filemeta::Error {
fn from(val: StorageError) -> Self {
match val {
StorageError::Unexpected => rustfs_filemeta::Error::Unexpected,
StorageError::FileNotFound => rustfs_filemeta::Error::FileNotFound,
StorageError::FileVersionNotFound => rustfs_filemeta::Error::FileVersionNotFound,
StorageError::FileCorrupt => rustfs_filemeta::Error::FileCorrupt,
StorageError::DoneForNow => rustfs_filemeta::Error::DoneForNow,
StorageError::MethodNotAllowed => rustfs_filemeta::Error::MethodNotAllowed,
StorageError::VolumeNotFound => rustfs_filemeta::Error::VolumeNotFound,
StorageError::Io(io_error) => io_error.into(),
_ => rustfs_filemeta::Error::other(val),
}
}
}
impl PartialEq for StorageError {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
@@ -1438,7 +1451,9 @@ mod tests {
for original in all_variants {
let storage_error: StorageError = original.clone().into();
let round_tripped: DiskError = storage_error.into();
let round_tripped: DiskError = storage_error
.narrow_to_disk()
.expect("every disk-representable StorageError must narrow back");
assert_eq!(
std::mem::discriminant(&original),
@@ -1452,7 +1467,7 @@ mod tests {
// message must both survive the round trip.
let io_original = DiskError::Io(IoError::new(ErrorKind::PermissionDenied, "denied"));
let storage_error: StorageError = io_original.clone().into();
let io_round_tripped: DiskError = storage_error.into();
let io_round_tripped: DiskError = storage_error.narrow_to_disk().expect("Io narrows through the bridge");
assert_eq!(io_original, io_round_tripped);
match io_round_tripped {
DiskError::Io(inner) => {
@@ -1700,8 +1715,13 @@ mod tests {
assert_eq!(converted_storage_error, expected_storage_error);
// Test reverse conversion
let converted_back: rustfs_filemeta::Error = converted_storage_error.into();
assert_eq!(converted_back, expected_storage_error.into());
let converted_back: rustfs_filemeta::Error = converted_storage_error
.narrow_to_filemeta()
.expect("matched variants must narrow to filemeta");
let expected_back: rustfs_filemeta::Error = expected_storage_error
.narrow_to_filemeta()
.expect("matched variants must narrow to filemeta");
assert_eq!(converted_back, expected_back);
}
}