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