Skip to content

Conversation

@valandreev
Copy link

added client side meta cache for Redis based meta engines

Copilot AI review requested due to automatic review settings July 18, 2025 15:30
@valandreev valandreev requested a review from CaitinChen as a code owner July 18, 2025 15:30
@CLAassistant
Copy link

CLAassistant commented Jul 18, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Redis client-side caching (CSC) support to JuiceFS, implementing a comprehensive caching layer for metadata operations. The implementation includes automatic cache invalidation, preloading capabilities, and robust error handling for Redis CSC responses.

  • Redis CSC integration with configurable cache size and expiration
  • Client-side caching for inode attributes, directory entries, and file chunks
  • Automatic cache invalidation on write operations and comprehensive documentation

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/meta/redis.go Core Redis client integration with CSC initialization and session handling
pkg/meta/redis_csc.go Main CSC implementation with cache setup and invalidation handling
pkg/meta/redis_attr_csc.go Cached attribute retrieval implementation
pkg/meta/redis_read_csc.go Cached file read operations with chunk-level caching
pkg/meta/redis_methods_csc.go Cached metadata operations (lookup, write, truncate, create)
pkg/meta/redis_invalidate_csc.go Cache invalidation helper methods
pkg/meta/redis_api_wrappers.go Minimal API wrapper file
pkg/meta/query_map.go Query parameter parsing utility
go.mod Added golang-lru/v2 dependency
docs/en/reference/redis-csc.md Comprehensive documentation for Redis CSC feature
.github/workflows/docs.txt Documentation workflow reference
Comments suppressed due to low confidence (1)

pkg/meta/redis_read_csc.go:43

  • [nitpick] The variable name 'i' is generic and could be more descriptive. Consider renaming to 'idx' or 'index' for better readability.
		for i, s := range slices {

Comment on lines +107 to +114
for _, k := range m.entryCache.Keys() {
keyStr := k
// Check if this entry references our inode
if strings.Contains(keyStr, fmt.Sprintf(":%d:", uint64(inode))) ||
strings.HasSuffix(keyStr, fmt.Sprintf(":%d", uint64(inode))) {
m.entryCache.Remove(keyStr)
}
}
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

The string formatting inside the loop could be inefficient. Consider formatting the inode string once before the loop to avoid repeated fmt.Sprintf calls.

Suggested change
for _, k := range m.entryCache.Keys() {
keyStr := k
// Check if this entry references our inode
if strings.Contains(keyStr, fmt.Sprintf(":%d:", uint64(inode))) ||
strings.HasSuffix(keyStr, fmt.Sprintf(":%d", uint64(inode))) {
m.entryCache.Remove(keyStr)
}
}
// Precompute formatted strings for efficiency
inodePrefix := fmt.Sprintf(":%d:", uint64(inode))
inodeSuffix := fmt.Sprintf(":%d", uint64(inode))
for _, k := range m.entryCache.Keys() {
keyStr := k
// Check if this entry references our inode
if strings.Contains(keyStr, inodePrefix) ||
strings.HasSuffix(keyStr, inodeSuffix) {
m.entryCache.Remove(keyStr)
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +273 to +274
} // shutdownClientSideCaching safely cleans up CSC resources
func (m *redisMeta) shutdownClientSideCaching() {
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment is placed on the same line as the closing brace, which is unusual. Consider moving the comment to a separate line above the function for better readability.

Suggested change
} // shutdownClientSideCaching safely cleans up CSC resources
func (m *redisMeta) shutdownClientSideCaching() {
}
// shutdownClientSideCaching safely cleans up CSC resources
func (m *redisMeta) shutdownClientSideCaching() {

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +24
import (
// syscall is used by CSC implementation
)
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

The comment indicates syscall is used by CSC implementation, but syscall is not actually imported or used in this file. Consider removing the unused import comment.

Suggested change
import (
// syscall is used by CSC implementation
)
import (
)

Copilot uses AI. Check for mistakes.
logger.Errorf("Recovered from panic in cache preloading goroutine: %v", r)
}
}()
m.preloadInodeCache(10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

there should be a option for this, otherwise it could overload the Redis when there are many clients.

// Add to cache
cachedAttr := *attr
m.cacheMu.Lock()
m.inodeCache.Add(inode, &cachedAttr)
Copy link
Contributor

Choose a reason for hiding this comment

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

there could be a race when a notification is received AFTER GET returned inmediately.


// handleCacheInvalidation processes invalidation messages from Redis
func (m *redisMeta) handleCacheInvalidation() {
ch := m.cacheSubscription.Channel()
Copy link
Contributor

Choose a reason for hiding this comment

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

the tracking and notifications are bind to each connection. Redis client use a pool of connections for concurrent requests. What will happen when a connection is closed?

_ = m.rdb.Do(ctx, "CLIENT", "TRACKING", "OFF").Err() // Ignore errors if not previously enabled

// Always use BCAST mode for simplicity
err := m.rdb.Do(ctx, "CLIENT", "TRACKING", "ON", "BCAST").Err()
Copy link
Contributor

Choose a reason for hiding this comment

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

BCAST mode may not work when there are many connections (>100). Especially when a single mount point may use tens of connections.

@davies
Copy link
Contributor

davies commented Jul 19, 2025

This is cool overall, thanks! We should be very careful on cache consistency.

@davies davies added this to the Release 1.4 milestone Jul 19, 2025
@jiefenghuang
Copy link
Contributor

jiefenghuang commented Jul 21, 2025

related issue: #97

目前已经有kernel cache + openfile cache(chunk cache),更通用一些,元数据引擎的客户端缓存是否必须,有哪些场景需要做这样一个替换,是否真的必要?

JuiceFS already have a kernel cache plus an open‑file cache (chunk cache), which are more general-purpose. Is a client-side cache in the metadata necessary? In what scenarios would it be useful to add or switch to such a cache?

@valandreev
Copy link
Author

In edge scenarios, when the latency to the meta engine exceeds 20-25ms, JuiceFS becomes slow; client-side caching makes JuiceFS usable.

@polyrabbit
Copy link
Contributor

The more attractive aspect of CSC is that server can proactively invalidate client’s cache. This allows us to safely set a longer cache TTL and significantly reduce redundant requests to Redis.

Of course, it would be even better if this invalidation logic could be integrated into existing caches, so we don’t have to maintain another layer of cache beneath a general-purpose one.

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.

5 participants