diff --git a/weed/mount/posix_file_lock.go b/weed/mount/posix_file_lock.go index c277e5f88..075b97e12 100644 --- a/weed/mount/posix_file_lock.go +++ b/weed/mount/posix_file_lock.go @@ -27,6 +27,12 @@ type inodeLocks struct { locks []lockRange // currently held locks, sorted by Start waiters []*lockWaiter // blocked SetLkw callers wakeRefs int // woken waiters still retrying on this inodeLocks + // dead marks an inodeLocks that has been removed from the table's map. + // A caller holding a pointer returned by getInodeLocks/getOrCreateInodeLocks + // must recheck this flag after acquiring il.mu: if true, the pointer is + // orphaned and must be refreshed from the table so findConflict runs + // against the live state. + dead bool } // lockWaiter represents a blocked SetLkw caller. @@ -48,12 +54,17 @@ func NewPosixLockTable() *PosixLockTable { } } -// getOrCreateInodeLocks returns the lock state for an inode, creating it if needed. +// getOrCreateInodeLocks returns the lock state for an inode, creating it if +// needed. A dead entry (which maybeCleanupInode normally evicts together +// with setting dead) is replaced transparently so callers never observe one +// via this path and the SetLk retry loop cannot spin on a stale map entry. +// The il.dead read is safe without il.mu: dead transitions only under both +// plt.mu and il.mu, so holding plt.mu here is sufficient happens-before. func (plt *PosixLockTable) getOrCreateInodeLocks(inode uint64) *inodeLocks { plt.mu.Lock() defer plt.mu.Unlock() il, ok := plt.inodes[inode] - if !ok { + if !ok || il.dead { il = &inodeLocks{} plt.inodes[inode] = il } @@ -61,14 +72,29 @@ func (plt *PosixLockTable) getOrCreateInodeLocks(inode uint64) *inodeLocks { } // getInodeLocks returns the lock state for an inode, or nil if none exists. +// A dead entry (which maybeCleanupInode normally evicts together with setting +// dead) is dropped so callers never observe an orphaned inodeLocks via this +// path. The il.dead read is safe without il.mu for the same reason as in +// getOrCreateInodeLocks: transitions happen under both plt.mu and il.mu. func (plt *PosixLockTable) getInodeLocks(inode uint64) *inodeLocks { plt.mu.Lock() defer plt.mu.Unlock() - return plt.inodes[inode] + il, ok := plt.inodes[inode] + if !ok { + return nil + } + if il.dead { + delete(plt.inodes, inode) + return nil + } + return il } // maybeCleanupInode removes the inodeLocks entry if it has no locks, no waiters, -// and no woken waiters still retrying against this inodeLocks. +// and no woken waiters still retrying against this inodeLocks. The removed +// inodeLocks is marked dead so any caller that is mid-way through acquiring +// il.mu with a stale pointer will notice and refetch from the map instead of +// mutating an orphaned instance (which would be invisible to future callers). func (plt *PosixLockTable) maybeCleanupInode(inode uint64, il *inodeLocks) { // Caller must NOT hold il.mu. We acquire both locks in the correct order. plt.mu.Lock() @@ -76,7 +102,10 @@ func (plt *PosixLockTable) maybeCleanupInode(inode uint64, il *inodeLocks) { il.mu.Lock() defer il.mu.Unlock() if len(il.locks) == 0 && len(il.waiters) == 0 && il.wakeRefs == 0 { - delete(plt.inodes, inode) + if plt.inodes[inode] == il { + delete(plt.inodes, inode) + } + il.dead = true } } @@ -247,22 +276,32 @@ func removeWaiter(il *inodeLocks, w *lockWaiter) { // GetLk checks for a conflicting lock. If found, it populates out with the // conflict details. If no conflict, out.Typ is set to F_UNLCK. func (plt *PosixLockTable) GetLk(inode uint64, proposed lockRange, out *fuse.LkOut) { - il := plt.getInodeLocks(inode) - if il == nil { - out.Lk.Typ = syscall.F_UNLCK - return - } - il.mu.Lock() - conflict, found := findConflict(il.locks, proposed) - il.mu.Unlock() + for { + il := plt.getInodeLocks(inode) + if il == nil { + out.Lk.Typ = syscall.F_UNLCK + return + } + il.mu.Lock() + if il.dead { + // Orphaned by a concurrent cleanup: the map may already hold a + // fresh inodeLocks with different state, so answering from this + // stale pointer would miss real conflicts. Refetch. + il.mu.Unlock() + continue + } + conflict, found := findConflict(il.locks, proposed) + il.mu.Unlock() - if found { - out.Lk.Start = conflict.Start - out.Lk.End = conflict.End - out.Lk.Typ = conflict.Typ - out.Lk.Pid = conflict.Pid - } else { - out.Lk.Typ = syscall.F_UNLCK + if found { + out.Lk.Start = conflict.Start + out.Lk.End = conflict.End + out.Lk.Typ = conflict.Typ + out.Lk.Pid = conflict.Pid + } else { + out.Lk.Typ = syscall.F_UNLCK + } + return } } @@ -276,6 +315,13 @@ func (plt *PosixLockTable) SetLk(inode uint64, lk lockRange) fuse.Status { return fuse.OK } il.mu.Lock() + if il.dead { + // Orphaned by a concurrent cleanup: any lock we may have held + // is gone with it, and there is no point waking waiters on a + // detached inodeLocks. + il.mu.Unlock() + return fuse.OK + } removeLocks(il, func(existing lockRange) bool { return existing.Owner == lk.Owner && existing.IsFlock == lk.IsFlock }, lk.Start, lk.End) @@ -285,15 +331,21 @@ func (plt *PosixLockTable) SetLk(inode uint64, lk lockRange) fuse.Status { return fuse.OK } - il := plt.getOrCreateInodeLocks(inode) - il.mu.Lock() - if _, found := findConflict(il.locks, lk); found { + for { + il := plt.getOrCreateInodeLocks(inode) + il.mu.Lock() + if il.dead { + il.mu.Unlock() + continue + } + if _, found := findConflict(il.locks, lk); found { + il.mu.Unlock() + return fuse.EAGAIN + } + insertAndCoalesce(il, lk) il.mu.Unlock() - return fuse.EAGAIN + return fuse.OK } - insertAndCoalesce(il, lk) - il.mu.Unlock() - return fuse.OK } // SetLkw attempts a blocking lock. It waits until the lock can be acquired @@ -307,6 +359,15 @@ func (plt *PosixLockTable) SetLkw(inode uint64, lk lockRange, cancel <-chan stru var waiter *lockWaiter for { il.mu.Lock() + if il.dead { + // The inodeLocks we were holding was orphaned by a concurrent + // cleanup. Refetch the live instance from the table and retry. + // A dead il never has waiters or wakeRefs (the cleanup condition + // requires both to be zero), so waiter is guaranteed nil here. + il.mu.Unlock() + il = plt.getOrCreateInodeLocks(inode) + continue + } releaseWakeRef(il, waiter) if _, found := findConflict(il.locks, lk); !found { insertAndCoalesce(il, lk) diff --git a/weed/mount/posix_file_lock_test.go b/weed/mount/posix_file_lock_test.go index 61d9a158f..37a4633dc 100644 --- a/weed/mount/posix_file_lock_test.go +++ b/weed/mount/posix_file_lock_test.go @@ -2,6 +2,9 @@ package mount import ( "math" + "runtime" + "sync" + "sync/atomic" "syscall" "testing" "time" @@ -615,3 +618,207 @@ func TestAdjacencyNoOverflowAtMaxUint64(t *testing.T) { t.Fatalf("expected 2 separate locks (no overflow merge), got %d", ownerLocks) } } + +// TestSetLkRetriesPastDeadInodeLocks deterministically exercises the +// getOrCreateInodeLocks vs maybeCleanupInode race: a caller holding a +// pointer to an inodeLocks that is concurrently marked dead must refetch +// from the map instead of mutating the orphaned instance (which would be +// invisible to subsequent callers and let two exclusive flock holders +// coexist). The test bypasses scheduling by hand-installing a dead il into +// the table and asserting that the next SetLk routes around it. +func TestSetLkRetriesPastDeadInodeLocks(t *testing.T) { + plt := NewPosixLockTable() + inode := uint64(42) + + // Acquire and release a lock so maybeCleanupInode marks the il dead and + // removes it from the map. + lock := lockRange{Start: 0, End: math.MaxUint64, Typ: syscall.F_WRLCK, Owner: 1, IsFlock: true} + if s := plt.SetLk(inode, lock); s != fuse.OK { + t.Fatalf("prime SetLk: got %v", s) + } + dead := plt.getInodeLocks(inode) + unlock := lock + unlock.Typ = syscall.F_UNLCK + if s := plt.SetLk(inode, unlock); s != fuse.OK { + t.Fatalf("prime unlock: got %v", s) + } + if !dead.dead { + t.Fatal("expected il to be marked dead after unlock+cleanup") + } + plt.mu.Lock() + _, stillMapped := plt.inodes[inode] + plt.mu.Unlock() + if stillMapped { + t.Fatal("expected the dead il to be removed from the map") + } + + // Simulate the race: the next caller's getOrCreateInodeLocks races with + // the cleanup and ends up holding a pointer to the dead il. We force that + // state by re-publishing `dead` into the map. + plt.mu.Lock() + plt.inodes[inode] = dead + plt.mu.Unlock() + + // SetLk must notice dead, refetch, and install the new lock in a fresh il. + if s := plt.SetLk(inode, lockRange{Start: 0, End: math.MaxUint64, Typ: syscall.F_WRLCK, Owner: 2, IsFlock: true}); s != fuse.OK { + t.Fatalf("SetLk after dead: got %v", s) + } + + dead.mu.Lock() + if n := len(dead.locks); n != 0 { + t.Fatalf("dead il should not have accepted the insert, found %d locks", n) + } + dead.mu.Unlock() + + plt.mu.Lock() + live := plt.inodes[inode] + plt.mu.Unlock() + if live == nil || live == dead { + t.Fatalf("expected a fresh live il, got %v", live) + } + + // A conflicting owner must see the new lock and be rejected. + if s := plt.SetLk(inode, lockRange{Start: 0, End: math.MaxUint64, Typ: syscall.F_WRLCK, Owner: 3, IsFlock: true}); s != fuse.EAGAIN { + t.Fatalf("second owner should conflict with owner 2, got %v", s) + } + + // GetLk must report the conflict as well: without the dead-recheck the + // GetLk path would answer F_UNLCK off the orphaned il. + var out fuse.LkOut + plt.GetLk(inode, lockRange{Start: 0, End: math.MaxUint64, Typ: syscall.F_WRLCK, Owner: 4, IsFlock: true}, &out) + if out.Lk.Typ != syscall.F_WRLCK { + t.Fatalf("GetLk should report the live conflict, got Typ=%d", out.Lk.Typ) + } +} + +// TestGetInodeLocksEvictsDeadEntry verifies that a dead inodeLocks which +// somehow ends up in the map (e.g. through a future refactor that reorders +// delete and dead=true) is dropped on read so callers never observe one. +// This is the backstop that lets GetLk's and SetLk's retry loops terminate. +func TestGetInodeLocksEvictsDeadEntry(t *testing.T) { + plt := NewPosixLockTable() + inode := uint64(42) + + lock := lockRange{Start: 0, End: math.MaxUint64, Typ: syscall.F_WRLCK, Owner: 1, IsFlock: true} + if s := plt.SetLk(inode, lock); s != fuse.OK { + t.Fatalf("prime SetLk: got %v", s) + } + dead := plt.getInodeLocks(inode) + unlock := lock + unlock.Typ = syscall.F_UNLCK + if s := plt.SetLk(inode, unlock); s != fuse.OK { + t.Fatalf("prime unlock: got %v", s) + } + if !dead.dead { + t.Fatal("expected dead after cleanup") + } + + // Force the broken state that production cannot reach but tests and + // future refactors might: dead entry still in the map. + plt.mu.Lock() + plt.inodes[inode] = dead + plt.mu.Unlock() + + if il := plt.getInodeLocks(inode); il != nil { + t.Fatalf("getInodeLocks should drop a dead map entry, got %p", il) + } + plt.mu.Lock() + _, stillMapped := plt.inodes[inode] + plt.mu.Unlock() + if stillMapped { + t.Fatal("expected dead entry to be removed from the map") + } + + // getOrCreateInodeLocks must also self-heal (replace the dead entry with + // a fresh live one) so SetLk's retry path cannot spin. + plt.mu.Lock() + plt.inodes[inode] = dead + plt.mu.Unlock() + fresh := plt.getOrCreateInodeLocks(inode) + if fresh == dead { + t.Fatal("getOrCreateInodeLocks should not return a dead entry") + } + if fresh.dead { + t.Fatal("fresh entry should not be dead") + } +} + +// TestConcurrentFlockChurnPreservesMutualExclusion is a stress companion to +// the deterministic tests above. It uses a Swap+CAS detector that flags +// overlap at two points (on acquire and on release), so a second granted +// holder is caught even if it sneaks in after the first goroutine's claim +// but before its release. 16 goroutines churn whole-file exclusive flock on +// one inode; with the race the detector fires hundreds of times per run, +// with the fix it stays at zero. +func TestConcurrentFlockChurnPreservesMutualExclusion(t *testing.T) { + plt := NewPosixLockTable() + const ( + inode = uint64(42) + numWorkers = 16 + iterations = 500 + ) + var ( + wg sync.WaitGroup + holder atomic.Int64 // 0 = nobody; otherwise = holder's claim token + overlapSeen atomic.Int32 + ) + + for w := 0; w < numWorkers; w++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + owner := uint64(100 + id) + lock := lockRange{ + Start: 0, + End: math.MaxUint64, + Typ: syscall.F_WRLCK, + Owner: owner, + Pid: uint32(id + 1), + IsFlock: true, + } + unlock := lock + unlock.Typ = syscall.F_UNLCK + token := int64(id + 1) + for i := 0; i < iterations; i++ { + // SetLk(WRLCK) may only return OK (granted) or EAGAIN + // (conflict); anything else indicates a bug and the test + // must fail rather than spin. Use Errorf + return because + // Fatalf is not safe from a non-test goroutine. + for { + s := plt.SetLk(inode, lock) + if s == fuse.OK { + break + } + if s != fuse.EAGAIN { + t.Errorf("worker %d iter %d: unexpected SetLk(WRLCK) status %v", id, i, s) + return + } + runtime.Gosched() + } + // Claim the slot. If Swap observes a non-zero predecessor, + // another goroutine already believes it holds the lock. + if prev := holder.Swap(token); prev != 0 { + overlapSeen.Add(1) + } + // Widen the window so a concurrently-granted peer has a + // chance to race into its own Swap before we release. + runtime.Gosched() + runtime.Gosched() + // Release the slot. If CAS fails someone else overwrote our + // claim, which only happens when two holders raced. + if !holder.CompareAndSwap(token, 0) { + overlapSeen.Add(1) + } + if s := plt.SetLk(inode, unlock); s != fuse.OK { + t.Errorf("worker %d iter %d: unexpected SetLk(UNLCK) status %v", id, i, s) + return + } + } + }(w) + } + wg.Wait() + + if n := overlapSeen.Load(); n != 0 { + t.Fatalf("flock overlap detected %d times: two owners simultaneously granted the same exclusive lock", n) + } +}