vfs: enforce mutual exclusion in MemFS.Lock

vfs/mem_fs.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 	"sync"
 	"sync/atomic"
+	"syscall"
 	"time"
 
 	"github.com/cockroachdb/errors"
@@ -78,6 +79,11 @@ type MemFS struct {
 	mu   sync.Mutex
 	root *memNode
 
+	// lockFiles holds a map of open file locks. Presence in this map indicates
+	// a file lock is currently held. Keys are strings holding the path of the
+	// locked file. The stored value is untyped and  unused; only presence of
+	// the key within the map is significant.
+	lockedFiles sync.Map
 	strict      bool
 	ignoreSyncs bool
 	// Windows has peculiar semantics with respect to hard links and deleting
@@ -457,9 +463,31 @@ func (y *MemFS) MkdirAll(dirname string, perm os.FileMode) error {
 // Lock implements FS.Lock.
 func (y *MemFS) Lock(fullname string) (io.Closer, error) {
 	// FS.Lock excludes other processes, but other processes cannot see this
-	// process' memory. We translate Lock into Create so that have the normal
-	// detection of non-existent directory paths.
-	return y.Create(fullname)
+	// process' memory. However some uses (eg, Cockroach tests) may open and
+	// close the same MemFS-backed database multiple times. We want mutual
+	// exclusion in this case too. See cockroachdb/cockroach#110645.
+	_, loaded := y.lockedFiles.Swap(fullname, nil /* the value itself is insignificant */)
+	if loaded {
+		// This file lock has already been acquired. On unix, this results in
+		// either EACCES or EAGAIN so we mimic.
+		return nil, syscall.EAGAIN
+	}
+	// Otherwise, we successfully acquired the lock. Locks are visible in the
+	// parent directory listing, and they also must be created under an existent
+	// 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)
+	if err != nil {
+		// "Release" the lock since we failed.
+		y.lockedFiles.Delete(fullname)
+		return nil, err
+	}
+	return &memFileLock{
+		y:        y,
+		f:        f,
+		fullname: fullname,
+	}, nil
 }
 
 // List implements FS.List.
@@ -787,3 +815,18 @@ func (f *memFile) Fd() uintptr {
 func (f *memFile) Flush() error {
 	return nil
 }
+
+type memFileLock struct {
+	y        *MemFS
+	f        File
+	fullname string
+}
+
+func (l *memFileLock) Close() error {
+	if l.y == nil {
+		return nil
+	}
+	l.y.lockedFiles.Delete(l.fullname)
+	l.y = nil
+	return l.f.Close()
+}

vfs/mem_fs_test.go

@@ -5,13 +5,15 @@
 package vfs
 
 import (
+	"fmt"
 	"io"
 	"os"
 	"sort"
 	"strconv"
 	"strings"
 	"testing"
 
+	"github.com/cockroachdb/datadriven"
 	"github.com/stretchr/testify/require"
 )
 
@@ -393,3 +395,66 @@ func TestStrictFS(t *testing.T) {
 	}
 	runTestCases(t, testCases, fs)
 }
+
+func TestMemFSLock(t *testing.T) {
+	filesystems := map[string]FS{}
+	fileLocks := map[string]io.Closer{}
+
+	datadriven.RunTest(t, "testdata/memfs_lock", func(t *testing.T, td *datadriven.TestData) string {
+		switch td.Cmd {
+		case "mkfs":
+			for _, arg := range td.CmdArgs {
+				filesystems[arg.String()] = NewMem()
+			}
+			return "OK"
+
+		// lock fs=<filesystem-name> handle=<handle> path=<path>
+		case "lock":
+			var filesystemName string
+			var path string
+			var handle string
+			td.ScanArgs(t, "fs", &filesystemName)
+			td.ScanArgs(t, "path", &path)
+			td.ScanArgs(t, "handle", &handle)
+			fs := filesystems[filesystemName]
+			if fs == nil {
+				return fmt.Sprintf("filesystem %q doesn't exist", filesystemName)
+			}
+			l, err := fs.Lock(path)
+			if err != nil {
+				return err.Error()
+			}
+			fileLocks[handle] = l
+			return "OK"
+
+		// mkdirall fs=<filesystem-name> path=<path>
+		case "mkdirall":
+			var filesystemName string
+			var path string
+			td.ScanArgs(t, "fs", &filesystemName)
+			td.ScanArgs(t, "path", &path)
+			fs := filesystems[filesystemName]
+			if fs == nil {
+				return fmt.Sprintf("filesystem %q doesn't exist", filesystemName)
+			}
+			err := fs.MkdirAll(path, 0755)
+			if err != nil {
+				return err.Error()
+			}
+			return "OK"
+
+		// close handle=<handle>
+		case "close":
+			var handle string
+			td.ScanArgs(t, "handle", &handle)
+			err := fileLocks[handle].Close()
+			delete(fileLocks, handle)
+			if err != nil {
+				return err.Error()
+			}
+			return "OK"
+		default:
+			return fmt.Sprintf("unrecognized command %q", td.Cmd)
+		}
+	})
+}

vfs/testdata/memfs_lock

@@ -0,0 +1,55 @@
+mkfs A B
+----
+OK
+
+#
+# Locking a path with parents that don't exist should error.
+#
+
+lock fs=A path=a/b/c handle=fsApathABC
+----
+open a/b/c: file does not exist
+
+#
+# If we create the parents, it should succeed.
+#
+
+mkdirall fs=A path=a/b
+----
+OK
+
+lock fs=A path=a/b/c handle=fsApathABC
+----
+OK
+
+#
+# Locking the same path on the same filesystem should fail with EAGAIN.
+#
+
+lock fs=A path=a/b/c handle=bogus
+----
+resource temporarily unavailable
+
+#
+# Locking the same path on a DIFFERENT filesystem should succeed.
+#
+
+mkdirall fs=B path=a/b
+----
+OK
+
+lock fs=B path=a/b/c handle=fsBpathABC
+----
+OK
+
+#
+# Releasing the lock on fs A should allow us to reacquire it.
+#
+
+close handle=fsApathABC
+----
+OK
+
+lock fs=A path=a/b/c handle=fsApathABC
+----
+OK