From 49f62df2cf249cc01d3f9a55395731b1680ec5e1 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 20 Apr 2026 17:55:20 -0700 Subject: [PATCH] fix(shell): mergeVolumes hard-link safety and cleaner cleanup logging (#9163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(shell): skip hard-linked entries in fs.mergeVolumes Hard-linked entries share a chunk list with their siblings, but the filer's UpdateEntry only rewrites one entry at a time. Moving a chunk here leaves every other hard-linked sibling pointing at a fid that either gets deleted by the filer's own garbage step after UpdateEntry or by deleteMovedSourceNeedles (#9160) — either way, the siblings end up with dangling references. Skip the entry with a visible log line so operators know the file was bypassed and can handle it explicitly (copy-then-unlink, or dedup before merge). Detected via entry.HardLinkId being non-empty, which is the same signal the filer itself uses (weed/pb/filer_pb/filer.pb.go:451). Flagged by coderabbit on #9160 post-merge. * fix(shell): mergeVolumes suppresses 404 alongside 304 in source cleanup BatchDelete also returns StatusNotFound (404) for an already-deleted needle when ReadVolumeNeedle can't find it in the cookie-check path, not only StatusNotModified (304) from DeleteVolumeNeedle returning size 0. Both are benign races against a concurrent fsck purge or a replica that already reconciled, so don't clutter the output with "delete ... not found" warnings for them. Flagged by coderabbit on #9160 post-merge. * fix(shell): mergeVolumes merges hard-linked files via dedup on HardLinkId Hard-linked siblings share one chunk list through a KV blob keyed by HardLinkId (see weed/filer/filerstore_hardlink.go). UpdateEntry's setHardLink rewrites that blob and maybeReadHardLink overrides per-entry chunks with the blob's on every read, so a single UpdateEntry propagates new fids to every sibling automatically — the previous skip-hardlinks bailout was overly conservative and left hard-linked files stuck on merge-source volumes forever. Process each HardLinkId exactly once per run with a sync.Map so BFS workers in different directories synchronize without a global lock. First sibling carries the chunk move + UpdateEntry; later siblings find the id in the map and return — preventing the real race, which is two siblings trying to re-download an already-moved source needle or double-queue the same fid for deletion. Also address the log-spam review on deleteMovedSourceNeedles: an unreachable volume server returns one error per needle, so collapse multiple failures into a single per-server line with the first error as an example. --- weed/shell/command_fs_merge_volumes.go | 61 ++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/weed/shell/command_fs_merge_volumes.go b/weed/shell/command_fs_merge_volumes.go index 996ffecb2..b1542b2d8 100644 --- a/weed/shell/command_fs_merge_volumes.go +++ b/weed/shell/command_fs_merge_volumes.go @@ -9,6 +9,7 @@ import ( "net/http" "sort" "strings" + "sync" "time" "slices" @@ -141,12 +142,34 @@ func (c *commandFsMergeVolumes) Do(args []string, commandEnv *CommandEnv, writer lookupFn := filer.LookupFn(commandEnv) + // Hard-linked siblings share ONE chunk list via a KV blob keyed by + // HardLinkId (see weed/filer/filerstore_hardlink.go): UpdateEntry's + // setHardLink() rewrites that blob, and every sibling read goes through + // maybeReadHardLink() which overrides the per-entry chunks with the + // blob's. So moving a chunk and calling UpdateEntry on one sibling + // propagates the new fids to every other sibling automatically — + // provided we do it exactly once per HardLinkId. Processing every + // sibling would race: the first succeeds, the next would either + // re-download an already-moved (and possibly already-deleted) source + // needle or double-queue the same fid for deletion. Track the ids we + // have already handled so BFS workers in different directories can + // synchronize without a global lock. + var processedHardLinks sync.Map + return commandEnv.WithFilerClient(false, func(filerClient filer_pb.SeaweedFilerClient) error { return filer_pb.TraverseBfs(context.Background(), commandEnv, util.FullPath(dir), func(parentPath util.FullPath, entry *filer_pb.Entry) error { if entry.IsDirectory { return nil } entryPath := parentPath.Child(entry.Name) + if len(entry.HardLinkId) > 0 { + if _, seen := processedHardLinks.LoadOrStore(string(entry.HardLinkId), struct{}{}); seen { + // Another sibling already carried the HardLinkId through + // the move + UpdateEntry path; the shared KV blob has the + // new fids, so this sibling is already correct on read. + return nil + } + } entryChanged := false // Every successful moveChunk or rewriteManifestChunk leaves the old // needle sitting on its source volume as a silent orphan — until @@ -245,17 +268,37 @@ func (c *commandFsMergeVolumes) deleteMovedSourceNeedles(commandEnv *CommandEnv, } for _, loc := range locations { results := operation.DeleteFileIdsAtOneVolumeServer(loc.ServerAddress(), commandEnv.option.GrpcDialOption, fids, false) + // Summarize per server: an unreachable volume server returns one + // error per needle, which for manifest-heavy files can mean + // hundreds of near-identical lines. Keep the first error as the + // example and report a single line with the total count. + var firstErr, firstFid string + errCount := 0 for _, r := range results { - // StatusNotModified means the needle was already deleted - // (e.g. a concurrent fsck purge or a replica that had - // already reconciled). That's the desired end state, so - // don't warn about it. Cast to int because r.Status is - // an int32 protobuf field and linters flag the mixed-type - // compare even though Go's untyped-constant rules make it - // valid. - if r.Error != "" && int(r.Status) != http.StatusNotModified { - fmt.Printf("source cleanup %s: delete %s on %v: %s\n", entryPath, r.FileId, loc.ServerAddress(), r.Error) + // StatusNotModified (304) means DeleteVolumeNeedle returned + // size 0 — the needle was already gone when we arrived. + // StatusNotFound (404) comes from the cookie-check path when + // ReadVolumeNeedle can't find the needle. Both are benign + // races against a concurrent fsck purge or a replica that + // had already reconciled, so skip them. Cast to int because + // r.Status is an int32 protobuf field and linters flag the + // mixed-type compare even though Go's untyped-constant rules + // make it valid. + status := int(r.Status) + if r.Error == "" || status == http.StatusNotModified || status == http.StatusNotFound { + continue } + if errCount == 0 { + firstErr = r.Error + firstFid = r.FileId + } + errCount++ + } + if errCount == 1 { + fmt.Printf("source cleanup %s: delete %s on %v: %s\n", entryPath, firstFid, loc.ServerAddress(), firstErr) + } else if errCount > 1 { + fmt.Printf("source cleanup %s: %d/%d needles failed on %v (e.g. %s: %s)\n", + entryPath, errCount, len(fids), loc.ServerAddress(), firstFid, firstErr) } } }