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

@@ -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")

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.

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()

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

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