From 4b7016b89f07b2514523e7ad9e0662e4fea568a2 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 28 Nov 2025 14:22:01 +0000 Subject: [PATCH 1/3] snapshot: prevent duplicate calls to mount returning different values In addition to being wasteful, without this, concurrent calls to `Mount` would have lost the previous `target` and so would never have been removed. Signed-off-by: Justin Chadwell --- snapshot/localmounter_darwin.go | 4 ++++ snapshot/localmounter_freebsd.go | 4 ++++ snapshot/localmounter_linux.go | 4 ++++ snapshot/localmounter_windows.go | 4 ++++ 4 files changed, 16 insertions(+) diff --git a/snapshot/localmounter_darwin.go b/snapshot/localmounter_darwin.go index 9acf3a950d6b..570bc930a503 100644 --- a/snapshot/localmounter_darwin.go +++ b/snapshot/localmounter_darwin.go @@ -13,6 +13,10 @@ func (lm *localMounter) Mount() (string, error) { lm.mu.Lock() defer lm.mu.Unlock() + if lm.target != "" { + return lm.target, nil + } + if lm.mounts == nil && lm.mountable != nil { mounts, release, err := lm.mountable.Mount() if err != nil { diff --git a/snapshot/localmounter_freebsd.go b/snapshot/localmounter_freebsd.go index 05db6c9269b6..b0d008684d7a 100644 --- a/snapshot/localmounter_freebsd.go +++ b/snapshot/localmounter_freebsd.go @@ -12,6 +12,10 @@ func (lm *localMounter) Mount() (string, error) { lm.mu.Lock() defer lm.mu.Unlock() + if lm.target != "" { + return lm.target, nil + } + if lm.mounts == nil && lm.mountable != nil { mounts, release, err := lm.mountable.Mount() if err != nil { diff --git a/snapshot/localmounter_linux.go b/snapshot/localmounter_linux.go index d188e5241b13..a8132c9a7d3b 100644 --- a/snapshot/localmounter_linux.go +++ b/snapshot/localmounter_linux.go @@ -16,6 +16,10 @@ func (lm *localMounter) Mount() (string, error) { lm.mu.Lock() defer lm.mu.Unlock() + if lm.target != "" { + return lm.target, nil + } + if lm.mounts == nil && lm.mountable != nil { mounts, release, err := lm.mountable.Mount() if err != nil { diff --git a/snapshot/localmounter_windows.go b/snapshot/localmounter_windows.go index 0bb4df884e88..5169ba77253d 100644 --- a/snapshot/localmounter_windows.go +++ b/snapshot/localmounter_windows.go @@ -16,6 +16,10 @@ func (lm *localMounter) Mount() (string, error) { lm.mu.Lock() defer lm.mu.Unlock() + if lm.target != "" { + return lm.target, nil + } + if lm.mounts == nil && lm.mountable != nil { mounts, release, err := lm.mountable.Mount() if err != nil { From 7aaa7974dd89049313b15bc7ddcc35e64873dbcd Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Fri, 28 Nov 2025 14:13:27 +0000 Subject: [PATCH 2/3] gateway: cache local mounts from fs operations Previously, each call to `ReadFile`/`ReadDir`/`StatFile` would trigger an entire `Mount`+`Unmount` operation. This could result in terrible performance for repeated filesystem accesses - potentially worse that one `Mount`+`Unmount` per file, if the file is read in ranges. To resolve this, we now directly store the `localMounter` in the gateway, and rely on the fact that repeated calls to it return the same value. These mounts will only be removed when the gateway itself is released. Signed-off-by: Justin Chadwell --- cache/util/fsutil.go | 145 ++++++++++---------------- frontend/gateway/forwarder/forward.go | 52 ++++++++- frontend/gateway/gateway.go | 82 ++++++++++----- 3 files changed, 161 insertions(+), 118 deletions(-) diff --git a/cache/util/fsutil.go b/cache/util/fsutil.go index 3fefd338530d..5bf04ef1fb03 100644 --- a/cache/util/fsutil.go +++ b/cache/util/fsutil.go @@ -7,7 +7,6 @@ import ( "path/filepath" "github.com/containerd/continuity/fs" - "github.com/moby/buildkit/snapshot" "github.com/pkg/errors" "github.com/tonistiigi/fsutil" fstypes "github.com/tonistiigi/fsutil/types" @@ -23,64 +22,34 @@ type FileRange struct { Length int } -func withMount(mount snapshot.Mountable, cb func(string) error) error { - lm := snapshot.LocalMounter(mount) - - root, err := lm.Mount() +func ReadFile(ctx context.Context, root string, req ReadRequest) ([]byte, error) { + fp, err := fs.RootPath(root, req.Filename) if err != nil { - return err + return nil, errors.WithStack(err) } - defer func() { - if lm != nil { - lm.Unmount() + f, err := os.Open(fp) + if err != nil { + // The filename here is internal to the mount, so we can restore + // the request base path for error reporting. + // See os.DirFS.Open for details. + pe := &os.PathError{} + if errors.As(err, &pe) { + pe.Path = req.Filename } - }() - - if err := cb(root); err != nil { - return err + return nil, errors.WithStack(err) } + defer f.Close() - if err := lm.Unmount(); err != nil { - return err + var rdr io.Reader = f + if req.Range != nil { + rdr = io.NewSectionReader(f, int64(req.Range.Offset), int64(req.Range.Length)) } - lm = nil - return nil -} - -func ReadFile(ctx context.Context, mount snapshot.Mountable, req ReadRequest) ([]byte, error) { - var dt []byte - - err := withMount(mount, func(root string) error { - fp, err := fs.RootPath(root, req.Filename) - if err != nil { - return errors.WithStack(err) - } - - f, err := os.Open(fp) - if err != nil { - // The filename here is internal to the mount, so we can restore - // the request base path for error reporting. - // See os.DirFS.Open for details. - pe := &os.PathError{} - if errors.As(err, &pe) { - pe.Path = req.Filename - } - return errors.WithStack(err) - } - defer f.Close() - - var rdr io.Reader = f - if req.Range != nil { - rdr = io.NewSectionReader(f, int64(req.Range.Offset), int64(req.Range.Length)) - } - dt, err = io.ReadAll(rdr) - if err != nil { - return errors.WithStack(err) - } - return nil - }) - return dt, err + dt, err := io.ReadAll(rdr) + if err != nil { + return nil, errors.WithStack(err) + } + return dt, nil } type ReadDirRequest struct { @@ -88,7 +57,7 @@ type ReadDirRequest struct { IncludePattern string } -func ReadDir(ctx context.Context, mount snapshot.Mountable, req ReadDirRequest) ([]*fstypes.Stat, error) { +func ReadDir(ctx context.Context, root string, req ReadDirRequest) ([]*fstypes.Stat, error) { var ( rd []*fstypes.Stat fo fsutil.FilterOpt @@ -96,48 +65,46 @@ func ReadDir(ctx context.Context, mount snapshot.Mountable, req ReadDirRequest) if req.IncludePattern != "" { fo.IncludePatterns = append(fo.IncludePatterns, req.IncludePattern) } - err := withMount(mount, func(root string) error { - fp, err := fs.RootPath(root, req.Path) + fp, err := fs.RootPath(root, req.Path) + if err != nil { + return nil, errors.WithStack(err) + } + err = fsutil.Walk(ctx, fp, &fo, func(path string, info os.FileInfo, err error) error { if err != nil { - return errors.WithStack(err) + return errors.Wrapf(err, "walking %q", root) } - return fsutil.Walk(ctx, fp, &fo, func(path string, info os.FileInfo, err error) error { - if err != nil { - return errors.Wrapf(err, "walking %q", root) - } - stat, ok := info.Sys().(*fstypes.Stat) - if !ok { - // This "can't happen(tm)". - return errors.Errorf("expected a *fsutil.Stat but got %T", info.Sys()) - } - rd = append(rd, stat) - - if info.IsDir() { - return filepath.SkipDir - } - return nil - }) - }) - return rd, err -} - -func StatFile(ctx context.Context, mount snapshot.Mountable, path string) (*fstypes.Stat, error) { - var st *fstypes.Stat - err := withMount(mount, func(root string) error { - fp, err := fs.RootPath(root, path) - if err != nil { - return errors.WithStack(err) + stat, ok := info.Sys().(*fstypes.Stat) + if !ok { + // This "can't happen(tm)". + return errors.Errorf("expected a *fsutil.Stat but got %T", info.Sys()) } - if st, err = fsutil.Stat(fp); err != nil { - // The filename here is internal to the mount, so we can restore - // the request base path for error reporting. - // See os.DirFS.Open for details. - replaceErrorPath(err, path) - return errors.WithStack(err) + rd = append(rd, stat) + + if info.IsDir() { + return filepath.SkipDir } return nil }) - return st, err + if err != nil { + return nil, err + } + return rd, nil +} + +func StatFile(ctx context.Context, root string, path string) (*fstypes.Stat, error) { + fp, err := fs.RootPath(root, path) + if err != nil { + return nil, errors.WithStack(err) + } + st, err := fsutil.Stat(fp) + if err != nil { + // The filename here is internal to the mount, so we can restore + // the request base path for error reporting. + // See os.DirFS.Open for details. + replaceErrorPath(err, path) + return nil, errors.WithStack(err) + } + return st, nil } // replaceErrorPath will override the path in an os.PathError in the error chain. diff --git a/frontend/gateway/forwarder/forward.go b/frontend/gateway/forwarder/forward.go index 4180896a1f86..9b4b517c065d 100644 --- a/frontend/gateway/forwarder/forward.go +++ b/frontend/gateway/forwarder/forward.go @@ -358,7 +358,14 @@ func (r *ref) ReadFile(ctx context.Context, req client.ReadRequest) ([]byte, err Length: r.Length, } } - return cacheutil.ReadFile(ctx, m, newReq) + + var dt []byte + err = withMount(m, func(root string) error { + var err error + dt, err = cacheutil.ReadFile(ctx, root, newReq) + return err + }) + return dt, err } func (r *ref) ReadDir(ctx context.Context, req client.ReadDirRequest) ([]*fstypes.Stat, error) { @@ -370,7 +377,14 @@ func (r *ref) ReadDir(ctx context.Context, req client.ReadDirRequest) ([]*fstype Path: req.Path, IncludePattern: req.IncludePattern, } - return cacheutil.ReadDir(ctx, m, newReq) + + var rd []*fstypes.Stat + err = withMount(m, func(root string) error { + var err error + rd, err = cacheutil.ReadDir(ctx, root, newReq) + return err + }) + return rd, err } func (r *ref) StatFile(ctx context.Context, req client.StatRequest) (*fstypes.Stat, error) { @@ -378,7 +392,14 @@ func (r *ref) StatFile(ctx context.Context, req client.StatRequest) (*fstypes.St if err != nil { return nil, err } - return cacheutil.StatFile(ctx, m, req.Path) + + var st *fstypes.Stat + err = withMount(m, func(root string) error { + var err error + st, err = cacheutil.StatFile(ctx, root, req.Path) + return err + }) + return st, err } func (r *ref) getMountable(ctx context.Context) (snapshot.Mountable, error) { @@ -392,3 +413,28 @@ func (r *ref) getMountable(ctx context.Context) (snapshot.Mountable, error) { } return ref.ImmutableRef.Mount(ctx, true, r.session) } + +func withMount(mount snapshot.Mountable, cb func(string) error) error { + lm := snapshot.LocalMounter(mount) + + root, err := lm.Mount() + if err != nil { + return err + } + + defer func() { + if lm != nil { + lm.Unmount() + } + }() + + if err := cb(root); err != nil { + return err + } + + if err := lm.Unmount(); err != nil { + return err + } + lm = nil + return nil +} diff --git a/frontend/gateway/gateway.go b/frontend/gateway/gateway.go index fd8bba5c582e..3f171f2da6e0 100644 --- a/frontend/gateway/gateway.go +++ b/frontend/gateway/gateway.go @@ -370,6 +370,10 @@ func (lbf *llbBridgeForwarder) Discard() { }) } + for _, mount := range lbf.mounts { + mount.Unmount() + } + for id, workerRef := range lbf.workerRefByID { workerRef.Release(context.TODO()) delete(lbf.workerRefByID, id) @@ -440,6 +444,7 @@ func newBridgeForwarder(ctx context.Context, llbBridge frontend.FrontendLLBBridg sid: sid, sm: sm, ctrs: map[string]gwclient.Container{}, + mounts: map[string]snapshot.Mounter{}, executor: exec, } return lbf @@ -553,8 +558,10 @@ type llbBridgeForwarder struct { sm *session.Manager executor executor.Executor *pipe - ctrs map[string]gwclient.Container - ctrsMu sync.Mutex + ctrs map[string]gwclient.Container + ctrsMu sync.Mutex + mounts map[string]snapshot.Mounter + mountsMu sync.Mutex } func (lbf *llbBridgeForwarder) ResolveSourceMeta(ctx context.Context, req *pb.ResolveSourceMetaRequest) (*pb.ResolveSourceMetaResponse, error) { @@ -869,6 +876,37 @@ func (lbf *llbBridgeForwarder) getImmutableRef(ctx context.Context, id string) ( return workerRef.ImmutableRef, nil } +func (lbf *llbBridgeForwarder) getMounter(ctx context.Context, id string, ref cache.ImmutableRef) (snapshot.Mounter, error) { + lbf.mountsMu.Lock() + defer lbf.mountsMu.Unlock() + + mounter, ok := lbf.mounts[id] + if ok { + return mounter, nil + } + var mountable snapshot.Mountable + if ref != nil { + var err error + mountable, err = ref.Mount(ctx, true, session.NewGroup(lbf.sid)) + if err != nil { + return nil, err + } + } + + mounter = snapshot.LocalMounter(mountable) + lbf.mounts[id] = mounter + return mounter, nil +} + +func (lbf *llbBridgeForwarder) getMount(ctx context.Context, id string, ref cache.ImmutableRef) (string, error) { + mounter, err := lbf.getMounter(ctx, id, ref) + if err != nil { + return "", err + } + // corresponding Unmount call is made in Discard() + return mounter.Mount() +} + func (lbf *llbBridgeForwarder) ReadFile(ctx context.Context, req *pb.ReadFileRequest) (*pb.ReadFileResponse, error) { ctx = tracing.ContextWithSpanFromContext(ctx, lbf.callCtx) @@ -876,6 +914,10 @@ func (lbf *llbBridgeForwarder) ReadFile(ctx context.Context, req *pb.ReadFileReq if err != nil { return nil, err } + root, err := lbf.getMount(ctx, req.Ref, ref) + if err != nil { + return nil, err + } newReq := cacheutil.ReadRequest{ Filename: req.FilePath, @@ -887,15 +929,7 @@ func (lbf *llbBridgeForwarder) ReadFile(ctx context.Context, req *pb.ReadFileReq } } - var m snapshot.Mountable - if ref != nil { - m, err = ref.Mount(ctx, true, session.NewGroup(lbf.sid)) - if err != nil { - return nil, err - } - } - - dt, err := cacheutil.ReadFile(ctx, m, newReq) + dt, err := cacheutil.ReadFile(ctx, root, newReq) if err != nil { return nil, lbf.wrapSolveError(err) } @@ -910,19 +944,17 @@ func (lbf *llbBridgeForwarder) ReadDir(ctx context.Context, req *pb.ReadDirReque if err != nil { return nil, err } + root, err := lbf.getMount(ctx, req.Ref, ref) + if err != nil { + return nil, err + } newReq := cacheutil.ReadDirRequest{ Path: req.DirPath, IncludePattern: req.IncludePattern, } - var m snapshot.Mountable - if ref != nil { - m, err = ref.Mount(ctx, true, session.NewGroup(lbf.sid)) - if err != nil { - return nil, err - } - } - entries, err := cacheutil.ReadDir(ctx, m, newReq) + + entries, err := cacheutil.ReadDir(ctx, root, newReq) if err != nil { return nil, lbf.wrapSolveError(err) } @@ -937,14 +969,12 @@ func (lbf *llbBridgeForwarder) StatFile(ctx context.Context, req *pb.StatFileReq if err != nil { return nil, err } - var m snapshot.Mountable - if ref != nil { - m, err = ref.Mount(ctx, true, session.NewGroup(lbf.sid)) - if err != nil { - return nil, err - } + root, err := lbf.getMount(ctx, req.Ref, ref) + if err != nil { + return nil, err } - st, err := cacheutil.StatFile(ctx, m, req.Path) + + st, err := cacheutil.StatFile(ctx, root, req.Path) if err != nil { return nil, err } From 2485ab71b447624141d13c920aedab2fdbfa6ceb Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Thu, 4 Dec 2025 11:33:12 +0000 Subject: [PATCH 3/3] gateway: cache local mounts for forwarder as well Signed-off-by: Justin Chadwell --- frontend/gateway/forwarder/forward.go | 78 +++++++++++---------------- 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/frontend/gateway/forwarder/forward.go b/frontend/gateway/forwarder/forward.go index 9b4b517c065d..7343ca6d6ed7 100644 --- a/frontend/gateway/forwarder/forward.go +++ b/frontend/gateway/forwarder/forward.go @@ -38,6 +38,7 @@ func LLBBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLL workers: w, workerRefByID: make(map[string]*worker.WorkerRef), executor: exec, + mounts: make(map[string]snapshot.Mounter), } bc.buildOpts = bc.loadBuildOpts() return bc, nil @@ -56,6 +57,7 @@ type BridgeClient struct { buildOpts client.BuildOpts ctrs []client.Container executor executor.Executor + mounts map[string]snapshot.Mounter } func (c *BridgeClient) Solve(ctx context.Context, req client.SolveRequest) (*client.Result, error) { @@ -213,6 +215,10 @@ func (c *BridgeClient) discard(err error) { ctr.Release(context.TODO()) } + for _, mount := range c.mounts { + mount.Unmount() + } + for id, workerRef := range c.workerRefByID { workerRef.ImmutableRef.Release(context.TODO()) delete(c.workerRefByID, id) @@ -345,7 +351,7 @@ func (r *ref) Evaluate(ctx context.Context) error { } func (r *ref) ReadFile(ctx context.Context, req client.ReadRequest) ([]byte, error) { - m, err := r.getMountable(ctx) + root, err := r.getMount(ctx) if err != nil { return nil, err } @@ -358,18 +364,11 @@ func (r *ref) ReadFile(ctx context.Context, req client.ReadRequest) ([]byte, err Length: r.Length, } } - - var dt []byte - err = withMount(m, func(root string) error { - var err error - dt, err = cacheutil.ReadFile(ctx, root, newReq) - return err - }) - return dt, err + return cacheutil.ReadFile(ctx, root, newReq) } func (r *ref) ReadDir(ctx context.Context, req client.ReadDirRequest) ([]*fstypes.Stat, error) { - m, err := r.getMountable(ctx) + root, err := r.getMount(ctx) if err != nil { return nil, err } @@ -377,32 +376,23 @@ func (r *ref) ReadDir(ctx context.Context, req client.ReadDirRequest) ([]*fstype Path: req.Path, IncludePattern: req.IncludePattern, } - - var rd []*fstypes.Stat - err = withMount(m, func(root string) error { - var err error - rd, err = cacheutil.ReadDir(ctx, root, newReq) - return err - }) - return rd, err + return cacheutil.ReadDir(ctx, root, newReq) } func (r *ref) StatFile(ctx context.Context, req client.StatRequest) (*fstypes.Stat, error) { - m, err := r.getMountable(ctx) + root, err := r.getMount(ctx) if err != nil { return nil, err } - - var st *fstypes.Stat - err = withMount(m, func(root string) error { - var err error - st, err = cacheutil.StatFile(ctx, root, req.Path) - return err - }) - return st, err + return cacheutil.StatFile(ctx, root, req.Path) } -func (r *ref) getMountable(ctx context.Context) (snapshot.Mountable, error) { +func (r *ref) getMounter(ctx context.Context) (snapshot.Mounter, error) { + id := r.resultProxy.ID() + if mounter, ok := r.c.mounts[id]; ok { + return mounter, nil + } + rr, err := r.resultProxy.Result(ctx) if err != nil { return nil, r.c.wrapSolveError(err) @@ -411,30 +401,22 @@ func (r *ref) getMountable(ctx context.Context) (snapshot.Mountable, error) { if !ok { return nil, errors.Errorf("invalid ref: %T", rr.Sys()) } - return ref.ImmutableRef.Mount(ctx, true, r.session) -} -func withMount(mount snapshot.Mountable, cb func(string) error) error { - lm := snapshot.LocalMounter(mount) - - root, err := lm.Mount() + mountable, err := ref.ImmutableRef.Mount(ctx, true, r.session) if err != nil { - return err + return nil, err } + mounter := snapshot.LocalMounter(mountable) - defer func() { - if lm != nil { - lm.Unmount() - } - }() - - if err := cb(root); err != nil { - return err - } + r.c.mounts[id] = mounter + return mounter, nil +} - if err := lm.Unmount(); err != nil { - return err +func (r *ref) getMount(ctx context.Context) (string, error) { + mounter, err := r.getMounter(ctx) + if err != nil { + return "", err } - lm = nil - return nil + // corresponding Unmount call is made in discard() + return mounter.Mount() }