vfs: fix MemFS cloneMu recursive RLock deadlock

Three MemFS methods deadlock against concurrent CrashClone due to
recursive cloneMu.RLock() acquisition. Go's sync.RWMutex is
writer-preferring: once CrashClone is waiting for the write lock, any
new RLock call blocks behind it. When a method already holding RLock
calls another method that also tries RLock, neither can proceed.

The three affected methods and their recursive locking chains:

  - ReuseForWrite: takes RLock, then calls Rename which takes RLock
  - Link: takes RLock at entry, then takes RLock again inside inner
    walk callback (copy-paste of the locking boilerplate)
  - Lock: takes RLock, then calls Create which takes RLock

The bug was introduced in a70d5b3 ("vfs: redesign MemFS strict mode",
2024-08-26), which replaced the old SetIgnoreSyncs/ResetToSyncedState
approach with the CrashClone model. That commit systematically added
cloneMu.RLock() to every mutation method, but did not trace call graphs
to catch methods that compose other MemFS methods.

The deadlock has existed for ~20 months as a sporadic CI flake (#4677,
#5199, #5327). CrashClone fires only once per test iteration (when the
error injector's write counter hits zero), so the contention window is
extremely narrow on fast hardware. It surfaces reliably only on slow
platforms: s390x QEMU (~10x slower), Windows CI, linux-race. Each
occurrence was closed without a root cause until #5987 provided a full
goroutine dump showing the RWMutex.Lock / RWMutex.RLock deadlock.

Fix: extract lock-free internal methods (rename, create) that assume the
caller already holds cloneMu. The public Rename/Create methods delegate
to these after acquiring the lock. ReuseForWrite calls rename() and Lock
calls create() to avoid the recursive acquisition. For Link, the inner
RLock/RUnlock is simply removed since the outer lock at method entry
already covers the entire body. Add a comment on the cloneMu field
documenting the recursive-locking constraint to prevent recurrence.

Add TestMemFSCrashCloneConcurrency which exercises concurrent CrashClone
against ReuseForWrite, Link, and Lock. Without this fix, the test
deadlocks within seconds.

Fixes #5987.
Informs #5959, #5972.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>

Author: xxmplus

vfs/mem_fs.go

@@ -85,6 +85,11 @@ type MemFS struct {
 
 	// cloneMu is used to block all modification operations while we clone the
 	// filesystem. Only used when crashable is true.
+	//
+	// Methods holding cloneMu must not call other public MemFS methods that also
+	// acquire cloneMu; use the unexported variants (rename, create) instead.
+	// Go's sync.RWMutex is writer-preferring, so recursive RLock deadlocks when
+	// a writer (CrashClone) is waiting.
 	cloneMu sync.RWMutex
 
 	// lockFiles holds a map of open file locks. Presence in this map indicates
@@ -224,6 +229,12 @@ func (y *MemFS) Create(fullname string, category DiskWriteCategory) (File, error
 		y.cloneMu.RLock()
 		defer y.cloneMu.RUnlock()
 	}
+	return y.create(fullname, category)
+}
+
+// create is the lock-free internal implementation of Create. The caller must
+// hold cloneMu when crashable is true.
+func (y *MemFS) create(fullname string, category DiskWriteCategory) (File, error) {
 	var ret *memFile
 	err := y.walk(fullname, func(dir *memNode, frag string, final bool) error {
 		if final {
@@ -281,8 +292,6 @@ func (y *MemFS) Link(oldname, newname string) error {
 			if frag == "" {
 				return errors.New("pebble/vfs: empty file name")
 			}
-			y.cloneMu.RLock()
-			defer y.cloneMu.RUnlock()
 			if _, ok := dir.children[frag]; ok {
 				return &os.LinkError{
 					Op:  "link",
@@ -423,6 +432,12 @@ func (y *MemFS) Rename(oldname, newname string) error {
 		y.cloneMu.RLock()
 		defer y.cloneMu.RUnlock()
 	}
+	return y.rename(oldname, newname)
+}
+
+// rename is the lock-free internal implementation of Rename. The caller must
+// hold cloneMu when crashable is true.
+func (y *MemFS) rename(oldname, newname string) error {
 	var n *memNode
 	err := y.walk(oldname, func(dir *memNode, frag string, final bool) error {
 		if final {
@@ -461,7 +476,7 @@ func (y *MemFS) ReuseForWrite(oldname, newname string, category DiskWriteCategor
 		y.cloneMu.RLock()
 		defer y.cloneMu.RUnlock()
 	}
-	if err := y.Rename(oldname, newname); err != nil {
+	if err := y.rename(oldname, newname); err != nil {
 		return nil, err
 	}
 	f, err := y.Open(newname)
@@ -530,7 +545,7 @@ func (y *MemFS) Lock(fullname string) (io.Closer, error) {
 	// directory. Create the path so that we have the normal detection of
 	// non-existent directory paths, and make the lock visible when listing
 	// directory entries.
-	f, err := y.Create(fullname, WriteCategoryUnspecified)
+	f, err := y.create(fullname, WriteCategoryUnspecified)
 	if err != nil {
 		// "Release" the lock since we failed.
 		y.lockedFiles.Delete(fullname)

vfs/mem_fs_test.go

@@ -5,13 +5,16 @@
 package vfs
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"math/rand/v2"
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/datadriven"
 	"github.com/stretchr/testify/require"
@@ -140,6 +143,117 @@ func TestMemFile(t *testing.T) {
 	}
 }
 
+// TestMemFSCrashCloneConcurrency exercises concurrent CrashClone (write lock)
+// against ReuseForWrite, Link, and Lock (all of which take read locks and call
+// other methods that also take read locks). Before the fix in this package,
+// these methods would recursively acquire cloneMu.RLock(), deadlocking when
+// CrashClone was waiting for the write lock (Go's sync.RWMutex is
+// writer-preferring, blocking new RLocks behind a pending Lock).
+func TestMemFSCrashCloneConcurrency(t *testing.T) {
+	const workers = 4
+	const duration = 5 * time.Second
+
+	fs := NewCrashableMem()
+	// Pre-create a directory and seed files for ReuseForWrite and Link.
+	require.NoError(t, fs.MkdirAll("/d", 0755))
+	for i := 0; i < workers; i++ {
+		name := fmt.Sprintf("/d/src-%d", i)
+		f, err := fs.Create(name, WriteCategoryUnspecified)
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), duration)
+	defer cancel()
+
+	var g sync.WaitGroup
+
+	// CrashClone goroutines (writer lock).
+	for i := 0; i < workers; i++ {
+		g.Add(1)
+		go func() {
+			defer g.Done()
+			rng := rand.New(rand.NewPCG(0, uint64(i)))
+			for ctx.Err() == nil {
+				fs.CrashClone(CrashCloneCfg{UnsyncedDataPercent: 50, RNG: rng})
+			}
+		}()
+	}
+
+	// ReuseForWrite goroutines.
+	for i := 0; i < workers; i++ {
+		g.Add(1)
+		go func() {
+			defer g.Done()
+			a := fmt.Sprintf("/d/reuse-a-%d", i)
+			b := fmt.Sprintf("/d/reuse-b-%d", i)
+			// Seed initial file.
+			f, err := fs.Create(a, WriteCategoryUnspecified)
+			if err != nil {
+				return
+			}
+			_ = f.Close()
+			useA := true
+			for ctx.Err() == nil {
+				var from, to string
+				if useA {
+					from, to = a, b
+				} else {
+					from, to = b, a
+				}
+				f, err := fs.ReuseForWrite(from, to, WriteCategoryUnspecified)
+				if err != nil {
+					// File may have been removed by CrashClone side effects;
+					// recreate and retry.
+					f2, err2 := fs.Create(a, WriteCategoryUnspecified)
+					if err2 == nil {
+						_ = f2.Close()
+					}
+					useA = true
+					continue
+				}
+				_ = f.Close()
+				useA = !useA
+			}
+		}()
+	}
+
+	// Link goroutines.
+	for i := 0; i < workers; i++ {
+		g.Add(1)
+		go func() {
+			defer g.Done()
+			src := fmt.Sprintf("/d/src-%d", i)
+			n := 0
+			for ctx.Err() == nil {
+				dst := fmt.Sprintf("/d/link-%d-%d", i, n)
+				_ = fs.Link(src, dst)
+				// Clean up to avoid unbounded growth.
+				_ = fs.Remove(dst)
+				n++
+			}
+		}()
+	}
+
+	// Lock goroutines.
+	for i := 0; i < workers; i++ {
+		g.Add(1)
+		go func() {
+			defer g.Done()
+			name := fmt.Sprintf("/d/lock-%d", i)
+			for ctx.Err() == nil {
+				closer, err := fs.Lock(name)
+				if err != nil {
+					continue
+				}
+				_ = closer.Close()
+			}
+		}()
+	}
+
+	g.Wait()
+}
+
 func TestMemFSLock(t *testing.T) {
 	filesystems := map[string]FS{}
 	fileLocks := map[string]io.Closer{}