Fixed up advice for handling already-exists cases when creating names.

Added tests that confirm behavior of "real" file systems, then made memfs make those tests pass, too. Doing this by returning EEXIST appears to work on both OS X and Linux, but I can't find documentation anywhere that says this is what is expected. More: https://github.com/osxfuse/osxfuse/issues/209 http://stackoverflow.com/q/30364838/1505451 http://sourceforge.net/p/fuse/mailman/message/34130996/
2015-05-21 15:30:37 +10:00 · 2015-05-21 15:30:37 +10:00 · 1e7620a379
parent f385176a96 bd4af3f5a5
commit 1e7620a379
5 changed files with 420 additions and 13 deletions
--- a/errors.go
+++ b/errors.go
@ -23,6 +23,7 @@ import (
 const (
 	// Errors corresponding to kernel error numbers. These may be treated
 	// specially by fuseops.Op.Respond methods.
+	EEXIST    = bazilfuse.EEXIST
 	EINVAL    = bazilfuse.Errno(syscall.EINVAL)
 	EIO       = bazilfuse.EIO
 	ENOENT    = bazilfuse.ENOENT
--- a/fuseops/ops.go
+++ b/fuseops/ops.go
@ -275,10 +275,14 @@ func (o *ForgetInodeOp) toBazilfuseResponse() (bfResp interface{}) {
 // Create a directory inode as a child of an existing directory inode. The
 // kernel sends this in response to a mkdir(2) call.
 //
-// The kernel appears to verify the name doesn't already exist (mkdir calls
-// mkdirat calls user_path_create calls filename_create, which verifies:
-// http://goo.gl/FZpLu5). But volatile file systems and paranoid non-volatile
-// file systems should check for the reasons described below on CreateFile.
+// The Linux kernel appears to verify the name doesn't already exist (mkdir
+// calls mkdirat calls user_path_create calls filename_create, which verifies:
+// http://goo.gl/FZpLu5). Indeed, the tests in samples/memfs that call in
+// parallel appear to bear this out. But osxfuse does not appear to guarantee
+// this (cf. https://goo.gl/PqzZDv). And if names may be created outside of the
+// kernel's control, it doesn't matter what the kernel does anyway.
+//
+// Therefore the file system should return EEXIST if the name already exists.
 type MkDirOp struct {
 	commonOp

@ -314,15 +318,12 @@ func (o *MkDirOp) toBazilfuseResponse() (bfResp interface{}) {
 //
 // The kernel sends this when the user asks to open a file with the O_CREAT
 // flag and the kernel has observed that the file doesn't exist. (See for
-// example lookup_open, http://goo.gl/PlqE9d).
+// example lookup_open, http://goo.gl/PlqE9d). However, osxfuse doesn't appear
+// to make this check atomically (cf. https://goo.gl/PqzZDv). And if names may
+// be created outside of the kernel's control, it doesn't matter what the
+// kernel does anyway.
 //
-// However it's impossible to tell for sure that all kernels make this check
-// in all cases and the official fuse documentation is less than encouraging
-// (" the file does not exist, first create it with the specified mode, and
-// then open it"). Therefore file systems would be smart to be paranoid and
-// check themselves, returning EEXIST when the file already exists. This of
-// course particularly applies to file systems that are volatile from the
-// kernel's point of view.
+// Therefore the file system should return EEXIST if the name already exists.
 type CreateFileOp struct {
 	commonOp

@ -371,7 +372,8 @@ func (o *CreateFileOp) toBazilfuseResponse() (bfResp interface{}) {
 	return
 }

-// Create a symlink inode.
+// Create a symlink inode. If the name already exists, the file system should
+// return EEXIST (cf. the notes on CreateFileOp and MkDirOp).
 type CreateSymlinkOp struct {
 	commonOp

--- a/samples/memfs/memfs.go
+++ b/samples/memfs/memfs.go
@ -301,6 +301,14 @@ func (fs *memFS) MkDir(
 	parent := fs.getInodeForModifyingOrDie(op.Parent)
 	defer parent.mu.Unlock()

+	// Ensure that the name doesn't already exist, so we don't wind up with a
+	// duplicate.
+	_, exists := parent.LookUpChild(op.Name)
+	if exists {
+		err = fuse.EEXIST
+		return
+	}
+
 	// Set up attributes from the child, using the credentials of the calling
 	// process as owner (matching inode_init_owner, cf. http://goo.gl/5qavg8).
 	childAttrs := fuseops.InodeAttributes{
@ -341,6 +349,14 @@ func (fs *memFS) CreateFile(
 	parent := fs.getInodeForModifyingOrDie(op.Parent)
 	defer parent.mu.Unlock()

+	// Ensure that the name doesn't already exist, so we don't wind up with a
+	// duplicate.
+	_, exists := parent.LookUpChild(op.Name)
+	if exists {
+		err = fuse.EEXIST
+		return
+	}
+
 	// Set up attributes from the child, using the credentials of the calling
 	// process as owner (matching inode_init_owner, cf. http://goo.gl/5qavg8).
 	now := fs.clock.Now()
@ -388,6 +404,14 @@ func (fs *memFS) CreateSymlink(
 	parent := fs.getInodeForModifyingOrDie(op.Parent)
 	defer parent.mu.Unlock()

+	// Ensure that the name doesn't already exist, so we don't wind up with a
+	// duplicate.
+	_, exists := parent.LookUpChild(op.Name)
+	if exists {
+		err = fuse.EEXIST
+		return
+	}
+
 	// Set up attributes from the child, using the credentials of the calling
 	// process as owner (matching inode_init_owner, cf. http://goo.gl/5qavg8).
 	now := fs.clock.Now()
--- a/samples/memfs/memfs_test.go
+++ b/samples/memfs/memfs_test.go
@ -1251,3 +1251,23 @@ func (t *MemFSTest) DeleteSymlink() {
 	AssertEq(nil, err)
 	ExpectThat(entries, ElementsAre())
 }
+
+func (t *MemFSTest) CreateInParallel_NoTruncate() {
+	runCreateInParallelTest_NoTruncate(t.Ctx, t.Dir)
+}
+
+func (t *MemFSTest) CreateInParallel_Truncate() {
+	runCreateInParallelTest_Truncate(t.Ctx, t.Dir)
+}
+
+func (t *MemFSTest) CreateInParallel_Exclusive() {
+	runCreateInParallelTest_Exclusive(t.Ctx, t.Dir)
+}
+
+func (t *MemFSTest) MkdirInParallel() {
+	runMkdirInParallelTest(t.Ctx, t.Dir)
+}
+
+func (t *MemFSTest) SymlinkInParallel() {
+	runSymlinkInParallelTest(t.Ctx, t.Dir)
+}
--- a/samples/memfs/posix_test.go
+++ b/samples/memfs/posix_test.go
@ -18,13 +18,20 @@
 package memfs_test

 import (
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
 	"path"
 	"runtime"
+	"sync/atomic"
 	"testing"
+	"time"

+	"golang.org/x/net/context"
+
+	"github.com/jacobsa/fuse/fusetesting"
+	"github.com/jacobsa/gcloud/syncutil"
 	. "github.com/jacobsa/oglematchers"
 	. "github.com/jacobsa/ogletest"
 )
@ -41,11 +48,342 @@ func getFileOffset(f *os.File) (offset int64, err error) {
 	return
 }

+func runCreateInParallelTest_NoTruncate(
+	ctx context.Context,
+	dir string) {
+	// Ensure that we get parallelism for this test.
+	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+
+	// Try for awhile to see if anything breaks.
+	const duration = 500 * time.Millisecond
+	startTime := time.Now()
+	for time.Since(startTime) < duration {
+		filename := path.Join(dir, "foo")
+
+		// Set up a function that opens the file with O_CREATE and then appends a
+		// byte to it.
+		worker := func(id byte) (err error) {
+			f, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Open: %v", id, err)
+				return
+			}
+
+			defer f.Close()
+
+			_, err = f.Write([]byte{id})
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Write: %v", id, err)
+				return
+			}
+
+			return
+		}
+
+		// Run several workers in parallel.
+		const numWorkers = 16
+		b := syncutil.NewBundle(ctx)
+		for i := 0; i < numWorkers; i++ {
+			id := byte(i)
+			b.Add(func(ctx context.Context) (err error) {
+				err = worker(id)
+				return
+			})
+		}
+
+		err := b.Join()
+		AssertEq(nil, err)
+
+		// Read the contents of the file. We should see each worker's ID once.
+		contents, err := ioutil.ReadFile(filename)
+		AssertEq(nil, err)
+
+		idsSeen := make(map[byte]struct{})
+		for i, _ := range contents {
+			id := contents[i]
+			AssertLt(id, numWorkers)
+
+			if _, ok := idsSeen[id]; ok {
+				AddFailure("Duplicate ID: %d", id)
+			}
+
+			idsSeen[id] = struct{}{}
+		}
+
+		AssertEq(numWorkers, len(idsSeen))
+
+		// Delete the file.
+		err = os.Remove(filename)
+		AssertEq(nil, err)
+	}
+}
+
+func runCreateInParallelTest_Truncate(
+	ctx context.Context,
+	dir string) {
+	// Ensure that we get parallelism for this test.
+	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+
+	// Try for awhile to see if anything breaks.
+	const duration = 500 * time.Millisecond
+	startTime := time.Now()
+	for time.Since(startTime) < duration {
+		filename := path.Join(dir, "foo")
+
+		// Set up a function that opens the file with O_CREATE and O_TRUNC and then
+		// appends a byte to it.
+		worker := func(id byte) (err error) {
+			f, err := os.OpenFile(
+				filename,
+				os.O_CREATE|os.O_WRONLY|os.O_APPEND|os.O_TRUNC,
+				0600)
+
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Open: %v", id, err)
+				return
+			}
+
+			defer f.Close()
+
+			_, err = f.Write([]byte{id})
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Write: %v", id, err)
+				return
+			}
+
+			return
+		}
+
+		// Run several workers in parallel.
+		const numWorkers = 16
+		b := syncutil.NewBundle(ctx)
+		for i := 0; i < numWorkers; i++ {
+			id := byte(i)
+			b.Add(func(ctx context.Context) (err error) {
+				err = worker(id)
+				return
+			})
+		}
+
+		err := b.Join()
+		AssertEq(nil, err)
+
+		// Read the contents of the file. We should see at least one ID (the last
+		// one that truncated), and at most all of them.
+		contents, err := ioutil.ReadFile(filename)
+		AssertEq(nil, err)
+
+		idsSeen := make(map[byte]struct{})
+		for i, _ := range contents {
+			id := contents[i]
+			AssertLt(id, numWorkers)
+
+			if _, ok := idsSeen[id]; ok {
+				AddFailure("Duplicate ID: %d", id)
+			}
+
+			idsSeen[id] = struct{}{}
+		}
+
+		AssertGe(len(idsSeen), 1)
+		AssertLe(len(idsSeen), numWorkers)
+
+		// Delete the file.
+		err = os.Remove(filename)
+		AssertEq(nil, err)
+	}
+}
+
+func runCreateInParallelTest_Exclusive(
+	ctx context.Context,
+	dir string) {
+	// Ensure that we get parallelism for this test.
+	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+
+	// Try for awhile to see if anything breaks.
+	const duration = 500 * time.Millisecond
+	startTime := time.Now()
+	for time.Since(startTime) < duration {
+		filename := path.Join(dir, "foo")
+
+		// Set up a function that opens the file with O_CREATE and O_EXCL, and then
+		// appends a byte to it if it was successfully opened.
+		var openCount uint64
+		worker := func(id byte) (err error) {
+			f, err := os.OpenFile(
+				filename,
+				os.O_CREATE|os.O_EXCL|os.O_WRONLY|os.O_APPEND,
+				0600)
+
+			// If we failed to open due to the file already existing, just leave.
+			if os.IsExist(err) {
+				err = nil
+				return
+			}
+
+			// Propgate other errors.
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Open: %v", id, err)
+				return
+			}
+
+			atomic.AddUint64(&openCount, 1)
+			defer f.Close()
+
+			_, err = f.Write([]byte{id})
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Write: %v", id, err)
+				return
+			}
+
+			return
+		}
+
+		// Run several workers in parallel.
+		const numWorkers = 16
+		b := syncutil.NewBundle(ctx)
+		for i := 0; i < numWorkers; i++ {
+			id := byte(i)
+			b.Add(func(ctx context.Context) (err error) {
+				err = worker(id)
+				return
+			})
+		}
+
+		err := b.Join()
+		AssertEq(nil, err)
+
+		// Exactly one worker should have opened successfully.
+		AssertEq(1, openCount)
+
+		// Read the contents of the file. It should contain that one worker's ID.
+		contents, err := ioutil.ReadFile(filename)
+		AssertEq(nil, err)
+
+		AssertEq(1, len(contents))
+		AssertLt(contents[0], numWorkers)
+
+		// Delete the file.
+		err = os.Remove(filename)
+		AssertEq(nil, err)
+	}
+}
+
+func runMkdirInParallelTest(
+	ctx context.Context,
+	dir string) {
+	// Ensure that we get parallelism for this test.
+	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+
+	// Try for awhile to see if anything breaks.
+	const duration = 500 * time.Millisecond
+	startTime := time.Now()
+	for time.Since(startTime) < duration {
+		filename := path.Join(dir, "foo")
+
+		// Set up a function that creates the directory, ignoring EEXIST errors.
+		worker := func(id byte) (err error) {
+			err = os.Mkdir(filename, 0700)
+
+			if os.IsExist(err) {
+				err = nil
+			}
+
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Mkdir: %v", id, err)
+				return
+			}
+
+			return
+		}
+
+		// Run several workers in parallel.
+		const numWorkers = 16
+		b := syncutil.NewBundle(ctx)
+		for i := 0; i < numWorkers; i++ {
+			id := byte(i)
+			b.Add(func(ctx context.Context) (err error) {
+				err = worker(id)
+				return
+			})
+		}
+
+		err := b.Join()
+		AssertEq(nil, err)
+
+		// The directory should have been created, once.
+		entries, err := fusetesting.ReadDirPicky(dir)
+		AssertEq(nil, err)
+		AssertEq(1, len(entries))
+		AssertEq("foo", entries[0].Name())
+
+		// Delete the directory.
+		err = os.Remove(filename)
+		AssertEq(nil, err)
+	}
+}
+
+func runSymlinkInParallelTest(
+	ctx context.Context,
+	dir string) {
+	// Ensure that we get parallelism for this test.
+	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+
+	// Try for awhile to see if anything breaks.
+	const duration = 500 * time.Millisecond
+	startTime := time.Now()
+	for time.Since(startTime) < duration {
+		filename := path.Join(dir, "foo")
+
+		// Set up a function that creates the symlink, ignoring EEXIST errors.
+		worker := func(id byte) (err error) {
+			err = os.Symlink("blah", filename)
+
+			if os.IsExist(err) {
+				err = nil
+			}
+
+			if err != nil {
+				err = fmt.Errorf("Worker %d: Symlink: %v", id, err)
+				return
+			}
+
+			return
+		}
+
+		// Run several workers in parallel.
+		const numWorkers = 16
+		b := syncutil.NewBundle(ctx)
+		for i := 0; i < numWorkers; i++ {
+			id := byte(i)
+			b.Add(func(ctx context.Context) (err error) {
+				err = worker(id)
+				return
+			})
+		}
+
+		err := b.Join()
+		AssertEq(nil, err)
+
+		// The symlink should have been created, once.
+		entries, err := fusetesting.ReadDirPicky(dir)
+		AssertEq(nil, err)
+		AssertEq(1, len(entries))
+		AssertEq("foo", entries[0].Name())
+
+		// Delete the directory.
+		err = os.Remove(filename)
+		AssertEq(nil, err)
+	}
+}
+
 ////////////////////////////////////////////////////////////////////////
 // Boilerplate
 ////////////////////////////////////////////////////////////////////////

 type PosixTest struct {
+	ctx context.Context
+
 	// A temporary directory.
 	dir string

@ -61,6 +399,8 @@ func init() { RegisterTestSuite(&PosixTest{}) }
 func (t *PosixTest) SetUp(ti *TestInfo) {
 	var err error

+	t.ctx = ti.Ctx
+
 	// Create a temporary directory.
 	t.dir, err = ioutil.TempDir("", "posix_test")
 	if err != nil {
@ -418,3 +758,23 @@ func (t *PosixTest) RmdirWhileOpenedForReading() {
 		ExpectThat(names, ElementsAre())
 	}
 }
+
+func (t *PosixTest) CreateInParallel_NoTruncate() {
+	runCreateInParallelTest_NoTruncate(t.ctx, t.dir)
+}
+
+func (t *PosixTest) CreateInParallel_Truncate() {
+	runCreateInParallelTest_Truncate(t.ctx, t.dir)
+}
+
+func (t *PosixTest) CreateInParallel_Exclusive() {
+	runCreateInParallelTest_Exclusive(t.ctx, t.dir)
+}
+
+func (t *PosixTest) MkdirInParallel() {
+	runMkdirInParallelTest(t.ctx, t.dir)
+}
+
+func (t *PosixTest) SymlinkInParallel() {
+	runSymlinkInParallelTest(t.ctx, t.dir)
+}