Skip to content

Commit

Permalink
chunked: fix reuse of the layers cache
Browse files Browse the repository at this point in the history
the global singleton was never updated, causing the cache to be always
recreated for each layer.

It is not possible to keep the layersCache mutex for the entire load()
since it calls into some store APIs causing a deadlock since
findDigestInternal() is already called while some store locks are
held.

Another benefit is that now only one goroutine can run load()
preventing multiple calls to load() to happen in parallel doing the
same work.

Closes: containers#2023

Signed-off-by: Giuseppe Scrivano <[email protected]>
  • Loading branch information
giuseppe committed Sep 19, 2024
1 parent 5f68205 commit e8ea2b2
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 40 deletions.
34 changes: 5 additions & 29 deletions layers.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,8 @@ type rwLayerStore interface {
// applies its changes to a specified layer.
ApplyDiff(to string, diff io.Reader) (int64, error)

// ApplyDiffWithDiffer applies the changes through the differ callback function.
// If to is the empty string, then a staging directory is created by the driver.
ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error)
// applyDiffWithDifferNoLock applies the changes through the differ callback function.
applyDiffWithDifferNoLock(options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error)

// CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors
CleanupStagingDirectory(stagingDirectory string) error
Expand Down Expand Up @@ -2543,37 +2542,14 @@ func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *driver
return err
}

// Requires startWriting.
func (r *layerStore) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) {
// It must be called without any c/storage locks held to allow differ to make c/storage calls.
func (r *layerStore) applyDiffWithDifferNoLock(options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) {
ddriver, ok := r.driver.(drivers.DriverWithDiffer)
if !ok {
return nil, ErrNotSupported
}

if to == "" {
output, err := ddriver.ApplyDiffWithDiffer("", "", options, differ)
return &output, err
}

layer, ok := r.lookup(to)
if !ok {
return nil, ErrLayerUnknown
}
if options == nil {
options = &drivers.ApplyDiffWithDifferOpts{
ApplyDiffOpts: drivers.ApplyDiffOpts{
Mappings: r.layerMappings(layer),
MountLabel: layer.MountLabel,
},
}
}
output, err := ddriver.ApplyDiffWithDiffer(layer.ID, layer.Parent, options, differ)
if err != nil {
return nil, err
}
layer.UIDs = output.UIDs
layer.GIDs = output.GIDs
err = r.saveFor(layer)
output, err := ddriver.ApplyDiffWithDiffer("", "", options, differ)
return &output, err
}

Expand Down
9 changes: 5 additions & 4 deletions pkg/chunked/cache_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func (c *layer) release() {
if err := unix.Munmap(c.mmapBuffer); err != nil {
logrus.Warnf("Error Munmap: layer %q: %v", c.id, err)
}
c.mmapBuffer = nil
}
}

Expand Down Expand Up @@ -110,7 +111,7 @@ func getLayersCacheRef(store storage.Store) *layersCache {
cache.refs++
return cache
}
cache := &layersCache{
cache = &layersCache{
store: store,
refs: 1,
}
Expand Down Expand Up @@ -779,14 +780,14 @@ func (c *layersCache) findDigestInternal(digest string) (string, string, int64,
return "", "", -1, nil
}

c.mutex.RLock()
defer c.mutex.RUnlock()

binaryDigest, err := makeBinaryDigest(digest)
if err != nil {
return "", "", 0, err
}

c.mutex.RLock()
defer c.mutex.RUnlock()

for _, layer := range c.layers {
if !layer.cacheFile.bloomFilter.maybeContains(binaryDigest) {
continue
Expand Down
16 changes: 9 additions & 7 deletions store.go
Original file line number Diff line number Diff line change
Expand Up @@ -3110,16 +3110,18 @@ func (s *store) CleanupStagedLayer(diffOutput *drivers.DriverWithDifferOutput) e
}

func (s *store) PrepareStagedLayer(options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) {
return s.ApplyDiffWithDiffer("", options, differ)
rlstore, err := s.getLayerStore()
if err != nil {
return nil, err
}
return rlstore.applyDiffWithDifferNoLock(options, differ)
}

func (s *store) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) {
return writeToLayerStore(s, func(rlstore rwLayerStore) (*drivers.DriverWithDifferOutput, error) {
if to != "" && !rlstore.Exists(to) {
return nil, ErrLayerUnknown
}
return rlstore.ApplyDiffWithDiffer(to, options, differ)
})
if to != "" {
return nil, fmt.Errorf("ApplyDiffWithDiffer does not support non-empty 'layer' parameter")
}
return s.PrepareStagedLayer(options, differ)
}

func (s *store) DifferTarget(id string) (string, error) {
Expand Down

0 comments on commit e8ea2b2

Please sign in to comment.