gian/seaweedfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(shell): mergeVolumes hard-link safety and cleaner cleanup logging (#9163)

Browse Source 
* 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.
This commit is contained in:
Chris Lu 2026-04-20 17:55:20 -07:00 committed by GitHub
parent 06ccd0e9fc
commit 49f62df2cf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 52 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
weed/shell/command_fs_merge_volumes.go
View File

@ -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)
}
}
}