From 45ca52fbbec9c821d4f5895cefb6407a3c7ba973 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Wed, 18 Sep 2024 10:45:51 -0600 Subject: [PATCH 1/4] Make OpenFile prevent file eviction (#323) * Make OpenFile prevent file eviction by incrementing file handle counter. --- component/file_cache/file_cache.go | 40 +++++++++------- component/file_cache/file_cache_test.go | 63 +++++++++++++++++++++---- 2 files changed, 77 insertions(+), 26 deletions(-) diff --git a/component/file_cache/file_cache.go b/component/file_cache/file_cache.go index 9550c66e4..cbaea7aa5 100644 --- a/component/file_cache/file_cache.go +++ b/component/file_cache/file_cache.go @@ -832,9 +832,6 @@ func (fc *FileCache) downloadFile(handle *handlemap.Handle) error { return err } - // Increment the handle count in this lock item as there is one handle open for this now - flock.Inc() - inf, err := f.Stat() if err == nil { handle.Size = inf.Size() @@ -858,6 +855,11 @@ func (fc *FileCache) downloadFile(handle *handlemap.Handle) error { func (fc *FileCache) OpenFile(options internal.OpenFileOptions) (*handlemap.Handle, error) { log.Trace("FileCache::OpenFile : name=%s, flags=%d, mode=%s", options.Name, options.Flags, options.Mode) + // get the file lock + flock := fc.fileLocks.Get(options.Name) + flock.Lock() + defer flock.Unlock() + attr, err := fc.NextComponent().GetAttr(internal.GetAttrOptions{Name: options.Name}) // return err in case of authorization permission mismatch @@ -884,9 +886,11 @@ func (fc *FileCache) OpenFile(options internal.OpenFileOptions) (*handlemap.Hand } } - // create handle and set value + // create handle and record openFileOptions for later handle := handlemap.NewHandle(options.Name) handle.SetValue("openFileOptions", openFileOptions{flags: options.Flags, fMode: options.Mode}) + // Increment the handle count in this lock item as there is one handle open for this now + flock.Inc() return handle, nil } @@ -919,10 +923,7 @@ func (fc *FileCache) closeFileInternal(options internal.CloseFileOptions, flock defer fc.fileCloseOpt.Done() // if file has not been interactively read or written to by end user, then there is no cached file to close. - _, found := options.Handle.GetValue("openFileOptions") - if found { - return nil - } + _, noCachedHandle := options.Handle.GetValue("openFileOptions") localPath := filepath.Join(fc.tmpPath, options.Handle.Path) @@ -932,17 +933,20 @@ func (fc *FileCache) closeFileInternal(options internal.CloseFileOptions, flock return err } - f := options.Handle.GetFileObject() - if f == nil { - log.Err("FileCache::closeFileInternal : error [missing fd in handle object] %s", options.Handle.Path) - return syscall.EBADF - } + if !noCachedHandle { + f := options.Handle.GetFileObject() + if f == nil { + log.Err("FileCache::closeFileInternal : error [missing fd in handle object] %s", options.Handle.Path) + return syscall.EBADF + } - err = f.Close() - if err != nil { - log.Err("FileCache::closeFileInternal : error closing file %s(%d) [%s]", options.Handle.Path, int(f.Fd()), err.Error()) - return err + err = f.Close() + if err != nil { + log.Err("FileCache::closeFileInternal : error closing file %s(%d) [%s]", options.Handle.Path, int(f.Fd()), err.Error()) + return err + } } + flock.Dec() // If it is an fsync op then purge the file @@ -959,7 +963,7 @@ func (fc *FileCache) closeFileInternal(options internal.CloseFileOptions, flock return nil } - fc.policy.CacheInvalidate(localPath) // Invalidate the file from the local cache. + fc.policy.CacheInvalidate(localPath) // Invalidate the file from the local cache if the timeout is zero. return nil } diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index 3393ed3ff..1d821319a 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -874,6 +874,60 @@ func (suite *fileCacheTestSuite) TestCloseFileTimeout() { suite.assert.True(err == nil || os.IsExist(err)) } +func (suite *fileCacheTestSuite) TestOpenCloseHandleCount() { + defer suite.cleanupTest() + // Setup + file := "file11" + handle, err := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: file, Mode: 0777}) + suite.assert.NoError(err) + err = suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle}) + suite.assert.NoError(err) + + handle, err = suite.fileCache.OpenFile(internal.OpenFileOptions{Name: file, Mode: 0777}) + suite.assert.NoError(err) + err = suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle}) + suite.assert.NoError(err) + + // check that flock handle count is correct + flock := suite.fileCache.fileLocks.Get(file) + suite.assert.Zero(flock.Count()) +} + +func (suite *fileCacheTestSuite) TestOpenPreventsEviction() { + defer suite.cleanupTest() + // Setup + suite.cleanupTest() // teardown the default file cache generated + cacheTimeout := 1 + config := fmt.Sprintf("file_cache:\n path: %s\n offload-io: true\n timeout-sec: %d\n\nloopbackfs:\n path: %s", + suite.cache_path, cacheTimeout, suite.fake_storage_path) + suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual) + + path := "file12" + + handle, err := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: path, Mode: 0777}) + suite.assert.NoError(err) + err = suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle}) + suite.assert.NoError(err) + // File should be in cache and cloud storage + suite.assert.FileExists(filepath.Join(suite.cache_path, path)) + suite.assert.FileExists(filepath.Join(suite.fake_storage_path, path)) + + // Open file (this should prevent eviction) + handle, err = suite.fileCache.OpenFile(internal.OpenFileOptions{Name: path, Mode: 0777}) + suite.assert.NoError(err) + + // wait until file would be evicted (if not for being opened) + time.Sleep(time.Second * time.Duration(cacheTimeout*3)) + + // File should still be in cache + suite.assert.FileExists(filepath.Join(suite.cache_path, path)) + suite.assert.True(suite.fileCache.policy.IsCached(filepath.Join(suite.cache_path, path))) + + // cleanup + err = suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: handle}) + suite.assert.NoError(err) +} + func (suite *fileCacheTestSuite) TestReadInBufferEmpty() { defer suite.cleanupTest() // Setup @@ -1169,8 +1223,6 @@ func (suite *fileCacheTestSuite) TestRenameFileInCache() { suite.assert.NoError(err) openHandle, err := suite.fileCache.OpenFile(internal.OpenFileOptions{Name: src, Mode: 0666}) suite.assert.NoError(err) - err = suite.fileCache.downloadFile(openHandle) - suite.assert.NoError(err) // Path should be in the file cache _, err = os.Stat(filepath.Join(suite.cache_path, src)) @@ -1275,11 +1327,9 @@ func (suite *fileCacheTestSuite) TestRenameFileAndCacheCleanupWithNoTimeout() { createHandle, _ := suite.fileCache.CreateFile(internal.CreateFileOptions{Name: src, Mode: 0666}) suite.fileCache.CloseFile(internal.CloseFileOptions{Handle: createHandle}) openHandle, _ := suite.fileCache.OpenFile(internal.OpenFileOptions{Name: src, Mode: 0666}) - err := suite.fileCache.downloadFile(openHandle) - suite.assert.NoError(err) // Path should be in the file cache - _, err = os.Stat(suite.cache_path + "/" + src) + _, err := os.Stat(suite.cache_path + "/" + src) suite.assert.True(err == nil || os.IsExist(err)) // Path should be in fake storage _, err = os.Stat(suite.fake_storage_path + "/" + src) @@ -1428,9 +1478,6 @@ func (suite *fileCacheTestSuite) TestCachePathSymlink() { handle, _ = suite.fileCache.OpenFile(internal.OpenFileOptions{Name: file, Mode: 0777}) - err = suite.fileCache.downloadFile(handle) - suite.assert.NoError(err) - output := make([]byte, 9) n, err := suite.fileCache.ReadInBuffer(internal.ReadInBufferOptions{Handle: handle, Offset: 0, Data: output}) suite.assert.NoError(err) From 8c97633b650407a9bc5937f504d005f0755fe504 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Wed, 18 Sep 2024 11:32:46 -0600 Subject: [PATCH 2/4] Don't use Unix paths in loopback (#320) --- component/loopback/loopback_fs.go | 47 ++++++++++++++------------ component/loopback/loopback_fs_test.go | 25 +++++++------- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/component/loopback/loopback_fs.go b/component/loopback/loopback_fs.go index 822dede8c..378234a73 100644 --- a/component/loopback/loopback_fs.go +++ b/component/loopback/loopback_fs.go @@ -53,7 +53,7 @@ const compName = "loopbackfs" type LoopbackFS struct { internal.BaseComponent - path string + path string // uses os.Separator (filepath.Join) } var _ internal.Component = &LoopbackFS{} @@ -97,19 +97,19 @@ func (lfs *LoopbackFS) Priority() internal.ComponentPriority { func (lfs *LoopbackFS) CreateDir(options internal.CreateDirOptions) error { log.Trace("LoopbackFS::CreateDir : name=%s", options.Name) - dirPath := common.JoinUnixFilepath(lfs.path, options.Name) + dirPath := filepath.Join(lfs.path, options.Name) return os.Mkdir(dirPath, options.Mode) } func (lfs *LoopbackFS) DeleteDir(options internal.DeleteDirOptions) error { log.Trace("LoopbackFS::DeleteDir : name=%s", options.Name) - dirPath := common.JoinUnixFilepath(lfs.path, options.Name) + dirPath := filepath.Join(lfs.path, options.Name) return os.Remove(dirPath) } func (lfs *LoopbackFS) IsDirEmpty(options internal.IsDirEmptyOptions) bool { log.Trace("LoopbackFS::IsDirEmpty : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) f, err := os.Open(path) if err != nil { log.Err("LoopbackFS::IsDirEmpty : error opening path [%s]", err) @@ -127,7 +127,7 @@ func (lfs *LoopbackFS) StreamDir(options internal.StreamDirOptions) ([]*internal } log.Trace("LoopbackFS::StreamDir : name=%s", options.Name) attrList := make([]*internal.ObjAttr, 0) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) log.Debug("LoopbackFS::StreamDir : requested for %s", path) files, err := os.ReadDir(path) @@ -160,8 +160,8 @@ func (lfs *LoopbackFS) StreamDir(options internal.StreamDirOptions) ([]*internal func (lfs *LoopbackFS) RenameDir(options internal.RenameDirOptions) error { log.Trace("LoopbackFS::RenameDir : %s -> %s", options.Src, options.Dst) - oldPath := common.JoinUnixFilepath(lfs.path, options.Src) - newPath := common.JoinUnixFilepath(lfs.path, options.Dst) + oldPath := filepath.Join(lfs.path, options.Src) + newPath := filepath.Join(lfs.path, options.Dst) return os.Rename(oldPath, newPath) } @@ -173,7 +173,7 @@ func (lfs *LoopbackFS) CreateFile(options internal.CreateFileOptions) (*handlema return nil, fmt.Errorf("LoopbackFS::CreateFile : Failed to create file %s", options.Name) } - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, options.Mode) if err != nil { @@ -188,7 +188,7 @@ func (lfs *LoopbackFS) CreateFile(options internal.CreateFileOptions) (*handlema func (lfs *LoopbackFS) CreateLink(options internal.CreateLinkOptions) error { log.Trace("LoopbackFS::CreateLink : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) err := os.Symlink(options.Target, path) @@ -197,13 +197,13 @@ func (lfs *LoopbackFS) CreateLink(options internal.CreateLinkOptions) error { func (lfs *LoopbackFS) DeleteFile(options internal.DeleteFileOptions) error { log.Trace("LoopbackFS::DeleteFile : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) return os.Remove(path) } func (lfs *LoopbackFS) OpenFile(options internal.OpenFileOptions) (*handlemap.Handle, error) { log.Trace("LoopbackFS::OpenFile : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) log.Debug("LoopbackFS::OpenFile : requested for %s", options.Name) f, err := os.OpenFile(path, options.Flags, options.Mode) if err != nil { @@ -229,20 +229,23 @@ func (lfs *LoopbackFS) CloseFile(options internal.CloseFileOptions) error { func (lfs *LoopbackFS) RenameFile(options internal.RenameFileOptions) error { log.Trace("LoopbackFS::RenameFile : %s -> %s", options.Src, options.Dst) - oldPath := common.JoinUnixFilepath(lfs.path, options.Src) - newPath := common.JoinUnixFilepath(lfs.path, options.Dst) + oldPath := filepath.Join(lfs.path, options.Src) + newPath := filepath.Join(lfs.path, options.Dst) return os.Rename(oldPath, newPath) } func (lfs *LoopbackFS) ReadLink(options internal.ReadLinkOptions) (string, error) { log.Trace("LoopbackFS::ReadLink : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) targetPath, err := os.Readlink(path) if err != nil { log.Err("LoopbackFS::ReadLink : error [%s]", err) return "", err } - return strings.TrimPrefix(targetPath, lfs.path), nil + // this is emulating cloud storage - it should use the unix path style + targetPath = common.NormalizeObjectName(targetPath) + prefix := common.NormalizeObjectName(lfs.path) + return strings.TrimPrefix(targetPath, prefix), nil } func (lfs *LoopbackFS) ReadInBuffer(options internal.ReadInBufferOptions) (int, error) { @@ -250,7 +253,7 @@ func (lfs *LoopbackFS) ReadInBuffer(options internal.ReadInBufferOptions) (int, f := options.Handle.GetFileObject() if f == nil { - f1, err := os.OpenFile(common.JoinUnixFilepath(lfs.path, options.Handle.Path), os.O_RDONLY, 0777) + f1, err := os.OpenFile(filepath.Join(lfs.path, options.Handle.Path), os.O_RDONLY, 0666) if err != nil { return 0, nil } @@ -287,7 +290,7 @@ func (lfs *LoopbackFS) WriteFile(options internal.WriteFileOptions) (int, error) func (lfs *LoopbackFS) TruncateFile(options internal.TruncateFileOptions) error { log.Trace("LoopbackFS::TruncateFile : name=%s", options.Name) - fsPath := common.JoinUnixFilepath(lfs.path, options.Name) + fsPath := filepath.Join(lfs.path, options.Name) return os.Truncate(fsPath, options.Size) } @@ -305,7 +308,7 @@ func (lfs *LoopbackFS) FlushFile(options internal.FlushFileOptions) error { func (lfs *LoopbackFS) CopyToFile(options internal.CopyToFileOptions) error { log.Trace("LoopbackFS::CopyToFile : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) fsrc, err := os.Open(path) if err != nil { log.Err("LoopbackFS::CopyToFile : error opening [%s]", err) @@ -326,7 +329,7 @@ func (lfs *LoopbackFS) CopyToFile(options internal.CopyToFileOptions) error { func (lfs *LoopbackFS) CopyFromFile(options internal.CopyFromFileOptions) error { log.Trace("LoopbackFS::CopyFromFile : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) fdst, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, os.FileMode(0666)) if err != nil { log.Err("LoopbackFS::CopyFromFile : error opening [%s]", err) @@ -347,7 +350,7 @@ func (lfs *LoopbackFS) CopyFromFile(options internal.CopyFromFileOptions) error func (lfs *LoopbackFS) GetAttr(options internal.GetAttrOptions) (*internal.ObjAttr, error) { log.Trace("LoopbackFS::GetAttr : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) info, err := os.Lstat(path) if err != nil { log.Err("LoopbackFS::GetAttr : error [%s]", err) @@ -378,13 +381,13 @@ func (lfs *LoopbackFS) GetAttr(options internal.GetAttrOptions) (*internal.ObjAt func (lfs *LoopbackFS) Chmod(options internal.ChmodOptions) error { log.Trace("LoopbackFS::Chmod : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) return os.Chmod(path, options.Mode) } func (lfs *LoopbackFS) Chown(options internal.ChownOptions) error { log.Trace("LoopbackFS::Chown : name=%s", options.Name) - path := common.JoinUnixFilepath(lfs.path, options.Name) + path := filepath.Join(lfs.path, options.Name) if runtime.GOOS == "windows" { return nil } diff --git a/component/loopback/loopback_fs_test.go b/component/loopback/loopback_fs_test.go index 7cdbb53f3..2819f0975 100644 --- a/component/loopback/loopback_fs_test.go +++ b/component/loopback/loopback_fs_test.go @@ -29,6 +29,7 @@ import ( "context" "fmt" "os" + "path/filepath" "testing" "github.com/Seagate/cloudfuse/common" @@ -75,19 +76,19 @@ func (suite *LoopbackFSTestSuite) SetupTest() { err := os.MkdirAll(testPath, os.FileMode(0777)) panicIfNotNil(err, "Failed to setup test directories") - err = os.MkdirAll(common.JoinUnixFilepath(testPath, dirOne), os.FileMode(0777)) + err = os.MkdirAll(filepath.Join(testPath, dirOne), os.FileMode(0777)) panicIfNotNil(err, "Failed to setup test directories") - err = os.MkdirAll(common.JoinUnixFilepath(testPath, dirEmpty), os.FileMode(0777)) + err = os.MkdirAll(filepath.Join(testPath, dirEmpty), os.FileMode(0777)) panicIfNotNil(err, "Failed to setup test directories") - f, err := os.OpenFile(common.JoinUnixFilepath(testPath, fileLorem), os.O_RDWR|os.O_CREATE, os.FileMode(0777)) + f, err := os.OpenFile(filepath.Join(testPath, fileLorem), os.O_RDWR|os.O_CREATE, os.FileMode(0777)) panicIfNotNil(err, "Failed to setup test files") _, err = f.WriteString(loremText) panicIfNotNil(err, "Failed to setup test files") err = f.Close() panicIfNotNil(err, "Failed to setup test files") - f, err = os.OpenFile(common.JoinUnixFilepath(testPath, fileHello), os.O_RDWR|os.O_CREATE, os.FileMode(0777)) + f, err = os.OpenFile(filepath.Join(testPath, fileHello), os.O_RDWR|os.O_CREATE, os.FileMode(0777)) panicIfNotNil(err, "Failed to setup test files") err = f.Close() panicIfNotNil(err, "Failed to setup test files") @@ -106,7 +107,7 @@ func (suite *LoopbackFSTestSuite) TestCreateDir() { err := suite.lfs.CreateDir(internal.CreateDirOptions{Name: dirTwo, Mode: os.FileMode(0777)}) assert.NoError(err, "CreateDir: Failed") - info, err := os.Stat(common.JoinUnixFilepath(testPath, dirTwo)) + info, err := os.Stat(filepath.Join(testPath, dirTwo)) assert.NoError(err, "CreateDir: Could not stat created dir") assert.True(info.IsDir(), "CreateDir: not a dir") } @@ -117,7 +118,7 @@ func (suite *LoopbackFSTestSuite) TestDeleteDir() { err := suite.lfs.DeleteDir(internal.DeleteDirOptions{Name: dirEmpty}) assert.NoError(err, "DeleteDir: Failed") - _, err = os.Stat(common.JoinUnixFilepath(testPath, dirEmpty)) + _, err = os.Stat(filepath.Join(testPath, dirEmpty)) assert.Error(err, "DeleteDir: Failed to delete") } @@ -125,7 +126,7 @@ func (suite *LoopbackFSTestSuite) TestStreamDir() { defer suite.cleanupTest() assert := assert.New(suite.T()) - info, _ := os.Stat(common.JoinUnixFilepath(testPath, fileLorem)) + info, _ := os.Stat(filepath.Join(testPath, fileLorem)) attrs, _, err := suite.lfs.StreamDir(internal.StreamDirOptions{Name: dirOne}) assert.NoError(err, "StreamDir: Failed") @@ -144,7 +145,7 @@ func (suite *LoopbackFSTestSuite) TestRenameDir() { err := suite.lfs.RenameDir(internal.RenameDirOptions{Src: dirEmpty, Dst: "newempty"}) assert.NoError(err, "RenameDir: Failed") - info, err := os.Stat(common.JoinUnixFilepath(testPath, "newempty")) + info, err := os.Stat(filepath.Join(testPath, "newempty")) assert.NoError(err, "RenameDir: Unable to stat renamed dir") assert.Equal("newempty", info.Name(), "RenameDir: name does not match") @@ -158,7 +159,7 @@ func (suite *LoopbackFSTestSuite) TestCreateFile() { assert.NoError(err, "CreateFile: Failed") assert.NotNil(handle) - info, err := os.Stat(common.JoinUnixFilepath(testPath, fileEmpty)) + info, err := os.Stat(filepath.Join(testPath, fileEmpty)) assert.NoError(err, "CreateFile: unable to stat created file") assert.Equal(fileEmpty, info.Name()) @@ -172,7 +173,7 @@ func (suite *LoopbackFSTestSuite) TestDeleteFile() { err := suite.lfs.DeleteFile(internal.DeleteFileOptions{Name: fileHello}) assert.NoError(err, "DeleteFile: Failed") - _, err = os.Stat(common.JoinUnixFilepath(testPath, fileHello)) + _, err = os.Stat(filepath.Join(testPath, fileHello)) assert.Error(err, "DeleteFile: file was not deleted") } @@ -242,7 +243,7 @@ func (suite *LoopbackFSTestSuite) TestTruncateFile() { err = suite.lfs.TruncateFile(internal.TruncateFileOptions{Name: fileLorem, Size: 0}) assert.NoError(err) - info, err := os.Stat(common.JoinUnixFilepath(testPath, fileLorem)) + info, err := os.Stat(filepath.Join(testPath, fileLorem)) assert.NoError(err, "TruncateFile: cannot stat file") assert.Equal(int64(0), info.Size()) @@ -256,7 +257,7 @@ func (suite *LoopbackFSTestSuite) TestGetAttr() { attr, err := suite.lfs.GetAttr(internal.GetAttrOptions{Name: fileLorem}) assert.NoError(err) - info, err := os.Stat(common.JoinUnixFilepath(testPath, fileLorem)) + info, err := os.Stat(filepath.Join(testPath, fileLorem)) assert.NoError(err) assert.Equal(attr.Size, info.Size()) From 36f53c9c054e144eafabe8513f8bec12d5278110 Mon Sep 17 00:00:00 2001 From: James Fantin-Hardesty <24646452+jfantinhardesty@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:18:53 -0600 Subject: [PATCH 3/4] Update to go 1.23 (#321) --- .github/workflows/code-coverage.yml | 6 +++--- .github/workflows/codeql-analysis.yml | 2 +- .github/workflows/publish-release.yml | 4 ++-- .github/workflows/unit-test.yml | 6 +++--- go.mod | 2 +- go_installer.sh | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index e110f7433..7df69fdee 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -14,7 +14,7 @@ jobs: BuildAndTest-Coverage: strategy: matrix: - go: ['1.22'] + go: ['1.23'] job_name: ['linux'] include: @@ -567,7 +567,7 @@ jobs: BuildAndTest-Coverage-Windows: strategy: matrix: - go: ['1.22'] + go: ['1.23'] job_name: ['windows'] include: @@ -1101,7 +1101,7 @@ jobs: - BuildAndTest-Coverage-Windows strategy: matrix: - go: ['1.22'] + go: ['1.23'] job_name: ['linux'] include: diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index e45e09e79..56f35147a 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -50,7 +50,7 @@ jobs: if: matrix.language == 'go' uses: actions/setup-go@v5 with: - go-version: '1.22' + go-version: '1.23' check-latest: true - name: Go Version diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 43f01ecd9..2e52ad048 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -67,7 +67,7 @@ jobs: needs: compile-gui runs-on: windows-latest env: - go: '1.22' + go: '1.23' cgo: '0' winfsp: winfsp-2.0.23075.msi steps: @@ -209,7 +209,7 @@ jobs: needs: create-installer runs-on: ubuntu-latest env: - go: '1.22' + go: '1.23' zig: 0.13.0 steps: diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index 7396ba2f1..b82788586 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -17,7 +17,7 @@ jobs: name: Build and Test on Linux runs-on: ubuntu-latest env: - go: '1.22' + go: '1.23' cgo: '' containerName: 'test-cnt-ubn' @@ -117,7 +117,7 @@ jobs: name: Build and Test on Windows runs-on: windows-latest env: - go: '1.22' + go: '1.23' cgo: '0' containerName: 'test-cnt-win' @@ -149,7 +149,7 @@ jobs: name: Lint runs-on: ubuntu-latest env: - go: '1.22' + go: '1.23' steps: - name: Checkout code uses: actions/checkout@v4 diff --git a/go.mod b/go.mod index 27089275c..667cb8fa8 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/Seagate/cloudfuse -go 1.22.0 +go 1.23.1 require ( github.com/Azure/azure-sdk-for-go/sdk/azcore v1.14.0 diff --git a/go_installer.sh b/go_installer.sh index ccab20c72..7703d1c18 100755 --- a/go_installer.sh +++ b/go_installer.sh @@ -1,6 +1,6 @@ #!/bin/bash work_dir=$(echo $1 | sed 's:/*$::') -version="1.22.2" +version="1.23.1" arch=`hostnamectl | grep "Arch" | rev | cut -d " " -f 1 | rev` if [ $arch != "arm64" ] From cb2d856527212ac6d867ca032e22abbf8aac3951 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Wed, 18 Sep 2024 14:47:20 -0600 Subject: [PATCH 4/4] Keep files cached on rename dir (#317) * use LoadOrStore * Separate local cache RenameFile code into a function for reuse. * Simplify invalidateDirectory() (use else case for stat error handling) * Wait for local files to be deleted before returning - to prevent race conditions. * Do purges for timeout=0 asynchronously --- component/file_cache/file_cache.go | 131 +++++++++++++++--------- component/file_cache/file_cache_test.go | 9 +- component/file_cache/lru_policy.go | 26 ++--- 3 files changed, 104 insertions(+), 62 deletions(-) diff --git a/component/file_cache/file_cache.go b/component/file_cache/file_cache.go index cbaea7aa5..87d5cc8a0 100644 --- a/component/file_cache/file_cache.go +++ b/component/file_cache/file_cache.go @@ -410,31 +410,38 @@ func (fc *FileCache) invalidateDirectory(name string) { log.Trace("FileCache::invalidateDirectory : %s", name) localPath := filepath.Join(fc.tmpPath, name) - _, err := os.Stat(localPath) - if os.IsNotExist(err) { - log.Info("FileCache::invalidateDirectory : %s does not exist in local cache.", name) - return - } else if err != nil { - log.Debug("FileCache::invalidateDirectory : %s stat err [%s].", name, err.Error()) - return - } // TODO : wouldn't this cause a race condition? a thread might get the lock before we purge - and the file would be non-existent // WalkDir goes through the tree in lexical order so 'dir' always comes before 'dir/file' - // Save the paths in lexical order and delete them in reverse order so folders are deleted after their children - var pathsToPurge []string - err = filepath.WalkDir(localPath, func(path string, d fs.DirEntry, err error) error { + var directoriesToPurge []string + err := filepath.WalkDir(localPath, func(path string, d fs.DirEntry, err error) error { if err == nil && d != nil { - pathsToPurge = append(pathsToPurge, path) + if !d.IsDir() { + log.Debug("FileCache::invalidateDirectory : removing file %s from cache", path) + fc.policy.CachePurge(path) + } else { + // remember to delete the directory later (after its children) + directoriesToPurge = append(directoriesToPurge, path) + } + } else { + // stat(localPath) failed. err is the one returned by stat + // documentation: https://pkg.go.dev/io/fs#WalkDirFunc + if os.IsNotExist(err) { + log.Info("FileCache::invalidateDirectory : %s does not exist in local cache.", name) + } else if err != nil { + log.Warn("FileCache::invalidateDirectory : %s stat err [%s].", name, err.Error()) + } } return nil }) - for i := len(pathsToPurge) - 1; i >= 0; i-- { - log.Debug("FileCache::invalidateDirectory : %s getting removed from cache", pathsToPurge[i]) - fc.policy.CachePurge(pathsToPurge[i]) + + // clean up leftover source directories in reverse order + for i := len(directoriesToPurge) - 1; i >= 0; i-- { + log.Debug("FileCache::invalidateDirectory : removing dir %s from cache", directoriesToPurge[i]) + fc.policy.CachePurge(directoriesToPurge[i]) } if err != nil { - log.Debug("FileCache::invalidateDirectory : Failed to iterate directory %s [%s].", localPath, err.Error()) + log.Debug("FileCache::invalidateDirectory : Failed to walk directory %s. Here's why: %v", localPath, err) return } } @@ -454,7 +461,7 @@ func (fc *FileCache) DeleteDir(options internal.DeleteDirOptions) error { // rest api delete will fail while we still need to cleanup the local cache for the same } - go fc.invalidateDirectory(options.Name) + fc.invalidateDirectory(options.Name) return err } @@ -569,7 +576,7 @@ func (fc *FileCache) IsDirEmpty(options internal.IsDirEmptyOptions) bool { return fc.NextComponent().IsDirEmpty(options) } -// RenameDir: Recursively invalidate the source directory and its children +// RenameDir: Recursively move the source directory func (fc *FileCache) RenameDir(options internal.RenameDirOptions) error { log.Trace("FileCache::RenameDir : src=%s, dst=%s", options.Src, options.Dst) @@ -579,9 +586,53 @@ func (fc *FileCache) RenameDir(options internal.RenameDirOptions) error { return err } - go fc.invalidateDirectory(options.Src) - // TLDR: Dst is guaranteed to be non-existent or empty. - // Note: We do not need to invalidate Dst due to the logic in our FUSE connector, see comments there. + // move the files in local storage + localSrcPath := filepath.Join(fc.tmpPath, options.Src) + localDstPath := filepath.Join(fc.tmpPath, options.Dst) + // WalkDir goes through the tree in lexical order so 'dir' always comes before 'dir/file' + var directoriesToPurge []string + _ = filepath.WalkDir(localSrcPath, func(path string, d fs.DirEntry, err error) error { + if err == nil && d != nil { + newPath := strings.Replace(path, localSrcPath, localDstPath, 1) + if !d.IsDir() { + log.Debug("FileCache::RenameDir : Renaming local file %s -> %s", path, newPath) + fc.renameCachedFile(path, newPath) + } else { + log.Debug("FileCache::RenameDir : Creating local destination directory %s", newPath) + // create the new directory + mkdirErr := os.MkdirAll(newPath, fc.defaultPermission) + if mkdirErr != nil { + // log any error but do nothing about it + log.Warn("FileCache::RenameDir : Failed to created directory %s. Here's why: %v", newPath, mkdirErr) + } + // remember to delete the src directory later (after its contents are deleted) + directoriesToPurge = append(directoriesToPurge, path) + } + } else { + // stat(localPath) failed. err is the one returned by stat + // documentation: https://pkg.go.dev/io/fs#WalkDirFunc + if os.IsNotExist(err) { + // none of the files that were moved actually exist in local storage + log.Info("FileCache::RenameDir : %s does not exist in local cache.", options.Src) + } else if err != nil { + log.Warn("FileCache::RenameDir : %s stat err [%v].", options.Src, err) + } + } + return nil + }) + + // clean up leftover source directories in reverse order + for i := len(directoriesToPurge) - 1; i >= 0; i-- { + log.Debug("FileCache::RenameDir : Removing local directory %s", directoriesToPurge[i]) + fc.policy.CachePurge(directoriesToPurge[i]) + } + + if fc.cacheTimeout == 0 { + // delete destination path immediately + log.Info("FileCache::RenameDir : Timeout is zero, so removing local destination %s", options.Dst) + go fc.invalidateDirectory(options.Dst) + } + return nil } @@ -1269,39 +1320,27 @@ func (fc *FileCache) RenameFile(options internal.RenameFileOptions) error { // if we do not perform rename operation locally and those destination files are cached then next time they are read // we will be serving the wrong content (as we did not rename locally, we still be having older destination files with // stale content). We either need to remove dest file as well from cache or just run rename to replace the content. - err = os.Rename(localSrcPath, localDstPath) - if err != nil && !os.IsNotExist(err) { - log.Err("FileCache::RenameFile : %s failed to rename local file %s [%s]", localSrcPath, err.Error()) - } + fc.renameCachedFile(localSrcPath, localDstPath) - if err != nil { - // If there was a problem in local rename then delete the destination file - // it might happen that dest file was already there and local rename failed - // so deleting local dest file ensures next open of that will get the updated file from container - err = deleteFile(localDstPath) - if err != nil && !os.IsNotExist(err) { - log.Err("FileCache::RenameFile : %s failed to delete local file %s [%s]", localDstPath, err.Error()) - } - - fc.policy.CachePurge(localDstPath) - } + return nil +} - err = deleteFile(localSrcPath) - if err != nil && !os.IsNotExist(err) { - log.Err("FileCache::RenameFile : %s failed to delete local file %s [%s]", localSrcPath, err.Error()) +func (fc *FileCache) renameCachedFile(localSrcPath string, localDstPath string) { + err := os.Rename(localSrcPath, localDstPath) + if err != nil { + // if rename fails, we just delete the source file anyway + log.Warn("FileCache::RenameDir : Failed to rename local file %s -> %s. Here's why: %v", localSrcPath, localDstPath, err) + } else { + fc.policy.CacheValid(localDstPath) } - + // delete the source from our cache policy + // this will also delete the source file from local storage (if rename failed) fc.policy.CachePurge(localSrcPath) if fc.cacheTimeout == 0 { // Destination file needs to be deleted immediately - fc.policy.CachePurge(localDstPath) - } else { - // Add destination file to cache, it will be removed on timeout - fc.policy.CacheValid(localDstPath) + go fc.policy.CachePurge(localDstPath) } - - return nil } // TruncateFile: Update the file with its new size. diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index 1d821319a..caac84413 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -561,11 +561,18 @@ func (suite *fileCacheTestSuite) TestRenameDir() { suite.assert.False(suite.fileCache.policy.IsCached(src)) // Directory should not be cached // wait for asynchronous deletion time.Sleep(1 * time.Second) - // directory should not exist in local filesystem + // src directory should not exist in local filesystem fInfo, err := os.Stat(filepath.Join(suite.cache_path, src)) suite.assert.Nil(fInfo) suite.assert.Error(err) suite.assert.True(os.IsNotExist(err)) + // dst directory should exist and have contents from src + dstEntries, err := os.ReadDir(filepath.Join(suite.cache_path, dst)) + suite.assert.NoError(err) + suite.assert.Len(dstEntries, 5) + for i, entry := range dstEntries { + suite.assert.Equal("file"+strconv.Itoa(i), entry.Name()) + } } func (suite *fileCacheTestSuite) TestCreateFile() { diff --git a/component/file_cache/lru_policy.go b/component/file_cache/lru_policy.go index fc803a556..7048ac2dc 100644 --- a/component/file_cache/lru_policy.go +++ b/component/file_cache/lru_policy.go @@ -222,21 +222,17 @@ func (p *lruPolicy) asyncCacheValid() { } func (p *lruPolicy) cacheValidate(name string) { - var node *lruNode = nil - val, found := p.nodeMap.Load(name) - if !found { - node = &lruNode{ - name: name, - next: nil, - prev: nil, - usage: 0, - deleted: false, - } - p.nodeMap.Store(name, node) - } else { - node = val.(*lruNode) - } + // get existing entry, or if it doesn't exist then + // write a new one and return it + val, _ := p.nodeMap.LoadOrStore(name, &lruNode{ + name: name, + next: nil, + prev: nil, + usage: 0, + deleted: false, + }) + node := val.(*lruNode) p.Lock() defer p.Unlock() @@ -444,7 +440,7 @@ func (p *lruPolicy) deleteItem(name string) { return } - // There are no open handles for this file so its safe to remove this + // There are no open handles for this file so it's safe to remove this // Check if the file exists first, since this is often the second time we're calling deleteFile _, err := os.Stat(name) if err != nil && os.IsNotExist(err) {