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 }