From fac8f05d8197fc1915ff485dc3c9e6e2fcaeaf4a Mon Sep 17 00:00:00 2001 From: Vedant Das Date: Fri, 10 Jul 2026 05:07:00 +0000 Subject: [PATCH 1/2] perf: changed dirInode prevDirListingTimeStamp to atomic.Int64 --- internal/fs/inode/dir.go | 12 +++++----- internal/fs/inode/dir_test.go | 42 +++++++++++++++++------------------ 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/fs/inode/dir.go b/internal/fs/inode/dir.go index 862bdc355c3..6a05c640d30 100644 --- a/internal/fs/inode/dir.go +++ b/internal/fs/inode/dir.go @@ -265,7 +265,7 @@ type dirInode struct { // (via kernel) the directory listing from the filesystem. // Specially used when kernelListCacheTTL > 0 that means kernel list-cache is // enabled. - prevDirListingTimeStamp time.Time + prevDirListingTimeStamp atomic.Int64 metadataCacheTtlSecs int64 @@ -634,7 +634,6 @@ func (d *dirInode) CancelCurrDirPrefetcher() { if d.prefetcher != nil { d.prefetcher.Cancel() } - return } // UpdateSize is a no-op for implicit directories. These directories are not @@ -1105,7 +1104,7 @@ func (d *dirInode) ReadEntryCores(ctx context.Context, tok string) (cores map[Na return } - d.prevDirListingTimeStamp = d.cacheClock.Now() + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) return } @@ -1508,11 +1507,12 @@ func (d *dirInode) LocalFileEntries(localFileInodes map[Name]Inode) (localEntrie func (d *dirInode) ShouldInvalidateKernelListCache(ttl time.Duration) bool { // prevDirListingTimeStamp.IsZero() true means listing has not happened yet, and we should // invalidate for clean start. - if d.prevDirListingTimeStamp.IsZero() { + prevNS := d.prevDirListingTimeStamp.Load() + if prevNS == 0 { return true } - cachedDuration := d.cacheClock.Now().Sub(d.prevDirListingTimeStamp) + cachedDuration := time.Duration(d.cacheClock.Now().UnixNano() - prevNS) return cachedDuration >= ttl } @@ -1571,7 +1571,7 @@ func (d *dirInode) RenameFolder(ctx context.Context, folderName string, destinat func (d *dirInode) InvalidateKernelListCache() { // Set prevDirListingTimeStamp to Zero time so that cache is invalidated. - d.prevDirListingTimeStamp = time.Time{} + d.prevDirListingTimeStamp.Store(0) } func (d *dirInode) isBucketHierarchical() bool { diff --git a/internal/fs/inode/dir_test.go b/internal/fs/inode/dir_test.go index 59c21a26ae5..8e2666774b6 100644 --- a/internal/fs/inode/dir_test.go +++ b/internal/fs/inode/dir_test.go @@ -933,13 +933,13 @@ func (t *DirTest) TestReadDescendants_NonEmpty() { func (t *DirTest) TestReadEntries_Empty() { d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) entries, err := t.readAllEntries() require.NoError(t.T(), err) assert.ElementsMatch(t.T(), []fuseutil.Dirent{}, entries) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { @@ -966,7 +966,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) // Read entries. entries, err := t.readAllEntries() @@ -1003,7 +1003,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { } // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { @@ -1033,7 +1033,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) // Read entries. entries, err := t.readAllEntries() @@ -1077,7 +1077,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { } // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntries_TypeCaching() { @@ -1097,7 +1097,7 @@ func (t *DirTest) TestReadEntries_TypeCaching() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) // Read the directory, priming the type cache. _, err = t.readAllEntries() @@ -1136,13 +1136,13 @@ func (t *DirTest) TestReadEntries_TypeCaching() { assert.Equal(t.T(), dirObjName, result.MinObject.Name) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntryCores_Empty() { d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) cores, unsupportedPaths, err := t.readAllEntryCores() @@ -1150,7 +1150,7 @@ func (t *DirTest) TestReadEntryCores_Empty() { assert.Equal(t.T(), 0, len(cores)) assert.Equal(t.T(), 0, len(unsupportedPaths)) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { @@ -1184,7 +1184,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) // Read cores. cores, _, _, err = t.in.ReadEntryCores(t.ctx, "") @@ -1196,7 +1196,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { t.validateCore(cores, "file", false, metadata.RegularFileType, testFileName) t.validateCore(cores, "symlink", false, metadata.SymlinkType, symlinkName) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { @@ -1238,7 +1238,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) // Read cores. cores, unsupportedPaths, err = t.readAllEntryCores() @@ -1253,7 +1253,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { t.validateCore(cores, "symlink", false, metadata.SymlinkType, symlinkName) assert.ElementsMatch(t.T(), []string{dirInodeName + "../", dirInodeName + "/"}, unsupportedPaths) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) TestCreateChildFile_DoesntExist() { @@ -2023,7 +2023,7 @@ func (t *DirTest) TestLocalFileEntriesWithUnlinkedLocalChildFiles() { func (t *DirTest) Test_ShouldInvalidateKernelListCache_ListingNotHappenedYet() { d := t.in.(*dirInode) - d.prevDirListingTimeStamp = time.Time{} + d.prevDirListingTimeStamp.Store(0) // Irrespective of the ttl value, this should always return true. shouldInvalidate := t.in.ShouldInvalidateKernelListCache(util.MaxTimeDuration) @@ -2033,7 +2033,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_ListingNotHappenedYet() { func (t *DirTest) Test_ShouldInvalidateKernelListCache_WithinTtl() { d := t.in.(*dirInode) - d.prevDirListingTimeStamp = d.cacheClock.Now() + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) ttl := time.Second * 10 t.clock.AdvanceTime(ttl / 2) @@ -2044,7 +2044,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_WithinTtl() { func (t *DirTest) Test_ShouldInvalidateKernelListCache_ExpiredTtl() { d := t.in.(*dirInode) - d.prevDirListingTimeStamp = d.cacheClock.Now() + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) ttl := 10 * time.Second t.clock.AdvanceTime(ttl + time.Second) @@ -2055,7 +2055,7 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_ExpiredTtl() { func (t *DirTest) Test_ShouldInvalidateKernelListCache_ZeroTtl() { d := t.in.(*dirInode) - d.prevDirListingTimeStamp = d.cacheClock.Now() + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) ttl := time.Duration(0) shouldInvalidate := t.in.ShouldInvalidateKernelListCache(ttl) @@ -2065,12 +2065,12 @@ func (t *DirTest) Test_ShouldInvalidateKernelListCache_ZeroTtl() { func (t *DirTest) Test_InvalidateKernelListCache() { d := t.in.(*dirInode) - d.prevDirListingTimeStamp = d.cacheClock.Now() - assert.False(t.T(), d.prevDirListingTimeStamp.IsZero()) + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) + assert.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) t.in.InvalidateKernelListCache() - assert.True(t.T(), d.prevDirListingTimeStamp.IsZero()) + assert.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) } func (t *DirTest) Test_ReadObjectsUnlocked() { From 4db3346815d4863d1b091dc6a99740dbd0bd062c Mon Sep 17 00:00:00 2001 From: Vedant Das Date: Fri, 10 Jul 2026 06:37:09 +0000 Subject: [PATCH 2/2] added test to check concurrency bug --- internal/fs/inode/dir.go | 19 ++++++++--- internal/fs/inode/dir_test.go | 62 +++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/internal/fs/inode/dir.go b/internal/fs/inode/dir.go index 6a05c640d30..ea1d6cc390e 100644 --- a/internal/fs/inode/dir.go +++ b/internal/fs/inode/dir.go @@ -1104,7 +1104,12 @@ func (d *dirInode) ReadEntryCores(ctx context.Context, tok string) (cores map[Na return } - d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) + if now := d.cacheClock.Now(); !now.IsZero() { + d.prevDirListingTimeStamp.Store(now.UnixNano()) + } else { + d.prevDirListingTimeStamp.Store(0) + } + return } @@ -1511,9 +1516,15 @@ func (d *dirInode) ShouldInvalidateKernelListCache(ttl time.Duration) bool { if prevNS == 0 { return true } - - cachedDuration := time.Duration(d.cacheClock.Now().UnixNano() - prevNS) - return cachedDuration >= ttl + now := d.cacheClock.Now() + if now.IsZero() { + return true + } + nowNS := now.UnixNano() + if nowNS < prevNS { + return true + } + return time.Duration(nowNS-prevNS) >= ttl } // LOCKS_REQUIRED(d) diff --git a/internal/fs/inode/dir_test.go b/internal/fs/inode/dir_test.go index 8e2666774b6..5cbe5a40170 100644 --- a/internal/fs/inode/dir_test.go +++ b/internal/fs/inode/dir_test.go @@ -21,6 +21,7 @@ import ( "os" "path" "sort" + "sync" "testing" "time" @@ -933,13 +934,13 @@ func (t *DirTest) TestReadDescendants_NonEmpty() { func (t *DirTest) TestReadEntries_Empty() { d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) entries, err := t.readAllEntries() require.NoError(t.T(), err) assert.ElementsMatch(t.T(), []fuseutil.Dirent{}, entries) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { @@ -966,7 +967,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) // Read entries. entries, err := t.readAllEntries() @@ -1003,7 +1004,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsDisabled() { } // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { @@ -1033,7 +1034,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) // Read entries. entries, err := t.readAllEntries() @@ -1077,7 +1078,7 @@ func (t *DirTest) TestReadEntries_NonEmpty_ImplicitDirsEnabled() { } // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntries_TypeCaching() { @@ -1097,7 +1098,7 @@ func (t *DirTest) TestReadEntries_TypeCaching() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) // Read the directory, priming the type cache. _, err = t.readAllEntries() @@ -1136,13 +1137,13 @@ func (t *DirTest) TestReadEntries_TypeCaching() { assert.Equal(t.T(), dirObjName, result.MinObject.Name) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntryCores_Empty() { d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) cores, unsupportedPaths, err := t.readAllEntryCores() @@ -1150,7 +1151,7 @@ func (t *DirTest) TestReadEntryCores_Empty() { assert.Equal(t.T(), 0, len(cores)) assert.Equal(t.T(), 0, len(unsupportedPaths)) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { @@ -1184,7 +1185,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) // Read cores. cores, _, _, err = t.in.ReadEntryCores(t.ctx, "") @@ -1196,7 +1197,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsDisabled() { t.validateCore(cores, "file", false, metadata.RegularFileType, testFileName) t.validateCore(cores, "symlink", false, metadata.SymlinkType, symlinkName) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { @@ -1238,7 +1239,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { // Nil prevDirListingTimeStamp d := t.in.(*dirInode) require.NotNil(t.T(), d) - require.True(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.Zero(t.T(), d.prevDirListingTimeStamp.Load()) // Read cores. cores, unsupportedPaths, err = t.readAllEntryCores() @@ -1253,7 +1254,7 @@ func (t *DirTest) TestReadEntryCores_NonEmpty_ImplicitDirsEnabled() { t.validateCore(cores, "symlink", false, metadata.SymlinkType, symlinkName) assert.ElementsMatch(t.T(), []string{dirInodeName + "../", dirInodeName + "/"}, unsupportedPaths) // Make sure prevDirListingTimeStamp is initialized. - require.False(t.T(), d.prevDirListingTimeStamp.Load() == 0) + require.NotZero(t.T(), d.prevDirListingTimeStamp.Load()) } func (t *DirTest) TestCreateChildFile_DoesntExist() { @@ -2402,3 +2403,36 @@ func (t *DirTest) TestMetadataPrefetcher_InitializationGuards() { }) } } + +func (t *DirTest) Test_ShouldInvalidateKernelListCache_RaceCondition() { + // This test demonstrates the concurrency data race that existed when using time.Time. + // If run with `go test -race`, it would fail prior to the atomic.Int64 refactor. + var wg sync.WaitGroup + wg.Add(3) + + go func() { + defer wg.Done() + for range 1000 { + t.in.InvalidateKernelListCache() + } + }() + + go func() { + defer wg.Done() + for range 1000 { + // Simulate concurrent lock-free reads + _ = t.in.ShouldInvalidateKernelListCache(time.Second) + } + }() + + go func() { + defer wg.Done() + d := t.in.(*dirInode) + for range 1000 { + // Simulate concurrent writes (like what ReadEntryCores does) + d.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano()) + } + }() + + wg.Wait() +}