perf(fs): faster reads and writes by replacing global mutex handles map with sync.Map - #4933
perf(fs): faster reads and writes by replacing global mutex handles map with sync.Map#4933AmatyaAvadhanula wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handles collection in internal/fs/fs.go to use a concurrent sync.Map instead of a standard map guarded by fs.mu. This change allows looking up handles without acquiring the global lock in several critical paths. The review feedback highlights multiple safety issues where direct type assertions on values retrieved from the sync.Map (in ReadDir, ReadDirPlus, ReadFile, WriteFile, and ReleaseFileHandle) could cause a panic and crash the daemon if an invalid or mismatched handle ID is provided. Additionally, the reviewer suggests validating handle types before deletion to prevent accidental resource leaks.
| val, ok := fs.handles.Load(op.Handle) | ||
| if !ok { | ||
| return syscall.EBADF | ||
| } | ||
| dh := val.(*handle.DirHandle) |
There was a problem hiding this comment.
A direct type assertion 'val.(*handle.DirHandle)' without checking the 'ok' boolean can cause a panic and crash the 'gcsfuse' daemon if an invalid handle ID or a handle ID of a different type (e.g., a 'FileHandle') is passed to 'ReadDir'. Use a safe type assertion to return 'syscall.EBADF' instead.
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| dh := val.(*handle.DirHandle) | |
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| dh, ok := val.(*handle.DirHandle) | |
| if !ok { | |
| return syscall.EBADF | |
| } |
| val, ok := fs.handles.Load(op.Handle) | ||
| if !ok { | ||
| return syscall.EBADF | ||
| } | ||
| dh := val.(*handle.DirHandle) |
There was a problem hiding this comment.
A direct type assertion 'val.(*handle.DirHandle)' without checking the 'ok' boolean can cause a panic and crash the 'gcsfuse' daemon if an invalid handle ID or a handle ID of a different type (e.g., a 'FileHandle') is passed to 'ReadDirPlus'. Use a safe type assertion to return 'syscall.EBADF' instead.
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| dh := val.(*handle.DirHandle) | |
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| dh, ok := val.(*handle.DirHandle) | |
| if !ok { | |
| return syscall.EBADF | |
| } |
|
|
||
| // Clear the entry from the map. | ||
| delete(fs.handles, op.Handle) | ||
| fs.handles.Delete(op.Handle) |
There was a problem hiding this comment.
Directly calling 'fs.handles.Delete(op.Handle)' without verifying the handle type can result in accidentally deleting a 'FileHandle' if a buggy or malicious client passes a 'FileHandle' ID to 'ReleaseDirHandle'. This would leak the 'FileHandle' resources as its 'Destroy' method would never be called. Validate that the handle is a 'DirHandle' before deleting it.
val, ok := fs.handles.Load(op.Handle)
if !ok {
return syscall.EBADF
}
if _, ok := val.(*handle.DirHandle); !ok {
return syscall.EBADF
}
fs.handles.Delete(op.Handle)| val, ok := fs.handles.Load(op.Handle) | ||
| if !ok { | ||
| return syscall.EBADF | ||
| } | ||
| fh := val.(*handle.FileHandle) |
There was a problem hiding this comment.
A direct type assertion 'val.(*handle.FileHandle)' without checking the 'ok' boolean can cause a panic and crash the 'gcsfuse' daemon if an invalid handle ID or a handle ID of a different type (e.g., a 'DirHandle') is passed to 'ReadFile'. Use a safe type assertion to return 'syscall.EBADF' instead.
val, ok := fs.handles.Load(op.Handle)
if !ok {
return syscall.EBADF
}
fh, ok := val.(*handle.FileHandle)
if !ok {
return syscall.EBADF
}| val, ok := fs.handles.Load(op.Handle) | ||
| if !ok { | ||
| return syscall.EBADF | ||
| } | ||
| fh := val.(*handle.FileHandle) |
There was a problem hiding this comment.
A direct type assertion 'val.(*handle.FileHandle)' without checking the 'ok' boolean can cause a panic and crash the 'gcsfuse' daemon if an invalid handle ID or a handle ID of a different type (e.g., a 'DirHandle') is passed to 'WriteFile'. Use a safe type assertion to return 'syscall.EBADF' instead.
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| fh := val.(*handle.FileHandle) | |
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| fh, ok := val.(*handle.FileHandle) | |
| if !ok { | |
| return syscall.EBADF | |
| } |
| val, loaded := fs.handles.LoadAndDelete(op.Handle) | ||
| if !loaded { | ||
| return syscall.EBADF | ||
| } | ||
| fileHandle := val.(*handle.FileHandle) |
There was a problem hiding this comment.
Using 'LoadAndDelete' followed by a direct type assertion 'val.(*handle.FileHandle)' can cause a panic and crash the 'gcsfuse' daemon if the handle ID belongs to a 'DirHandle'. Additionally, it would have already deleted the 'DirHandle' from the map. Use 'Load' and a safe type assertion first, followed by 'CompareAndDelete' to atomically delete the handle only if it is a valid 'FileHandle'.
| val, loaded := fs.handles.LoadAndDelete(op.Handle) | |
| if !loaded { | |
| return syscall.EBADF | |
| } | |
| fileHandle := val.(*handle.FileHandle) | |
| val, ok := fs.handles.Load(op.Handle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| fileHandle, ok := val.(*handle.FileHandle) | |
| if !ok { | |
| return syscall.EBADF | |
| } | |
| if !fs.handles.CompareAndDelete(op.Handle, val) { | |
| return syscall.EBADF | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4933 +/- ##
==========================================
- Coverage 83.83% 83.75% -0.08%
==========================================
Files 173 173
Lines 21064 21094 +30
==========================================
+ Hits 17658 17667 +9
- Misses 2743 2758 +15
- Partials 663 669 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Description
1. Summary
This optimization addresses a high-concurrency scalability bottleneck in GCSFuse (fs.go:L625).
In baseline GCSFuse, file handle lookups for
ReadFileandWriteFileoperations acquired the global filesystem mutex (fs.mu.Lock()). At high concurrency levels (32 to 96 parallel streams), total IOPS (Operations Per Second) plateaued as worker threads competed for mutex access.Replacing the mutex-guarded handle map with Go's lock-free
sync.Mapenables atomic pointer loads (Load) during streaming operations. This unlocks linear IOPS scaling across multi-core systems for both Reads and Writes:2. Transition from Mutex-Bound to Network-Bound (1MB Workloads)
A key finding of this benchmark suite is how block size shifts the bottleneck from Internal CPU Mutexing to Physical Network Limits:
16KB Block Size (CPU/Mutex Bound):
fs.mucontention delivers pure linear IOPS scaling (+132.8% / 2.33x IOPS multiplier).1MB Block Size (Network Bandwidth Bound):
sync.Mappushes GCSFuse performance to the absolute physical limit of the Cloud Storage network interface.3. Usage of Handles in GCSFuse & Why
sync.Mapis the Ideal EnhancementA. What is a Handle in GCSFuse?
A
fuseops.HandleIDis a 64-bit numerical descriptor allocated by the Linux FUSE kernel driver when an application opens or creates a file or directory:open/create): GCSFuse registers the file handle pointer infs.handles[op.Handle].read/write/readdir): The Linux kernel passesop.Handleon every single FUSE read/write request to identify which open file descriptor is being accessed.close/release): GCSFuse deregistersop.Handlefromfs.handleswhen the file is closed.B. The Lifecycle Match: Write-Once, Read-Millions, Delete-Once
Go's official standard library documentation specifies:
The access pattern of open file handles in GCSFuse matches this design criteria perfectly:
OpenFile/CreateFile(fs.handles.Store).ReadFile/WriteFilecalls (fs.handles.Load).ReleaseFileHandle(fs.handles.LoadAndDelete).C. How
sync.MapExecutes Lock-Free Handle ResolutionInternally,
sync.Mapmaintains an atomicreadOnlypointer map structure (atomic.Value):fs.handles.Load(op.Handle)is called during streaming reads or writes,sync.Mapchecks its internalreadOnlyatomic map.open(),.Load()finds the key inreadOnlyand returns the pointer using a single atomic CPU instruction (atomic.LoadPointer).4. Unlocking Linear IOPS Scaling (Amdahl's Law)
fs.mu), maximum system IOPS is limited because of the time spent in the critical section. As thread count increases from 8 to 96, threads spend time waiting forfs.mu, causing IOPS to plateau.sync.MapLock-Free Linear Scaling:sync.Map.Load()performs an atomic pointer load without acquiring any mutex lock or mutating shared memory. All 96 CPU cores execute handle lookups simultaneously in true hardware parallelism. IOPS scales linearly with thread count (5. Direct IOPS Side-by-Side Comparison
A. Live Cloud Storage Network Mode (
gs://gcsfuse-perf-bucket-avoidnull)1. 16KB Block Size (High IOPS Workloads)
sync.Map)2. 1MB Block Size (Bandwidth Bound Workloads)
sync.Map)B. 0ms Dummy I/O Mode (Pure Internal CPU/Locking Performance)
1. 16KB Block Size (Internal Pure IOPS Limit)
sync.Map)2. 1MB Block Size (Max Internal Bandwidth Limit)
sync.Map)Testing details
Any backward incompatible change? If so, please explain.
No