fix: Resolve deadlock in directory operations using generation-based invalidation (#39)

* fix: Resolve deadlock in directory operations using generation-based invalidation

This fixes issue #37 where tigrisfs would deadlock during concurrent directory
operations, particularly when listing directories.

The deadlock occurred because:
1. listObjectsFlat() held dh.mu while calling sealDir()
2. sealDir() -> removeExpired() -> removeChildUnlocked() tried to lock ALL
   directory handles' mutexes
3. Other threads could hold different directory handle mutexes while waiting
   for dh.mu, creating a circular lock dependency

The solution uses a generation counter approach:
- Each directory maintains an atomic generation counter
- The counter increments on structural changes (add/remove children)
- Directory handles check the generation before operations
- If generation changed, handles reset their position without locking

* fix: Address race condition in generation handling after sealDir

* fix: Initialize DirHandle generation correctly to prevent spurious invalidations

Author: ovaistariq

core/cluster_fs.go

@@ -430,6 +430,7 @@ func (fs *ClusterFs) readDir(handleId fuseops.HandleID, offset fuseops.DirOffset
 		dh.lastExternalOffset = 0
 		dh.lastInternalOffset = 0
 		dh.lastName = ""
+		dh.generation = atomic.LoadUint64(&dh.inode.dir.generation)
 	}
 
 	for {

core/cluster_fs_fuse.go

@@ -550,7 +550,10 @@ func (fs *ClusterFsFuse) OpenDir(ctx context.Context, op *fuseops.OpenDirOp) (er
 
 			// 2nd phase
 			fs.Goofys.mu.Lock()
-			dh := &DirHandle{inode: inode}
+			dh := &DirHandle{
+				inode:      inode,
+				generation: atomic.LoadUint64(&inode.dir.generation),
+			}
 			fs.Goofys.dirHandles[fuseops.HandleID(resp.HandleId)] = dh
 			fs.Goofys.mu.Unlock()
 

core/dir.go

@@ -60,6 +60,7 @@ type DirInodeData struct {
 	DeletedChildren map[string]*Inode
 	Gaps            []*SlurpGap
 	handles         []*DirHandle
+	generation      uint64 // incremented on structural changes
 }
 
 // Returns the position of first char < '/' in `inp` after prefixLen + any continued '/' characters.
@@ -87,11 +88,15 @@ type DirHandle struct {
 	// or from the previous offset
 	lastExternalOffset fuseops.DirOffset
 	lastInternalOffset int
+	generation         uint64 // tracks directory structure changes
 	lastName           string
 }
 
 func NewDirHandle(inode *Inode) (dh *DirHandle) {
-	dh = &DirHandle{inode: inode}
+	dh = &DirHandle{
+		inode:      inode,
+		generation: atomic.LoadUint64(&inode.dir.generation),
+	}
 	return
 }
 
@@ -385,7 +390,9 @@ func (dir *DirInodeData) checkGapLoaded(key string, newerThan time.Time) bool {
 	return false
 }
 
+// sealDir completes directory listing and cleans up expired entries
 // LOCKS_REQUIRED(inode.mu)
+// LOCKS_EXCLUDED(dh.mu for all directory handles)
 func (inode *Inode) sealDir() {
 	inode.dir.listMarker = ""
 	inode.dir.listDone = true
@@ -397,6 +404,10 @@ func (inode *Inode) sealDir() {
 	} else {
 		inode.Attributes.Mtime, inode.Attributes.Ctime = inode.findChildMaxTime()
 	}
+	
+	// Increment generation to signal all handles need revalidation
+	atomic.AddUint64(&inode.dir.generation, 1)
+	
 	inode.removeExpired("")
 }
 
@@ -622,7 +633,14 @@ func (dh *DirHandle) listObjectsFlat() (start string, err error) {
 			dh.inode.dir.listMarker = lastName
 		}
 	} else {
+		// We must release dh.mu before calling sealDir to avoid deadlock
+		dh.mu.Unlock()
 		dh.inode.sealDir()
+		// Reload generation immediately after sealDir completes to get accurate state
+		currentGen := atomic.LoadUint64(&dh.inode.dir.generation)
+		dh.mu.Lock()
+		// Update our generation to match the new state
+		dh.generation = currentGen
 	}
 
 	dh.inode.mu.Unlock()
@@ -633,6 +651,16 @@ func (dh *DirHandle) listObjectsFlat() (start string, err error) {
 // LOCKS_REQUIRED(dh.mu)
 // LOCKS_REQUIRED(dh.inode.mu)
 func (dh *DirHandle) checkDirPosition() {
+	// Check if directory structure changed since we last checked
+	// Note: There's a benign race here where generation could change between
+	// the load and assignment. This is acceptable as we'll catch it on the
+	// next operation. The worst case is an unnecessary position reset.
+	currentGen := atomic.LoadUint64(&dh.inode.dir.generation)
+	if dh.generation != currentGen {
+		dh.lastInternalOffset = -1
+		dh.generation = currentGen
+	}
+	
 	if dh.lastInternalOffset < 0 {
 		parent := dh.inode
 		// Directory position invalidated, try to find it again using lastName
@@ -752,6 +780,7 @@ func (dh *DirHandle) Seek(newOffset fuseops.DirOffset) {
 		dh.lastExternalOffset = 0
 		dh.lastInternalOffset = 0
 		dh.lastName = ""
+		dh.generation = atomic.LoadUint64(&dh.inode.dir.generation)
 	}
 }
 
@@ -996,6 +1025,10 @@ func (parent *Inode) removeChildUnlocked(inode *Inode) {
 	if l == 0 {
 		return
 	}
+	
+	// Increment generation to invalidate all directory handles
+	atomic.AddUint64(&parent.dir.generation, 1)
+	
 	i := sort.Search(l, parent.findInodeFunc(inode.Name))
 	if i >= l || parent.dir.Children[i].Name != inode.Name {
 		panic(fmt.Sprintf("%v.removeName(%v) but child not found: %v",
@@ -1004,11 +1037,7 @@ func (parent *Inode) removeChildUnlocked(inode *Inode) {
 
 	// POSIX allows parallel readdir() and modifications,
 	// so preserve position of all directory handles
-	for _, dh := range parent.dir.handles {
-		dh.mu.Lock()
-		dh.lastInternalOffset = -1
-		dh.mu.Unlock()
-	}
+	// Handles will detect the generation change and reset themselves
 	// >= because we use the "last open dir" as the "next" one
 	if parent.dir.lastOpenDirIdx >= i {
 		parent.dir.lastOpenDirIdx--
@@ -1038,11 +1067,8 @@ func (parent *Inode) removeAllChildrenUnlocked() {
 		child.DeRef(1)
 		child.mu.Unlock()
 	}
-	// POSIX allows parallel readdir() and modifications,
-	// so reset position of all directory handles
-	for _, dh := range parent.dir.handles {
-		dh.lastInternalOffset = -1
-	}
+	// Increment generation to invalidate all directory handles
+	atomic.AddUint64(&parent.dir.generation, 1)
 	parent.dir.Children = nil
 }
 
@@ -1091,11 +1117,8 @@ func (parent *Inode) insertChildUnlocked(inode *Inode) {
 			panic(fmt.Sprintf("double insert of %v", parent.getChildName(inode.Name)))
 		}
 
-		// POSIX allows parallel readdir() and modifications,
-		// so preserve position of all directory handles
-		for _, dh := range parent.dir.handles {
-			dh.lastInternalOffset = -1
-		}
+		// Increment generation to invalidate all directory handles
+		atomic.AddUint64(&parent.dir.generation, 1)
 		if parent.dir.lastOpenDirIdx >= i {
 			parent.dir.lastOpenDirIdx++
 		}