From da087f77b392e8de065f9863cfec609eee23bf3e Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 26 Aug 2026 08:51:37 -0700 Subject: [PATCH] mount: stop a replaced rename destination from flushing over the rename (#10965) * mount: stop a replaced rename destination from flushing over the rename Rename replaces whatever the destination held, which deletes that entry, but only the source handle was told. A handle still open on the replaced entry went on flushing its metadata under that name, and on Windows -- where the close carrying the flush runs after the application's CloseHandle has already returned -- the flush landed after the rename and put the destination's old content back: dir Rename old_entry:{name:"src"} new_entry:{name:"dst" ... inode:...3416} doFlush /dst fh 1521468582993181449 /dst saveToStorage 1,6872462993 [0,3) flushMetadataToFiler /dst inode 11939747521756968515 InsertEntry /dst The next read of the destination returned the content the rename was supposed to replace. Unlink already handles this with markHandleDeleted, which raises the flag under the handle's flush lock so a flush already writing finishes first and any later one sees it; a rename that replaces an entry deletes it just the same, so it now does likewise. Verified on the Windows runner: TestRenameOverExisting 300/300, where the same loop reproduced the corruption twice without this. * test/winfsp: say which layer kept a renamed-away name The failure only reported the stat. Which layer answered narrows the search a lot: a listing reads no per-path cache, the mount's own forgets within a second, and a name that survives both is still in the meta cache. * mount: keep the destination barrier honest when the rename does not happen Two gaps in the barrier the previous commit put in front of a replaced rename destination: The flag was raised before the filer rename, which can still fail. The destination then stays exactly where it was, with its handle marked deleted and its dirty metadata silently dropped from then on, so a rename that returned an error has to put the flag back. The handle was only found through the path mapping, which Forget drops while the handle is still open. The source side already falls back to the inode the entry carries; the destination now does the same, off the entry the sticky-bit check had already loaded. * mount: let only the caller that raised a delete mark lift it Restoring the destination handle after a failed rename cleared isDeleted outright, so an unlink that marked the same handle in between lost its mark and a later flush could write the unlinked entry back. Every raise of the flag already happens under the handle's flush lock, so counting them there is enough to tell one caller's mark from another's: the rename lifts only the mark it made itself. * mount: drain the destination flush before marking it deleted A flush already queued for the destination belongs to the entry as it stands. Marking first meant the drain waited on a flush that then skipped its metadata as deleted and released its handle, so a rename that failed afterwards had nothing left to restore and the queued update was gone, its chunks orphaned. Draining first lets that flush finish as itself, before the rename has taken anything away. --- test/winfsp/semantics_test.go | 24 +++++++++++- weed/mount/filehandle.go | 6 ++- weed/mount/weedfs.go | 1 + weed/mount/weedfs_file_mkrm.go | 36 ++++++++++++++---- weed/mount/weedfs_rename.go | 69 +++++++++++++++++++++++++--------- 5 files changed, 109 insertions(+), 27 deletions(-) diff --git a/test/winfsp/semantics_test.go b/test/winfsp/semantics_test.go index 04abbdf95..12bf3fb26 100644 --- a/test/winfsp/semantics_test.go +++ b/test/winfsp/semantics_test.go @@ -180,11 +180,31 @@ func TestRenameOverExisting(t *testing.T) { // they indict different layers; a second look says whether it persists. time.Sleep(200 * time.Millisecond) fi2, err2 := os.Stat(src) - t.Fatalf("stat of the renamed-away source did not return not-exist: stat=%v err=%v; 200ms later stat=%v err=%v", - describeFileInfo(fi), err, describeFileInfo(fi2), err2) + // A listing reads no per-path cache and the mount's own forgets + // within a second, so a name that survives both is back on the filer. + listed := dirNames(t, dir) + time.Sleep(2 * time.Second) + _, errLater := os.Stat(src) + t.Fatalf("stat of the renamed-away source did not return not-exist: stat=%v err=%v; 200ms later stat=%v err=%v; past the path cache err=%v; %s lists %v", + describeFileInfo(fi), err, describeFileInfo(fi2), err2, errLater, dir, listed) } } +// dirNames lists a directory for a failure message, reporting the error in +// place of the names rather than failing a test that is already failing. +func dirNames(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + return []string{"readdir: " + err.Error()} + } + names := make([]string, 0, len(entries)) + for _, entry := range entries { + names = append(names, entry.Name()) + } + return names +} + func TestRenameAcrossDirectories(t *testing.T) { dir := testRoot(t) from := filepath.Join(dir, "from") diff --git a/weed/mount/filehandle.go b/weed/mount/filehandle.go index 98f7b274a..a2c5560d4 100644 --- a/weed/mount/filehandle.go +++ b/weed/mount/filehandle.go @@ -40,7 +40,11 @@ type FileHandle struct { savedName string // last known file name if inode-to-path state is forgotten isDeleted bool - isRenamed bool // set by Rename before waiting for async flush; skips old-path metadata flush + // deleteEpoch counts the times isDeleted was raised, all of them under the + // handle's flush lock. A caller that raised it and then found it had + // nothing to delete after all can tell its own mark from a later one. + deleteEpoch uint64 + isRenamed bool // set by Rename before waiting for async flush; skips old-path metadata flush // entryVersionTsNs is the filer log position the handle's entry reflects. // State at or below it must not replace the entry — that rolls it back. diff --git a/weed/mount/weedfs.go b/weed/mount/weedfs.go index 7a6b14168..45dc75d66 100644 --- a/weed/mount/weedfs.go +++ b/weed/mount/weedfs.go @@ -938,6 +938,7 @@ func (wfs *WFS) invalidateOpenFileHandle(invalidation meta_cache.EntryInvalidati // dirty pages stay, so the open fd still reads its buffered writes. if invalidation.Deleted { fh.isDeleted = true + fh.deleteEpoch++ } if !fh.dirtyMetadata { fh.dirtyPages.Destroy() diff --git a/weed/mount/weedfs_file_mkrm.go b/weed/mount/weedfs_file_mkrm.go index 316642bfc..d00f479d4 100644 --- a/weed/mount/weedfs_file_mkrm.go +++ b/weed/mount/weedfs_file_mkrm.go @@ -436,17 +436,39 @@ func (wfs *WFS) createRegularFile(dirFullPath util.FullPath, name string, mode u } // markHandleDeleted flags the inode's open handle so its flushes stop writing -// the entry back. Taken and released under the handle's flush lock: a flush -// already holding it finishes before the caller's delete runs, and any later -// flush sees the flag; setting the flag bare raced the flush's own check and -// resurrected the entry right after the delete. -func (wfs *WFS) markHandleDeleted(inode uint64) { +// the entry back, and reports which mark this was. Taken and released under +// the handle's flush lock: a flush already holding it finishes before the +// caller's delete runs, and any later flush sees the flag; setting the flag +// bare raced the flush's own check and resurrected the entry right after the +// delete. +func (wfs *WFS) markHandleDeleted(inode uint64) uint64 { + fh, found := wfs.fhMap.FindFileHandle(inode) + if !found { + return 0 + } + fhActiveLock := wfs.fhLockTable.AcquireLock("markHandleDeleted", fh.fh, util.ExclusiveLock) + fh.isDeleted = true + fh.deleteEpoch++ + epoch := fh.deleteEpoch + wfs.fhLockTable.ReleaseLock(fh.fh, fhActiveLock) + return epoch +} + +// clearHandleDeleted lifts a mark whose delete did not go through. Only the +// mark epoch names: an unlink that raised the flag after it owns the entry's +// fate and keeps it. +func (wfs *WFS) clearHandleDeleted(inode uint64, epoch uint64) { + if epoch == 0 { + return + } fh, found := wfs.fhMap.FindFileHandle(inode) if !found { return } - fhActiveLock := wfs.fhLockTable.AcquireLock("Unlink", fh.fh, util.ExclusiveLock) - fh.isDeleted = true + fhActiveLock := wfs.fhLockTable.AcquireLock("clearHandleDeleted", fh.fh, util.ExclusiveLock) + if fh.deleteEpoch == epoch { + fh.isDeleted = false + } wfs.fhLockTable.ReleaseLock(fh.fh, fhActiveLock) } diff --git a/weed/mount/weedfs_rename.go b/weed/mount/weedfs_rename.go index 995d34417..e4fd86682 100644 --- a/weed/mount/weedfs_rename.go +++ b/weed/mount/weedfs_rename.go @@ -214,17 +214,22 @@ func (wfs *WFS) Rename(cancel <-chan struct{}, in *fuse.RenameIn, oldName string } } - // POSIX: enforce sticky bit on the destination directory when replacing an existing entry. + var newEntry *filer_pb.Entry if in.Flags != RenameNoReplace { - if newEntry, _, newStatus := wfs.maybeLoadEntry(newPath); newStatus == fuse.OK && newEntry != nil { - if newDirEntry, _, dirCode := wfs.maybeLoadEntry(newDir); dirCode == fuse.OK && newDirEntry != nil && newDirEntry.Attributes != nil { - targetUid := uint32(0) - if newEntry.Attributes != nil { - targetUid = newEntry.Attributes.Uid - } - if code := checkStickyBit(newDirEntry.Attributes.FileMode, newDirEntry.Attributes.Uid, targetUid, in.Uid); code != fuse.OK { - return code - } + if loaded, _, newStatus := wfs.maybeLoadEntry(newPath); newStatus == fuse.OK { + newEntry = loaded + } + } + + // POSIX: enforce sticky bit on the destination directory when replacing an existing entry. + if newEntry != nil { + if newDirEntry, _, dirCode := wfs.maybeLoadEntry(newDir); dirCode == fuse.OK && newDirEntry != nil && newDirEntry.Attributes != nil { + targetUid := uint32(0) + if newEntry.Attributes != nil { + targetUid = newEntry.Attributes.Uid + } + if code := checkStickyBit(newDirEntry.Attributes.FileMode, newDirEntry.Attributes.Uid, targetUid, in.Uid); code != fuse.OK { + return code } } } @@ -240,10 +245,11 @@ func (wfs *WFS) Rename(cancel <-chan struct{}, in *fuse.RenameIn, oldName string // 1. deferFilerCreate=true — file handle still open, dirtyMetadata set. // 2. writebackCache — close() triggered async flush, handle released. // The filer rename will fail with ENOENT unless we flush/wait first. - if inode, found := wfs.inodeToPath.GetInode(oldPath); found { + sourceInode, sourceMapped := wfs.inodeToPath.GetInode(oldPath) + if sourceMapped { // Case 1: handle still open with deferred metadata — flush synchronously // BEFORE any async flush interference. - if fh, ok := wfs.fhMap.FindFileHandle(inode); ok && fh.dirtyMetadata { + if fh, ok := wfs.fhMap.FindFileHandle(sourceInode); ok && fh.dirtyMetadata { glog.V(4).Infof("dir Rename %s: flushing deferred metadata before rename", oldPath) // Prerequisite for the rename, so it must complete: non-cancellable context. if flushStatus := wfs.doFlush(context.Background(), fh, oldEntry.Attributes.Uid, oldEntry.Attributes.Gid, false); flushStatus != fuse.OK { @@ -255,14 +261,43 @@ func (wfs *WFS) Rename(cancel <-chan struct{}, in *fuse.RenameIn, oldName string // Mark ALL handles for this inode as renamed so the async flush // skips old-path metadata creation (which would re-insert the // renamed entry into the meta cache after rename events clean it up). - wfs.fhMap.MarkInodeRenamed(inode) - wfs.waitForPendingAsyncFlush(inode) + wfs.fhMap.MarkInodeRenamed(sourceInode) + wfs.waitForPendingAsyncFlush(sourceInode) } else if oldEntry != nil && oldEntry.Attributes != nil && oldEntry.Attributes.Inode != 0 { // GetInode failed (Forget already removed the mapping), but the // entry's stored inode can still identify pending async flushes. - inode = oldEntry.Attributes.Inode - wfs.fhMap.MarkInodeRenamed(inode) - wfs.waitForPendingAsyncFlush(inode) + sourceInode = oldEntry.Attributes.Inode + wfs.fhMap.MarkInodeRenamed(sourceInode) + wfs.waitForPendingAsyncFlush(sourceInode) + } + + // Replacing the destination deletes what it held, so a handle still open + // on that entry has to stop writing the name back, exactly as an unlink + // makes it. Windows is where this bites: the close carrying the flush runs + // after the application's CloseHandle returned, so the flush can land on + // top of what the rename just put there. + if in.Flags == RenameEmptyFlag { + targetInode, targetMapped := wfs.inodeToPath.GetInode(newPath) + if !targetMapped && newEntry != nil && newEntry.Attributes != nil { + // Forget dropped the mapping, but the entry's stored inode still + // names the handle, the way the source side falls back. + targetInode = newEntry.Attributes.Inode + } + if targetInode != 0 && targetInode != sourceInode { + // Drained before the mark, not after: a flush already queued + // belongs to the destination as it stands, and one that runs + // marked is skipped and takes its handle with it, past the reach + // of the restore below. + wfs.waitForPendingAsyncFlush(targetInode) + epoch := wfs.markHandleDeleted(targetInode) + // A rename that does not happen leaves the destination where it + // was, so its handle has to go back to writing that name. + defer func() { + if code != fuse.OK { + wfs.clearHandleDeleted(targetInode, epoch) + } + }() + } } // Acquire DLM locks on both old and new paths to prevent another mount