From 9bc216b9eb5bccb9cbd0b842afdb93df9b705bf2 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 30 Jun 2026 19:48:13 -0700 Subject: [PATCH] style(ec): align bitrot comments and test setup for merge-cleanliness Match the shared bitrot code (write_ec_files, encode_one_batch, checksum_scrub, the encode sidecar test) to the canonical wording/layout so the volume-server Rust port stays line-aligned across trees, keeping periodic merges conflict-free. No behavior change. Claude-Session: https://claude.ai/code/session_015EE9Sc9EvNp8BCVva4RKdo --- .../src/storage/erasure_coding/ec_encoder.rs | 40 ++++++++++----- .../src/storage/erasure_coding/ec_volume.rs | 51 ++++++++++++++----- 2 files changed, 63 insertions(+), 28 deletions(-) diff --git a/seaweed-volume/src/storage/erasure_coding/ec_encoder.rs b/seaweed-volume/src/storage/erasure_coding/ec_encoder.rs index db50707e6..4461e0fd8 100644 --- a/seaweed-volume/src/storage/erasure_coding/ec_encoder.rs +++ b/seaweed-volume/src/storage/erasure_coding/ec_encoder.rs @@ -10,7 +10,9 @@ use std::io::{Read, Seek, SeekFrom}; use reed_solomon_erasure::galois_8::ReedSolomon; -use crate::pb::volume_server_pb::{ChecksumAlgorithm, EcBitrotProtection, EcShardChecksums}; +use crate::pb::volume_server_pb::{ + ChecksumAlgorithm, EcBitrotProtection, EcShardChecksums, +}; use crate::storage::erasure_coding::ec_bitrot::{ self, ShardChecksumBuilder, DEFAULT_BITROT_BLOCK_SIZE, }; @@ -82,9 +84,7 @@ pub fn write_ec_files( // Write the generation-0 bitrot sidecar (`.ecsum`). Finalizing each // builder yields covered_size (== total bytes written to that shard) and - // the packed little-endian CRC32C array. Best-effort: a failed sidecar must - // not fail the encode — the shards are already written and valid; the volume - // simply runs with bitrot protection off for this generation. + // the packed little-endian CRC32C array. let mut shard_checksums: Vec = Vec::with_capacity(total_shards); for (i, builder) in builders.into_iter().enumerate() { let (covered_size, packed) = builder.finalize(); @@ -107,6 +107,9 @@ pub fn write_ec_files( }; let sidecar_path = ec_bitrot::bitrot_sidecar_path(&base, 0); if let Err(e) = ec_bitrot::save_bitrot_sidecar(&sidecar_path, &prot) { + // A failed sidecar must not fail the encode — the shards are already + // written and valid. The volume simply runs with bitrot protection + // off for this generation until the sidecar is regenerated. tracing::warn!( volume_id = volume_id.0, path = %sidecar_path, @@ -695,8 +698,8 @@ fn encode_one_batch( ) })?; - // Write all shard buffers to files, feeding each shard's bytes into its - // bitrot checksum builder so covered_size == the on-disk shard length. + // Write all shard buffers to files and feed the same bytes to each + // shard's bitrot checksum builder, keeping covered_size == on-disk length. for (i, buf) in buffers.iter().enumerate() { shards[i].write_all(buf)?; builders[i].write(buf); @@ -767,10 +770,7 @@ mod tests { assert!(std::path::Path::new(&ecx_path).exists()); } - /// Encode-time capture writes a valid generation-0 `.ecsum` sidecar whose - /// recorded checksums match the actual on-disk shards. - #[test] - fn test_encode_writes_valid_bitrot_sidecar() { + fn make_volume_with_needles(n: u64) -> TempDir { let tmp = TempDir::new().unwrap(); let dir = tmp.path().to_str().unwrap(); let mut v = Volume::new( @@ -785,20 +785,32 @@ mod tests { Version::current(), ) .unwrap(); - for i in 1..=5 { - let data = format!("test data for needle {}", i); - let mut n = Needle { + for i in 1..=n { + // Larger payloads so encoded shards span multiple bitrot blocks + // would require huge data; small payloads are fine for correctness. + let data = format!("test data for needle {} {}", i, "x".repeat(64)); + let mut needle = Needle { id: NeedleId(i), cookie: Cookie(i as u32), data: data.as_bytes().to_vec(), data_size: data.len() as u32, ..Needle::default() }; - v.write_needle(&mut n, true).unwrap(); + v.write_needle(&mut needle, true).unwrap(); } v.sync_to_disk().unwrap(); v.close(); + tmp + } + /// Encode-time capture writes a valid generation-0 `.ecsum` sidecar whose + /// recorded checksums match the actual on-disk shards. + #[test] + fn test_encode_writes_valid_bitrot_sidecar() { + use crate::storage::erasure_coding::ec_bitrot; + + let tmp = make_volume_with_needles(5); + let dir = tmp.path().to_str().unwrap(); write_ec_files(dir, dir, "", VolumeId(1), 10, 4).unwrap(); let base = format!("{}/1", dir); diff --git a/seaweed-volume/src/storage/erasure_coding/ec_volume.rs b/seaweed-volume/src/storage/erasure_coding/ec_volume.rs index 3a1c95c42..10df2040b 100644 --- a/seaweed-volume/src/storage/erasure_coding/ec_volume.rs +++ b/seaweed-volume/src/storage/erasure_coding/ec_volume.rs @@ -305,23 +305,40 @@ impl EcVolume { (self.bitrot.clone(), self.bitrot_status) } - /// Verify every locally-held EC shard's raw bytes against the active-generation - /// bitrot checksum sidecar. Read-only and purely diagnostic: it detects and - /// reports corruption but never mutates or quarantines anything. It is the only - /// path that exercises cold parity shards, never read during normal serving. + /// Read-only EC bitrot checksum scrub of the LOCAL shards of this volume's + /// active generation. /// - /// Returns (blocks scanned, mismatched shard ids, errors). An absent/ - /// generation-mismatched sidecar is `Off` — a clean, empty no-op — so - /// unprotected volumes are never reported broken. If more shards mismatch - /// wholesale than parity can mask, the sidecar itself is the likely culprit - /// (stale/wrong): the shard-corruption verdict is suppressed and an - /// integrity note is added instead. Mirrors Go's `ChecksumScrub`. + /// Loads the active-generation `.ecsum` sidecar and, for each locally-held + /// shard, reads the on-disk shard file in `block_size` chunks and compares + /// each block's CRC32C against the sidecar. Returns + /// `(blocks_scanned, mismatched_shards, errors)`. + /// + /// If more than `parity_shards` shards mismatch wholesale (i.e. every block + /// of those shards is wrong — the signature of a stale/wrong sidecar rather + /// than localized disk rot), the result is classified as a suspect sidecar: + /// `mismatched_shards` is cleared and an integrity note is added to `errors` + /// instead. A genuine multi-shard disk failure of that magnitude is + /// already unrecoverable, so treating it as a sidecar-integrity issue avoids + /// raising false shard-corruption alarms. + /// + /// This method NEVER deletes or mutates anything — it is purely diagnostic. pub fn checksum_scrub(&self) -> (u64, Vec, Vec) { use crate::storage::erasure_coding::ec_bitrot; use crate::storage::erasure_coding::ec_bitrot::BitrotStatus; let mut errors: Vec = Vec::new(); + // Resolve the active-generation protection AND its status, mirroring + // Go's `ChecksumScrub` (`prot, status := ecv.BitrotProtection()`): + // - BitrotOff => sidecars are OPTIONAL; an absent (or generation/ + // config-mismatched) sidecar simply means protection is not enabled + // for this generation. Return a CLEAN, EMPTY result — NOT an error — + // so legacy/intentionally-unprotected volumes are never reported + // broken. (Go: `case BitrotOff: return 0, nil, nil`.) + // - BitrotInvalid => the sidecar is PRESENT but malformed/unverifiable + // (self-integrity or manifest failure). That is the only status that + // yields an integrity error here. + // - BitrotOn => scan local shards against it. let prot = match self.bitrot_protection() { (_, BitrotStatus::Off) => { // Unprotected generation: nothing to verify. Not an error. @@ -339,7 +356,8 @@ impl EcVolume { } (Some(p), BitrotStatus::On) => p, (None, BitrotStatus::On) => { - // Unreachable: BitrotOn always carries a loaded sidecar. + // Unreachable: BitrotOn always carries a loaded sidecar. Treat a + // missing payload defensively as protection off (clean no-op). return (0, Vec::new(), Vec::new()); } }; @@ -350,6 +368,8 @@ impl EcVolume { let mut blocks_scanned: u64 = 0; let mut mismatched_shards: Vec = Vec::new(); + // Track shards whose blocks ALL mismatch (wholesale) to detect a + // stale/wrong sidecar. let mut wholesale_mismatch = 0usize; for (i, slot) in self.shards.iter().enumerate() { @@ -365,6 +385,8 @@ impl EcVolume { continue; }; + // Resolve the on-disk shard file path for the active generation, + // mirroring EcVolumeShard::reopen_against_generation's convention. let path = if generation == 0 { format!("{}.ec{:02}", base, shard_id) } else { @@ -391,9 +413,10 @@ impl EcVolume { } } - // More wholesale mismatches than parity can mask => the sidecar itself is - // suspect (stale generation / wrong volume): suppress the shard-corruption - // verdict and flag a sidecar-integrity issue instead. + // If more shards mismatch wholesale than parity can mask, the sidecar + // itself is the likely culprit (stale generation / wrong volume), so + // suppress the shard-corruption verdict and flag a sidecar-integrity + // issue instead. if wholesale_mismatch > self.parity_shards as usize { errors.push(format!( "EC volume {}: {} shards mismatch wholesale (> {} parity); suspect stale/wrong sidecar, not shard corruption",