From 4fc41872e3b10ac6fe77e3205723e6e8e19654ca Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 13 Nov 2026 16:14:22 -0800 Subject: [PATCH] mount: keep an open file publishing when a peer unlinks one of its hard link names (#12613) * mount: record a hard link's looked-up names on its inode Lookup mapped a second name of a hard-linked file to the existing inode without adding it to the inode's names, so GetAllPaths missed it and the final forget left it mapped to a released inode. Co-Authored-By: Claude Opus 5.5 * mount: keep an open file publishing when a peer unlinks one of its names A delete event from another client marked the inode's open handle deleted even when other names still held the hard-linked file, so every later fsync reported success without saving metadata. The delete event's entry carries the link count the filer decided on, so an alias delete now drops only that name, keeps the handle on a name this mount knows, and has the next flush read the survivor's stored entry before publishing. Co-Authored-By: Claude Opus 5.5 * mount: test peer unlinks of a hard link name held open Co-Authored-By: Claude Opus 5.5 * mount: leave an unlinked file out of the periodic metadata flush flushFileMetadata did not check the handle's delete mark. A file unlinked while open before its deferred create reached the filer was created again by the next periodic flush, which expects no entry. Co-Authored-By: Claude Opus 5.5 * mount: read a pending link change before a periodic metadata flush Only flushMetadataToFiler applied linkRefresh. After a peer unlinked one name, the periodic flush kept publishing over the old link count, so the filer refused every one until a regular flush ran. Co-Authored-By: Claude Opus 5.5 * mount: test the periodic flush after a peer unlinks a hard link name Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- weed/mount/inode_to_path.go | 8 +- weed/mount/inode_to_path_test.go | 22 +++ weed/mount/meta_cache/meta_cache.go | 9 +- weed/mount/weedfs.go | 24 +++ weed/mount/weedfs_metadata_flush.go | 9 ++ .../weedfs_metadata_flush_unlinked_test.go | 31 ++++ weed/mount/weedfs_peer_unlink_test.go | 153 ++++++++++++++++++ 7 files changed, 253 insertions(+), 3 deletions(-) create mode 100644 weed/mount/weedfs_metadata_flush_unlinked_test.go create mode 100644 weed/mount/weedfs_peer_unlink_test.go diff --git a/weed/mount/inode_to_path.go b/weed/mount/inode_to_path.go index 5ac35bbc42..055a82cc14 100644 --- a/weed/mount/inode_to_path.go +++ b/weed/mount/inode_to_path.go @@ -201,9 +201,13 @@ func (i *InodeToPath) Lookup(path util.FullPath, unixTime int64, isDirectory boo } i.path2inode[path] = inode - if _, found := i.inode2path[inode]; found { + if ie, known := i.inode2path[inode]; known { + // Another name of a hard link the table already holds. + if !found { + ie.addPath(path) + } if isLookup { - i.inode2path[inode].nlookup++ + ie.nlookup++ } } else { nlookup := uint64(0) diff --git a/weed/mount/inode_to_path_test.go b/weed/mount/inode_to_path_test.go index 2625ed20aa..c502838b46 100644 --- a/weed/mount/inode_to_path_test.go +++ b/weed/mount/inode_to_path_test.go @@ -208,3 +208,25 @@ func TestMovePathWithNoSourceLeavesTheTarget(t *testing.T) { t.Errorf("/a/g.txt resolves to %d (found %v), want %d", got, found, inode) } } + +// A second hard link name found by a lookup belongs to the inode, so it is +// reported with the others and released by the final forget. +func TestLookupRecordsHardLinkNames(t *testing.T) { + table := NewInodeToPath(util.FullPath("/"), 0) + inode := table.Lookup("/a", 1, false, true, 42, true) + if got := table.Lookup("/b", 1, false, true, 42, true); got != inode { + t.Fatalf("second name got inode %d, want %d", got, inode) + } + table.Lookup("/b", 1, false, true, 42, true) + if got := table.GetAllPaths(inode); len(got) != 2 || got[0] != "/a" || got[1] != "/b" { + t.Fatalf("paths %v, want [/a /b]", got) + } + table.RemovePath("/a", nil) + if got, status := table.GetPath(inode); status != fuse.OK || got != "/b" { + t.Fatalf("after removing /a the inode answers to %q (%v), want /b", got, status) + } + table.Forget(inode, 3, nil, nil) + if _, found := table.GetInode("/b"); found { + t.Fatal("the final forget left /b mapped to a released inode") + } +} diff --git a/weed/mount/meta_cache/meta_cache.go b/weed/mount/meta_cache/meta_cache.go index d5f89c7294..e4c16979df 100644 --- a/weed/mount/meta_cache/meta_cache.go +++ b/weed/mount/meta_cache/meta_cache.go @@ -937,6 +937,9 @@ type EntryInvalidation struct { // once the path is empty, so this is the only thing left saying whether a // directory or a file went away. WasDirectory bool + // LiveHardLinkId is set on a delete that left other names holding the + // hard-linked file, by the count the filer kept its chunks on. + LiveHardLinkId []byte } type metadataResponseSideEffects struct { @@ -1440,7 +1443,11 @@ func collectEntryInvalidations(resp *filer_pb.SubscribeMetadataResponse) []Entry if filer_pb.IsDelete(resp) && message.OldEntry != nil { oldKey := util.NewFullPath(resp.Directory, message.OldEntry.Name) - invalidations = append(invalidations, EntryInvalidation{Path: oldKey, TsNs: resp.TsNs, Deleted: true, Signatures: signatures, WasDirectory: message.OldEntry.IsDirectory}) + invalidation := EntryInvalidation{Path: oldKey, TsNs: resp.TsNs, Deleted: true, Signatures: signatures, WasDirectory: message.OldEntry.IsDirectory} + if len(message.OldEntry.HardLinkId) > 0 && message.OldEntry.HardLinkCounter > 1 { + invalidation.LiveHardLinkId = message.OldEntry.HardLinkId + } + invalidations = append(invalidations, invalidation) } return invalidations diff --git a/weed/mount/weedfs.go b/weed/mount/weedfs.go index 9a85bf8476..2455ea5e49 100644 --- a/weed/mount/weedfs.go +++ b/weed/mount/weedfs.go @@ -1023,6 +1023,12 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati // 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.Deleted { + // The next flush reads the survivor's stored entry before + // publishing, so the version stays where it was. + if len(invalidation.LiveHardLinkId) > 0 && wfs.followSurvivingLink(fh, inode, filePath) { + fh.linkRefresh = invalidation.LiveHardLinkId + return + } fh.isDeleted = true fh.deleteEpoch++ } @@ -1067,6 +1073,24 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati return } +// followSurvivingLink drops a hard link name another client deleted and +// keeps the handle on a name of the inode this mount still knows, reporting +// false when it knows none. +func (wfs *WFS) followSurvivingLink(fh *FileHandle, inode uint64, removed util.FullPath) bool { + survives := false + for _, p := range wfs.inodeToPath.GetAllPaths(inode) { + survives = survives || p != removed + } + if !survives { + return false + } + wfs.inodeToPath.RemovePath(removed, nil) + if survivor, status := wfs.inodeToPath.GetPath(inode); status == fuse.OK { + fh.RememberPath(survivor) + } + return true +} + func (wfs *WFS) LookupFn() wdclient.LookupFileIdFunctionType { if wfs.option.VolumeServerAccess == "filerProxy" { return func(ctx context.Context, fileId string) (targetUrls []string, err error) { diff --git a/weed/mount/weedfs_metadata_flush.go b/weed/mount/weedfs_metadata_flush.go index 7c023067d7..e9ebc38e3b 100644 --- a/weed/mount/weedfs_metadata_flush.go +++ b/weed/mount/weedfs_metadata_flush.go @@ -99,6 +99,10 @@ func (wfs *WFS) flushFileMetadata(fh *FileHandle) error { if !fh.dirtyMetadata { return nil } + // Unlink marks under this lock; publishing would recreate the name. + if fh.isDeleted { + return nil + } fileFullPath := fh.FullPath() dir, name := fileFullPath.DirAndName() @@ -108,6 +112,11 @@ func (wfs *WFS) flushFileMetadata(fh *FileHandle) error { if fh.GetEntry() == nil { return nil } + if fh.linkRefresh != nil { + if err := wfs.adoptStoredLink(context.Background(), fh, fileFullPath, fh.linkRefresh); err != nil { + return err + } + } // Snapshot the current chunk list. Async uploader goroutines call // entry.AppendChunks while we run CompactFileChunks / MaybeManifestize diff --git a/weed/mount/weedfs_metadata_flush_unlinked_test.go b/weed/mount/weedfs_metadata_flush_unlinked_test.go new file mode 100644 index 0000000000..8033646739 --- /dev/null +++ b/weed/mount/weedfs_metadata_flush_unlinked_test.go @@ -0,0 +1,31 @@ +//go:build !freebsd + +package mount + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" +) + +// A file unlinked while open, before its deferred create reached the filer, +// must not be created by the periodic metadata flush. +func TestPeriodicFlushSkipsUnlinkedHandle(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + fake.mu.Lock() + delete(fake.records, "tmp") + fake.mu.Unlock() + fh.baseEntry.Store(nil) + wfs.markHandleDeleted(inode) + fh.UpdateEntry(func(e *filer_pb.Entry) { + e.Chunks = append(e.Chunks, linkHandleChunk(2, 100)) + e.Attributes.FileSize = 200 + }) + fh.dirtyMetadata = true + if err := wfs.flushFileMetadata(fh); err != nil { + t.Fatalf("periodic flush: %v", err) + } + if fake.stored("tmp") != nil { + t.Fatal("the periodic flush recreated the unlinked name") + } +} diff --git a/weed/mount/weedfs_peer_unlink_test.go b/weed/mount/weedfs_peer_unlink_test.go new file mode 100644 index 0000000000..be63c7abde --- /dev/null +++ b/weed/mount/weedfs_peer_unlink_test.go @@ -0,0 +1,153 @@ +//go:build !freebsd + +package mount + +import ( + "context" + "testing" + + "github.com/seaweedfs/go-fuse/v2/fuse" + "github.com/seaweedfs/seaweedfs/weed/mount/meta_cache" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" +) + +// peerUnlink removes name on the filer the way another mount does and +// delivers the delete event this mount's subscription receives. +func peerUnlink(t *testing.T, wfs *WFS, fake *hardLinkFiler, name string) { + t.Helper() + old := fake.stored(name) + if _, err := fake.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{Directory: "/", Name: name}); err != nil { + t.Fatalf("peer delete %s: %v", name, err) + } + fake.mu.Lock() + fake.stamp++ + ts := fake.stamp + fake.mu.Unlock() + del := &filer_pb.SubscribeMetadataResponse{Directory: "/", TsNs: ts, EventNotification: &filer_pb.EventNotification{OldEntry: old}} + if err := wfs.metaCache.ApplyMetadataResponse(context.Background(), del, meta_cache.SubscriberMetadataResponseApplyOptions); err != nil { + t.Fatalf("apply peer delete: %v", err) + } + wfs.metaCache.WaitForEntryInvalidations() +} + +// Another mount removes one of the two names of a file open here. The handle +// keeps publishing through the other name, over the link count and ctime the +// filer stored there, and the removed name stays removed. +func TestPeerUnlinkedAliasKeepsOpenHandlePublishing(t *testing.T) { + for _, removed := range []string{"tmp", "final"} { + for _, dirty := range []bool{false, true} { + t.Run(removed+map[bool]string{false: "/clean", true: "/dirty"}[dirty], func(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + linkInRoot(t, wfs, inode, "final") + chunks := 2 + if dirty { + fh.UpdateEntry(func(e *filer_pb.Entry) { + e.Chunks = append(e.Chunks, linkHandleChunk(9, 100)) + e.Attributes.FileSize = 200 + }) + fh.dirtyMetadata = true + chunks = 3 + } + survivor := map[string]string{"tmp": "final", "final": "tmp"}[removed] + peerUnlink(t, wfs, fake, removed) + if fh.isDeleted { + t.Fatal("a peer unlinking one of two names marked the open handle deleted") + } + unlinked := fake.stored(survivor).Attributes + if code := appendAndFlush(t, wfs, fh, 2); code != fuse.OK { + t.Fatalf("flush after the peer unlink: %v", code) + } + wantStored(t, fake, survivor, chunks, 1) + if got := fake.stored(survivor).Attributes; got.Ctime != unlinked.Ctime || got.CtimeNs != unlinked.CtimeNs { + t.Fatalf("the flush moved the ctime from the unlink's %d.%09d back to %d.%09d", unlinked.Ctime, unlinked.CtimeNs, got.Ctime, got.CtimeNs) + } + if fake.stored(removed) != nil { + t.Fatalf("the flush recreated %s", removed) + } + }) + } + } +} + +// The surviving name was found by a lookup, not linked by this mount. +func TestPeerUnlinkedAliasKeepsLookedUpSurvivor(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + linkInRoot(t, wfs, inode, "final") + wfs.inodeToPath.RemovePath("/final", nil) + wfs.inodeToPath.Lookup("/final", 1, false, true, inode, true) + peerUnlink(t, wfs, fake, "tmp") + if fh.isDeleted { + t.Fatal("a peer unlinking the open name of a file with a looked-up second name marked the handle deleted") + } + if code := appendAndFlush(t, wfs, fh, 2); code != fuse.OK { + t.Fatalf("flush after the peer unlink: %v", code) + } + wantStored(t, fake, "final", 2, 1) + if fake.stored("tmp") != nil { + t.Fatal("the flush recreated tmp") + } +} + +// With no other name known here, the handle stops rather than recreating the +// removed one. +func TestPeerUnlinkedAliasWithUnknownSurvivorStops(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + linkInRoot(t, wfs, inode, "final") + wfs.inodeToPath.RemovePath("/final", nil) + peerUnlink(t, wfs, fake, "tmp") + if !fh.isDeleted { + t.Fatal("a handle with no known surviving name was left publishing") + } + if code := appendAndFlush(t, wfs, fh, 2); code != fuse.OK { + t.Fatalf("flush: %v", code) + } + if fake.stored("tmp") != nil { + t.Fatal("the flush recreated tmp") + } +} + +// A peer removes one name, then this mount unlinks the last one. +func TestPeerThenLocalUnlinkKeepsHandleDeleted(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + linkInRoot(t, wfs, inode, "final") + peerUnlink(t, wfs, fake, "final") + unlinkInRoot(t, wfs, "tmp") + if !fh.isDeleted { + t.Fatal("the handle of a file with no names left is not marked deleted") + } + if code := appendAndFlush(t, wfs, fh, 2); code != fuse.OK { + t.Fatalf("flush after the last unlink: %v", code) + } + for _, name := range []string{"tmp", "final"} { + if fake.stored(name) != nil { + t.Fatalf("the flush recreated %s", name) + } + } +} + +func TestPeerUnlinkLastNameKeepsHandleDeleted(t *testing.T) { + wfs, fake, fh, _ := openLinkTestFile(t) + peerUnlink(t, wfs, fake, "tmp") + if !fh.isDeleted { + t.Fatal("a peer unlinking the only name did not mark the handle deleted") + } +} + +// The periodic metadata flush publishes through the survivor as well. +func TestPeerUnlinkedAliasKeepsPeriodicFlushPublishing(t *testing.T) { + wfs, fake, fh, inode := openLinkTestFile(t) + linkInRoot(t, wfs, inode, "final") + peerUnlink(t, wfs, fake, "tmp") + fh.UpdateEntry(func(e *filer_pb.Entry) { + e.Chunks = append(e.Chunks, linkHandleChunk(2, 100)) + e.Attributes.FileSize = 200 + }) + fh.dirtyMetadata = true + if err := wfs.flushFileMetadata(fh); err != nil { + t.Fatalf("periodic flush after the peer unlink: %v", err) + } + wantStored(t, fake, "final", 2, 1) + if fake.stored("tmp") != nil { + t.Fatal("the periodic flush recreated tmp") + } +}