diff --git a/cmd/mountlib/mounttest/edge_cases.go b/cmd/mountlib/mounttest/edge_cases.go new file mode 100644 index 000000000..2111f9fe6 --- /dev/null +++ b/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, "") +} diff --git a/cmd/mountlib/mounttest/fs.go b/cmd/mountlib/mounttest/fs.go index 489a66d29..f10afa256 100644 --- a/cmd/mountlib/mounttest/fs.go +++ b/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) diff --git a/vfs/dir.go b/vfs/dir.go index 4d831cff8..7d54ebf45 100644 --- a/vfs/dir.go +++ b/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 diff --git a/vfs/file.go b/vfs/file.go index 95c11216b..fd2a6970a 100644 --- a/vfs/file.go +++ b/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)