From 0a45c4d0971504ebfd18b179419c9aa569e722cc Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 18 Jun 2026 17:38:28 -0700 Subject: [PATCH] mount: cache supplementary group IDs for non-root access performance (#10008) * mount: cache supplementary group IDs to improve non-root access performance * mount: clear supplementary group cache between tests and add cache verification test * mount: add docstrings and benchmarks for supplementary group cache * mount: add performance test demonstrating cache effectiveness * mount: add TTL-based cache expiry for supplementary group IDs (5-minute refresh) --- weed/mount/weedfs_access.go | 65 +++++++++++++++--- weed/mount/weedfs_access_bench_test.go | 45 +++++++++++++ weed/mount/weedfs_access_perf_test.go | 93 ++++++++++++++++++++++++++ weed/mount/weedfs_file_mkrm_test.go | 58 ++++++++++++++++ 4 files changed, 253 insertions(+), 8 deletions(-) create mode 100644 weed/mount/weedfs_access_bench_test.go create mode 100644 weed/mount/weedfs_access_perf_test.go diff --git a/weed/mount/weedfs_access.go b/weed/mount/weedfs_access.go index d701d5782..463827eb4 100644 --- a/weed/mount/weedfs_access.go +++ b/weed/mount/weedfs_access.go @@ -3,26 +3,75 @@ package mount import ( "os/user" "strconv" + "sync" "syscall" + "time" "github.com/seaweedfs/go-fuse/v2/fuse" "github.com/seaweedfs/seaweedfs/weed/glog" ) -var lookupSupplementaryGroupIDs = func(callerUid uint32) ([]string, error) { - u, err := user.LookupId(strconv.Itoa(int(callerUid))) +type cachedGroupIDs struct { + groups []string + expiresAt time.Time +} + +var ( + supplementaryGroupCache = make(map[uint32]*cachedGroupIDs) + supplementaryGroupCacheMu sync.RWMutex + supplementaryGroupCacheTTL = 5 * time.Minute + + lookupSupplementaryGroupIDs = func(callerUid uint32) ([]string, error) { + u, err := user.LookupId(strconv.Itoa(int(callerUid))) + if err != nil { + glog.Warningf("hasAccess: user.LookupId for uid %d failed: %v", callerUid, err) + return nil, err + } + groupIDs, err := u.GroupIds() + if err != nil { + glog.Warningf("hasAccess: u.GroupIds for uid %d failed: %v", callerUid, err) + return nil, err + } + return groupIDs, nil + } +) + +// cachedLookupSupplementaryGroupIDs returns supplementary group IDs for a UID, +// caching results for 5 minutes to avoid repeated expensive system calls. +func cachedLookupSupplementaryGroupIDs(callerUid uint32) ([]string, error) { + now := time.Now() + + supplementaryGroupCacheMu.RLock() + cached, ok := supplementaryGroupCache[callerUid] + supplementaryGroupCacheMu.RUnlock() + if ok && now.Before(cached.expiresAt) { + return cached.groups, nil + } + + groupIDs, err := lookupSupplementaryGroupIDs(callerUid) if err != nil { - glog.Warningf("hasAccess: user.LookupId for uid %d failed: %v", callerUid, err) return nil, err } - groupIDs, err := u.GroupIds() - if err != nil { - glog.Warningf("hasAccess: u.GroupIds for uid %d failed: %v", callerUid, err) - return nil, err + + supplementaryGroupCacheMu.Lock() + supplementaryGroupCache[callerUid] = &cachedGroupIDs{ + groups: groupIDs, + expiresAt: now.Add(supplementaryGroupCacheTTL), } + supplementaryGroupCacheMu.Unlock() + return groupIDs, nil } +// clearSupplementaryGroupCache wipes the UID->groups cache for test isolation. +func clearSupplementaryGroupCache() { + supplementaryGroupCacheMu.Lock() + defer supplementaryGroupCacheMu.Unlock() + for k := range supplementaryGroupCache { + delete(supplementaryGroupCache, k) + } +} + /** * Check file access permissions * @@ -67,7 +116,7 @@ func hasAccess(callerUid, callerGid, fileUid, fileGid uint32, perm uint32, mask isMember := callerGid == fileGid if !isMember { - groupIDs, err := lookupSupplementaryGroupIDs(callerUid) + groupIDs, err := cachedLookupSupplementaryGroupIDs(callerUid) if err != nil { // Cannot determine supplementary group membership. // Fall through to "other" permission check since we already diff --git a/weed/mount/weedfs_access_bench_test.go b/weed/mount/weedfs_access_bench_test.go new file mode 100644 index 000000000..58a35088c --- /dev/null +++ b/weed/mount/weedfs_access_bench_test.go @@ -0,0 +1,45 @@ +package mount + +import ( + "testing" + + "github.com/seaweedfs/go-fuse/v2/fuse" +) + +// BenchmarkCachedLookupSupplementaryGroupIDs measures cache hit performance. +func BenchmarkCachedLookupSupplementaryGroupIDs(b *testing.B) { + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + return []string{"456", "789", "1011"}, nil + } + clearSupplementaryGroupCache() + defer func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() + }() + + cachedLookupSupplementaryGroupIDs(999) + b.ResetTimer() + for i := 0; i < b.N; i++ { + cachedLookupSupplementaryGroupIDs(999) + } +} + +// BenchmarkHasAccessPermissionCheck measures permission checks with the cache. +// Simulates repeated permission checks for the same user against different files. +func BenchmarkHasAccessPermissionCheck(b *testing.B) { + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + return []string{"456", "789", "1011"}, nil + } + clearSupplementaryGroupCache() + defer func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() + }() + + b.ResetTimer() + for i := 0; i < b.N; i++ { + hasAccess(999, 999, 123, uint32(i%10), 0o040, fuse.R_OK|fuse.W_OK) + } +} diff --git a/weed/mount/weedfs_access_perf_test.go b/weed/mount/weedfs_access_perf_test.go new file mode 100644 index 000000000..ebc8cab0b --- /dev/null +++ b/weed/mount/weedfs_access_perf_test.go @@ -0,0 +1,93 @@ +package mount + +import ( + "fmt" + "testing" + "time" + + "github.com/seaweedfs/go-fuse/v2/fuse" +) + +// TestPermissionCheckPerformance simulates permission checks during a large copy operation. +// Shows that with caching, even 50,000 permission checks only trigger 1 system lookup. +func TestPermissionCheckPerformance(t *testing.T) { + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + lookupCount := 0 + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + lookupCount++ + return []string{"456", "789", "1011"}, nil + } + defer func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + }() + + // Simulate copying 10,000 files as non-root user. + // Each file access requires multiple permission checks. + // Without caching, this would trigger 50,000 system lookups! + fileCount := 10000 + checksPerFile := 5 + + clearSupplementaryGroupCache() + start := time.Now() + for i := 0; i < fileCount; i++ { + for j := 0; j < checksPerFile; j++ { + gid := uint32(100 + (i % 10)) + hasAccess(999, 999, 123, gid, 0o040, fuse.R_OK|fuse.W_OK) + } + } + elapsed := time.Since(start) + + totalChecks := fileCount * checksPerFile + if lookupCount != 1 { + t.Fatalf("Expected 1 system lookup (cache hit for UID 999), got %d", lookupCount) + } + + opsPerSecond := float64(totalChecks) / elapsed.Seconds() + fmt.Printf("\n=== Permission Check Performance Test (WITH CACHE) ===\n") + fmt.Printf("Files simulated: %d\n", fileCount) + fmt.Printf("Checks per file: %d\n", checksPerFile) + fmt.Printf("Total checks: %d\n", totalChecks) + fmt.Printf("System lookups: %d (would be %d without cache)\n", lookupCount, totalChecks) + fmt.Printf("Lookups eliminated: %d (%.1f%% reduction)\n", totalChecks-lookupCount, + (1-float64(lookupCount)/float64(totalChecks))*100) + fmt.Printf("Time elapsed: %v\n", elapsed) + fmt.Printf("Throughput: %.0f checks/sec\n", opsPerSecond) +} + +// BenchmarkPermissionCheckScaling shows how performance scales with unique users. +func BenchmarkPermissionCheckScaling(b *testing.B) { + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + return []string{"456", "789", "1011"}, nil + } + clearSupplementaryGroupCache() + defer func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() + }() + + // Simulate repeated permission checks for the same user + // (typical single-user copy operation) + b.Run("single-user", func(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + hasAccess(999, 999, 123, 456, 0o040, fuse.R_OK|fuse.W_OK) + } + }) + + b.Run("multi-user-10", func(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + uid := uint32(1000 + (i % 10)) + hasAccess(uid, 100, 123, 456, 0o040, fuse.R_OK|fuse.W_OK) + } + }) + + b.Run("multi-user-100", func(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + uid := uint32(1000 + (i % 100)) + hasAccess(uid, 100, 123, 456, 0o040, fuse.R_OK|fuse.W_OK) + } + }) +} diff --git a/weed/mount/weedfs_file_mkrm_test.go b/weed/mount/weedfs_file_mkrm_test.go index e384eb101..8511f9473 100644 --- a/weed/mount/weedfs_file_mkrm_test.go +++ b/weed/mount/weedfs_file_mkrm_test.go @@ -307,8 +307,10 @@ func TestAccessChecksPermissions(t *testing.T) { lookupSupplementaryGroupIDs = func(uint32) ([]string, error) { return nil, nil } + clearSupplementaryGroupCache() t.Cleanup(func() { lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() }) fullPath := util.FullPath("/visible.txt") @@ -380,8 +382,10 @@ func TestHasAccessUsesSupplementaryGroups(t *testing.T) { lookupSupplementaryGroupIDs = func(uint32) ([]string, error) { return []string{"456"}, nil } + clearSupplementaryGroupCache() t.Cleanup(func() { lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() }) if got := hasAccess(999, 999, 123, 456, 0o060, fuse.R_OK|fuse.W_OK); !got { @@ -389,6 +393,60 @@ func TestHasAccessUsesSupplementaryGroups(t *testing.T) { } } +func TestSupplementaryGroupCaching(t *testing.T) { + callCount := 0 + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + callCount++ + return []string{"456"}, nil + } + clearSupplementaryGroupCache() + t.Cleanup(func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + clearSupplementaryGroupCache() + }) + + cachedLookupSupplementaryGroupIDs(999) + cachedLookupSupplementaryGroupIDs(999) + cachedLookupSupplementaryGroupIDs(999) + + if callCount != 1 { + t.Fatalf("lookupSupplementaryGroupIDs called %d times, expected 1 (cache should prevent repeated calls)", callCount) + } + + cachedLookupSupplementaryGroupIDs(1000) + if callCount != 2 { + t.Fatalf("lookupSupplementaryGroupIDs called %d times after different UID, expected 2", callCount) + } +} + +func TestSupplementaryGroupCacheExpiry(t *testing.T) { + callCount := 0 + oldLookupSupplementaryGroupIDs := lookupSupplementaryGroupIDs + oldTTL := supplementaryGroupCacheTTL + supplementaryGroupCacheTTL = 0 + lookupSupplementaryGroupIDs = func(uid uint32) ([]string, error) { + callCount++ + return []string{"456"}, nil + } + clearSupplementaryGroupCache() + t.Cleanup(func() { + lookupSupplementaryGroupIDs = oldLookupSupplementaryGroupIDs + supplementaryGroupCacheTTL = oldTTL + clearSupplementaryGroupCache() + }) + + cachedLookupSupplementaryGroupIDs(999) + if callCount != 1 { + t.Fatalf("Expected 1 lookup on first call, got %d", callCount) + } + + cachedLookupSupplementaryGroupIDs(999) + if callCount != 2 { + t.Fatalf("Expected 2 lookups after TTL expiry, got %d", callCount) + } +} + func TestCreateExistingFileIgnoresQuotaPreflight(t *testing.T) { wfs, _ := newCreateTestWFS(t) wfs.option.Quota = 1