From bc06505b404cfdc518b24f2bdcca376cc3978973 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 27 Aug 2026 16:22:25 -0700 Subject: [PATCH] mount: keep metadata operations working on an unlinked open file (#10989) * mount: serve metadata ops from the open handle of an unlinked file ftruncate on a descriptor whose file was unlinked failed with ENOENT: maybeReadEntry resolved the inode to a path first, and unlink had already dropped it. GetAttr worked around that with its own handle fallback; SetAttr and the xattr handlers had none. Look the handle up first and let it answer whether or not a name still points at the inode. GetAttr keeps reporting nlink 0 there, now off the empty path. Claude-Session: https://claude.ai/code/session_01U1R8BM4bVT46KwPDEj2Ega * mount: read an open handle's attributes under the handle lock too GetAttr held only the LockedEntry lock, which covers the async uploader's chunk appends but not Write or the metadata flush: those rewrite size, times and the whole chunk slice under the handle lock, so FileSize could walk a slice mid-reassignment. The branch this replaced took both locks; take both here, outer handle lock first, as Read and Lseek do. Claude-Session: https://claude.ai/code/session_01U1R8BM4bVT46KwPDEj2Ega * mount: report nlink 0 from SetAttr for an unlinked open file The kernel caches the attributes a SETATTR reply carries, so an ftruncate on an unlinked file left fstat reporting nlink 1 until the cache expired, even though GetAttr had it right. Both replies go through the same rule. Claude-Session: https://claude.ai/code/session_01U1R8BM4bVT46KwPDEj2Ega --- weed/mount/weedfs.go | 17 ++- weed/mount/weedfs_attr.go | 67 ++++++------ weed/mount/weedfs_attr_race_test.go | 1 + weed/mount/weedfs_attr_unlinked_test.go | 140 ++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 39 deletions(-) create mode 100644 weed/mount/weedfs_attr_unlinked_test.go diff --git a/weed/mount/weedfs.go b/weed/mount/weedfs.go index 45dc75d66..63f89cab9 100644 --- a/weed/mount/weedfs.go +++ b/weed/mount/weedfs.go @@ -521,21 +521,26 @@ func (wfs *WFS) Init(server *fuse.Server) { wfs.fuseServer = server } +// maybeReadEntry resolves an inode to the entry metadata operations act on. An +// open handle answers ahead of the path: unlink drops the name while the +// descriptor stays valid, so an unlinked-but-open file returns its handle's +// entry with an empty path instead of ENOENT. func (wfs *WFS) maybeReadEntry(inode uint64) (path util.FullPath, fh *FileHandle, entry *filer_pb.Entry, status fuse.Status) { - path, status = wfs.inodeToPath.GetPath(inode) - if status != fuse.OK { - return - } var found bool if fh, found = wfs.fhMap.FindFileHandle(inode); found { + path, _ = wfs.inodeToPath.GetPath(inode) entry = fh.UpdateEntry(func(entry *filer_pb.Entry) { if entry != nil && fh.entry.Attributes == nil { entry.Attributes = &filer_pb.FuseAttributes{} } }) - } else { - entry, _, status = wfs.maybeLoadEntry(path) + return path, fh, entry, fuse.OK } + path, status = wfs.inodeToPath.GetPath(inode) + if status != fuse.OK { + return + } + entry, _, status = wfs.maybeLoadEntry(path) return } diff --git a/weed/mount/weedfs_attr.go b/weed/mount/weedfs_attr.go index 32561d078..c4d2eb539 100644 --- a/weed/mount/weedfs_attr.go +++ b/weed/mount/weedfs_attr.go @@ -25,43 +25,34 @@ func (wfs *WFS) GetAttr(cancel <-chan struct{}, input *fuse.GetAttrIn, out *fuse inode := input.NodeId path, fh, entry, status := wfs.maybeReadEntry(inode) - if status == fuse.OK { - out.AttrValid = wfs.attrValidSec - // When an open handle owns the entry, async upload workers append - // chunks under the LockedEntry lock; take it for reading so FileSize - // does not iterate the chunk slice mid-reallocation. Re-read under the - // lock in case SetEntry swapped the pointer since maybeReadEntry. - if fh != nil { - fh.entry.RLock() - entry = fh.entry.Entry - } - wfs.setAttrByPbEntry(&out.Attr, inode, entry, true) - if fh != nil { - fh.entry.RUnlock() - } - wfs.applyInMemoryAtime(&out.Attr, inode) - if entry.IsDirectory { - wfs.applyInMemoryDirMtime(&out.Attr, inode) - if wfs.option.PosixDirNlink { - wfs.applyDirNlink(&out.Attr, path) - } - } + if status != fuse.OK { return status + } + out.AttrValid = wfs.attrValidSec + // An open handle's entry has two sets of writers: async upload workers + // append chunks under the LockedEntry lock, while Write and the metadata + // flush rewrite size, times and the whole chunk slice under the handle + // lock. Hold both for reading, in that order, so FileSize never iterates a + // slice mid-reallocation. Re-read the entry under them in case SetEntry + // swapped the pointer since maybeReadEntry. + if fh != nil { + fhActiveLock := wfs.fhLockTable.AcquireLock("GetAttr", fh.fh, util.SharedLock) + fh.entry.RLock() + entry = fh.entry.Entry + wfs.setAttrByPbEntry(&out.Attr, inode, entry, true) + fh.entry.RUnlock() + wfs.fhLockTable.ReleaseLock(fh.fh, fhActiveLock) } else { - if fh, found := wfs.fhMap.FindFileHandle(inode); found { - out.AttrValid = wfs.attrValidSec - // Use shared lock to prevent race with Write operations - fhActiveLock := wfs.fhLockTable.AcquireLock("GetAttr", fh.fh, util.SharedLock) - fh.entry.RLock() - wfs.setAttrByPbEntry(&out.Attr, inode, fh.entry.Entry, true) - fh.entry.RUnlock() - wfs.fhLockTable.ReleaseLock(fh.fh, fhActiveLock) - wfs.applyInMemoryAtime(&out.Attr, inode) - out.Nlink = 0 - return fuse.OK + wfs.setAttrByPbEntry(&out.Attr, inode, entry, true) + } + wfs.applyInMemoryAtime(&out.Attr, inode) + applyUnlinkedNlink(&out.Attr, path) + if entry.GetIsDirectory() { + wfs.applyInMemoryDirMtime(&out.Attr, inode) + if wfs.option.PosixDirNlink { + wfs.applyDirNlink(&out.Attr, path) } } - return status } @@ -202,6 +193,7 @@ func (wfs *WFS) SetAttr(cancel <-chan struct{}, input *fuse.SetAttrIn, out *fuse } wfs.setAttrByPbEntry(&out.Attr, input.NodeId, entry, !includeSize) wfs.applyInMemoryAtime(&out.Attr, input.NodeId) + applyUnlinkedNlink(&out.Attr, path) if fh != nil { fh.dirtyMetadata = true @@ -423,6 +415,15 @@ func (wfs *WFS) applyInMemoryAtime(out *fuse.Attr, inode uint64) { wfs.atimeMu.Unlock() } +// applyUnlinkedNlink zeroes nlink for an inode no name points at any more: the +// file was unlinked while open and lives on only through its handle. Every +// reply carrying attributes must say so, or the kernel caches a live nlink. +func applyUnlinkedNlink(out *fuse.Attr, path util.FullPath) { + if path == "" { + out.Nlink = 0 + } +} + // applyDirNlink sets nlink = 2 + number_of_subdirectories for a directory. // Uses the in-memory subdirectory count tracked by mkdir/rmdir/rename. func (wfs *WFS) applyDirNlink(out *fuse.Attr, dirPath util.FullPath) { diff --git a/weed/mount/weedfs_attr_race_test.go b/weed/mount/weedfs_attr_race_test.go index 3ffbbc97b..f78b28782 100644 --- a/weed/mount/weedfs_attr_race_test.go +++ b/weed/mount/weedfs_attr_race_test.go @@ -23,6 +23,7 @@ func TestAttrChunkRace(t *testing.T) { option: &Option{}, inodeToPath: NewInodeToPath(util.FullPath("/"), 0), fhMap: NewFileHandleToInode(), + fhLockTable: util.NewLockTable[FileHandleId](), openMtimeCache: make(map[uint64][2]int64, 8), } diff --git a/weed/mount/weedfs_attr_unlinked_test.go b/weed/mount/weedfs_attr_unlinked_test.go new file mode 100644 index 000000000..619431515 --- /dev/null +++ b/weed/mount/weedfs_attr_unlinked_test.go @@ -0,0 +1,140 @@ +package mount + +import ( + "testing" + + "github.com/seaweedfs/go-fuse/v2/fuse" + "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/util" +) + +// newUnlinkedOpenFile builds a WFS holding one open handle whose name has +// already been removed, the state a file is in between unlink() and the last +// close() of a descriptor still pointing at it. +func newUnlinkedOpenFile(t *testing.T) (*WFS, uint64, *FileHandle) { + t.Helper() + + wfs := &WFS{ + option: &Option{ChunkSizeLimit: 1024, ConcurrentReaders: 1}, + inodeToPath: NewInodeToPath(util.FullPath("/"), 0), + fhMap: NewFileHandleToInode(), + fhLockTable: util.NewLockTable[FileHandleId](), + openMtimeCache: make(map[uint64][2]int64, 8), + } + + const inode = uint64(42) + fullPath := util.FullPath("/dir/file") + wfs.inodeToPath.Lookup(fullPath, 1, false, false, inode, true) + + entry := &filer_pb.Entry{ + Name: "file", + Attributes: &filer_pb.FuseAttributes{FileMode: 0644}, + } + chunkGroup, err := filer.NewChunkGroup(nil, nil, nil, 1, nil) + if err != nil { + t.Fatalf("NewChunkGroup: %v", err) + } + fh := &FileHandle{ + fh: FileHandleId(1), + inode: inode, + wfs: wfs, + entry: &LockedEntry{Entry: entry}, + entryChunkGroup: chunkGroup, + } + fh.dirtyPages = newPageWriter(fh, 1<<20) + fh.RememberPath(fullPath) + wfs.fhMap.inode2fh[inode] = fh + wfs.fhMap.fh2inode[fh.fh] = inode + + wfs.inodeToPath.RemovePath(fullPath) + fh.isDeleted = true + + return wfs, inode, fh +} + +// TestSetAttrOnUnlinkedOpenFile covers ftruncate/fchmod on a descriptor whose +// file has been unlinked: POSIX keeps the open file alive, so these must not +// fail with ENOENT. +func TestSetAttrOnUnlinkedOpenFile(t *testing.T) { + wfs, inode, fh := newUnlinkedOpenFile(t) + + in := &fuse.SetAttrIn{} + in.NodeId = inode + in.Valid = fuse.FATTR_SIZE + in.Size = 128 + var out fuse.AttrOut + if status := wfs.SetAttr(nil, in, &out); status != fuse.OK { + t.Fatalf("SetAttr size on unlinked open file: got %v, want OK", status) + } + if out.Attr.Size != 128 { + t.Fatalf("SetAttr size: got %d, want 128", out.Attr.Size) + } + if got := fh.GetEntry().GetEntry().Attributes.FileSize; got != 128 { + t.Fatalf("handle entry FileSize: got %d, want 128", got) + } + if !fh.dirtyMetadata { + t.Fatal("SetAttr did not mark the handle dirty") + } + // The kernel caches what this reply carries, so it has to agree with fstat. + if out.Attr.Nlink != 0 { + t.Fatalf("SetAttr nlink: got %d, want 0", out.Attr.Nlink) + } + + in = &fuse.SetAttrIn{} + in.NodeId = inode + in.Valid = fuse.FATTR_MODE + in.Mode = 0600 + if status := wfs.SetAttr(nil, in, &out); status != fuse.OK { + t.Fatalf("SetAttr mode on unlinked open file: got %v, want OK", status) + } + if got := fh.GetEntry().GetEntry().Attributes.FileMode & 0777; got != 0600 { + t.Fatalf("handle entry FileMode: got %o, want 600", got) + } +} + +// TestGetAttrOnUnlinkedOpenFile pins fstat on an unlinked descriptor: the size +// stays visible and nlink drops to zero. +func TestGetAttrOnUnlinkedOpenFile(t *testing.T) { + wfs, inode, fh := newUnlinkedOpenFile(t) + fh.GetEntry().GetEntry().Attributes.FileSize = 128 + + in := &fuse.GetAttrIn{} + in.NodeId = inode + var out fuse.AttrOut + if status := wfs.GetAttr(nil, in, &out); status != fuse.OK { + t.Fatalf("GetAttr on unlinked open file: got %v, want OK", status) + } + if out.Attr.Size != 128 { + t.Fatalf("GetAttr size: got %d, want 128", out.Attr.Size) + } + if out.Attr.Nlink != 0 { + t.Fatalf("GetAttr nlink: got %d, want 0", out.Attr.Nlink) + } +} + +// TestXAttrOnUnlinkedOpenFile covers fsetxattr/fgetxattr/fremovexattr on a +// descriptor whose file has been unlinked. +func TestXAttrOnUnlinkedOpenFile(t *testing.T) { + wfs, inode, _ := newUnlinkedOpenFile(t) + + setIn := &fuse.SetXAttrIn{} + setIn.NodeId = inode + if status := wfs.SetXAttr(nil, setIn, "user.k", []byte("v")); status != fuse.OK { + t.Fatalf("SetXAttr on unlinked open file: got %v, want OK", status) + } + + header := &fuse.InHeader{NodeId: inode} + dest := make([]byte, 8) + n, status := wfs.GetXAttr(nil, header, "user.k", dest) + if status != fuse.OK { + t.Fatalf("GetXAttr on unlinked open file: got %v, want OK", status) + } + if string(dest[:n]) != "v" { + t.Fatalf("GetXAttr value: got %q, want %q", dest[:n], "v") + } + + if status := wfs.RemoveXAttr(nil, header, "user.k"); status != fuse.OK { + t.Fatalf("RemoveXAttr on unlinked open file: got %v, want OK", status) + } +}