mount: delay rename if file has open writers instead of failing outright - fixes #2130 (#2249)

cmd/mountlib/mounttest/edge_cases.go

@@ -0,0 +1,59 @@
+package mounttest
+
+import (
+	"os"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestTouchAndDelete checks that writing a zero byte file and immediately
+// deleting it is not racy. See https://github.com/ncw/rclone/issues/1181
+func TestTouchAndDelete(t *testing.T) {
+	run.skipIfNoFUSE(t)
+	run.checkDir(t, "")
+
+	run.createFile(t, "touched", "")
+	run.rm(t, "touched")
+
+	run.checkDir(t, "")
+}
+
+// TestRenameOpenHandle checks that a file with open writers is successfully
+// renamed after all writers close. See https://github.com/ncw/rclone/issues/2130
+func TestRenameOpenHandle(t *testing.T) {
+	run.skipIfNoFUSE(t)
+	if runtime.GOOS == "windows" {
+		t.Skip("Skipping test on Windows")
+	}
+
+	run.checkDir(t, "")
+
+	// create file
+	example := []byte("Some Data")
+	path := run.path("rename")
+	file, err := osCreate(path)
+	require.NoError(t, err)
+
+	// write some data
+	_, err = file.Write(example)
+	require.NoError(t, err)
+	err = file.Sync()
+	require.NoError(t, err)
+
+	// attempt to rename open file
+	err = os.Rename(path, path+"bla")
+	require.NoError(t, err)
+
+	// close open writers to allow rename on remote to go through
+	err = file.Close()
+	require.NoError(t, err)
+
+	// verify file was renamed properly
+	run.checkDir(t, "renamebla 9")
+
+	// cleanup
+	run.rm(t, "renamebla")
+	run.checkDir(t, "")
+}

cmd/mountlib/mounttest/fs.go

@@ -52,6 +52,8 @@ func RunTests(t *testing.T, fn MountFn) {
 		run.cacheMode(cacheMode)
 		log.Printf("Starting test run with cache mode %v", cacheMode)
 		ok := t.Run(fmt.Sprintf("CacheMode=%v", cacheMode), func(t *testing.T) {
+			t.Run("TestTouchAndDelete", TestTouchAndDelete)
+			t.Run("TestRenameOpenHandle", TestRenameOpenHandle)
 			t.Run("TestDirLs", TestDirLs)
 			t.Run("TestDirCreateAndRemoveDir", TestDirCreateAndRemoveDir)
 			t.Run("TestDirCreateAndRemoveFile", TestDirCreateAndRemoveFile)

vfs/dir.go

@@ -442,29 +442,25 @@ func (d *Dir) Rename(oldName, newName string, destDir *Dir) error {
 	}
 	switch x := oldNode.DirEntry().(type) {
 	case nil:
-		fs.Errorf(oldPath, "Dir.Rename cant rename open file")
-		return EPERM
-	case fs.Object:
-		oldObject := x
-		// FIXME: could Copy then Delete if Move not available
-		// - though care needed if case insensitive...
-		doMove := d.f.Features().Move
-		if doMove == nil {
-			err := errors.Errorf("Fs %q can't rename files (no Move)", d.f)
-			fs.Errorf(oldPath, "Dir.Rename error: %v", err)
-			return err
-		}
-		newObject, err := doMove(oldObject, newPath)
-		if err != nil {
-			fs.Errorf(oldPath, "Dir.Rename error: %v", err)
-			return err
-		}
-		// Update the node with the new details
-		if oldNode != nil {
-			if oldFile, ok := oldNode.(*File); ok {
-				fs.Debugf(x, "Updating file with %v %p", newObject, oldFile)
-				oldFile.rename(destDir, newObject)
+		if oldFile, ok := oldNode.(*File); ok {
+			if err = oldFile.rename(destDir, newName); err != nil {
+				fs.Errorf(oldPath, "Dir.Rename error: %v", err)
+				return err
 			}
+		} else {
+			fs.Errorf(oldPath, "Dir.Rename can't rename open file that is not a vfs.File")
+			return EPERM
+		}
+	case fs.Object:
+		if oldFile, ok := oldNode.(*File); ok {
+			if err = oldFile.rename(destDir, newName); err != nil {
+				fs.Errorf(oldPath, "Dir.Rename error: %v", err)
+				return err
+			}
+		} else {
+			err := errors.Errorf("Fs %q can't rename file that is not a vfs.File", d.f)
+			fs.Errorf(oldPath, "Dir.Rename error: %v", err)
+			return err
 		}
 	case fs.Directory:
 		doDirMove := d.f.Features().DirMove

vfs/file.go

@@ -18,16 +18,17 @@ type File struct {
 	size  int64  // size of file - read and written with atomic int64 - must be 64 bit aligned
 	d     *Dir   // parent directory - read only
 
-	mu                sync.Mutex // protects the following
-	o                 fs.Object  // NB o may be nil if file is being written
-	leaf              string     // leaf name of the object
-	rwOpenCount       int        // number of open files on this handle
-	writers           []Handle   // writers for this file
-	nwriters          int32      // len(writers) which is read/updated with atomic
-	readWriters       int        // how many RWFileHandle are open for writing
-	readWriterClosing bool       // is a RWFileHandle currently cosing?
-	modified          bool       // has the cache file be modified by a RWFileHandle?
-	pendingModTime    time.Time  // will be applied once o becomes available, i.e. after file was written
+	mu                sync.Mutex   // protects the following
+	o                 fs.Object    // NB o may be nil if file is being written
+	leaf              string       // leaf name of the object
+	rwOpenCount       int          // number of open files on this handle
+	writers           []Handle     // writers for this file
+	nwriters          int32        // len(writers) which is read/updated with atomic
+	readWriters       int          // how many RWFileHandle are open for writing
+	readWriterClosing bool         // is a RWFileHandle currently cosing?
+	modified          bool         // has the cache file be modified by a RWFileHandle?
+	pendingModTime    time.Time    // will be applied once o becomes available, i.e. after file was written
+	pendingRenameFun  func() error // will be run/renamed after all writers close
 
 	muRW sync.Mutex // synchonize RWFileHandle.openPending(), RWFileHandle.close() and File.Remove
 }
@@ -90,13 +91,61 @@ func (f *File) Node() Node {
 	return f
 }
 
-// rename should be called to update the internals after a rename
-func (f *File) rename(d *Dir, o fs.Object) {
-	f.mu.Lock()
-	f.o = o
-	f.d = d
-	f.leaf = path.Base(o.Remote())
-	f.mu.Unlock()
+// applyPendingRename runs a previously set rename operation if there are no
+// more remaining writers. Call without lock held.
+func (f *File) applyPendingRename() {
+	fun := f.pendingRenameFun
+	if fun == nil || f.writingInProgress() {
+		return
+	}
+	fs.Debugf(f.o, "Running delayed rename now")
+	err := fun()
+	fs.Errorf(f.Path(), "File.Rename error: %v", err)
+}
+
+// rename attempts to immediately rename a file if there are no open writers.
+// Otherwise it will queue the rename operation on the remote until no writers
+// remain.
+func (f *File) rename(destDir *Dir, newName string) error {
+	// FIXME: could Copy then Delete if Move not available
+	// - though care needed if case insensitive...
+	doMove := f.d.f.Features().Move
+	if doMove == nil {
+		err := errors.Errorf("Fs %q can't rename files (no Move)", f.d.f)
+		fs.Errorf(f.Path(), "Dir.Rename error: %v", err)
+		return err
+	}
+
+	renameCall := func() error {
+		newPath := path.Join(destDir.path, newName)
+		newObject, err := doMove(f.o, newPath)
+		if err != nil {
+			fs.Errorf(f.Path(), "File.Rename error: %v", err)
+			return err
+		}
+		// Update the node with the new details
+		fs.Debugf(f.o, "Updating file with %v %p", newObject, f)
+		// f.rename(destDir, newObject)
+		f.mu.Lock()
+		f.o = newObject
+		f.d = destDir
+		f.leaf = path.Base(newObject.Remote())
+		f.pendingRenameFun = nil
+		f.mu.Unlock()
+		return nil
+	}
+
+	if f.writingInProgress() {
+		fs.Debugf(f.o, "File is currently open, delaying rename %p", f)
+		f.mu.Lock()
+		f.d = destDir
+		f.leaf = newName
+		f.pendingRenameFun = renameCall
+		f.mu.Unlock()
+		return nil
+	}
+
+	return renameCall()
 }
 
 // addWriter adds a write handle to the file
@@ -138,6 +187,7 @@ func (f *File) delWriter(h Handle, modifiedCacheFile bool) (lastWriterAndModifie
 	if lastWriterAndModified {
 		f.modified = false
 	}
+	defer f.applyPendingRename()
 	return
 }
 
@@ -171,6 +221,7 @@ func (f *File) finishWriterClose() {
 	f.mu.Lock()
 	f.readWriterClosing = false
 	f.mu.Unlock()
+	f.applyPendingRename()
 }
 
 // activeWriters returns the number of writers on the file
@@ -216,7 +267,7 @@ func (f *File) Size() int64 {
 	defer f.mu.Unlock()
 
 	// if o is nil it isn't valid yet or there are writers, so return the size so far
-	if f.o == nil || len(f.writers) != 0 || f.readWriterClosing {
+	if f.writingInProgress() {
 		return atomic.LoadInt64(&f.size)
 	}
 	return nonNegative(f.o.Size())
@@ -233,7 +284,7 @@ func (f *File) SetModTime(modTime time.Time) error {
 	f.pendingModTime = modTime
 
 	// Only update the ModTime when there are no writers, setObject will do it
-	if f.o != nil && len(f.writers) == 0 && !f.readWriterClosing {
+	if !f.writingInProgress() {
 		return f.applyPendingModTime()
 	}
 
@@ -267,6 +318,11 @@ func (f *File) applyPendingModTime() error {
 	return nil
 }
 
+// writingInProgress returns true of there are any open writers
+func (f *File) writingInProgress() bool {
+	return f.o == nil || len(f.writers) != 0 || f.readWriterClosing
+}
+
 // Update the size while writing
 func (f *File) setSize(n int64) {
 	atomic.StoreInt64(&f.size, n)