From 455b8dcb503d5fc4d72c5e9aa144a963175466aa Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivan@redhat.com>
Date: Thu, 12 Sep 2024 11:07:48 +0200
Subject: [PATCH] chunked: fix reuse of the layers cache

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: https://github.com/containers/storage/issues/2023

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
---
 layers.go                  | 34 +++++-----------------------------
 pkg/chunked/cache_linux.go |  9 +++++----
 store.go                   | 16 ++++++++++------
 3 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/layers.go b/layers.go
index 0f0fc2b22f..6516917093 100644
--- a/layers.go
+++ b/layers.go
@@ -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
@@ -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
 }
 
diff --git a/pkg/chunked/cache_linux.go b/pkg/chunked/cache_linux.go
index 0f6b7b63b8..a7dc18be42 100644
--- a/pkg/chunked/cache_linux.go
+++ b/pkg/chunked/cache_linux.go
@@ -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
 	}
 }
 
@@ -110,7 +111,7 @@ func getLayersCacheRef(store storage.Store) *layersCache {
 		cache.refs++
 		return cache
 	}
-	cache := &layersCache{
+	cache = &layersCache{
 		store: store,
 		refs:  1,
 	}
@@ -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
diff --git a/store.go b/store.go
index e1890f90ed..26906f8df4 100644
--- a/store.go
+++ b/store.go
@@ -3114,12 +3114,16 @@ func (s *store) PrepareStagedLayer(options *drivers.ApplyDiffWithDifferOpts, dif
 }
 
 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)
-	})
+	rlstore, err := s.getLayerStore()
+	if err != nil {
+		return nil, err
+	}
+
+	if to != "" {
+		return nil, fmt.Errorf("ApplyDiffWithDiffer does not support non-empty 'layer' parameter")
+	}
+
+	return rlstore.applyDiffWithDifferNoLock(nil, options, differ)
 }
 
 func (s *store) DifferTarget(id string) (string, error) {