chore: [cache-dir-size-fix] Part 4: Implement the fix for dir-size in file-cache

[gargnitingoogle]

Description

• Dependent on chore: [cache-dir-size-fix] Part 3: Add disk size calculator for file cache #4416

Link to the issue in case of a bug fix.

b/477828938

Testing details

1. Manual - NA
2. Unit tests - NA
3. Integration tests - NA

Any backward incompatible change? If so, please explain.

[gargnitingoogle]

/gemini summary

[gargnitingoogle]

/gemini review

[gemini-code-assist]

Summary of Changes

This pull request is the fourth part of a series aimed at fixing directory size calculation within the file cache. It integrates new components for precise disk utilization tracking and introduces a shared locking mechanism for cache directories. These changes enhance the file cache's ability to manage disk space effectively and ensure thread-safe operations, particularly in scenarios involving sparse files and parallel downloads.

Highlights

• File Cache Disk Utilization: Introduced FileCacheDiskUtilizationCalculator to accurately track the disk usage of the file cache, addressing a fix for directory size calculation.
• Shared Directory Locking: Implemented a SharedDirLocker mechanism to manage concurrent access to cache directories, ensuring data consistency during file operations.
• Cache Handler Enhancements: The CacheHandler now incorporates volumeBlockSize, diskSizeCalculator, and sharedDirLocker to provide more robust and accurate cache management.
• Conditional Disk Size Scanning: The file cache initialization now conditionally enables disk size scanning and uses a custom size calculator for the LRU cache based on configuration settings.
• File Creation with Locking: File creation within the downloader now utilizes a SafeCreateFile function, integrating the sharedDirLocker for safer concurrent file access.

Changelog

• internal/cache/file/cache_handle_test.go
  • Updated NewCacheHandle calls to pass nil for the newly added parameter.
• internal/cache/file/cache_handler.go
  • Added volumeBlockSize, diskSizeCalculator, and sharedDirLocker fields to the CacheHandler struct.
  • Modified NewCacheHandler to accept and initialize the new diskSizeCalculator parameter, retrieve the volume block size, and set the sharedDirLocker for the job manager and disk size calculator.
  • Integrated diskSizeCalculator.Stop() into the Destroy method for proper cleanup.
• internal/cache/file/cache_handler_test.go
  • Updated NewCacheHandler calls to pass nil for the new diskSizeCalculator parameter.
• internal/cache/file/downloader/downloader.go
  • Added a sharedDirLocker field to the JobManager struct.
  • Implemented the SetSharedDirLocker method to allow setting the shared directory locker.
  • Modified CreateJobIfNotExists to pass the sharedDirLocker to the NewJob constructor.
• internal/cache/file/downloader/job.go
  • Added a sharedDirLocker field to the Job struct.
  • Updated the NewJob constructor to accept and initialize the sharedDirLocker.
  • Modified createCacheFile to use cacheutil.SafeCreateFile with the sharedDirLocker for file creation.
• internal/cache/file/downloader/job_test.go
  • Updated NewJob calls to pass nil for the new sharedDirLocker parameter.
• internal/cache/file/downloader/job_testify_test.go
  • Updated NewJob calls to pass nil for the new sharedDirLocker parameter.
• internal/fs/fs.go
  • Modified createFileCacheHandler to conditionally create a FileCacheDiskUtilizationCalculator and use lru.NewCacheWithCustomSizeCalculator if SizeScanEnable is configured.
  • Updated the NewCacheHandler call to pass the diskSizeCalculator.
• internal/fs/fs_test.go
  • Added SizeScanEnable, SizeScanFiles, and SizeScanFrequencySeconds fields to the defaultFileCacheConfig.
• internal/gcsx/file_cache_reader_test.go
  • Updated NewCacheHandler calls to pass nil for the new diskSizeCalculator parameter.
• internal/gcsx/random_reader_stretchr_test.go
  • Updated NewCacheHandler calls to pass nil for the new diskSizeCalculator parameter.
• internal/gcsx/random_reader_test.go
  • Updated NewCacheHandler calls to pass nil for the new diskSizeCalculator parameter.
• internal/gcsx/read_manager/read_manager_test.go
  • Updated NewCacheHandler calls to pass nil for the new diskSizeCalculator parameter.
• internal/util/disk_util.go
  • Added a new utility function PrettyPrintOf to format unsigned 64-bit integers with comma separators.
• internal/util/disk_util_test.go
  • Added unit tests for the PrettyPrintOf function.

Activity

• gargnitingoogle requested a summary of the pull request.

[gemini-code-assist]

Code Review

This pull request implements the logic for more accurate file cache size calculation by introducing a diskSizeCalculator. The changes involve plumbing this new component and a sharedDirLocker through several layers of the cache and downloader packages. The overall approach seems correct, but I've found a critical issue with the error handling in the new NewCacheHandler function, which uses panic instead of returning errors. This could lead to application crashes and should be addressed.

[gemini-code-assist]

Using panic for recoverable errors, such as a dependency being set more than once, is dangerous as it can crash the entire gcsfuse process. The TODO comments correctly identify that these errors should be returned to the caller for graceful handling.

I recommend refactoring NewCacheHandler to return an error, for example:
func NewCacheHandler(...) (*CacheHandler, error)

Then, you can replace the panic calls with return nil, fmt.Errorf(...). This change can be propagated up to createFileCacheHandler in internal/fs/fs.go, which already returns an error and can handle this failure condition.