From 7d0d7e66ca0b088a79c5ae0a0eb9148cfd537dec Mon Sep 17 00:00:00 2001 From: Brett Dutro Date: Sun, 6 Oct 2019 15:05:21 -0500 Subject: [PATCH] vfs: move writeback of dirty data out of close() method into its own method (FlushWrites) and remove close() call from Flush() If a file handle is duplicated with dup() and the duplicate handle is flushed, rclone will go ahead and close the file, making the original file handle stale. This change removes the close() call from Flush() and replaces it with FlushWrites() so that the file only gets closed when Release() is called. The new FlushWrites method takes care of actually writing the file back to the underlying storage. Fixes #3381 --- cmd/mountlib/mounttest/fs.go | 1 + cmd/mountlib/mounttest/write.go | 48 ++++++++++++++++++ cmd/mountlib/mounttest/write_non_unix.go | 12 +++++ cmd/mountlib/mounttest/write_unix.go | 30 +++++++++--- vfs/read_write.go | 62 +++++++++++++++--------- vfs/read_write_test.go | 9 +++- vfs/vfs.go | 3 +- 7 files changed, 131 insertions(+), 34 deletions(-) diff --git a/cmd/mountlib/mounttest/fs.go b/cmd/mountlib/mounttest/fs.go index 02eb1208b..e8bb8415b 100644 --- a/cmd/mountlib/mounttest/fs.go +++ b/cmd/mountlib/mounttest/fs.go @@ -77,6 +77,7 @@ func RunTests(t *testing.T, fn MountFn) { t.Run("TestWriteFileOverwrite", TestWriteFileOverwrite) t.Run("TestWriteFileDoubleClose", TestWriteFileDoubleClose) t.Run("TestWriteFileFsync", TestWriteFileFsync) + t.Run("TestWriteFileDup", TestWriteFileDup) }) log.Printf("Finished test run with cache mode %v (ok=%v)", cacheMode, ok) if !ok { diff --git a/cmd/mountlib/mounttest/write.go b/cmd/mountlib/mounttest/write.go index c2ab5d65a..29812a54a 100644 --- a/cmd/mountlib/mounttest/write.go +++ b/cmd/mountlib/mounttest/write.go @@ -1,10 +1,13 @@ package mounttest import ( + "os" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/rclone/rclone/vfs" ) // TestWriteFileNoWrite tests writing a file with no write()'s to it @@ -82,3 +85,48 @@ func TestWriteFileFsync(t *testing.T) { run.waitForWriters() run.rm(t, "to be synced") } + +// TestWriteFileDup tests behavior of mmap() in Python by using dup() on a file handle +func TestWriteFileDup(t *testing.T) { + run.skipIfNoFUSE(t) + + if run.vfs.Opt.CacheMode < vfs.CacheModeWrites { + t.Skip("not supported on vfs-cache-mode < writes") + return + } + + filepath := run.path("to be synced") + fh, err := osCreate(filepath) + require.NoError(t, err) + + testData := []byte("0123456789") + + err = fh.Truncate(int64(len(testData) + 2)) + require.NoError(t, err) + + err = fh.Sync() + require.NoError(t, err) + + var dupFd uintptr + dupFd, err = writeTestDup(fh.Fd()) + require.NoError(t, err) + + dupFile := os.NewFile(dupFd, fh.Name()) + _, err = dupFile.Write(testData) + require.NoError(t, err) + + err = dupFile.Close() + require.NoError(t, err) + + _, err = fh.Seek(int64(len(testData)), 0) + require.NoError(t, err) + + _, err = fh.Write([]byte("10")) + require.NoError(t, err) + + err = fh.Close() + require.NoError(t, err) + + run.waitForWriters() + run.rm(t, "to be synced") +} diff --git a/cmd/mountlib/mounttest/write_non_unix.go b/cmd/mountlib/mounttest/write_non_unix.go index 7ef701270..17e865e4d 100644 --- a/cmd/mountlib/mounttest/write_non_unix.go +++ b/cmd/mountlib/mounttest/write_non_unix.go @@ -5,9 +5,21 @@ package mounttest import ( "runtime" "testing" + + "golang.org/x/sys/windows" ) // TestWriteFileDoubleClose tests double close on write func TestWriteFileDoubleClose(t *testing.T) { t.Skip("not supported on " + runtime.GOOS) } + +// writeTestDup performs the platform-specific implementation of the dup() syscall +func writeTestDup(oldfd uintptr) (uintptr, error) { + p, err := windows.GetCurrentProcess() + if err != nil { + return 0, err + } + var h windows.Handle + return uintptr(h), windows.DuplicateHandle(p, windows.Handle(oldfd), p, &h, 0, true, windows.DUPLICATE_SAME_ACCESS) +} diff --git a/cmd/mountlib/mounttest/write_unix.go b/cmd/mountlib/mounttest/write_unix.go index f9548c2af..4eecc7a58 100644 --- a/cmd/mountlib/mounttest/write_unix.go +++ b/cmd/mountlib/mounttest/write_unix.go @@ -4,10 +4,12 @@ package mounttest import ( "runtime" - "syscall" "testing" "github.com/stretchr/testify/assert" + "golang.org/x/sys/unix" + + "github.com/rclone/rclone/vfs" ) // TestWriteFileDoubleClose tests double close on write @@ -21,14 +23,14 @@ func TestWriteFileDoubleClose(t *testing.T) { assert.NoError(t, err) fd := out.Fd() - fd1, err := syscall.Dup(int(fd)) + fd1, err := unix.Dup(int(fd)) assert.NoError(t, err) - fd2, err := syscall.Dup(int(fd)) + fd2, err := unix.Dup(int(fd)) assert.NoError(t, err) // close one of the dups - should produce no error - err = syscall.Close(fd1) + err = unix.Close(fd1) assert.NoError(t, err) // write to the file @@ -41,14 +43,26 @@ func TestWriteFileDoubleClose(t *testing.T) { err = out.Close() assert.NoError(t, err) - // write to the other dup - should produce an error - _, err = syscall.Write(fd2, buf) - assert.Error(t, err, "input/output error") + // write to the other dup + _, err = unix.Write(fd2, buf) + if run.vfs.Opt.CacheMode < vfs.CacheModeWrites { + // produces an error if cache mode < writes + assert.Error(t, err, "input/output error") + } else { + // otherwise does not produce an error + assert.NoError(t, err) + } // close the dup - should not produce an error - err = syscall.Close(fd2) + err = unix.Close(fd2) assert.NoError(t, err) run.waitForWriters() run.rm(t, "testdoubleclose") } + +// writeTestDup performs the platform-specific implementation of the dup() unix +func writeTestDup(oldfd uintptr) (uintptr, error) { + newfd, err := unix.Dup(int(oldfd)) + return uintptr(newfd), err +} diff --git a/vfs/read_write.go b/vfs/read_write.go index 2bbd269e3..58742e9d9 100644 --- a/vfs/read_write.go +++ b/vfs/read_write.go @@ -230,28 +230,14 @@ func (fh *RWFileHandle) modified() bool { return true } -// close the file handle returning EBADF if it has been -// closed already. +// flushWrites flushes any pending writes to cloud storage // -// Must be called with fh.mu held -// -// Note that we leave the file around in the cache on error conditions -// to give the user a chance to recover it. -func (fh *RWFileHandle) close() (err error) { - defer log.Trace(fh.logPrefix(), "")("err=%v", &err) - fh.file.muRW.Lock() - defer fh.file.muRW.Unlock() - - if fh.closed { - return ECLOSED +// Must be called with fh.muRW held +func (fh *RWFileHandle) flushWrites(closeFile bool) error { + if fh.closed && !closeFile { + return nil } - fh.closed = true - defer func() { - if fh.opened { - fh.file.delRWOpen() - } - fh.d.vfs.cache.close(fh.remote) - }() + rdwrMode := fh.flags & accessModeMask writer := rdwrMode != os.O_RDONLY @@ -284,8 +270,8 @@ func (fh *RWFileHandle) close() (err error) { } // Close the underlying file - if fh.opened { - err = fh.File.Close() + if fh.opened && closeFile { + err := fh.File.Close() if err != nil { err = errors.Wrap(err, "failed to close cache file") return err @@ -314,6 +300,32 @@ func (fh *RWFileHandle) close() (err error) { return nil } +// close the file handle returning EBADF if it has been +// closed already. +// +// Must be called with fh.mu held +// +// Note that we leave the file around in the cache on error conditions +// to give the user a chance to recover it. +func (fh *RWFileHandle) close() (err error) { + defer log.Trace(fh.logPrefix(), "")("err=%v", &err) + fh.file.muRW.Lock() + defer fh.file.muRW.Unlock() + + if fh.closed { + return ECLOSED + } + fh.closed = true + defer func() { + if fh.opened { + fh.file.delRWOpen() + } + fh.d.vfs.cache.close(fh.remote) + }() + + return fh.flushWrites(true) +} + // Close closes the file func (fh *RWFileHandle) Close() error { fh.mu.Lock() @@ -346,7 +358,11 @@ func (fh *RWFileHandle) Flush() error { fs.Debugf(fh.logPrefix(), "RWFileHandle.Flush ignoring flush on unwritten handle") return nil } - err := fh.close() + + fh.file.muRW.Lock() + defer fh.file.muRW.Unlock() + err := fh.flushWrites(false) + if err != nil { fs.Errorf(fh.logPrefix(), "RWFileHandle.Flush error: %v", err) } else { diff --git a/vfs/read_write_test.go b/vfs/read_write_test.go index 6b8d96bfb..257d120ce 100644 --- a/vfs/read_write_test.go +++ b/vfs/read_write_test.go @@ -457,14 +457,19 @@ func TestRWFileHandleFlushWrite(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 5, n) - // Check Flush closes file if write called + // Check Flush does not close file if write called err = fh.Flush() assert.NoError(t, err) - assert.True(t, fh.closed) + assert.False(t, fh.closed) // Check flush does nothing if called again err = fh.Flush() assert.NoError(t, err) + assert.False(t, fh.closed) + + // Check that Close closes the file + err = fh.Close() + assert.NoError(t, err) assert.True(t, fh.closed) } diff --git a/vfs/vfs.go b/vfs/vfs.go index 62324f041..889d1c949 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -254,7 +254,7 @@ func (vfs *VFS) Fs() fs.Fs { func (vfs *VFS) SetCacheMode(cacheMode CacheMode) { vfs.Shutdown() vfs.cache = nil - if vfs.Opt.CacheMode > CacheModeOff { + if cacheMode > CacheModeOff { ctx, cancel := context.WithCancel(context.Background()) cache, err := newCache(ctx, vfs.f, &vfs.Opt) // FIXME pass on context or get from Opt? if err != nil { @@ -263,6 +263,7 @@ func (vfs *VFS) SetCacheMode(cacheMode CacheMode) { cancel() return } + vfs.Opt.CacheMode = cacheMode vfs.cancel = cancel vfs.cache = cache }