From b4ccd7020ad564476d6d6f09474ad7c34f35e000 Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Thu, 14 Sep 2023 18:42:14 -0400 Subject: [PATCH] vfs: enforce mutual exclusion in MemFS.Lock Previously MemFS's Lock method did not implement mutual exclusion under the assumption that databases are given separate processes which do not share memory. This commit adapts Lock to enforce mutual exclusion, preventing accidentally using same FS concurrently from more than one Pebble instance. It's believed that cockroachdb/cockroach#110645 is the result of Cockroach acceptance tests opening a MemFS store twice, with one Pebble instance deleting files from beneath another Pebble instance. --- vfs/mem_fs.go | 47 +++++++++++++++++++++++++++-- vfs/mem_fs_test.go | 65 +++++++++++++++++++++++++++++++++++++++++ vfs/testdata/memfs_lock | 55 ++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 vfs/testdata/memfs_lock diff --git a/vfs/mem_fs.go b/vfs/mem_fs.go index bb9440a0da..632f013996 100644 --- a/vfs/mem_fs.go +++ b/vfs/mem_fs.go @@ -14,6 +14,7 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "time" "github.com/cockroachdb/errors" @@ -78,6 +79,10 @@ 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. + // string path -> bool + lockedFiles sync.Map strict bool ignoreSyncs bool // Windows has peculiar semantics with respect to hard links and deleting @@ -457,9 +462,30 @@ 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, true) + 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 + // file, 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 +813,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() +} diff --git a/vfs/mem_fs_test.go b/vfs/mem_fs_test.go index 84e23de1bd..592b392a28 100644 --- a/vfs/mem_fs_test.go +++ b/vfs/mem_fs_test.go @@ -5,6 +5,7 @@ package vfs import ( + "fmt" "io" "os" "sort" @@ -12,6 +13,7 @@ import ( "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= handle= 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= 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= + 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) + } + }) +} diff --git a/vfs/testdata/memfs_lock b/vfs/testdata/memfs_lock new file mode 100644 index 0000000000..0ac265580e --- /dev/null +++ b/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