From 7f6f55acba2256d5c6da9b7d33b65b0102179d30 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 6 Oct 2026 16:40:53 +0000 Subject: [PATCH] gomodfs, store/remotestore: fetch less over the network in WinFsp mode Two changes that cut HTTP requests from a WinFsp mount to its remote store: Cache metadata files (.info, .mod and .ziphash) in remotestore.Store. They never change for a module version, but every stat and open of one fetched it from the server again, and the go command reads hundreds of them per invocation, mostly one at a time while loading the module graph. Over 8 cached go commands, that was about 13,600 requests at about 1ms each. Like modmaps, they're cached for the life of the Store, with concurrent fetches merged. Misses aren't cached. Fetch a file's contents on its first read rather than when it's opened. Windows opens files for many things that don't read them, including every os.Stat, and each open downloaded the whole file. Opens now get the size and type from the store's (cached) modmap, so an HTTP error fails the first read instead of the open. In a Windows VM with the module cache and Tailscale Go toolchain served from a remote store, fully cached go commands (median of 30, p < 1e-10): go build tailscale.com/cmd/tailscale/cli: 4.90s -> 2.21s go run github.com/tc-hib/go-winres: 2.59s -> 1.58s and the first command after mounting goes from 5.73s to 3.10s and from 2.50s to 1.26s. (On NTFS, both take under 0.3s.) "go mod verify" of all 75 modules of the build still passes over the mount. Updates tailscale/corp#24037 Signed-off-by: Brad Fitzpatrick --- store/remotestore/remotestore.go | 41 +++++++++++++++ store/remotestore/remotestore_test.go | 62 +++++++++++++++++++++++ winfsp.go | 73 +++++++++++++++++++-------- 3 files changed, 156 insertions(+), 20 deletions(-) diff --git a/store/remotestore/remotestore.go b/store/remotestore/remotestore.go index 1779547..e0ceff7 100644 --- a/store/remotestore/remotestore.go +++ b/store/remotestore/remotestore.go @@ -36,9 +36,17 @@ type Store struct { mu sync.Mutex modmaps map[store.ModuleVersion]*remoteModHandle // cached modmap per MV + meta map[metaKey][]byte // cached .info, .mod and .ziphash files tmpDir string // lazy-created temp dir for large files } +// metaKey is the key of a cached metadata file: ext is "info", "mod" or +// "ziphash". +type metaKey struct { + mv store.ModuleVersion + ext string +} + // Close cleans up temporary files. It should be called when the store is no longer needed. func (s *Store) Close() error { s.mu.Lock() @@ -274,7 +282,40 @@ func (s *Store) GetZipHash(ctx context.Context, h store.ModHandle) ([]byte, erro return s.getMetaFile(ctx, rmh.mv, "ziphash") } +// getMetaFile returns the metadata file with extension ext for mv. +// +// Metadata files never change for a module version, and the go command +// reads hundreds of them per invocation (each stat and open of one is a +// call here), so they're cached for the life of the Store. Misses aren't +// cached, as the server may get the module later. func (s *Store) getMetaFile(ctx context.Context, mv store.ModuleVersion, ext string) ([]byte, error) { + k := metaKey{mv, ext} + s.mu.Lock() + v, ok := s.meta[k] + s.mu.Unlock() + if ok { + return v, nil + } + vi, err, _ := s.sf.Do("meta:"+mv.Module+"@"+mv.Version+"."+ext, func() (any, error) { + v, err := s.fetchMetaFile(ctx, mv, ext) + if err != nil { + return nil, err + } + s.mu.Lock() + defer s.mu.Unlock() + if s.meta == nil { + s.meta = make(map[metaKey][]byte) + } + s.meta[k] = v + return v, nil + }) + if err != nil { + return nil, err + } + return vi.([]byte), nil +} + +func (s *Store) fetchMetaFile(ctx context.Context, mv store.ModuleVersion, ext string) ([]byte, error) { escaped, err := mvToEscapedPath(mv) if err != nil { return nil, err diff --git a/store/remotestore/remotestore_test.go b/store/remotestore/remotestore_test.go index d9c6148..960f42b 100644 --- a/store/remotestore/remotestore_test.go +++ b/store/remotestore/remotestore_test.go @@ -5,11 +5,14 @@ package remotestore import ( "context" + "errors" "io/fs" + "maps" "net" "net/http" "net/http/httptest" "slices" + "strings" "sync" "sync/atomic" "testing" @@ -60,6 +63,65 @@ func TestDefaultClientReusesConns(t *testing.T) { } } +// TestMetaFileCache tests that a Store fetches each metadata file from +// the server once, even when asked concurrently, and doesn't cache misses. +func TestMetaFileCache(t *testing.T) { + var mu sync.Mutex + reqs := map[string]int{} // URL path -> requests + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + reqs[r.URL.Path]++ + mu.Unlock() + if strings.Contains(r.URL.Path, "/missing/") { + http.NotFound(w, r) + return + } + w.Write([]byte("contents of " + r.URL.Path)) + })) + defer srv.Close() + + s := &Store{BaseURL: srv.URL} + ctx := context.Background() + mv := store.ModuleVersion{Module: "example.com/m", Version: "v1.0.0"} + var wg sync.WaitGroup + for range 10 { + wg.Go(func() { + for range 5 { + mod, err := s.GetModFile(ctx, mv) + if err != nil { + t.Error(err) + return + } + if want := "contents of " + metaPrefix + "example.com/m/@v/v1.0.0.mod"; string(mod) != want { + t.Errorf("GetModFile = %q; want %q", mod, want) + } + if _, err := s.GetInfoFile(ctx, mv); err != nil { + t.Error(err) + } + } + }) + } + wg.Wait() + + missing := store.ModuleVersion{Module: "example.com/missing", Version: "v1.0.0"} + for range 2 { + if _, err := s.GetModFile(ctx, missing); !errors.Is(err, store.ErrCacheMiss) { + t.Errorf("GetModFile(missing) error = %v; want ErrCacheMiss", err) + } + } + + mu.Lock() + defer mu.Unlock() + want := map[string]int{ + metaPrefix + "example.com/m/@v/v1.0.0.mod": 1, + metaPrefix + "example.com/m/@v/v1.0.0.info": 1, + metaPrefix + "example.com/missing/@v/v1.0.0.mod": 2, + } + if !maps.Equal(reqs, want) { + t.Errorf("server requests = %v; want %v", reqs, want) + } +} + func TestBuildDirMap(t *testing.T) { tests := []struct { name string diff --git a/winfsp.go b/winfsp.go index e2efe75..dba094d 100644 --- a/winfsp.go +++ b/winfsp.go @@ -15,10 +15,10 @@ import ( "path" "path/filepath" "strings" + "sync" "syscall" "time" - "github.com/tailscale/gomodfs/store" "github.com/winfsp/go-winfsp" "github.com/winfsp/go-winfsp/gofs" "golang.org/x/sys/windows" @@ -295,25 +295,30 @@ func (pfs *fspFS) OpenFile(name string, flag int, perm os.FileMode) (retFile gof if err != nil { return nil, err } - spanGF := d.fs.Stats.StartSpan("fsp.OpenFile-GetFile") - contents, err := d.fs.Store.GetFile(ctx, mh, dp.Path) + fi, err := d.fs.Store.Stat(ctx, mh, dp.Path) if err != nil { - if errors.Is(err, store.ErrIsDir) { - spanRD := d.fs.Stats.StartSpan("fsp.OpenFile-Readdir") - ents, err := d.fs.Store.Readdir(ctx, mh, dp.Path) - spanRD.End(err) - if err != nil { - spanGF.End(err) - return nil, err - } - spanGF.End(nil) - return wdDir{pathInZip: dp.Path, baseName: base, ents: ents}, nil - } - spanGF.End(err) return nil, err } - spanGF.End(nil) - return newFWPFileFromContents(name, contents), nil + if fi.IsDir() { + spanRD := d.fs.Stats.StartSpan("fsp.OpenFile-Readdir") + ents, err := d.fs.Store.Readdir(ctx, mh, dp.Path) + spanRD.End(err) + if err != nil { + return nil, err + } + return wdDir{pathInZip: dp.Path, baseName: base, ents: ents}, nil + } + // Get the contents on the first read, not now: Windows opens files for + // many things that don't read them, including every os.Stat. + return &winFSPRegularFile{ + fi: regFileInfo{name: name, size: fi.Size()}, + getContents: func() ([]byte, error) { + sp := d.fs.Stats.StartSpan("fsp.ReadAt-GetFile") + contents, err := d.fs.Store.GetFile(ctx, mh, dp.Path) + sp.End(err) + return contents, err + }, + }, nil } func (s *winFSPRunner) Unmount() error { @@ -350,8 +355,32 @@ func newFWPFileFromContents(baseName string, contents []byte) gofs.File { } type winFSPRegularFile struct { - fi os.FileInfo - contents []byte + fi os.FileInfo + + // getContents, if non-nil, returns the file's contents. It's called on + // the first ReadAt, and again on later ones until it succeeds. + getContents func() ([]byte, error) + + mu sync.Mutex + contents []byte // the contents, once known + haveData bool // contents is valid +} + +// data returns the file's contents, getting them first if needed. +func (f *winFSPRegularFile) data() ([]byte, error) { + if f.getContents == nil { + return f.contents, nil + } + f.mu.Lock() + defer f.mu.Unlock() + if !f.haveData { + contents, err := f.getContents() + if err != nil { + return nil, err + } + f.contents, f.haveData = contents, true + } + return f.contents, nil } func (f *winFSPRegularFile) Close() error { return nil } @@ -374,7 +403,11 @@ func (f *winFSPRegularFile) Read(p []byte) (n int, err error) { } func (f *winFSPRegularFile) ReadAt(p []byte, off int64) (n int, err error) { - n = copy(p, f.contents[min(off, int64(len(f.contents))):]) + contents, err := f.data() + if err != nil { + return 0, err + } + n = copy(p, contents[min(off, int64(len(contents))):]) if n == 0 && len(p) > 0 { return 0, io.EOF }