diff --git a/snapshot/backup.go b/snapshot/backup.go index 2dc6ada7..8418c213 100644 --- a/snapshot/backup.go +++ b/snapshot/backup.go @@ -1,6 +1,7 @@ package snapshot import ( + "context" "encoding/binary" "errors" "fmt" @@ -207,15 +208,67 @@ func (snap *Builder) processRecord(idx int, sourceCtx *sourceContext, record *co return nil } -func (snap *Builder) importSource(imp importer.Importer, sourceCtx *sourceContext, stats *scanStats) error { - if sourceCtx.vfsCache != nil { - // Memory wise the cache has a small footprint so we can safely go a bit - // big here. The 64 figure was chosen empirically after various tests. - window := snap.AppContext().MaxConcurrency * 64 - sourceCtx.vfsCache.StartDirpackPrefetch(window, 64) - defer sourceCtx.vfsCache.StopDirpackPrefetch() +func (snap *Builder) warmVFSStage(ctx context.Context, pvfs *vfs.Filesystem, in <-chan *connectors.Record, out chan<- *connectors.Record, window int) { + defer close(out) + ready := make(chan []*connectors.Record, 1) + + go func() { + defer close(ready) + + warm := func(batch []*connectors.Record) bool { + dirs := make([]string, 0, len(batch)) + seen := make(map[string]struct{}, len(batch)) + for _, r := range batch { + if r.Err != nil { + continue + } + parent := path.Dir(r.Pathname) + if _, ok := seen[parent]; !ok { + seen[parent] = struct{}{} + dirs = append(dirs, parent) + } + } + + pvfs.PrefetchDirs(ctx, dirs) + + select { + case ready <- batch: + return true + case <-ctx.Done(): + return false + } + } + + batch := make([]*connectors.Record, 0, window) + for rec := range in { + batch = append(batch, rec) + if len(batch) == window { + if !warm(batch) { + return + } + + batch = make([]*connectors.Record, 0, window) + } + } + + // flush last batch + if len(batch) > 0 { + warm(batch) + } + }() + + for batch := range ready { + for _, r := range batch { + select { + case out <- r: + case <-ctx.Done(): + return + } + } } +} +func (snap *Builder) importSource(imp importer.Importer, sourceCtx *sourceContext, stats *scanStats) error { var ckers []*chunkers.Chunker for range snap.AppContext().MaxConcurrency { cker, err := snap.repository.Chunker(nil) @@ -240,6 +293,12 @@ func (snap *Builder) importSource(imp importer.Importer, sourceCtx *sourceContex results = make(chan *connectors.Result, size) } + scanned := records + if sourceCtx.vfsCache != nil { + scanned = make(chan *connectors.Record, size) + go snap.warmVFSStage(ctx, sourceCtx.vfsCache, scanned, records, 8192) + } + for i, cker := range ckers { ck := cker idx := i @@ -299,7 +358,7 @@ func (snap *Builder) importSource(imp importer.Importer, sourceCtx *sourceContex } }() - importerErr := imp.Import(ctx, records, results) + importerErr := imp.Import(ctx, scanned, results) if results != nil { for range results { // drain the results channel so that we unblock the diff --git a/snapshot/vfs/dirpack_prefetch.go b/snapshot/vfs/dirpack_prefetch.go index 140060dc..987e4c47 100644 --- a/snapshot/vfs/dirpack_prefetch.go +++ b/snapshot/vfs/dirpack_prefetch.go @@ -1,169 +1,164 @@ package vfs import ( + "bytes" + "context" + "maps" + "slices" "sync" + "time" - "github.com/PlakarKorp/kloset/caching/lru" - "github.com/PlakarKorp/kloset/iterator" "github.com/PlakarKorp/kloset/objects" + "github.com/PlakarKorp/kloset/repository" + "github.com/PlakarKorp/kloset/resources" ) -// dirpackPrefetcher warms fsc.dirpackCache ahead of the backup walk. -// The feeder keeps at most `window` directories in flight: it primes the -// window, then advances the cursor by one each time the walk reports a fresh -// directory consumed (via onConsume). Loads route through the same -// dirpackSF singleflight group the walk uses, so a prefetch and an on-demand -// load of the same directory never both hit the backend. -type dirpackPrefetcher struct { - fsc *Filesystem - window int - workers int - - jobs chan prefetchJob - consumed chan struct{} - quit chan struct{} - - feederWg sync.WaitGroup - workerWg sync.WaitGroup - - // seen dedups consume signals to one per directory. It is bounded (FIFO): - // it only needs to remember directories that may still have in-flight walk - // records, and the importer emits each directory contiguously and never - // revisits, so the in-flight span is ~walk concurrency. Sizing it to the - // look-ahead window leaves a large margin while keeping memory flat on huge - // sources (hundreds of millions of directories). A rare premature eviction - // only costs one duplicate consume signal (one extra dir prefetched). - seenMu sync.Mutex - seen *lru.Cache[string, struct{}] -} - -type prefetchJob struct { - path string - mac objects.MAC -} - -// window is the maximum number of directories kept in flight -func (fsc *Filesystem) StartDirpackPrefetch(window, workers int) { - if fsc.dirpack == nil || fsc.dirpackCache == nil || fsc.prefetcher != nil { - return +func (fsc *Filesystem) PrefetchDirs(ctx context.Context, dirs []string) error { + if fsc.dirpack == nil || fsc.dirpackCache == nil { + return nil } - cursor, err := fsc.dirpack.ScanFrom("/") - if err != nil { - // Not a fatal error, we just run without prefetching! - return + t0 := time.Now() + warmed := 0 + var findsDuration time.Duration + defer func() { + fsc.repo.Logger().Trace("vfs", "PrefetchDirs(%d dirs): %d warmed: finds %s, total %s", + len(dirs), warmed, findsDuration, time.Since(t0)) + }() + + // Phase 1 dirs -> object MAC, parallelized + cached := 0 + reqs := map[string]repository.BlobReq{} + reqsMtx := sync.Mutex{} + toFind := make([]string, 0, len(dirs)) + for _, dir := range dirs { + if _, ok := fsc.dirpackCache.Get(dir); ok { + cached++ + continue + } + toFind = append(toFind, dir) } - fsc.prefetcher = &dirpackPrefetcher{ - fsc: fsc, - window: window, - workers: workers, - jobs: make(chan prefetchJob, workers), - consumed: make(chan struct{}, window), - quit: make(chan struct{}), - seen: lru.New[string, struct{}](window, nil), - } + /* + var eg errgroup.Group + eg.SetLimit(16) - // The dirpack cache is plain FIFO: a prefetched-but-not-yet-consumed - // entry must not be evicted before the walk reaches it, so the cache - // must hold at least window+workers entries. The cache is empty at this - // point (the walk has not started), so resizing it here is safe. - if need := 2 * (window + workers); need > fsc.dirpackCacheSize { - fsc.dirpackCacheSize = need - fsc.dirpackCache = lru.New[string, map[string]*Entry](need, nil) - } - fsc.prefetcher.workerWg.Add(workers) - for range workers { - go fsc.prefetcher.worker() - } + for _, dir := range dirs { + if _, ok := fsc.dirpackCache.Get(dir); ok { + cached++ + continue + } - fsc.prefetcher.feederWg.Add(1) - go fsc.prefetcher.feed(cursor) -} + eg.Go(func() error { + mac, found, err := fsc.dirpack.Find(dir) + if err != nil || !found { + return nil + } + + reqsMtx.Lock() + reqs[dir] = repository.BlobReq{ + Type: resources.RT_OBJECT, + MAC: mac, + } + reqsMtx.Unlock() + + return nil + }) + } -func (fsc *Filesystem) StopDirpackPrefetch() { - if fsc.prefetcher == nil { - return + eg.Wait() + */ + + const shards = 32 + var wg sync.WaitGroup + per := (len(toFind) + shards - 1) / shards + for start := 0; start < len(toFind); start += per { + chunk := toFind[start:min(start+per, len(toFind))] + wg.Go(func() { + for _, dir := range chunk { + mac, found, err := fsc.dirpack.Find(dir) + if err != nil || !found { + continue + } + reqsMtx.Lock() + reqs[dir] = repository.BlobReq{Type: resources.RT_OBJECT, MAC: mac} + reqsMtx.Unlock() + } + }) } + wg.Wait() - close(fsc.prefetcher.quit) - fsc.prefetcher.feederWg.Wait() - close(fsc.prefetcher.jobs) - fsc.prefetcher.workerWg.Wait() - - fsc.prefetcher = nil -} + findsDuration = time.Since(t0) + warmed = cached // already-warm dirs count as warmed for the trace -func (p *dirpackPrefetcher) feed(cursor iterator.Iterator[string, objects.MAC]) { - defer p.feederWg.Done() + if len(reqs) == 0 { + return nil + } - // enqueue advances the cursor by one directory and dispatches it to a - // worker. Returns false when the cursor is exhausted/errored or we are - // shutting down. - enqueue := func() bool { - if !cursor.Next() { - return false - } - dir, mac := cursor.Current() - select { - case p.jobs <- prefetchJob{path: dir, mac: mac}: - return true - case <-p.quit: - return false + // Phase 2 batch resolve RT_OBJECT->dirpack + objs := map[objects.MAC]*objects.Object{} + chunksReq := []repository.BlobReq{} + for b, err := range fsc.repo.GetBlobs(ctx, slices.Collect(maps.Values(reqs)), &repository.GetBlobsOpts{Concurrency: 32}) { + if err != nil { + // Soft error + continue } - } - exhausted := false - // Fill the cache first. - for range p.window { - if !enqueue() { - exhausted = true - break + if obj, err := objects.NewObjectFromBytes(b.Data); err == nil { + objs[b.MAC] = obj + + for _, c := range obj.Chunks { + chunksReq = append(chunksReq, repository.BlobReq{ + Type: resources.RT_CHUNK, + MAC: c.ContentMAC, + }) + } } } - // Now as consummer advance the cursor replenish the cache (since it's the - // fifo older one gets eviced). - // Note even when exhausted we keep looping to drain the channel. - for { - select { - case <-p.quit: - return - case <-p.consumed: - if !exhausted && !enqueue() { - exhausted = true - } + // Phase 3 batch resolve the Content of dirpacks + chunks := map[objects.MAC][]byte{} + for b, err := range fsc.repo.GetBlobs(ctx, chunksReq, &repository.GetBlobsOpts{Concurrency: 32}) { + if err != nil { + // Soft error + continue } + + chunks[b.MAC] = b.Data } -} -func (p *dirpackPrefetcher) worker() { - defer p.workerWg.Done() - for job := range p.jobs { - _, _, _ = p.fsc.dirpackSF.Do(job.path, func() (any, error) { - if m, exists := p.fsc.dirpackCache.Get(job.path); exists { - return m, nil + // Phase 4 assemble everything, we have to go through reqs to tie everything + // together because GetBlobs only return mac->[]byte and we are lacking the + // original string. +Outer: + for path, req := range reqs { + if obj, ok := objs[req.MAC]; ok { + dirpackData := []byte{} + + for _, c := range obj.Chunks { + if cData, ok := chunks[c.ContentMAC]; ok { + dirpackData = append(dirpackData, cData...) + } else { + continue Outer + } } - return p.fsc.loadDirpackMapByMAC(job.path, job.mac) - }) - } -} -// onConsume reports that the walk has reached directory dir. The first report -// for a directory advances the prefetch window by one; subsequent reports for -// the same directory are ignored. -func (p *dirpackPrefetcher) onConsume(dir string) { - p.seenMu.Lock() - if _, ok := p.seen.Get(dir); ok { - p.seenMu.Unlock() - return + if _, err, _ := fsc.dirpackSF.Do(path, func() (any, error) { + if m, ok := fsc.dirpackCache.Get(path); ok { + return m, nil + } + m, err := fsc.decodeDirpackMap(bytes.NewReader(dirpackData)) + if err == nil { + fsc.dirpackCache.Put(path, m) + } + + return m, err + }); err == nil { + warmed++ + } + } } - _ = p.seen.Put(dir, struct{}{}) - p.seenMu.Unlock() - select { - case p.consumed <- struct{}{}: - case <-p.quit: - } + return nil } diff --git a/snapshot/vfs/dirpack_prefetch_internal_test.go b/snapshot/vfs/dirpack_prefetch_internal_test.go deleted file mode 100644 index da4549ab..00000000 --- a/snapshot/vfs/dirpack_prefetch_internal_test.go +++ /dev/null @@ -1,116 +0,0 @@ -package vfs - -import ( - "testing" - "time" - - "github.com/PlakarKorp/kloset/objects" - "github.com/stretchr/testify/require" -) - -// sliceCursor is a fake btree iterator over a fixed list of directory keys. It -// mirrors forwardIter semantics: Next() advances first, then reports validity, -// so it starts positioned before the first element. -type sliceCursor struct { - keys []string - idx int -} - -func newSliceCursor(keys []string) *sliceCursor { return &sliceCursor{keys: keys, idx: -1} } - -func (c *sliceCursor) Next() bool { - c.idx++ - return c.idx < len(c.keys) -} - -func (c *sliceCursor) Current() (string, objects.MAC) { return c.keys[c.idx], objects.MAC{} } - -func (c *sliceCursor) Err() error { return nil } - -func recvJobPath(t *testing.T, jobs <-chan prefetchJob) (string, bool) { - t.Helper() - select { - case job := <-jobs: - return job.path, true - case <-time.After(2 * time.Second): - return "", false - } -} - -// TestDirpackFeederDeliversAllDirsInOrder drives the feeder directly (no repo, -// no workers — the test drains the jobs channel itself) and asserts every -// directory is enqueued, in cursor order, as consume signals arrive. This pins -// the feeder's enqueue logic: a feeder that stops after the first directory -// (e.g. an inverted enqueue() return check) fails here. -func TestDirpackFeederDeliversAllDirsInOrder(t *testing.T) { - dirs := []string{"/a", "/b", "/c", "/d", "/e"} - - p := &dirpackPrefetcher{ - window: 3, - jobs: make(chan prefetchJob, len(dirs)), - consumed: make(chan struct{}, len(dirs)), - quit: make(chan struct{}), - // seen is unused by feed(); these tests don't call onConsume. - } - // Pre-load one consume signal per directory: priming covers `window`, the - // remaining directories are pulled in by these signals until the cursor is - // exhausted. Extra signals past exhaustion are harmless no-ops. - for range dirs { - p.consumed <- struct{}{} - } - - p.feederWg.Add(1) - go p.feed(newSliceCursor(dirs)) - - var got []string - for range dirs { - path, ok := recvJobPath(t, p.jobs) - require.Truef(t, ok, "timed out after %d/%d jobs: %v", len(got), len(dirs), got) - got = append(got, path) - } - - close(p.quit) - p.feederWg.Wait() - - require.Equal(t, dirs, got) -} - -// TestDirpackFeederRespectsWindow checks the feeder primes exactly `window` -// directories up front, then waits for a consume signal before advancing — it -// never lets more than `window` directories be in flight unprompted. -func TestDirpackFeederRespectsWindow(t *testing.T) { - dirs := []string{"/a", "/b", "/c", "/d", "/e"} - const window = 3 - - p := &dirpackPrefetcher{ - window: window, - jobs: make(chan prefetchJob, len(dirs)), - consumed: make(chan struct{}, len(dirs)), - quit: make(chan struct{}), - // seen is unused by feed(); these tests don't call onConsume. - } - - p.feederWg.Add(1) - go p.feed(newSliceCursor(dirs)) - - // The first `window` directories are primed without any consume signal. - for i := 0; i < window; i++ { - _, ok := recvJobPath(t, p.jobs) - require.Truef(t, ok, "feeder did not prime %d directories (stalled at %d)", window, i) - } - - // With no consume signal sent, the feeder must not enqueue a (window+1)th. - select { - case extra := <-p.jobs: - t.Fatalf("feeder enqueued %q beyond the window without a consume signal", extra.path) - case <-time.After(200 * time.Millisecond): - } - - // A single consume signal releases exactly one more directory. - p.consumed <- struct{}{} - _, ok := recvJobPath(t, p.jobs) - require.True(t, ok, "feeder did not advance after a consume signal") - - close(p.quit) - p.feederWg.Wait() -} diff --git a/snapshot/vfs/dirpack_prefetch_test.go b/snapshot/vfs/dirpack_prefetch_test.go index 93800aa4..ca70f50e 100644 --- a/snapshot/vfs/dirpack_prefetch_test.go +++ b/snapshot/vfs/dirpack_prefetch_test.go @@ -1,6 +1,7 @@ package vfs_test import ( + "context" "fmt" "io" "os" @@ -32,39 +33,47 @@ func fileRec(p, content string) *connectors.Record { }) } -// prefetchTree emits a multi-directory tree (10 dirs of 3 files each) so the -// dirpack spans more directories than a small prefetch window, exercising the -// feeder's consume-driven advance rather than just the initial priming. +// warmTree emits a multi-directory tree (10 dirs of 3 files each) so a +// PrefetchDirs batch spans several dirpack directories. const ( - prefetchTreeDirs = 10 - prefetchTreeFiles = 3 - prefetchTreeFileSlots = prefetchTreeDirs * prefetchTreeFiles + warmTreeDirs = 10 + warmTreeFiles = 3 ) -func prefetchTree(ch chan<- *connectors.Record) { +func warmTree(ch chan<- *connectors.Record) { ch <- dirRec("/") - for d := 0; d < prefetchTreeDirs; d++ { + for d := range warmTreeDirs { dir := fmt.Sprintf("/dir%02d", d) ch <- dirRec(dir) - for f := 0; f < prefetchTreeFiles; f++ { + for f := range warmTreeFiles { p := fmt.Sprintf("%s/file%02d.txt", dir, f) ch <- fileRec(p, fmt.Sprintf("content of %s", p)) } } } -func prefetchTreeFilePaths() []string { - files := make([]string, 0, prefetchTreeFileSlots) - for d := 0; d < prefetchTreeDirs; d++ { - for f := 0; f < prefetchTreeFiles; f++ { +func warmTreeFilePaths() []string { + files := make([]string, 0, warmTreeDirs*warmTreeFiles) + for d := range warmTreeDirs { + for f := range warmTreeFiles { files = append(files, fmt.Sprintf("/dir%02d/file%02d.txt", d, f)) } } return files } +// warmTreeParentDirs is what the backup's warm stage would extract from a +// batch covering the whole tree: every distinct parent directory. +func warmTreeParentDirs() []string { + dirs := []string{"/"} + for d := range warmTreeDirs { + dirs = append(dirs, fmt.Sprintf("/dir%02d", d)) + } + return dirs +} + // freshCacheFS loads the snapshot anew and returns a cache-backed filesystem, -// so each caller gets an independent (cold) dirpack cache and prefetcher slot. +// so each caller gets an independent (cold) dirpack cache. func freshCacheFS(t *testing.T, repo *repository.Repository, id objects.MAC) *vfs.Filesystem { t.Helper() snap, err := snapshot.Load(repo, id) @@ -86,25 +95,23 @@ func walkForBackup(t *testing.T, fs *vfs.Filesystem, files []string) map[string] return out } -// TestDirpackPrefetchSameResultAsCold is the core safety net: resolving every -// entry through GetEntryForBackup with the prefetcher running must produce the -// exact same entries as resolving them on a cold cache with no prefetcher. This -// guards the loadDirpackMap/loadDirpackMapByMAC split, the restored pre- -// singleflight cache fast-path, and the prefetch/on-demand singleflight -// coalescing. -func TestDirpackPrefetchSameResultAsCold(t *testing.T) { +// TestPrefetchDirsSameResultAsCold is the core safety net: entries resolved +// through a PrefetchDirs-warmed cache must be identical to entries resolved +// on-demand on a cold cache. This guards the whole warm pipeline — Find, +// the GetBlobs rounds, chunk assembly, and decodeDirpackMap — against the +// on-demand loadDirpackMap path. +func TestPrefetchDirsSameResultAsCold(t *testing.T) { repo := ptesting.GenerateRepository(t, nil, nil, nil) - base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(prefetchTree)) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) defer base.Close() id := base.Header.Identifier - files := prefetchTreeFilePaths() + files := warmTreeFilePaths() coldFS := freshCacheFS(t, repo, id) cold := walkForBackup(t, coldFS, files) warmFS := freshCacheFS(t, repo, id) - warmFS.StartDirpackPrefetch(8, 4) - defer warmFS.StopDirpackPrefetch() + require.NoError(t, warmFS.PrefetchDirs(context.Background(), warmTreeParentDirs())) warm := walkForBackup(t, warmFS, files) for _, p := range files { @@ -118,50 +125,106 @@ func TestDirpackPrefetchSameResultAsCold(t *testing.T) { } } -// TestDirpackPrefetchLifecycle exercises the start/stop contract: stopping with -// nothing running, starting twice (second is a no-op), and restarting after stop. -func TestDirpackPrefetchLifecycle(t *testing.T) { +// TestPrefetchDirsActuallyWarms proves warming is not a silent no-op, with no +// reach into internals: warm the cache, then destroy the backend entirely — +// every lookup must still resolve from the warmed cache alone. A regression +// that quietly stops warming (say, an inverted error check) fails this +// immediately. +func TestPrefetchDirsActuallyWarms(t *testing.T) { + repo := ptesting.GenerateRepository(t, nil, nil, nil) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) + defer base.Close() + + fs := freshCacheFS(t, repo, base.Header.Identifier) + require.NoError(t, fs.PrefetchDirs(context.Background(), warmTreeParentDirs())) + + for mac := range repo.ListPackfiles() { + require.NoError(t, repo.DeletePackfile(mac)) + } + + got := walkForBackup(t, fs, warmTreeFilePaths()) + require.Len(t, got, warmTreeDirs*warmTreeFiles) +} + +// TestPrefetchDirsUnknownDirsAreSoft: directories that do not exist in the +// snapshot (new dirs, from the backup's perspective) are silently skipped, +// and known dirs in the same batch still warm. +func TestPrefetchDirsUnknownDirsAreSoft(t *testing.T) { repo := ptesting.GenerateRepository(t, nil, nil, nil) - base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(prefetchTree)) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) defer base.Close() - files := prefetchTreeFilePaths() fs := freshCacheFS(t, repo, base.Header.Identifier) - // Stop without start must be a safe no-op. - require.NotPanics(t, func() { fs.StopDirpackPrefetch() }) - - // Double start: the second call must be a no-op, not leak a prefetcher. - fs.StartDirpackPrefetch(8, 4) - fs.StartDirpackPrefetch(8, 4) - walkForBackup(t, fs, files) - fs.StopDirpackPrefetch() - - // Restart on the same filesystem must work and still resolve entries. - fs2 := freshCacheFS(t, repo, base.Header.Identifier) - fs2.StartDirpackPrefetch(4, 2) - fs2.StopDirpackPrefetch() - fs2.StartDirpackPrefetch(4, 2) - defer fs2.StopDirpackPrefetch() - got := walkForBackup(t, fs2, files) - require.Len(t, got, len(files)) + dirs := append(warmTreeParentDirs(), "/does/not/exist", "/neither/does/this") + require.NoError(t, fs.PrefetchDirs(context.Background(), dirs)) + + // The known dirs must have warmed regardless: destroy the backend and walk. + for mac := range repo.ListPackfiles() { + require.NoError(t, repo.DeletePackfile(mac)) + } + walkForBackup(t, fs, warmTreeFilePaths()) } -// TestDirpackPrefetchNoCacheNoop verifies the prefetcher is inert on a -// filesystem built without a dirpack cache (the non-backup NewFilesystem path): -// Start is a no-op and GetEntryForBackup still resolves via the fallback path. -func TestDirpackPrefetchNoCacheNoop(t *testing.T) { +// TestPrefetchDirsBackendGoneIsSoft: if the store reads fail, PrefetchDirs +// must not error or panic — dirs stay cold and the (walk-side) on-demand +// load is the one that reports the problem. +func TestPrefetchDirsBackendGoneIsSoft(t *testing.T) { repo := ptesting.GenerateRepository(t, nil, nil, nil) - base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(prefetchTree)) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) + defer base.Close() + + fs := freshCacheFS(t, repo, base.Header.Identifier) + + for mac := range repo.ListPackfiles() { + require.NoError(t, repo.DeletePackfile(mac)) + } + + require.NotPanics(t, func() { + require.NoError(t, fs.PrefetchDirs(context.Background(), warmTreeParentDirs())) + }) + + // Cold dir + dead backend: the on-demand path is the one that errors. + _, err := fs.GetEntryForBackup(warmTreeFilePaths()[0]) + require.Error(t, err) +} + +// TestPrefetchDirsAlreadyWarm: warming the same dirs twice is a cheap no-op +// (the already-cached skip) and never disturbs previously warmed entries. +func TestPrefetchDirsAlreadyWarm(t *testing.T) { + repo := ptesting.GenerateRepository(t, nil, nil, nil) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) + defer base.Close() + + fs := freshCacheFS(t, repo, base.Header.Identifier) + dirs := warmTreeParentDirs() + require.NoError(t, fs.PrefetchDirs(context.Background(), dirs)) + + // Second warm runs against a dead backend: it must not need it (all + // dirs cached) and must not damage the cache. + for mac := range repo.ListPackfiles() { + require.NoError(t, repo.DeletePackfile(mac)) + } + require.NoError(t, fs.PrefetchDirs(context.Background(), dirs)) + + walkForBackup(t, fs, warmTreeFilePaths()) +} + +// TestPrefetchDirsNoCacheNoop: inert on a filesystem built without a dirpack +// cache (the non-backup Filesystem() path) — no panic, lookups unaffected. +func TestPrefetchDirsNoCacheNoop(t *testing.T) { + repo := ptesting.GenerateRepository(t, nil, nil, nil) + base := ptesting.GenerateSnapshot(t, repo, nil, ptesting.WithGenerator(warmTree)) defer base.Close() - // Snapshot.Filesystem() uses NewFilesystem (no dirpack cache). fs, err := base.Filesystem() require.NoError(t, err) - require.NotPanics(t, func() { fs.StartDirpackPrefetch(8, 4) }) + require.NotPanics(t, func() { + require.NoError(t, fs.PrefetchDirs(context.Background(), warmTreeParentDirs())) + }) - for _, p := range prefetchTreeFilePaths() { + for _, p := range warmTreeFilePaths() { e, err := fs.GetEntryForBackup(p) require.NoError(t, err, p) require.Equal(t, path.Base(p), e.FileInfo.Lname, p) diff --git a/snapshot/vfs/vfs.go b/snapshot/vfs/vfs.go index a145717e..227d24ab 100644 --- a/snapshot/vfs/vfs.go +++ b/snapshot/vfs/vfs.go @@ -55,10 +55,6 @@ type Filesystem struct { repo *repository.Repository dirpackCache *lru.Cache[string, map[string]*Entry] dirpackSF singleflight.Group - - dirpackCacheSize int - - prefetcher *dirpackPrefetcher } func PathCmp(a, b string) int { @@ -141,8 +137,7 @@ func NewFilesystemWithCache(repo *repository.Repository, root, xattrs, errors ob if err != nil { return nil, err } - fs.dirpackCacheSize = 256 - fs.dirpackCache = lru.New[string, map[string]*Entry](fs.dirpackCacheSize, nil) + fs.dirpackCache = lru.New[string, map[string]*Entry](4096*2, nil) return fs, nil } @@ -390,10 +385,6 @@ func (fsc *Filesystem) getEntryForBackup(entrypath string) (*Entry, error) { parentPath := path.Dir(entrypath) base := path.Base(entrypath) - if prefetcher := fsc.prefetcher; prefetcher != nil { - prefetcher.onConsume(parentPath) - } - // Fast path: if the prefetcher (or an earlier lookup) already warmed this // directory, serve from cache without entering the singleflight group. if m, exists := fsc.dirpackCache.Get(parentPath); exists { @@ -538,6 +529,17 @@ func (fsc *Filesystem) loadDirpackMapByMAC(parentPath string, objectMac objects. //rd := NewObjectReader(fsc.repo, obj, size, -1) rd := NewObjectReader(fsc.repo, obj, size, 8<<20) + m, err := fsc.decodeDirpackMap(rd) + if err != nil { + return nil, err + } + + _ = fsc.dirpackCache.Put(parentPath, m) + + return m, nil +} + +func (fsc *Filesystem) decodeDirpackMap(rd io.Reader) (map[string]*Entry, error) { cache := make(map[string]*Entry) for { _, siz, err := readDirPackHdr(rd) @@ -582,8 +584,6 @@ func (fsc *Filesystem) loadDirpackMapByMAC(parentPath string, objectMac objects. cache[entry.Name()] = &entry } - _ = fsc.dirpackCache.Put(parentPath, cache) - return cache, nil } diff --git a/snapshot/warmvfs_stage_test.go b/snapshot/warmvfs_stage_test.go new file mode 100644 index 00000000..b91c767c --- /dev/null +++ b/snapshot/warmvfs_stage_test.go @@ -0,0 +1,153 @@ +package snapshot + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/PlakarKorp/kloset/connectors" + "github.com/PlakarKorp/kloset/objects" + "github.com/PlakarKorp/kloset/snapshot/vfs" + "github.com/stretchr/testify/require" +) + +// stageRec builds the minimal record the warm stage inspects (Pathname, Err). +func stageRec(p string) *connectors.Record { + return connectors.NewRecord(p, "", objects.FileInfo{}, nil, nil) +} + +// runWarmStage wires warmVFSStage against an inert filesystem (no dirpack → +// PrefetchDirs is a no-op) so the channel mechanics can be tested in +// isolation. It returns the output channel and a done channel closed when +// the stage function itself has returned. +func runWarmStage(ctx context.Context, in chan *connectors.Record, window int) (chan *connectors.Record, chan struct{}) { + out := make(chan *connectors.Record) + done := make(chan struct{}) + go func() { + defer close(done) + (&Builder{}).warmVFSStage(ctx, &vfs.Filesystem{}, in, out, window) + }() + return out, done +} + +// collect drains out until closed, failing the test if it takes too long — +// every stage bug in this area is a hang, so everything is deadline-guarded. +func collect(t *testing.T, out <-chan *connectors.Record) []*connectors.Record { + t.Helper() + var got []*connectors.Record + deadline := time.After(5 * time.Second) + for { + select { + case rec, ok := <-out: + if !ok { + return got + } + got = append(got, rec) + case <-deadline: + t.Fatalf("stage output did not close (got %d records so far)", len(got)) + } + } +} + +func waitDone(t *testing.T, done <-chan struct{}) { + t.Helper() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("warmVFSStage did not return") + } +} + +// TestWarmVFSStagePassthroughOrder: records cross the stage unmodified and +// in order, across multiple full windows plus a partial tail, and the output +// channel closes when the input does. +func TestWarmVFSStagePassthroughOrder(t *testing.T) { + in := make(chan *connectors.Record) + out, done := runWarmStage(context.Background(), in, 4) + + const n = 10 // window 4: two full batches + a tail of 2 + go func() { + for i := range n { + in <- stageRec(fmt.Sprintf("/dir/file%02d", i)) + } + close(in) + }() + + got := collect(t, out) + require.Len(t, got, n) + for i, rec := range got { + require.Equal(t, fmt.Sprintf("/dir/file%02d", i), rec.Pathname, "order not preserved") + } + waitDone(t, done) +} + +// TestWarmVFSStageTailFlush: fewer records than one window must still be +// delivered when the importer closes the channel — the blocking fill cannot +// hold the tail hostage. +func TestWarmVFSStageTailFlush(t *testing.T) { + in := make(chan *connectors.Record) + out, done := runWarmStage(context.Background(), in, 100) + + go func() { + in <- stageRec("/a") + in <- stageRec("/b") + close(in) + }() + + got := collect(t, out) + require.Len(t, got, 2) + waitDone(t, done) +} + +// TestWarmVFSStageEmptyInput: closing the input with no records closes the +// output with no records, no hang. +func TestWarmVFSStageEmptyInput(t *testing.T) { + in := make(chan *connectors.Record) + out, done := runWarmStage(context.Background(), in, 8) + close(in) + + require.Empty(t, collect(t, out)) + waitDone(t, done) +} + +// TestWarmVFSStageErrorRecordsFlow: records carrying an importer error are +// not warmed but must flow through untouched — dropping them would silently +// lose error reporting for those paths. +func TestWarmVFSStageErrorRecordsFlow(t *testing.T) { + in := make(chan *connectors.Record) + out, done := runWarmStage(context.Background(), in, 4) + + go func() { + r := stageRec("/broken") + r.Err = fmt.Errorf("importer failed on this one") + in <- r + in <- stageRec("/fine") + close(in) + }() + + got := collect(t, out) + require.Len(t, got, 2) + require.Error(t, got[0].Err) + require.NoError(t, got[1].Err) + waitDone(t, done) +} + +// TestWarmVFSStageCtxCancel: cancelling the context mid-stream terminates +// the stage — output closes and the stage function returns — even when the +// consumer has stopped reading. The importer side still owns closing `in`. +func TestWarmVFSStageCtxCancel(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + in := make(chan *connectors.Record) + out, done := runWarmStage(ctx, in, 2) + + // Fill one window so a batch is in flight, then cancel and close. + in <- stageRec("/a") + in <- stageRec("/b") + cancel() + close(in) + + // Drain whatever the stage manages to deliver; it must close out. + collect(t, out) + waitDone(t, done) +}