From b122308bf4cb6221857f280b28bac10d9d1a96ab Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 6 May 2026 16:09:35 -0700 Subject: [PATCH] fix(ec): skip 0-byte residue and verify shard sizes before rebuild (#9340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A previously aborted ec.rebuild leaves 0-byte placeholder files for the shards it was about to write. On the next attempt, findShardFile saw those ghosts as "present", the read loop returned nil on the first n==0, and rebuild claimed success while leaving the volume mounted with empty stripes — manifesting downstream as "too few shards given" or silent corruption. Now: skip 0-byte files when scanning, validate that all surviving shards agree on size, bound the read loop by that size instead of bailing on the first short read, and remove residue after a successful rebuild so it can't shadow real shards next time. --- weed/storage/erasure_coding/ec_encoder.go | 130 ++++++--- .../storage/erasure_coding/ec_rebuild_test.go | 249 ++++++++++++++++++ 2 files changed, 337 insertions(+), 42 deletions(-) create mode 100644 weed/storage/erasure_coding/ec_rebuild_test.go diff --git a/weed/storage/erasure_coding/ec_encoder.go b/weed/storage/erasure_coding/ec_encoder.go index cd4d95fd8..e5b687a89 100644 --- a/weed/storage/erasure_coding/ec_encoder.go +++ b/weed/storage/erasure_coding/ec_encoder.go @@ -13,7 +13,6 @@ import ( "github.com/seaweedfs/seaweedfs/weed/storage/needle_map" "github.com/seaweedfs/seaweedfs/weed/storage/types" "github.com/seaweedfs/seaweedfs/weed/storage/volume_info" - "github.com/seaweedfs/seaweedfs/weed/util" ) const ( @@ -128,19 +127,31 @@ func generateEcFiles(baseFileName string, bufferSize int, largeBlockSize int64, } // findShardFile looks for a shard file at baseFileName+ext, then in additionalDirs. -func findShardFile(baseFileName string, ext string, additionalDirs []string) string { - primary := baseFileName + ext - if util.FileExists(primary) { - return primary - } +// Returns the first non-empty shard file path found. Any 0-byte residue files +// (typically left behind by a previously aborted rebuild) are returned in ghosts +// so the caller can remove them — otherwise they shadow real shards on the next +// rebuild attempt and the volume gets silently mounted with empty stripes. +func findShardFile(baseFileName string, ext string, additionalDirs []string) (shardPath string, ghosts []string) { + candidates := make([]string, 0, 1+len(additionalDirs)) + candidates = append(candidates, baseFileName+ext) baseName := filepath.Base(baseFileName) for _, dir := range additionalDirs { - candidate := filepath.Join(dir, baseName+ext) - if util.FileExists(candidate) { - return candidate + candidates = append(candidates, filepath.Join(dir, baseName+ext)) + } + for _, c := range candidates { + fi, statErr := os.Stat(c) + if statErr != nil { + continue + } + if fi.Size() == 0 { + ghosts = append(ghosts, c) + continue + } + if shardPath == "" { + shardPath = c } } - return "" + return } func generateMissingEcFiles(baseFileName string, bufferSize int, largeBlockSize int64, smallBlockSize int64, ctx *ECContext, additionalDirs []string) (generatedShardIds []uint32, err error) { @@ -150,22 +161,35 @@ func generateMissingEcFiles(baseFileName string, bufferSize int, largeBlockSize shardHasData := make([]bool, ctx.Total()) shardPaths := make([]string, ctx.Total()) // non-empty for present shards inputFiles := make([]*os.File, ctx.Total()) + var ghostPaths []string + expectedShardSize := int64(-1) presentCount := 0 for shardId := 0; shardId < ctx.Total(); shardId++ { ext := ctx.ToExt(shardId) - shardPath := findShardFile(baseFileName, ext, additionalDirs) - if shardPath != "" { - shardHasData[shardId] = true - shardPaths[shardId] = shardPath - inputFiles[shardId], err = os.OpenFile(shardPath, os.O_RDONLY, 0) - if err != nil { - return nil, err - } - defer inputFiles[shardId].Close() - presentCount++ - } else { + shardPath, ghosts := findShardFile(baseFileName, ext, additionalDirs) + ghostPaths = append(ghostPaths, ghosts...) + if shardPath == "" { generatedShardIds = append(generatedShardIds, uint32(shardId)) + continue } + fi, statErr := os.Stat(shardPath) + if statErr != nil { + return nil, fmt.Errorf("stat shard %s: %w", shardPath, statErr) + } + if expectedShardSize < 0 { + expectedShardSize = fi.Size() + } else if fi.Size() != expectedShardSize { + return nil, fmt.Errorf("ec shard size mismatch: %s is %d bytes, expected %d (refusing to rebuild from inconsistent shards)", + shardPath, fi.Size(), expectedShardSize) + } + shardHasData[shardId] = true + shardPaths[shardId] = shardPath + inputFiles[shardId], err = os.OpenFile(shardPath, os.O_RDONLY, 0) + if err != nil { + return nil, err + } + defer inputFiles[shardId].Close() + presentCount++ } // Pre-check: bail out before creating any output files. @@ -174,8 +198,8 @@ func generateMissingEcFiles(baseFileName string, bufferSize int, largeBlockSize baseFileName, presentCount, ctx.DataShards, generatedShardIds) } - glog.V(0).Infof("rebuilding %s: %d shards present, %d missing %v, config %s", - baseFileName, presentCount, len(generatedShardIds), generatedShardIds, ctx.String()) + glog.V(0).Infof("rebuilding %s: %d shards present (size %d), %d missing %v, %d ghost residue %v, config %s", + baseFileName, presentCount, expectedShardSize, len(generatedShardIds), generatedShardIds, len(ghostPaths), ghostPaths, ctx.String()) // Pass 2: create output files for missing shards now that we know // reconstruction is possible. @@ -192,10 +216,32 @@ func generateMissingEcFiles(baseFileName string, bufferSize int, largeBlockSize defer outputFiles[shardId].Close() } - err = rebuildEcFiles(shardHasData, inputFiles, outputFiles, ctx) + err = rebuildEcFiles(shardHasData, inputFiles, outputFiles, ctx, expectedShardSize) if err != nil { return nil, fmt.Errorf("rebuildEcFiles: %w", err) } + + // Reconstruction succeeded — remove any 0-byte residue from prior aborted + // rebuilds so they don't shadow real shards on the next pass and don't get + // mounted as empty stripes. Skip residue paths that the output-file step + // has already overwritten with real data, since those entries in + // ghostPaths refer to the just-rebuilt shard's location. + freshOutputs := make(map[string]struct{}, len(outputFiles)) + for _, f := range outputFiles { + if f != nil { + freshOutputs[f.Name()] = struct{}{} + } + } + for _, ghost := range ghostPaths { + if _, overwritten := freshOutputs[ghost]; overwritten { + continue + } + if removeErr := os.Remove(ghost); removeErr != nil && !os.IsNotExist(removeErr) { + glog.Warningf("failed to remove 0-byte ec shard residue %s: %v", ghost, removeErr) + } else { + glog.V(0).Infof("removed 0-byte ec shard residue %s", ghost) + } + } return } @@ -320,7 +366,7 @@ func encodeDatFile(remainingSize int64, baseFileName string, bufferSize int, lar return nil } -func rebuildEcFiles(shardHasData []bool, inputFiles []*os.File, outputFiles []*os.File, ctx *ECContext) error { +func rebuildEcFiles(shardHasData []bool, inputFiles []*os.File, outputFiles []*os.File, ctx *ECContext, expectedShardSize int64) error { enc, err := ctx.CreateEncoder() if err != nil { @@ -334,23 +380,22 @@ func rebuildEcFiles(shardHasData []bool, inputFiles []*os.File, outputFiles []*o } } - var startOffset int64 - var inputBufferDataSize int - for { + for startOffset := int64(0); startOffset < expectedShardSize; { + chunkSize := int64(ErasureCodingSmallBlockSize) + if remaining := expectedShardSize - startOffset; remaining < chunkSize { + chunkSize = remaining + } // read the input data from files for i := 0; i < ctx.Total(); i++ { if shardHasData[i] { - n, _ := inputFiles[i].ReadAt(buffers[i], startOffset) - if n == 0 { - return nil - } - if inputBufferDataSize == 0 { - inputBufferDataSize = n - } - if inputBufferDataSize != n { - return fmt.Errorf("ec shard size expected %d actual %d", inputBufferDataSize, n) + buf := buffers[i][:chunkSize] + n, readErr := inputFiles[i].ReadAt(buf, startOffset) + if int64(n) != chunkSize { + return fmt.Errorf("short read on shard %s at offset %d: got %d, want %d (err=%v)", + inputFiles[i].Name(), startOffset, n, chunkSize, readErr) } + buffers[i] = buf } else { buffers[i] = nil } @@ -365,15 +410,16 @@ func rebuildEcFiles(shardHasData []bool, inputFiles []*os.File, outputFiles []*o // write the data to output files for i := 0; i < ctx.Total(); i++ { if !shardHasData[i] { - n, _ := outputFiles[i].WriteAt(buffers[i][:inputBufferDataSize], startOffset) - if inputBufferDataSize != n { - return fmt.Errorf("fail to write to %s", outputFiles[i].Name()) + n, writeErr := outputFiles[i].WriteAt(buffers[i][:chunkSize], startOffset) + if int64(n) != chunkSize { + return fmt.Errorf("short write to %s at offset %d: got %d, want %d (err=%v)", + outputFiles[i].Name(), startOffset, n, chunkSize, writeErr) } } } - startOffset += int64(inputBufferDataSize) + startOffset += chunkSize } - + return nil } func readNeedleMap(baseFileName string) (*needle_map.MemDb, error) { diff --git a/weed/storage/erasure_coding/ec_rebuild_test.go b/weed/storage/erasure_coding/ec_rebuild_test.go new file mode 100644 index 000000000..36b60bf4f --- /dev/null +++ b/weed/storage/erasure_coding/ec_rebuild_test.go @@ -0,0 +1,249 @@ +package erasure_coding + +import ( + "bytes" + "crypto/rand" + "os" + "path/filepath" + "sort" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb" + "github.com/seaweedfs/seaweedfs/weed/storage/volume_info" +) + +// TestRebuildEcFiles_BasicTwoMissing covers the simple repair path: 12 of 14 +// shards present, 2 truly missing — must rebuild both and produce shards +// identical to the originals. +func TestRebuildEcFiles_BasicTwoMissing(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "_109") + originalShards := encodeFixture(t, base, 50*1024*1024) + + // Drop shards 1 and 8. + mustRemove(t, base+ToExt(1)) + mustRemove(t, base+ToExt(8)) + + rebuilt, err := RebuildEcFiles(base) + if err != nil { + t.Fatalf("RebuildEcFiles: %v", err) + } + sort.Slice(rebuilt, func(i, j int) bool { return rebuilt[i] < rebuilt[j] }) + if want := []uint32{1, 8}; !equalUint32(rebuilt, want) { + t.Fatalf("rebuilt = %v, want %v", rebuilt, want) + } + + for _, idx := range []int{1, 8} { + got := mustReadFile(t, base+ToExt(idx)) + if !bytes.Equal(got, originalShards[idx]) { + t.Fatalf("shard %d content does not match the original encoding", idx) + } + } +} + +// TestRebuildEcFiles_RejectsZeroByteResidue is the regression for issue #9340. +// +// When a previous ec.rebuild attempt aborts (e.g. crash, network error, +// reedsolomon "too few shards" mid-rebuild), it leaves 0-byte placeholder +// files for the shards it was about to write. On the next attempt, the old +// findShardFile saw those 0-byte ghosts as "present", short-circuited the +// reconstruction loop on the first n==0 read, and returned nil — making +// ec.rebuild claim success while leaving the volume with empty shards. +// +// The fix: skip 0-byte files when scanning, treat the shards as still-missing, +// and remove the residue after a successful rebuild so they don't shadow real +// shards on the next pass. +func TestRebuildEcFiles_RejectsZeroByteResidue(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "_109") + originalShards := encodeFixture(t, base, 50*1024*1024) + + // Simulate the residue from a prior aborted rebuild: shard files for the + // truly-missing shards exist on disk but are 0 bytes. + for _, idx := range []int{1, 8} { + mustRemove(t, base+ToExt(idx)) + mustWriteFile(t, base+ToExt(idx), nil) + } + + rebuilt, err := RebuildEcFiles(base) + if err != nil { + t.Fatalf("RebuildEcFiles: %v", err) + } + sort.Slice(rebuilt, func(i, j int) bool { return rebuilt[i] < rebuilt[j] }) + if want := []uint32{1, 8}; !equalUint32(rebuilt, want) { + t.Fatalf("rebuilt = %v, want %v (the old code reported [] and silently left 0-byte residue)", rebuilt, want) + } + + for _, idx := range []int{1, 8} { + fi, err := os.Stat(base + ToExt(idx)) + if err != nil { + t.Fatalf("stat shard %d: %v", idx, err) + } + if fi.Size() == 0 { + t.Fatalf("shard %d still 0 bytes after rebuild — residue was not cleared", idx) + } + got := mustReadFile(t, base+ToExt(idx)) + if !bytes.Equal(got, originalShards[idx]) { + t.Fatalf("rebuilt shard %d does not match the original encoding", idx) + } + } +} + +// TestRebuildEcFiles_ResidueOnAdditionalDisk reproduces the multi-disk variant +// of the residue bug. The .ecx and the real shards live on the rebuild disk; +// a sibling disk holds 0-byte placeholders for the shards that need to be +// regenerated (e.g. an earlier rebuild crashed while writing them there). The +// pre-fix findShardFile preferred the first existing path it saw — including +// the 0-byte one — so the rebuild silently succeeded with empty stripes. +func TestRebuildEcFiles_ResidueOnAdditionalDisk(t *testing.T) { + root := t.TempDir() + disk1 := filepath.Join(root, "d1") + disk2 := filepath.Join(root, "d2") + mustMkdir(t, disk1) + mustMkdir(t, disk2) + + stagingBase := filepath.Join(root, "_109") + originalShards := encodeFixture(t, stagingBase, 50*1024*1024) + + // Real shards (except 1 and 8) and .vif live on disk1. + disk1Base := filepath.Join(disk1, "_109") + for i := 0; i < TotalShardsCount; i++ { + if i == 1 || i == 8 { + continue + } + mustRename(t, stagingBase+ToExt(i), disk1Base+ToExt(i)) + } + mustRename(t, stagingBase+".vif", disk1Base+".vif") + + // disk2 carries 0-byte residue for the missing shards. + disk2Base := filepath.Join(disk2, "_109") + for _, idx := range []int{1, 8} { + mustWriteFile(t, disk2Base+ToExt(idx), nil) + } + + rebuilt, err := RebuildEcFiles(disk1Base, disk2) + if err != nil { + t.Fatalf("RebuildEcFiles: %v", err) + } + sort.Slice(rebuilt, func(i, j int) bool { return rebuilt[i] < rebuilt[j] }) + if want := []uint32{1, 8}; !equalUint32(rebuilt, want) { + t.Fatalf("rebuilt = %v, want %v", rebuilt, want) + } + + for _, idx := range []int{1, 8} { + got := mustReadFile(t, disk1Base+ToExt(idx)) + if !bytes.Equal(got, originalShards[idx]) { + t.Fatalf("rebuilt shard %d on disk1 does not match the original", idx) + } + // Residue on the sibling disk must be cleaned up so it can't shadow + // the real shard on a future scan. + if _, err := os.Stat(disk2Base + ToExt(idx)); !os.IsNotExist(err) { + t.Fatalf("shard %d residue on disk2 was not removed (err=%v)", idx, err) + } + } +} + +// TestRebuildEcFiles_RejectsMismatchedShardSizes ensures we surface a clear +// error when surviving shards disagree on size, instead of feeding inconsistent +// data into reedsolomon and producing corrupted output. +func TestRebuildEcFiles_RejectsMismatchedShardSizes(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "_109") + encodeFixture(t, base, 50*1024*1024) + + mustRemove(t, base+ToExt(1)) + // Truncate one surviving shard so its size disagrees with the others. + if err := os.Truncate(base+ToExt(2), 1024); err != nil { + t.Fatalf("truncate: %v", err) + } + + if _, err := RebuildEcFiles(base); err == nil { + t.Fatalf("expected size-mismatch error, got nil") + } +} + +// encodeFixture writes a .dat file of the requested size, EC-encodes it, saves +// a matching .vif, and returns a snapshot of every original shard's bytes. +func encodeFixture(t *testing.T, baseFileName string, datSize int64) [TotalShardsCount][]byte { + t.Helper() + + data := make([]byte, datSize) + if _, err := rand.Read(data); err != nil { + t.Fatalf("rand: %v", err) + } + if err := os.WriteFile(baseFileName+".dat", data, 0644); err != nil { + t.Fatalf("write dat: %v", err) + } + if err := WriteEcFiles(baseFileName); err != nil { + t.Fatalf("WriteEcFiles: %v", err) + } + + // A minimal .vif so RebuildEcFiles picks up the right ec config rather + // than warning and falling back to defaults. + vif := &volume_server_pb.VolumeInfo{ + Version: 3, + DatFileSize: datSize, + EcShardConfig: &volume_server_pb.EcShardConfig{ + DataShards: DataShardsCount, + ParityShards: ParityShardsCount, + }, + } + if err := volume_info.SaveVolumeInfo(baseFileName+".vif", vif); err != nil { + t.Fatalf("save vif: %v", err) + } + + var shards [TotalShardsCount][]byte + for i := 0; i < TotalShardsCount; i++ { + shards[i] = mustReadFile(t, baseFileName+ToExt(i)) + } + return shards +} + +func mustRemove(t *testing.T, path string) { + t.Helper() + if err := os.Remove(path); err != nil { + t.Fatalf("remove %s: %v", path, err) + } +} + +func mustWriteFile(t *testing.T, path string, data []byte) { + t.Helper() + if err := os.WriteFile(path, data, 0644); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +func mustReadFile(t *testing.T, path string) []byte { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + return data +} + +func mustRename(t *testing.T, from, to string) { + t.Helper() + if err := os.Rename(from, to); err != nil { + t.Fatalf("rename %s -> %s: %v", from, to, err) + } +} + +func mustMkdir(t *testing.T, dir string) { + t.Helper() + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } +} + +func equalUint32(a, b []uint32) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +}