From 0ce0388fc0641ea6174a66c70b9350097feef96f Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Tue, 7 Jul 2026 11:47:43 +0800 Subject: [PATCH] fix(ecstore): fsync inline rename_data rollback backup (#4346) * fix(ecstore): fsync inline rename_data rollback backup The inline branch of LocalDisk::rename_data writes the previous version's xl.meta rollback backup (/xl.meta.bkp) with a bare std::fs::write, fsyncing neither the file nor its new directory entry, even when drive_sync_enabled() is true. The same closure already syncs the new xl.meta and the commit rename directory, and the non-inline branch persists its backup durably. That backup is the sole restore source for delete_version(undo_write=true) on a set-level write-quorum failure. With sync enabled, an inline overwrite plus power loss plus a quorum failure that triggers undo_write could restore a lost or truncated backup and fail to roll back to the prior committed object. Write the inline backup via OpenOptions + write_all and, when sync is enabled, sync_data() it and fsync_dir_std its parent, mirroring the non-inline branch. Extend the inline backup test to assert the .bkp contents equal the previous metadata bytes. Refs backlog#868 (disk-durability-01). * fix(ecstore): preserve to_file_error mapping on inline backup write Address review: the rewritten inline rollback-backup write must keep the same DiskError classification as the previous std::fs::write(...).map_err( to_file_error)? call. Without it, io errors (PermissionDenied/NotFound/ StorageFull/...) would surface as DiskError::Io(_) instead of the specific DiskError variants (FileAccessDenied/FileNotFound/DiskFull/...), since From for DiskError only recovers variants that were wrapped via to_file_error. Map open/write_all/sync_data/fsync_dir_std through to_file_error, matching write_all_internal. --------- Co-authored-by: houseme --- crates/ecstore/src/disk/local.rs | 40 +++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index a0a8c50d3..8aeaa8232 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -3869,10 +3869,29 @@ impl DiskAPI for LocalDisk { && let Some(dst_parent) = dst.parent() { let old_path = dst_parent.join(old_dir.to_string()).join(STORAGE_FORMAT_FILE_BACKUP); - if let Some(old_parent) = old_path.parent() { + let old_parent = old_path.parent().map(|p| p.to_path_buf()); + if let Some(ref old_parent) = old_parent { std::fs::create_dir_all(old_parent)?; } - std::fs::write(&old_path, buf).map_err(to_file_error)?; + // This rollback backup is the sole restore source for a later + // undo_write when the set-level write quorum fails. Persist it as + // durably as the new xl.meta written above (and as the non-inline + // branch does): a bare std::fs::write leaves both the bytes and the + // new directory entry in the page cache, so a crash before a + // rollback could restore a lost or truncated backup. + let mut backup = std::fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(true) + .open(&old_path) + .map_err(to_file_error)?; + std::io::Write::write_all(&mut backup, buf).map_err(to_file_error)?; + if sync { + backup.sync_data().map_err(to_file_error)?; + if let Some(ref old_parent) = old_parent { + os::fsync_dir_std(old_parent).map_err(to_file_error)?; + } + } } match std::fs::rename(&src, &dst) { @@ -4581,11 +4600,12 @@ mod test { ensure_test_volume(&disk, RUSTFS_META_TMP_BUCKET).await; let old_fi = test_file_info(object, version_id, Some(old_data_dir), None); + let old_meta = test_meta(old_fi); let dst_object_dir = dir.path().join(bucket).join(object); fs::create_dir_all(dst_object_dir.join(old_data_dir.to_string())) .await .expect("old data dir should be created"); - fs::write(dst_object_dir.join(STORAGE_FORMAT_FILE), test_meta(old_fi)) + fs::write(dst_object_dir.join(STORAGE_FORMAT_FILE), &old_meta) .await .expect("old metadata should be written"); @@ -4601,11 +4621,15 @@ mod test { .expect("inline rename_data should commit"); assert_eq!(resp.old_data_dir, Some(old_data_dir)); - assert!( - dst_object_dir - .join(old_data_dir.to_string()) - .join(STORAGE_FORMAT_FILE_BACKUP) - .exists() + let backup_path = dst_object_dir.join(old_data_dir.to_string()).join(STORAGE_FORMAT_FILE_BACKUP); + assert!(backup_path.exists()); + // The rollback backup must contain the previous metadata bytes verbatim so + // that undo_write can restore the prior committed object; guards the inline + // backup write against truncation/corruption regressions. + assert_eq!( + fs::read(&backup_path).await.expect("backup should be readable"), + old_meta, + "inline rollback backup must contain the previous metadata bytes verbatim" ); assert!( !dst_object_dir