Skip to content

Add new option to read ahead and cache listings in directories - #1060

Open
jfantinhardesty wants to merge 2 commits into
mainfrom
perf/metadata-cache
Open

jfantinhardesty wants to merge 2 commits into
mainfrom
perf/metadata-cache

Conversation

@jfantinhardesty

Copy link
Copy Markdown
Contributor

What type of Pull Request is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Describe your changes in brief

This adds a new directory prefetching feature to the attribute cache aimed at improving performance and caching for metadata heavy applications. When enabled, after a configurable number of misses within a directory, the cache will fetch and store attributes for all entries in that directory. Right now I have it disabled by default, but not sure what a good default value could be for this? Thoughts?

Checklist

  • Tested locally
  • Added new dependencies
  • Updated documentation
  • Added tests

Related Issues

  • Related Issue #
  • Closes #

@foodprocessor foodprocessor 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.

I think this is a great feature to add! I like the cautious instincts here.
I think the implementation could probably be a lot simpler. Bascially, I think we could add a few fields to AttrCacheItem (misses atomic int, prefetchStartedAt time, prefetchToken string), then we could drop the new struct, map, and all the new concurrency code.
I also think we'd be better off not exposing any config value for this, since it's basically harmless at worst. We can always add it if someone ever asks for that to be made configurable.

Here's Opus's summary, intended for an agent. Note: this prompt does not suggest removing the config option:

# Task: simplify dir-listing prefetch in attr_cache (branch perf/metadata-cache, PR #1060)

Files: component/attr_cache/attr_cache.go, component/attr_cache/cacheMap.go, component/attr_cache/attr_cache_test.go.
No existing tests reference the symbols being removed.

## Remove
- `AttrCache.prefetchLock`, `AttrCache.prefetchState`, the `dirPrefetchState` type
- `cleanupPrefetchState()` and its call at the end of `cleanupExpiredEntries()`
- `prefetchDir()` and the `dirPrefetchMaxPages` const
- Keep the `dir-prefetch-threshold` config option, `ac.dirPrefetchThreshold`, and the Configure log line.

## Add to `attrCacheItem` (cacheMap.go), right after `listingComplete`
```go
prefetchMisses atomic.Uint32 // misses since the last prefetch
prefetchToken  string        // next page to prefetch; guarded by cacheLock
prefetchStart  time.Time     // when the current token chain began; guarded by cacheLock
```
- Import `sync/atomic`. The struct grows 72 → 112 bytes, which is acceptable.
- `atomic.Uint32` is noCopy, so never copy `attrCacheItem` by value (go vet copylocks).

## New GetAttr flow (one page per trigger, resume from the stored token)
1. Change `lookupAttr` so it also returns `dir`: the immediate parent (`getParentDir(name)`, NOT `getCachedParent`), and only when found && `exists()` && `attr.IsDir()`. Otherwise return nil. It is read under the existing RLock.
2. Gate: `!respondFromCache && dir != nil && ac.dirPrefetchThreshold > 0 && ac.cacheOnList && ac.cacheTimeout > 0`.
3. Winner check: `dir.prefetchMisses.Add(1) == ac.dirPrefetchThreshold`. Exactly one caller wins per round, so listings of one directory never overlap. Losers fall through to the normal cloud GetAttr, with no waiting or coalescing.
4. The winner:
   - Under `RLock`, read `token, start := dir.prefetchToken, dir.prefetchStart`.
   - If `token != ""` and `time.Since(start) >= cacheTimeout`, set `token = ""` (stale chain, restart).
   - If `token == ""`, set `start = time.Now()`.
   - Call `ac.StreamDir(internal.StreamDirOptions{Name: dir.attr.Path, Token: token})` exactly ONCE. No page loop, and no `cacheLock` held (StreamDir locks internally; RWMutex is not reentrant).
   - Under `Lock`: if `err == nil`, set `dir.prefetchToken = next` and `dir.prefetchStart = start`. If `next == "" && token != "" && dir.listingComplete`, set `dir.cachedAt = start`.
   - Always call `dir.prefetchMisses.Store(0)`, including on error, AFTER the token write. The atomic orders the token handoff to the next winner.
   - Re-run `lookupAttr` and serve from cache if possible; otherwise continue to the normal cloud GetAttr path.

## Why `cachedAt = start` is required (correctness, not an optimization)
- `markListingComplete` sets `listingComplete = true` and `cachedAt = now` when the last page arrives.
- With pages spread across triggers, page-1 children can expire, and be removed by `cleanupExpiredEntries`, while the parent looks fresh and complete.
- `lookupAttr` then returns ENOENT for files that exist. Backdating the parent's `cachedAt` to the chain start ensures the parent expires no later than its oldest listed children.
- The same reasoning is why a stale chain (older than the timeout) restarts from `""`.
- Accepted: a tiny window between `markListingComplete` and the backdating write.

## Lifecycle / cleanup
- No explicit resets anywhere. Do NOT touch `invalidate`/`markDeleted`:
  - Invalid or deleted directories fail `exists()`, so their misses are not counted.
  - `insert` always creates a fresh node, so state starts at zero.
  - `cleanupExpiredEntries` deleting a node discards its state.
  - `next == ""` resets the chain naturally.
- The token can't be derived from `listCache`: non-dirlist inserts (e.g. a GetAttr success in that directory) set `parent.listCache = nil`.
- Writes to a `dir` pointer that was replaced in the map meanwhile go to an orphan node, which is harmless.

## Accepted tradeoffs
- No wait on an in-flight listing; concurrent missers do their own cloud GetAttr.
- The miss counter has no time window; it accumulates over the node's lifetime (the root is never deleted). Cost is bounded at one page fetch per N misses.
- Prefetch can also trigger via the internal `ac.GetAttr` calls in `WriteFile`/`CopyFromFile`. That's fine.

## Tests to add (attr_cache_test.go, follow existing mock patterns)
- Threshold 0 disables prefetch. The Nth miss in a directory triggers exactly one StreamDir.
- Concurrent misses produce a single StreamDir.
- Multi-page directory: successive triggers pass the stored token. After the final page, a nonexistent name returns ENOENT from cache, and `dir.cachedAt == chain start`.
- Stale chain (older than the timeout) restarts with an empty token.
- A StreamDir error resets misses and leaves the token unchanged.

## Validate
```bash
go test -race -timeout=10m ./component/attr_cache/... --tags=unittest,fuse3
gofmt -s -l -d .
$(go env GOPATH)/bin/golangci-lint run --tests=false --build-tags fuse3 --max-issues-per-linter=0
```

## Style
- Comments terse and factual, one short line, only for things the code can't show.
- Prefer fewer helpers; inline the prefetch in GetAttr, or use at most one small helper.
- Use `log.Debug` when a prefetch fires (dir, token).

Comment on lines +88 to +89
// number of misses in one directory that triggers listing the whole directory (0 = disabled)
DirPrefetchThreshold uint32 `config:"dir-prefetch-threshold" yaml:"dir-prefetch-threshold,omitempty"`

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.

I honestly think this is so deep in the weeds that we shouldn't expose this option to the user until someone asks us to.

This branch was successfully deployed

1 active deployment
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.

2 participants