From 5e44caafacfa32b37fa18c2e4058d8a4a5082325 Mon Sep 17 00:00:00 2001 From: James Fantin-Hardesty <24646452+jfantinhardesty@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:59:06 -0600 Subject: [PATCH 1/2] Minot performance optimizations --- common/lock_map.go | 4 ++++ component/file_cache/file_cache.go | 18 +++++++++++++----- component/file_cache/lru_policy.go | 15 +-------------- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/common/lock_map.go b/common/lock_map.go index bfbada53d..98cab2dcf 100644 --- a/common/lock_map.go +++ b/common/lock_map.go @@ -54,6 +54,10 @@ func NewLockMap() *LockMap { // Get the lock item based on file name, if item does not exists create it func (l *LockMap) Get(name string) *LockMapItem { + // avoid allocating a new item when one already exists (the common case) + if lockIntf, found := l.locks.Load(name); found { + return lockIntf.(*LockMapItem) + } lockIntf, _ := l.locks.LoadOrStore(name, &LockMapItem{handleCount: 0}) item := lockIntf.(*LockMapItem) return item diff --git a/component/file_cache/file_cache.go b/component/file_cache/file_cache.go index bea74d487..e94b1c3ae 100644 --- a/component/file_cache/file_cache.go +++ b/component/file_cache/file_cache.go @@ -2205,12 +2205,8 @@ func (fc *FileCache) GetAttr(options internal.GetAttrOptions) (*internal.ObjAttr // Path in local cache, open, and dirty so cache is the source of truth for attributes. localPath := filepath.Join(fc.tmpPath, options.Name) - info, localErr := os.Stat(localPath) - if localErr != nil && !isNotExist(localErr) { - log.Warn("FileCache::GetAttr : %s unexpected stat error [%v]", options.Name, localErr) - } if flock.Count() > 0 && flock.DirtyCount() > 0 { - if localErr == nil && !info.IsDir() { + if info, err := os.Stat(localPath); err == nil && !info.IsDir() { flock.RUnlock() return newObjAttr(options.Name, info), nil } @@ -2219,7 +2215,19 @@ func (fc *FileCache) GetAttr(options internal.GetAttrOptions) (*internal.ObjAttr // To cover case 1, get attributes from storage inCloud := false attrs, remoteErr := fc.NextComponent().GetAttr(options) + + // Only stat the local copy when it can change the answer: the file is tracked by the cache policy + // (case 3), or cloud storage did not return it (case 2). This avoids a syscall on most lookups. + var info os.FileInfo + localErr := os.ErrNotExist + if remoteErr != nil || attrs == nil || (!attrs.IsDir() && fc.policy.IsCached(localPath)) { + info, localErr = os.Stat(localPath) + if localErr != nil && !isNotExist(localErr) { + log.Warn("FileCache::GetAttr : %s unexpected stat error [%v]", options.Name, localErr) + } + } flock.RUnlock() + switch { case remoteErr == nil: // object found inCloud = true diff --git a/component/file_cache/lru_policy.go b/component/file_cache/lru_policy.go index 72208f384..497a8196a 100644 --- a/component/file_cache/lru_policy.go +++ b/component/file_cache/lru_policy.go @@ -354,21 +354,8 @@ func (p *lruPolicy) CachePurge(name string) { } func (p *lruPolicy) IsCached(name string) bool { - log.Trace("lruPolicy::IsCached : %s", name) - val, found := p.nodeMap.Load(name) - if found { - node := val.(*lruNode) - node.RLock() - defer node.RUnlock() - deleted := node.deleted.Load() - log.Debug("lruPolicy::IsCached : %s, deleted:%t", name, deleted) - if !deleted { - return true - } - } - log.Trace("lruPolicy::IsCached : %s, found %t", name, found) - return false + return found && !val.(*lruNode).deleted.Load() } func (p *lruPolicy) Name() string { From 1b84ee081bc8e9f193b2cd47b2e626b80286424d Mon Sep 17 00:00:00 2001 From: James Fantin-Hardesty <24646452+jfantinhardesty@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:01:59 -0600 Subject: [PATCH 2/2] Add tests --- component/file_cache/file_cache_test.go | 56 +++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/component/file_cache/file_cache_test.go b/component/file_cache/file_cache_test.go index fbf2c3ac7..6977502ba 100644 --- a/component/file_cache/file_cache_test.go +++ b/component/file_cache/file_cache_test.go @@ -3330,6 +3330,62 @@ func (suite *fileCacheTestSuite) TestGetAttrDirtyOpenHandle() { suite.assert.NoError(err) } +func (suite *fileCacheTestSuite) TestGetAttrLocalOverlayRequiresCachePolicy() { + // enable mock component + suite.cleanupTest() + defaultConfig := fmt.Sprintf( + "file_cache:\n path: %s\n offload-io: true", + suite.cache_path, + ) + suite.useMock = true + suite.setupTestHelper(defaultConfig) + defer suite.cleanupTest() + + file := "overlay-file" + localPath := filepath.Join(suite.cache_path, file) + err := os.WriteFile(localPath, []byte("local data"), 0777) + suite.assert.NoError(err) + cloudAttr := &internal.ObjAttr{Path: file, Name: file, Size: 3, Mtime: time.Now()} + suite.mock.EXPECT(). + GetAttr(internal.GetAttrOptions{Name: file}). + Return(cloudAttr, nil). + Times(2) + + // a stray local file the cache policy does not track would be re-downloaded, so cloud wins + attr, err := suite.fileCache.GetAttr(internal.GetAttrOptions{Name: file}) + suite.assert.NoError(err) + suite.assert.EqualValues(3, attr.Size) + + // a tracked local file overrides the cloud size and mtime + suite.fileCache.policy.CacheValid(localPath) + attr, err = suite.fileCache.GetAttr(internal.GetAttrOptions{Name: file}) + suite.assert.NoError(err) + suite.assert.EqualValues(len("local data"), attr.Size) + suite.assert.EqualValues(3, cloudAttr.Size, "cloud attributes must not be modified") +} + +func (suite *fileCacheTestSuite) TestGetAttrDirectoryIgnoresLocalCopy() { + // enable mock component + suite.cleanupTest() + defaultConfig := fmt.Sprintf( + "file_cache:\n path: %s\n offload-io: true", + suite.cache_path, + ) + suite.useMock = true + suite.setupTestHelper(defaultConfig) + defer suite.cleanupTest() + + dir := "overlay-dir" + err := os.Mkdir(filepath.Join(suite.cache_path, dir), 0777) + suite.assert.NoError(err) + cloudAttr := internal.CreateObjAttrDir(dir) + suite.mock.EXPECT().GetAttr(internal.GetAttrOptions{Name: dir}).Return(cloudAttr, nil) + + attr, err := suite.fileCache.GetAttr(internal.GetAttrOptions{Name: dir}) + suite.assert.NoError(err) + suite.assert.Same(cloudAttr, attr) +} + func (suite *fileCacheTestSuite) TestGetAttrCase4() { defer suite.cleanupTest()