Skip to content

fix(inode): use atomic.Int64 for prevDirListingTimeStamp to fix lock-free data race - #4999

Draft
kislaykishore wants to merge 1 commit into
masterfrom
pr/opt-dir-listing-timestamp-race
Draft

fix(inode): use atomic.Int64 for prevDirListingTimeStamp to fix lock-free data race#4999
kislaykishore wants to merge 1 commit into
masterfrom
pr/opt-dir-listing-timestamp-race

Conversation

@kislaykishore

Copy link
Copy Markdown
Collaborator

Description

This PR refactors dirInode.prevDirListingTimeStamp from a 24-byte non-atomic time.Time struct to an 8-byte atomic.Int64 (storing unix nanoseconds).

Motivation & Problem Statement

ShouldInvalidateKernelListCache(), InvalidateKernelListCache(), and ReadEntryCores() accessed prevDirListingTimeStamp concurrently without synchronization locks. Because time.Time is a 24-byte 3-word composite struct (wall, ext, loc), concurrent non-atomic reads and writes constituted a multi-word data race on 64-bit architectures.
Replacing time.Time with atomic.Int64 provides lock-free hardware atomics (Load() / Store()), completely eliminating the data race while retaining lock-free cache invalidation performance.

Struct Sizing & Performance Impact

  • Shrinks dirInode by 16 bytes (240 B -> 224 B).
  • CPU Throughput: Evaluates in ~0.75 ns/op via lock-free atomic load.
  • Race Verification: Verified with 0 data races under go test -race across 128 parallel goroutines.
Benchmark Target Master Baseline Optimized Branch CPU Latency Delta Concurrency & Race Status (-race)
ShouldInvalidateKernelListCache_Parallel 0.803 ns/op 0.756 ns/op -5.85% ($p=0.029$) 0 races (Lock-free atomic load)
ShouldInvalidateKernelListCache_Expired 42.79 ns/op 42.12 ns/op -1.55% ($p=0.000$) 0 races
ShouldInvalidateKernelListCache_NotListed 2.043 ns/op 1.918 ns/op -6.14% ($p=0.000$) 0 races (Fast-path zero check)

Link to the issue in case of a bug fix.

N/A

Testing details

  1. Manual - Executed make build and verified formatting/linter with 0 issues.
  2. Unit tests - Executed unit tests and concurrency race tests: go test -v -race ./internal/fs/inode/....
  3. Integration tests - N/A

Any backward incompatible change? If so, please explain.

N/A

…free data race

## Summary
This commit refactors `dirInode.prevDirListingTimeStamp` from a 24-byte non-atomic `time.Time` struct to an 8-byte `atomic.Int64` (storing unix nanoseconds).

## Motivation & Data Race Fix
`ShouldInvalidateKernelListCache()`, `InvalidateKernelListCache()`, and `ReadEntryCores()` accessed `prevDirListingTimeStamp` concurrently without locking. Because `time.Time` is a 24-byte 3-word composite struct (`wall`, `ext`, `loc`), concurrent non-atomic reads and writes constituted a multi-word data race on 64-bit platforms.
Switching to `atomic.Int64` provides lock-free hardware atomics (`Load()` / `Store()`), eliminating the data race completely.

## Struct Sizing & Memory Impact
Shrinks `dirInode` by 16 bytes.

| Struct / Type | Baseline Size | Optimized Size | Struct Delta | Go Heap Class | Heap Allocation Delta | RAM Saved (per 1M dirs) |
|---|:---:|:---:|:---:|:---:|:---:|:---:|
| `dirInode` | 240 B | **224 B** | -16 B (-6.7%) | 240 B -> **224 B** | **-16 B (-6.7%)** | **16.0 MB** |

## Benchmark & Concurrency Performance
Measured via `benchstat` (n=10, p < 0.05) and `go test -race`:

| Benchmark Target | Master Baseline | Optimized Branch | CPU Latency Delta | Concurrency & Data Race Status (-race) |
|---|:---:|:---:|:---:|:---:|
| `ShouldInvalidateKernelListCache_Parallel` | 0.803 ns/op | **0.756 ns/op** | **-5.85% (p=0.029)** | **0 races** (Lock-free hardware atomic load) |
| `ShouldInvalidateKernelListCache_Expired` | 42.79 ns/op | **42.12 ns/op** | **-1.55% (p=0.000)** | **0 races** |
| `ShouldInvalidateKernelListCache_NotListed` | 2.043 ns/op | **1.918 ns/op** | **-6.14% (p=0.000)** | **0 races** (Fast-path zero check) |

## Verification
- Race test: `go test -v -race -run=Test_ShouldInvalidateKernelListCache_RaceCondition ./internal/fs/inode/...`
- Unit tests: `go test -v -race ./internal/fs/inode/...`

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors prevDirListingTimeStamp in dirInode from time.Time to atomic.Int64 to resolve concurrency data races during lock-free reads and concurrent writes. Corresponding updates are made to ReadEntryCores, ShouldInvalidateKernelListCache, and InvalidateKernelListCache to use atomic operations, and a new concurrency test is added to verify the fix. There are no review comments, so I have no feedback to provide.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.82%. Comparing base (0673d19) to head (63897ab).

Files with missing lines Patch % Lines
internal/fs/inode/dir.go 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4999   +/-   ##
=======================================
  Coverage   83.81%   83.82%           
=======================================
  Files         174      174           
  Lines       21376    21385    +9     
=======================================
+ Hits        17916    17925    +9     
  Misses       2779     2779           
  Partials      681      681           
Flag Coverage Δ
unittests 83.82% <85.71%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant