Integrate the temporary directory into layer deletion

This enables deferred deletion of physical files outside of global locks, improving performance and reducing lock contention.

Signed-off-by: Jan Rodák <[email protected]>

Author: Honny1

drivers/aufs/aufs.go

@@ -36,6 +36,7 @@ import (
 	"time"
 
 	graphdriver "github.com/containers/storage/drivers"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chrootarchive"
 	"github.com/containers/storage/pkg/directory"
@@ -781,3 +782,9 @@ func (a *Driver) SupportsShifting(uidmap, gidmap []idtools.IDMap) bool {
 func (a *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) {
 	return graphdriver.DedupResult{}, nil
 }
+
+// DeferredRemove is not implemented.
+// It calls Remove directly.
+func (a *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	return nil, a.Remove(id)
+}

drivers/btrfs/btrfs.go

@@ -30,6 +30,7 @@ import (
 	"unsafe"
 
 	graphdriver "github.com/containers/storage/drivers"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/fileutils"
 	"github.com/containers/storage/pkg/idtools"
@@ -678,3 +679,9 @@ func (d *Driver) AdditionalImageStores() []string {
 func (d *Driver) Dedup(req graphdriver.DedupArgs) (graphdriver.DedupResult, error) {
 	return graphdriver.DedupResult{}, nil
 }
+
+// DeferredRemove is not implemented.
+// It calls Remove directly.
+func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	return nil, d.Remove(id)
+}

drivers/driver.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/containers/storage/internal/dedup"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/fileutils"
@@ -124,6 +125,10 @@ type ProtoDriver interface {
 	CreateFromTemplate(id, template string, templateIDMappings *idtools.IDMappings, parent string, parentIDMappings *idtools.IDMappings, opts *CreateOpts, readWrite bool) error
 	// Remove attempts to remove the filesystem layer with this id.
 	Remove(id string) error
+	// DeferredRemove is used to remove the filesystem layer with this id
+	// at a later time, e.g., when the last reference to the layer is removed.
+	// It returns a function that can be used to remove the layer later.
+	DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error)
 	// Get returns the mountpoint for the layered filesystem referred
 	// to by this id. You can optionally specify a mountLabel or "".
 	// Optionally it gets the mappings used to create the layer.

drivers/overlay/overlay.go

@@ -24,6 +24,7 @@ import (
 	"github.com/containers/storage/drivers/quota"
 	"github.com/containers/storage/internal/dedup"
 	"github.com/containers/storage/internal/staging_lockfile"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chrootarchive"
 	"github.com/containers/storage/pkg/directory"
@@ -1327,6 +1328,35 @@ func (d *Driver) Remove(id string) error {
 	return nil
 }
 
+func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	t, err := tempdir.NewTempDir(filepath.Join(d.home, stagingDir))
+	if err != nil {
+		return nil, err
+	}
+
+	dir := d.dir(id)
+	lid, err := os.ReadFile(path.Join(dir, "link"))
+	if err == nil {
+		if err := t.Add(path.Join(d.home, linkDir, string(lid))); err != nil {
+			logrus.Debugf("Failed to Add to stage Directory link: %v", err)
+		}
+	}
+
+	d.releaseAdditionalLayerByID(id)
+
+	if err := t.Add(dir); err != nil {
+		return t.Cleanup, fmt.Errorf("failed to add to stage directory: %w", err)
+	}
+
+	if d.quotaCtl != nil {
+		d.quotaCtl.ClearQuota(dir)
+		if d.imageStore != "" {
+			d.quotaCtl.ClearQuota(d.imageStore)
+		}
+	}
+	return t.Cleanup, nil
+}
+
 // recreateSymlinks goes through the driver's home directory and checks if the diff directory
 // under each layer has a symlink created for it under the linkDir. If the symlink does not
 // exist, it creates them
@@ -1353,8 +1383,8 @@ func (d *Driver) recreateSymlinks() error {
 		// Check that for each layer, there's a link in "l" with the name in
 		// the layer's "link" file that points to the layer's "diff" directory.
 		for _, dir := range dirs {
-			// Skip over the linkDir and anything that is not a directory
-			if dir.Name() == linkDir || !dir.IsDir() {
+			// Skip over the linkDir and anything that is not a directory or tempDir
+			if dir.Name() == linkDir || !dir.IsDir() || dir.Name() == stagingDir {
 				continue
 			}
 			// Read the "link" file under each layer to get the name of the symlink

drivers/vfs/driver.go

@@ -11,6 +11,7 @@ import (
 
 	graphdriver "github.com/containers/storage/drivers"
 	"github.com/containers/storage/internal/dedup"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/fileutils"
@@ -244,6 +245,18 @@ func (d *Driver) Remove(id string) error {
 	return system.EnsureRemoveAll(d.dir(id))
 }
 
+func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	pathToRemove := d.dir(id)
+	t, err := tempdir.NewTempDir(filepath.Dir(pathToRemove))
+	if err != nil {
+		return nil, err
+	}
+	if err := t.Add(pathToRemove); err != nil {
+		return nil, err
+	}
+	return t.Cleanup, nil
+}
+
 // Get returns the directory for the given id.
 func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) {
 	dir := d.dir(id)

drivers/windows/windows.go

@@ -24,6 +24,7 @@ import (
 	"github.com/Microsoft/go-winio/backuptar"
 	"github.com/Microsoft/hcsshim"
 	graphdriver "github.com/containers/storage/drivers"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/fileutils"
@@ -1014,3 +1015,9 @@ func parseStorageOpt(storageOpt map[string]string) (*storageOptions, error) {
 	}
 	return &options, nil
 }
+
+// DeferredRemove is not implemented.
+// It calls Remove directly.
+func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	return nil, d.Remove(id)
+}

drivers/zfs/zfs.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	graphdriver "github.com/containers/storage/drivers"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/containers/storage/pkg/mount"
@@ -406,6 +407,12 @@ func (d *Driver) Remove(id string) error {
 	return nil
 }
 
+// DeferredRemove is not implemented.
+// It calls Remove directly.
+func (d *Driver) DeferredRemove(id string) (tempdir.CleanupTempDirFunc, error) {
+	return nil, d.Remove(id)
+}
+
 // Get returns the mountpoint for the given id after creating the target directories if necessary.
 func (d *Driver) Get(id string, options graphdriver.MountOpts) (_ string, retErr error) {
 	mountpoint := d.mountPath(id)

layers.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	drivers "github.com/containers/storage/drivers"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/containers/storage/pkg/ioutils"
@@ -43,6 +44,7 @@ const (
 	// in readers (which, for implementation reasons, gives other writers the opportunity to create more inconsistent state)
 	// until we just give up.
 	maxLayerStoreCleanupIterations = 3
+	tempDirPath                    = "tmp"
 )
 
 type layerLocations uint8
@@ -293,6 +295,9 @@ type rwLayerStore interface {
 	// Delete deletes a layer with the specified name or ID.
 	Delete(id string) error
 
+	// deferredDelete deletes a layer with the specified name or ID.
+	deferredDelete(id string) ([]tempdir.CleanupTempDirFunc, error)
+
 	// Wipe deletes all layers.
 	Wipe() error
 
@@ -794,6 +799,12 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
 	layers := []*Layer{}
 	ids := make(map[string]*Layer)
 
+	if r.lockfile.IsReadWrite() {
+		if err := tempdir.RecoverStaleDirs(filepath.Join(r.layerdir, tempDirPath)); err != nil {
+			return false, err
+		}
+	}
+
 	for locationIndex := range numLayerLocationIndex {
 		location := layerLocationFromIndex(locationIndex)
 		rpath := r.jsonPath[locationIndex]
@@ -935,7 +946,12 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
 		// Now actually delete the layers
 		for _, layer := range layersToDelete {
 			logrus.Warnf("Found incomplete layer %q, deleting it", layer.ID)
-			err := r.deleteInternal(layer.ID)
+			cleanFunctions, err := r.deleteWhileHoldingLock(layer.ID)
+			defer func() {
+				if err := tempdir.CleanupTemporaryDirectories(cleanFunctions...); err != nil {
+					logrus.Errorf("Error cleaning up temporary directories: %v", err)
+				}
+			}()
 			if err != nil {
 				// Don't return the error immediately, because deleteInternal does not saveLayers();
 				// Even if deleting one incomplete layer fails, call saveLayers() so that other possible successfully
@@ -1920,13 +1936,15 @@ func layerHasIncompleteFlag(layer *Layer) bool {
 }
 
 // Requires startWriting.
-func (r *layerStore) deleteInternal(id string) error {
+// Caller MUST run all returned cleanup functions after this. Ideally outside of
+// the startWriting.
+func (r *layerStore) deleteWhileHoldingLock(id string) ([]tempdir.CleanupTempDirFunc, error) {
 	if !r.lockfile.IsReadWrite() {
-		return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly)
+		return nil, fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly)
 	}
 	layer, ok := r.lookup(id)
 	if !ok {
-		return ErrLayerUnknown
+		return nil, ErrLayerUnknown
 	}
 	// Ensure that if we are interrupted, the layer will be cleaned up.
 	if !layerHasIncompleteFlag(layer) {
@@ -1935,16 +1953,29 @@ func (r *layerStore) deleteInternal(id string) error {
 		}
 		layer.Flags[incompleteFlag] = true
 		if err := r.saveFor(layer); err != nil {
-			return err
+			return nil, err
 		}
 	}
+	tempDirectory, err := tempdir.NewTempDir(filepath.Join(r.layerdir, tempDirPath))
+	if err != nil {
+		return nil, err
+	}
 	// We never unset incompleteFlag; below, we remove the entire object from r.layers.
 	id = layer.ID
-	if err := r.driver.Remove(id); err != nil && !errors.Is(err, os.ErrNotExist) {
-		return err
+	cleanFunctions := []tempdir.CleanupTempDirFunc{}
+	cleanFunc, err := r.driver.DeferredRemove(id)
+	cleanFunctions = append(cleanFunctions, cleanFunc)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		return cleanFunctions, err
+	}
+
+	cleanFunctions = append(cleanFunctions, tempDirectory.Cleanup)
+	if err := tempDirectory.Add(r.tspath(id)); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return cleanFunctions, err
+	}
+	if err := tempDirectory.Add(r.datadir(id)); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return cleanFunctions, err
 	}
-	os.Remove(r.tspath(id))
-	os.RemoveAll(r.datadir(id))
 	delete(r.byid, id)
 	for _, name := range layer.Names {
 		delete(r.byname, name)
@@ -1968,7 +1999,7 @@ func (r *layerStore) deleteInternal(id string) error {
 	}) {
 		selinux.ReleaseLabel(mountLabel)
 	}
-	return nil
+	return cleanFunctions, nil
 }
 
 // Requires startWriting.
@@ -1989,9 +2020,18 @@ func (r *layerStore) deleteInDigestMap(id string) {
 
 // Requires startWriting.
 func (r *layerStore) Delete(id string) error {
+	cleanupFunctions, deferErr := r.deferredDelete(id)
+	cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...)
+	return errors.Join(deferErr, cleanupErr)
+}
+
+// Requires startWriting.
+// Caller MUST run all returned cleanup functions after this. Ideally outside of
+// the startWriting.
+func (r *layerStore) deferredDelete(id string) ([]tempdir.CleanupTempDirFunc, error) {
 	layer, ok := r.lookup(id)
 	if !ok {
-		return ErrLayerUnknown
+		return nil, ErrLayerUnknown
 	}
 	id = layer.ID
 	// The layer may already have been explicitly unmounted, but if not, we
@@ -2003,13 +2043,14 @@ func (r *layerStore) Delete(id string) error {
 			break
 		}
 		if err != nil {
-			return err
+			return nil, err
 		}
 	}
-	if err := r.deleteInternal(id); err != nil {
-		return err
+	cleanFunctions, err := r.deleteWhileHoldingLock(id)
+	if err != nil {
+		return cleanFunctions, err
 	}
-	return r.saveFor(layer)
+	return cleanFunctions, r.saveFor(layer)
 }
 
 // Requires startReading or startWriting.

store.go

@@ -22,6 +22,7 @@ import (
 
 	drivers "github.com/containers/storage/drivers"
 	"github.com/containers/storage/internal/dedup"
+	"github.com/containers/storage/internal/tempdir"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/directory"
 	"github.com/containers/storage/pkg/idtools"
@@ -2540,7 +2541,13 @@ func (s *store) Lookup(name string) (string, error) {
 	return "", ErrLayerUnknown
 }
 
-func (s *store) DeleteLayer(id string) error {
+func (s *store) DeleteLayer(id string) (err error) {
+	cleanupFunctions := []tempdir.CleanupTempDirFunc{}
+	defer func() {
+		if cleanupErr := tempdir.CleanupTemporaryDirectories(cleanupFunctions...); cleanupErr != nil {
+			err = errors.Join(cleanupErr, err)
+		}
+	}()
 	return s.writeToAllStores(func(rlstore rwLayerStore) error {
 		if rlstore.Exists(id) {
 			if l, err := rlstore.Get(id); err != nil {
@@ -2574,7 +2581,9 @@ func (s *store) DeleteLayer(id string) error {
 					return fmt.Errorf("layer %v used by container %v: %w", id, container.ID, ErrLayerUsedByContainer)
 				}
 			}
-			if err := rlstore.Delete(id); err != nil {
+			cf, err := rlstore.deferredDelete(id)
+			cleanupFunctions = append(cleanupFunctions, cf...)
+			if err != nil {
 				return fmt.Errorf("delete layer %v: %w", id, err)
 			}