From da1e5e714fdc9482e7239a9f5e123fd6677dbdb2 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 19 Aug 2026 14:57:45 -0700 Subject: [PATCH] mount: move the inode table when a rename arrives from the cluster (#10822) * mount: leave the target alone when a move has no source MovePath cleared whatever sat at the target before it checked that the source was still there, so a move it then declined to make had already taken the target's mapping apart. The same rename reaching the table twice - once for an open handle, once for the invalidation behind it - was enough to leave the moved inode with no path at all. * mount: move the inode table when a rename arrives from the cluster A rename made by another client reaches this mount only as a metadata event, and the only table update on that path sat inside the open-file-handle branch. Every other inode the kernel still addresses by nodeid kept resolving to its pre-rename path, so the next operation on it went to a path the filer no longer has. Move the entry for every rename invalidation. The filer sends one event per moved entry, so a renamed directory's children follow their parent without a descendant walk. * mount: move the inode table exactly once per rename event Making MovePath return early on a missing source was the wrong half to fix. The source is also missing when the rename came from a client that never visited it, and there the destination really was replaced and has to be unlinked - the early return kept the name resolving to a file the rename destroyed, so a dirty handle on it could still flush over what took its place. The two cases are indistinguishable from inside MovePath, so leave it alone and stop calling it twice: invalidateOpenFileHandle reports whether it moved, and the handler moves only when it did not. Both paths mark a replaced file's handle deleted, which the no-handle path previously did not do at all. * mount: leave a rename alone unless the source is still ours to move Two ways the fallback move could act on state it did not own. A handle whose version guard skipped the event never reached RememberPath, so moving the table under it left the handle flushing to the pre-rename path; the handle path owns its inode's rename, so the fallback now runs only for an inode without one. And the subscription can redeliver a rename once it falls out of the 4096-entry dedup ring. A replay found no source and a live target, unlinked the mapping the first delivery had just made, and marked the moved file's handle deleted - worse than the stale path this set out to fix. Only a source still in the table is moved now. That gives up unlinking a destination the rename replaced when the source was never visited here, which is where this started. It is what the mount already did before this branch, and it is the safer of the two: retaining a stale name costs a wrong lookup, while unlinking the wrong one costs a file's dirty data. * mount: decide a rename move inside the table's lock The source-presence check sat outside MovePath, so two invalidations for one rename could both see the source and the loser would unlink what the winner had just placed - the same damage the check was added to prevent. MovePath makes the decision under its own lock now and reports that nothing moved. The handle a rename destroyed is also marked from the caller rather than from inside invalidateOpenFileHandle, which was setting isDeleted bare on a second handle while holding the first one's lock. markHandleDeleted already takes the lock the flush reads that flag under, and marking from the caller keeps it to one handle lock at a time. The invalidation test harness wires onEntryInvalidation now, the way the mount does, rather than reaching past it. * mount: apply a rename ahead of the handle's version fence The fence exists so an old event cannot roll a handle's entry back to stale content. A rename carries no content: it says the name the inode answered to is gone. Skipping one on the strength of the fence left the inode and the handle both pointing at a name the filer had vacated, and no fallback ran either, since a handle owns its inode's rename. Applied before the fence now, and only when MovePath reports the source was still ours to move - which is what keeps a replayed rename from remembering a path the handle has already moved past. --- weed/mount/inode_to_path.go | 17 +-- weed/mount/inode_to_path_test.go | 21 +++ weed/mount/weedfs.go | 64 ++++++--- .../weedfs_invalidate_open_handle_test.go | 2 +- weed/mount/weedfs_remote_rename_test.go | 122 ++++++++++++++++++ 5 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 weed/mount/weedfs_remote_rename_test.go diff --git a/weed/mount/inode_to_path.go b/weed/mount/inode_to_path.go index 4b669535f..705658ba0 100644 --- a/weed/mount/inode_to_path.go +++ b/weed/mount/inode_to_path.go @@ -524,19 +524,20 @@ func (i *InodeToPath) MovePath(sourcePath, targetPath util.FullPath) (sourceInod i.Lock() defer i.Unlock() sourceInode, sourceFound := i.path2inode[sourcePath] + if !sourceFound { + // Nothing of ours to move: the source was never visited here, or a + // redelivery already moved it. Whatever sits at the target is not ours + // to take apart on the strength of an absent source, and deciding that + // outside this lock would race a concurrent move to the same target. + return 0, 0 + } targetInode, targetFound := i.path2inode[targetPath] if targetFound { i.removePathFromInode2Path(targetInode, targetPath) delete(i.path2inode, targetPath) } - if sourceFound { - delete(i.path2inode, sourcePath) - i.path2inode[targetPath] = sourceInode - } else { - // it is possible some source folder items has not been visited before - // so no need to worry about their source inodes - return - } + delete(i.path2inode, sourcePath) + i.path2inode[targetPath] = sourceInode if entry, entryFound := i.inode2path[sourceInode]; entryFound { entry.replacePath(sourcePath, targetPath) if d := i.dirStates[sourceInode]; d != nil { diff --git a/weed/mount/inode_to_path_test.go b/weed/mount/inode_to_path_test.go index 378bfd02f..2625ed20a 100644 --- a/weed/mount/inode_to_path_test.go +++ b/weed/mount/inode_to_path_test.go @@ -5,6 +5,7 @@ import ( "time" "unsafe" + "github.com/seaweedfs/go-fuse/v2/fuse" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -187,3 +188,23 @@ func TestForgetOnReleaseRunsUnderLock(t *testing.T) { t.Fatalf("onRelease called %d times, want 1", calls) } } + +// A move with no source is not a move. Deciding that inside the lock is what +// keeps two invalidations racing on the same rename from unlinking each other's +// work. +func TestMovePathWithNoSourceLeavesTheTarget(t *testing.T) { + itp := NewInodeToPath(util.FullPath("/"), 0) + inode := itp.Lookup("/a/f.txt", time.Now().Unix(), false, false, 0, true) + + itp.MovePath("/a/f.txt", "/a/g.txt") + if sourceInode, targetInode := itp.MovePath("/a/f.txt", "/a/g.txt"); sourceInode != 0 || targetInode != 0 { + t.Errorf("repeat move reported %d -> %d, want nothing moved", sourceInode, targetInode) + } + + if got, status := itp.GetPath(inode); status != fuse.OK || got != "/a/g.txt" { + t.Errorf("inode resolves to %q (%v), want /a/g.txt", got, status) + } + if got, found := itp.GetInode("/a/g.txt"); !found || got != inode { + t.Errorf("/a/g.txt resolves to %d (found %v), want %d", got, found, inode) + } +} diff --git a/weed/mount/weedfs.go b/weed/mount/weedfs.go index 44a268896..7a6b14168 100644 --- a/weed/mount/weedfs.go +++ b/weed/mount/weedfs.go @@ -811,7 +811,20 @@ func (wfs *WFS) onEntryInvalidation(invalidation meta_cache.EntryInvalidation) { listener(invalidation) } wfs.invalidateKernelDirListing(invalidation.Path) - wfs.invalidateOpenFileHandle(invalidation) + // An inode with an open handle has its rename applied above, together with + // the handle's own path bookkeeping. This is for the rest: the kernel goes + // on addressing a moved inode by nodeid whether or not anything holds it + // open. + handled, replacedInode := wfs.invalidateOpenFileHandle(invalidation) + if !handled && invalidation.RenamedTo != "" { + _, replacedInode = wfs.inodeToPath.MovePath(invalidation.Path, invalidation.RenamedTo) + } + // A rename over an existing file destroys it, so its handle must not flush + // the old content back over what took the name. Marked from here, holding + // no other handle's lock. + if replacedInode != 0 { + wfs.markHandleDeleted(replacedInode) + } } // invalidateKernelDirListing drops the kernel's cached listing of the directory @@ -844,7 +857,12 @@ func (wfs *WFS) MountRoot() util.FullPath { return util.FullPath(wfs.option.FilerMountRootPath) } -func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidation) { +// invalidateOpenFileHandle applies one event to the handle that has the entry +// open, reporting whether it found one: a handle owns its inode's rename +// bookkeeping, and the caller moves the table only for inodes without one. Any +// inode a rename destroyed comes back for the caller to mark, since marking it +// here would hold two handle locks at once. +func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidation) (handled bool, replacedInode uint64) { filePath, eventEntry, eventTsNs := invalidation.Path, invalidation.Entry, invalidation.TsNs inode, inodeFound := wfs.inodeToPath.GetInode(filePath) if !inodeFound { @@ -854,9 +872,31 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati if !fhFound { return } + handled = true fhActiveLock := wfs.fhLockTable.AcquireLock("invalidateFunc", fh.fh, util.ExclusiveLock) defer wfs.fhLockTable.ReleaseLock(fh.fh, fhActiveLock) + // A rename changes which name the inode answers to, not which version of + // its content the handle holds, so it is applied ahead of the version fence + // below - a fence that skipped it would strand the inode and the handle on + // a name the filer has vacated. MovePath reports whether the source was + // still ours to move, which is what makes a replayed rename a no-op here. + if invalidation.RenamedTo != "" { + if sourceInode, replaced := wfs.inodeToPath.MovePath(filePath, invalidation.RenamedTo); sourceInode != 0 { + if replaced != inode { + replacedInode = replaced + } + fh.RememberPath(invalidation.RenamedTo) + if _, newName := invalidation.RenamedTo.DirAndName(); newName != "" { + fh.UpdateEntry(func(entry *filer_pb.Entry) { + if entry != nil { + entry.Name = newName + } + }) + } + } + } + // Invalidations apply asynchronously: the handle may already reflect this // event or newer state, and rolling it back would never be corrected. // Only skip within one clock domain — the handle's position may have been @@ -896,25 +936,6 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati // updating the renamed file. An actual delete instead marks the handle // so no flush recreates the unlinked name. Either way the entry and // dirty pages stay, so the open fd still reads its buffered writes. - if invalidation.RenamedTo != "" { - _, replacedInode := wfs.inodeToPath.MovePath(filePath, invalidation.RenamedTo) - // A rename over an existing file destroys that file. Mark its - // handle deleted so its flush cannot resurrect it on top of the - // renamed source now occupying the name. - if replacedInode != 0 && replacedInode != inode { - if replacedFh, found := wfs.fhMap.FindFileHandle(replacedInode); found { - replacedFh.isDeleted = true - } - } - fh.RememberPath(invalidation.RenamedTo) - if _, newName := invalidation.RenamedTo.DirAndName(); newName != "" { - fh.UpdateEntry(func(entry *filer_pb.Entry) { - if entry != nil { - entry.Name = newName - } - }) - } - } if invalidation.Deleted { fh.isDeleted = true } @@ -950,6 +971,7 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati } fh.baseEntry.Store(proto.Clone(candidate).(*filer_pb.Entry)) fh.advanceEntryVersion(candidateTsNs, 0) + return } func (wfs *WFS) LookupFn() wdclient.LookupFileIdFunctionType { diff --git a/weed/mount/weedfs_invalidate_open_handle_test.go b/weed/mount/weedfs_invalidate_open_handle_test.go index b79e6d596..07c4ca895 100644 --- a/weed/mount/weedfs_invalidate_open_handle_test.go +++ b/weed/mount/weedfs_invalidate_open_handle_test.go @@ -60,7 +60,7 @@ func newInvalidateTestWFS(t *testing.T) *WFS { false, func(path util.FullPath) { wfs.inodeToPath.MarkChildrenCached(path) }, func(path util.FullPath) bool { return wfs.inodeToPath.IsChildrenCached(path) }, - wfs.invalidateOpenFileHandle, + wfs.onEntryInvalidation, nil, ) t.Cleanup(wfs.metaCache.Shutdown) diff --git a/weed/mount/weedfs_remote_rename_test.go b/weed/mount/weedfs_remote_rename_test.go new file mode 100644 index 000000000..ba04f0500 --- /dev/null +++ b/weed/mount/weedfs_remote_rename_test.go @@ -0,0 +1,122 @@ +package mount + +import ( + "testing" + "time" + + "github.com/seaweedfs/go-fuse/v2/fuse" + "github.com/seaweedfs/seaweedfs/weed/mount/meta_cache" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// The filer sends one event per moved entry, so children follow their parent. +func TestRemoteRenameMovesInodePaths(t *testing.T) { + dir := util.FullPath("/images") + wfs := newPagingWFS(t, dir, []string{"f.jpg"}, 0) + + now := time.Now().Unix() + subInode := wfs.inodeToPath.Lookup(dir.Child("sub"), now, true, false, 0, true) + childInode := wfs.inodeToPath.Lookup(dir.Child("sub").Child("f.jpg"), now, false, false, 0, true) + + wfs.onEntryInvalidation(meta_cache.EntryInvalidation{ + Path: dir.Child("sub"), + RenamedTo: dir.Child("moved"), + WasDirectory: true, + }) + wfs.onEntryInvalidation(meta_cache.EntryInvalidation{ + Path: dir.Child("sub").Child("f.jpg"), + RenamedTo: dir.Child("moved").Child("f.jpg"), + }) + + if got, _ := wfs.inodeToPath.GetPath(subInode); got != dir.Child("moved") { + t.Errorf("renamed directory resolves to %s, want %s", got, dir.Child("moved")) + } + if got, _ := wfs.inodeToPath.GetPath(childInode); got != dir.Child("moved").Child("f.jpg") { + t.Errorf("child resolves to %s, want %s", got, dir.Child("moved").Child("f.jpg")) + } + if wfs.inodeToPath.HasPath(dir.Child("sub")) { + t.Error("pre-rename directory path still in the table") + } +} + +// The handle path and the table move are two halves of one rename. Doing the +// move twice would unlink what the first one just put at the target, leaving +// the renamed inode with no path at all. +func TestRemoteRenameMovesOnceWithAnOpenHandle(t *testing.T) { + wfs := newInvalidateTestWFS(t) + oldPath, newPath := util.FullPath("/dir/file"), util.FullPath("/dir/renamed") + + inode := wfs.inodeToPath.Lookup(oldPath, time.Now().Unix(), false, false, 0, true) + wfs.fhMap.AcquireFileHandle(wfs, inode, &filer_pb.Entry{ + Name: "file", + Attributes: &filer_pb.FuseAttributes{FileSize: 88}, + }, 0, 0) + + wfs.onEntryInvalidation(meta_cache.EntryInvalidation{Path: oldPath, RenamedTo: newPath, TsNs: 1000}) + + if got, status := wfs.inodeToPath.GetPath(inode); status != fuse.OK || got != newPath { + t.Errorf("inode resolves to %q (%v), want %s", got, status, newPath) + } + if got, found := wfs.inodeToPath.GetInode(newPath); !found || got != inode { + t.Errorf("%s resolves to %d (found %v), want %d", newPath, got, found, inode) + } +} + +// The subscription can redeliver a rename once it falls out of the dedup ring. +// Replaying one must not unlink what the first delivery put at the target, nor +// mark the moved file's handle deleted. +func TestRemoteRenameReplayLeavesTheTargetAlone(t *testing.T) { + wfs := newInvalidateTestWFS(t) + oldPath, newPath := util.FullPath("/dir/file"), util.FullPath("/dir/renamed") + + inode := wfs.inodeToPath.Lookup(oldPath, time.Now().Unix(), false, false, 0, true) + fh, _ := wfs.fhMap.AcquireFileHandle(wfs, inode, &filer_pb.Entry{ + Name: "file", + Attributes: &filer_pb.FuseAttributes{FileSize: 88}, + }, 0, 0) + + rename := meta_cache.EntryInvalidation{Path: oldPath, RenamedTo: newPath, TsNs: 1000} + wfs.onEntryInvalidation(rename) + wfs.onEntryInvalidation(rename) + + if got, status := wfs.inodeToPath.GetPath(inode); status != fuse.OK || got != newPath { + t.Errorf("inode resolves to %q (%v), want %s", got, status, newPath) + } + if got, found := wfs.inodeToPath.GetInode(newPath); !found || got != inode { + t.Errorf("%s resolves to %d (found %v), want %d", newPath, got, found, inode) + } + if fh.isDeleted { + t.Error("the moved file's handle was marked deleted by the replay") + } +} + +// A rename event older than the handle's version fence is still a rename. The +// fence is about which version of the content the handle holds, and skipping +// the move would leave both the inode and the handle on a name the filer has +// vacated. +func TestRenameBelowTheVersionFenceStillMoves(t *testing.T) { + wfs := newInvalidateTestWFS(t) + oldPath, newPath := util.FullPath("/dir/file"), util.FullPath("/dir/renamed") + + inode := wfs.inodeToPath.Lookup(oldPath, time.Now().Unix(), false, false, 0, true) + fh, _ := wfs.fhMap.AcquireFileHandle(wfs, inode, &filer_pb.Entry{ + Name: "file", + Attributes: &filer_pb.FuseAttributes{FileSize: 88}, + }, 0, 0) + // The handle already reflects a later position in the same clock domain, so + // the fence would drop this event. + fh.advanceEntryVersion(5000, wfs.signature) + + wfs.onEntryInvalidation(meta_cache.EntryInvalidation{ + Path: oldPath, RenamedTo: newPath, TsNs: 1000, + Signatures: []int32{wfs.signature}, + }) + + if got, status := wfs.inodeToPath.GetPath(inode); status != fuse.OK || got != newPath { + t.Errorf("inode resolves to %q (%v), want %s", got, status, newPath) + } + if dir, name := fh.savedDir, fh.savedName; util.FullPath(dir).Child(name) != newPath { + t.Errorf("handle remembers %s/%s, want %s", dir, name, newPath) + } +}