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

@@ -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 {

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

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 {

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)

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