From b1fecf3b44fce36a8218b4a1da3b6d1a7c420d49 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 4 Aug 2026 16:42:33 -0700 Subject: [PATCH] mount: mark windows files archived and ignore a zero timestamp (#10559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * mount: mark windows files archived and ignore a zero timestamp Windows synthesises NORMAL when a file reports no attributes at all, which is not the same as ARCHIVE and is what create_fileattr_test checks. Utimens also wrote a zero timestamp through. Windows sends zero for a field it is not setting, and storing it put 1970 in the atime overlay, which then overrode the entry's real time — so a file created a moment ago reported an access time of 1970 whenever the caller asked through an open handle. Reading the path instead went down a different route and looked right, which is why a probe of a fresh file showed nothing wrong. * mount: match the file type by its mask, and only treat the epoch as unset S_IFDIR is part of the multi-bit type field rather than a flag, so masking against it alone also matched a symlink, which shares the bit. A regular file is now identified by the type mask. Rejecting every timestamp at or below zero also rejected a date genuinely before 1970. Only the epoch itself is what Windows sends for a field it is not setting, so that is all that is refused. create_fileattr goes back on the known-failures list: the archive fix works and the test simply moves on to ask for READONLY too, which needs Chflags. Taking it off was premature. * mount: drop the time overlays when an inode is released atimeMap and dirMtimeMap are keyed by inode and were only ever trimmed by a random eviction at capacity. Inodes are derived from the path, so a delete and recreate hands the same number to a different file, which then reported the previous file's access time — a file created a moment ago answering with a time from long before it existed. Cleared when Forget actually releases the inode, not on every decrement: a partial forget still has users. Forget now reports that so callers holding state keyed by the inode know when to drop it. * ci: keep getfileinfo listed while its access time is unexplained Two causes have been fixed and neither closed it, so the honest state is listed-with-a-reason rather than removed in hope. * mount: drop timestamp overlays while the inode table is locked Forget released the inode under the table's lock but cleaned up the atime and dir-mtime overlays after returning from it. Inode numbers are derived from the path, so a lookup arriving in that window is handed the same number back and can store a time that the cleanup then deletes. Run the cleanup at the release point instead, as a callback under the lock. The directory-cache purge stays deferred until after the unlock, where it has to be. Claude-Session: https://claude.ai/code/session_01EgY2QA3iiPtiu6ww3P2EBn --- test/winfsp-conformance/known_failures.txt | 13 ++++++-- weed/mount/filehandle_test.go | 4 +-- weed/mount/inode_to_path.go | 9 +++++- weed/mount/inode_to_path_test.go | 36 +++++++++++++++++++++- weed/mount/weedfs_attr.go | 14 +++++++++ weed/mount/weedfs_forget.go | 16 ++++++---- weed/mount/winfsp/fs_windows.go | 25 ++++++++++++--- 7 files changed, 100 insertions(+), 17 deletions(-) diff --git a/test/winfsp-conformance/known_failures.txt b/test/winfsp-conformance/known_failures.txt index e4833a615..a45c838b4 100644 --- a/test/winfsp-conformance/known_failures.txt +++ b/test/winfsp-conformance/known_failures.txt @@ -58,11 +58,18 @@ exec_rename_dir_test # File information # ---------------- -# Windows attributes (hidden, system, readonly) and creation time are not -# round-tripped: attrToStat leaves Flags unset and reports ctime as birthtime. +# A regular file reports as archived now, but hidden, system and readonly are +# still not round-tripped: create_fileattr gets past the archive check and then +# asks for READONLY as well, which needs Chflags. Creation time is reported as +# ctime because the raw protocol's Attr carries no field for it outside darwin. create_fileattr_test -create_readonlydir_test +# getfileinfo asserts a file created a moment ago has an access time within ten +# seconds of now, and it does not. Two causes have been fixed and neither was +# it: Utimens no longer stores a zero timestamp, and the in-memory overlays are +# dropped when an inode is released so a recycled number cannot inherit an old +# time. Still unexplained, so it stays listed rather than pretended about. getfileinfo_test +create_readonlydir_test setfileinfo_test # Directory enumeration diff --git a/weed/mount/filehandle_test.go b/weed/mount/filehandle_test.go index 2c3539b81..ebf3cd51f 100644 --- a/weed/mount/filehandle_test.go +++ b/weed/mount/filehandle_test.go @@ -20,7 +20,7 @@ func TestFileHandleFullPathFallsBackAfterForget(t *testing.T) { } fh.RememberPath(fullPath) - wfs.inodeToPath.Forget(inode, 1, nil) + wfs.inodeToPath.Forget(inode, 1, nil, nil) if got := fh.FullPath(); got != fullPath { t.Fatalf("FullPath() after forget = %q, want %q", got, fullPath) @@ -44,7 +44,7 @@ func TestFileHandleFullPathUsesSavedRenamePathAfterForget(t *testing.T) { wfs.inodeToPath.MovePath(oldPath, newPath) fh.RememberPath(newPath) - wfs.inodeToPath.Forget(inode, 1, nil) + wfs.inodeToPath.Forget(inode, 1, nil, nil) if got := fh.FullPath(); got != newPath { t.Fatalf("FullPath() after rename+forget = %q, want %q", got, newPath) diff --git a/weed/mount/inode_to_path.go b/weed/mount/inode_to_path.go index 59dfe835b..e0129db1a 100644 --- a/weed/mount/inode_to_path.go +++ b/weed/mount/inode_to_path.go @@ -510,7 +510,11 @@ func (i *InodeToPath) MovePath(sourcePath, targetPath util.FullPath) (sourceInod return } -func (i *InodeToPath) Forget(inode, nlookup uint64, onForgetDir func(dir util.FullPath)) { +// Forget drops nlookup references. onRelease, if given, runs at the moment the +// inode is released and while the table is still locked: state keyed by the +// inode number has to be dropped there, because the number is derived from the +// path and a lookup arriving after the unlock would be handed the same one. +func (i *InodeToPath) Forget(inode, nlookup uint64, onRelease func(inode uint64), onForgetDir func(dir util.FullPath)) { var dirPaths []util.FullPath callOnForgetDir := false @@ -525,6 +529,9 @@ func (i *InodeToPath) Forget(inode, nlookup uint64, onForgetDir func(dir util.Fu } glog.V(4).Infof("kernel forget: inode %d paths %v nlookup %d", inode, path.paths, path.nlookup) if path.nlookup == 0 { + if onRelease != nil { + onRelease(inode) + } if _, isDir := i.dirStates[inode]; isDir && onForgetDir != nil { dirPaths = append([]util.FullPath(nil), path.paths...) callOnForgetDir = true diff --git a/weed/mount/inode_to_path_test.go b/weed/mount/inode_to_path_test.go index 93f0ccfb6..3699653a3 100644 --- a/weed/mount/inode_to_path_test.go +++ b/weed/mount/inode_to_path_test.go @@ -120,7 +120,7 @@ func TestOnlyDirectoriesGetDirState(t *testing.T) { } // forgetting the directory drops its dirState - itp.Forget(dirInode, 1, nil) + itp.Forget(dirInode, 1, nil, nil) if _, ok := itp.dirStates[dirInode]; ok { t.Fatal("forgotten directory must be removed from dirStates") } @@ -165,3 +165,37 @@ func TestMarkChildrenCachedClearsReadThroughMode(t *testing.T) { t.Fatal("directory should leave read-through mode after caching") } } + +// State keyed by an inode number has to be dropped while the table is locked. +// The numbers come from the path, so a lookup racing a forget is handed the +// same one back, and a cleanup running after the unlock would wipe what that +// lookup just stored. +func TestForgetOnReleaseRunsUnderLock(t *testing.T) { + itp := NewInodeToPath(util.FullPath("/"), 60) + file := util.FullPath("/data/f.txt") + inode := itp.Lookup(file, time.Now().Unix(), false, false, 0, true) + + itp.Lookup(file, time.Now().Unix(), false, false, 0, true) // nlookup is 2 now + + calls := 0 + onRelease := func(released uint64) { + calls++ + if released != inode { + t.Errorf("released inode = %d, want %d", released, inode) + } + if itp.TryLock() { + itp.Unlock() + t.Error("onRelease ran outside the critical section") + } + } + + itp.Forget(inode, 1, onRelease, nil) + if calls != 0 { + t.Fatalf("partial forget must not release: onRelease called %d times", calls) + } + + itp.Forget(inode, 1, onRelease, nil) + if calls != 1 { + t.Fatalf("onRelease called %d times, want 1", calls) + } +} diff --git a/weed/mount/weedfs_attr.go b/weed/mount/weedfs_attr.go index 8d26da4cd..9b42ceb51 100644 --- a/weed/mount/weedfs_attr.go +++ b/weed/mount/weedfs_attr.go @@ -387,6 +387,20 @@ func (wfs *WFS) setAtime(inode uint64, t time.Time) { } // applyInMemoryAtime overlays the in-memory atime onto a fuse.Attr if present. +// forgetInMemoryTimes drops the overlays for an inode. Both maps are keyed by +// inode and inodes are derived from the path, so a delete and recreate can +// hand the same number to a different file — which would then inherit the +// previous one's access or modification time. +func (wfs *WFS) forgetInMemoryTimes(inode uint64) { + wfs.atimeMu.Lock() + delete(wfs.atimeMap, inode) + wfs.atimeMu.Unlock() + + wfs.dirMtimeMu.Lock() + delete(wfs.dirMtimeMap, inode) + wfs.dirMtimeMu.Unlock() +} + func (wfs *WFS) applyInMemoryAtime(out *fuse.Attr, inode uint64) { wfs.atimeMu.Lock() if t, ok := wfs.atimeMap[inode]; ok { diff --git a/weed/mount/weedfs_forget.go b/weed/mount/weedfs_forget.go index fb7c986f7..ab9d1ac59 100644 --- a/weed/mount/weedfs_forget.go +++ b/weed/mount/weedfs_forget.go @@ -65,10 +65,14 @@ func (wfs *WFS) Forget(nodeid, nlookup uint64) { // lifecycle is driven independently by FUSE Open/Release — touching the // fhMap here would couple two unrelated refcounts and could tear down a // still-live handle if Forget ever raced ahead of Release. - wfs.inodeToPath.Forget(nodeid, nlookup, func(dir util.FullPath) { - // Runs after Forget releases its lock; a concurrent lookup+rebuild can - // re-cache the directory in that window, so purge through the apply loop - // rather than wiping the store directly. - wfs.purgeDirectoryCache(dir) - }) + wfs.inodeToPath.Forget(nodeid, nlookup, + // Runs at the release, under the table's lock, so a lookup that + // rebuilds the same inode number afterwards keeps the times it sets. + wfs.forgetInMemoryTimes, + func(dir util.FullPath) { + // Runs after Forget releases its lock; a concurrent lookup+rebuild can + // re-cache the directory in that window, so purge through the apply loop + // rather than wiping the store directly. + wfs.purgeDirectoryCache(dir) + }) } diff --git a/weed/mount/winfsp/fs_windows.go b/weed/mount/winfsp/fs_windows.go index f5ab7f212..34a5c8dcf 100644 --- a/weed/mount/winfsp/fs_windows.go +++ b/weed/mount/winfsp/fs_windows.go @@ -231,9 +231,16 @@ func (w *WinFS) attrToStat(attr *fuse.Attr, stat *cgofuse.Stat_t) { stat.Atim = cgofuse.Timespec{Sec: int64(attr.Atime), Nsec: int64(attr.Atimensec)} stat.Mtim = cgofuse.Timespec{Sec: int64(attr.Mtime), Nsec: int64(attr.Mtimensec)} stat.Ctim = cgofuse.Timespec{Sec: int64(attr.Ctime), Nsec: int64(attr.Ctimensec)} - // Windows shows a creation time and has nothing to derive it from; ctime - // is the closest the filer tracks. + // The filer records a creation time, but the raw protocol's Attr carries no + // field for it outside darwin, so ctime stands in until there is a way to + // read it through. stat.Birthtim = stat.Ctim + // Windows expects a regular file to be marked archived; with no flags at + // all it synthesises NORMAL, which is a different thing and what + // create_fileattr_test checks. + if attr.Mode&cgofuse.S_IFMT == cgofuse.S_IFREG { + stat.Flags |= cgofuse.UF_ARCHIVE + } } // translateOpenFlags converts cgofuse's open flags, which follow MSVC's @@ -534,9 +541,19 @@ func (w *WinFS) Utimens(path string, tmsp []cgofuse.Timespec) int { // applyTimespec reports whether a timestamp should be written. UTIME_OMIT asks // for the existing value to be kept, and a time at or below Windows' own 1601 -// epoch arrives as a large negative second count that would be stored verbatim. +// epoch arrives as a large negative second count. Exactly zero is what Windows +// sends for a field it is not setting; storing it put 1970 in the atime +// overlay, which then read back as the file's access time. A date genuinely +// before 1970 is still allowed through — only the epoch itself is the +// sentinel. func applyTimespec(ts cgofuse.Timespec) bool { - return ts.Nsec != utimeOmit && ts.Sec > windowsEpochCutoff + if ts.Nsec == utimeOmit { + return false + } + if ts.Sec == 0 && ts.Nsec == 0 { + return false + } + return ts.Sec > windowsEpochCutoff } func (w *WinFS) Flush(path string, fh uint64) int {