From bd6977a98ae0ce86b9ba9054ff73050c82af97ea Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Mon, 15 Sep 2025 15:53:58 +0200 Subject: [PATCH 1/4] storage/internal/tempdir: add StageAddition() Add a new function to stage additions. This should be used to extract the layer content into a temp directory without holding the storage lock and then under the lock just rename the directory into the final location to reduce the lock contention. Signed-off-by: Paul Holzinger --- storage/internal/tempdir/tempdir.go | 37 +++++++++++++++++++++ storage/internal/tempdir/tempdir_test.go | 42 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/storage/internal/tempdir/tempdir.go b/storage/internal/tempdir/tempdir.go index 6522c45d18..545deef2e2 100644 --- a/storage/internal/tempdir/tempdir.go +++ b/storage/internal/tempdir/tempdir.go @@ -91,6 +91,19 @@ type TempDir struct { counter uint64 } +type stageAddition struct { + source string +} + +// Commit the staged content into its final destination by using os.Rename(). +// That means the dest must be on the same on the same fs as the root directory +// that was given to NewTempDir() and the dest must not exist yet. +// Commit must only be called once per instance returned from the +// StageAddition() call. +func (s *stageAddition) Commit(destination string) error { + return os.Rename(s.source, destination) +} + // CleanupTempDirFunc is a function type that can be returned by operations // which need to perform cleanup actions later. type CleanupTempDirFunc func() error @@ -190,6 +203,30 @@ func NewTempDir(rootDir string) (*TempDir, error) { return td, nil } +// StageAddition creates a new temp directory which is then passed as argument to the +// given callback function. The function should be used to populate the directory with +// content. +// On success StageAddition returns a type with the Commit() function, that function then +// must be used to move the content from the temp directory into its final location. +// +// The caller MUST ensure .Cleanup() is called after Commit(). +// If the TempDir has been cleaned up, this method will return an error. +func (td *TempDir) StageAddition(callback func(path string) error) (*stageAddition, error) { + if td.tempDirLock == nil { + return nil, fmt.Errorf("temp dir instance not initialized or already cleaned up") + } + fileName := fmt.Sprintf("%d-", td.counter) + "addition" + tmpAddPath := filepath.Join(td.tempDirPath, fileName) + if err := os.Mkdir(tmpAddPath, 0o700); err != nil { + return nil, fmt.Errorf("creating temp directory for addition failed: %w", err) + } + td.counter++ + if err := callback(tmpAddPath); err != nil { + return nil, err + } + return &stageAddition{source: tmpAddPath}, nil +} + // StageDeletion moves the specified file into the instance's temporary directory. // The temporary directory must already exist (created during NewTempDir). // Files are renamed with a counter-based prefix (e.g., "0-filename", "1-filename") to ensure uniqueness. diff --git a/storage/internal/tempdir/tempdir_test.go b/storage/internal/tempdir/tempdir_test.go index a556950524..487653e8fe 100644 --- a/storage/internal/tempdir/tempdir_test.go +++ b/storage/internal/tempdir/tempdir_test.go @@ -1,6 +1,7 @@ package tempdir import ( + "errors" "fmt" "os" "path/filepath" @@ -271,3 +272,44 @@ func TestTempDirFileNaming(t *testing.T) { assert.True(t, found, "Expected file %s not found", expectedName) } } + +func TestStageAddition(t *testing.T) { + rootDir := t.TempDir() + td, err := NewTempDir(rootDir) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, td.Cleanup()) + }) + + sa, err := td.StageAddition(func(path string) error { + f, err := os.Create(filepath.Join(path, "file1")) + require.NoError(t, err) + require.NoError(t, f.Close()) + return nil + }) + require.NoError(t, err) + + // need to use a dest which does not exist yet + dest := filepath.Join(t.TempDir(), "dest") + + err = sa.Commit(dest) + require.NoError(t, err) + assert.FileExists(t, filepath.Join(dest, "file1")) + assert.NoDirExists(t, sa.source) +} + +func TestStageAdditionCallbackError(t *testing.T) { + rootDir := t.TempDir() + td, err := NewTempDir(rootDir) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, td.Cleanup()) + }) + + customErr := errors.New("custom error") + + _, err = td.StageAddition(func(path string) error { + return customErr + }) + require.ErrorIs(t, err, customErr) +} From 5f3c6c69a0a53e65d34ef2ff96dd75a28e0a16c6 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 2 Oct 2025 14:48:05 +0200 Subject: [PATCH 2/4] storage: avoid layer lookup in applyDiffWithOptions() That caller in create() already had the layer created in memory so another lookup roundtrip is unnecessary here. Signed-off-by: Paul Holzinger --- storage/layers.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/storage/layers.go b/storage/layers.go index 64d3f5c72c..e9b7e294da 100644 --- a/storage/layers.go +++ b/storage/layers.go @@ -1569,7 +1569,7 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount size = -1 if diff != nil { - if size, err = r.applyDiffWithOptions(layer.ID, moreOptions, diff); err != nil { + if size, err = r.applyDiffWithOptions(layer, moreOptions, diff); err != nil { cleanupFailureContext = "applying layer diff" return nil, -1, err } @@ -2395,20 +2395,19 @@ func updateDigestMap(m *map[digest.Digest][]string, oldvalue, newvalue digest.Di // Requires startWriting. func (r *layerStore) ApplyDiff(to string, diff io.Reader) (size int64, err error) { - return r.applyDiffWithOptions(to, nil, diff) + layer, ok := r.lookup(to) + if !ok { + return -1, ErrLayerUnknown + } + return r.applyDiffWithOptions(layer, nil, diff) } // Requires startWriting. -func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions, diff io.Reader) (size int64, err error) { +func (r *layerStore) applyDiffWithOptions(layer *Layer, layerOptions *LayerOptions, diff io.Reader) (size int64, err error) { if !r.lockfile.IsReadWrite() { return -1, fmt.Errorf("not allowed to modify layer contents at %q: %w", r.layerdir, ErrStoreIsReadOnly) } - layer, ok := r.lookup(to) - if !ok { - return -1, ErrLayerUnknown - } - header := make([]byte, 10240) n, err := diff.Read(header) if err != nil && err != io.EOF { From 164637c44bf9a2213d374d20fa0c2f6a8dc954a2 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 8 Oct 2025 12:36:16 +0200 Subject: [PATCH 3/4] storage: avoid layer lookup in applyDiffFromStagingDirectory() That caller in create() already had the layer created in memory so another lookup roundtrip is unnecessary here. Signed-off-by: Paul Holzinger --- storage/layers.go | 16 ++++++---------- storage/store.go | 2 +- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/storage/layers.go b/storage/layers.go index e9b7e294da..c576f2d371 100644 --- a/storage/layers.go +++ b/storage/layers.go @@ -339,7 +339,7 @@ type rwLayerStore interface { CleanupStagingDirectory(stagingDirectory string) error // applyDiffFromStagingDirectory uses diffOutput.Target to create the diff. - applyDiffFromStagingDirectory(id string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error + applyDiffFromStagingDirectory(layer *Layer, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error // DifferTarget gets the location where files are stored for the layer. DifferTarget(id string) (string, error) @@ -1574,7 +1574,7 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount return nil, -1, err } } else if slo != nil { - if err := r.applyDiffFromStagingDirectory(layer.ID, slo.DiffOutput, slo.DiffOptions); err != nil { + if err := r.applyDiffFromStagingDirectory(layer, slo.DiffOutput, slo.DiffOptions); err != nil { cleanupFailureContext = "applying staged directory diff" return nil, -1, err } @@ -2552,15 +2552,11 @@ func (r *layerStore) DifferTarget(id string) (string, error) { } // Requires startWriting. -func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { +func (r *layerStore) applyDiffFromStagingDirectory(layer *Layer, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { ddriver, ok := r.driver.(drivers.DriverWithDiffer) if !ok { return ErrNotSupported } - layer, ok := r.lookup(id) - if !ok { - return ErrLayerUnknown - } if options == nil { options = &drivers.ApplyDiffWithDifferOpts{ ApplyDiffOpts: drivers.ApplyDiffOpts{ @@ -2621,9 +2617,9 @@ func (r *layerStore) applyDiffFromStagingDirectory(id string, diffOutput *driver } } for k, v := range diffOutput.BigData { - if err := r.SetBigData(id, k, bytes.NewReader(v)); err != nil { - if err2 := r.deleteWhileHoldingLock(id); err2 != nil { - logrus.Errorf("While recovering from a failure to set big data, error deleting layer %#v: %v", id, err2) + if err := r.SetBigData(layer.ID, k, bytes.NewReader(v)); err != nil { + if err2 := r.deleteWhileHoldingLock(layer.ID); err2 != nil { + logrus.Errorf("While recovering from a failure to set big data, error deleting layer %#v: %v", layer.ID, err2) } return err } diff --git a/storage/store.go b/storage/store.go index 84019a4947..b7498b1336 100644 --- a/storage/store.go +++ b/storage/store.go @@ -3177,7 +3177,7 @@ func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { // This code path exists only for cmd/containers/storage.applyDiffUsingStagingDirectory; we have tests that // assume layer creation and applying a staged layer are separate steps. Production pull code always uses the // other path, where layer creation is atomic. - return layer, rlstore.applyDiffFromStagingDirectory(args.ID, args.DiffOutput, args.DiffOptions) + return layer, rlstore.applyDiffFromStagingDirectory(layer, args.DiffOutput, args.DiffOptions) } // if the layer doesn't exist yet, try to create it. From 348a11e66d71897264bdc33c008a248c9eb41da4 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 8 Oct 2025 12:40:56 +0200 Subject: [PATCH 4/4] storage: simplify ApplyDiff() in overlay driver It is not clear to me when it will hit the code path there, by normal layer creation we always pass a valid parent so this branch is never reached AFAICT. Let's remove it and see if all tests still pass in podman, buildah and others... Signed-off-by: Paul Holzinger --- storage/drivers/overlay/overlay.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/storage/drivers/overlay/overlay.go b/storage/drivers/overlay/overlay.go index c08e060466..5e894216c1 100644 --- a/storage/drivers/overlay/overlay.go +++ b/storage/drivers/overlay/overlay.go @@ -2371,16 +2371,6 @@ func (d *Driver) DifferTarget(id string) (string, error) { // ApplyDiff applies the new layer into a root func (d *Driver) ApplyDiff(id, parent string, options graphdriver.ApplyDiffOpts) (size int64, err error) { - if !d.isParent(id, parent) { - if d.options.ignoreChownErrors { - options.IgnoreChownErrors = d.options.ignoreChownErrors - } - if d.options.forceMask != nil { - options.ForceMask = d.options.forceMask - } - return d.naiveDiff.ApplyDiff(id, parent, options) - } - idMappings := options.Mappings if idMappings == nil { idMappings = &idtools.IDMappings{}