Merge pull request #13321 from dmcgowan/remove-erofs-fsmerge-threshold

erofs: remove fsmerge threshold from snapshotter

plugins/snapshots/erofs/erofs.go

@@ -20,11 +20,8 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"runtime"
-	"strings"
-	"time"
 
 	"github.com/containerd/continuity/fs"
 	"github.com/containerd/errdefs"
@@ -48,9 +45,7 @@ type SnapshotterConfig struct {
 	setImmutable bool
 	// defaultSize creates a default size writable layer for active snapshots
 	defaultSize int64
-	// fsMergeThreshold (>0) enables fsmerge when the number of image layers exceeds this value
-	fsMergeThreshold uint
-	remapIDs         bool
+	remapIDs    bool
 	// dmverityMode controls dm-verity behavior: "auto" (use if .dmverity exists), "on" (require .dmverity), "off" (disable)
 	dmverityMode string
 }
@@ -93,13 +88,6 @@ func WithDefaultSize(size int64) Opt {
 	}
 }
 
-// WithFsMergeThreshold (>0) enables fsmerge when the number of image layers exceeds this value
-func WithFsMergeThreshold(v uint) Opt {
-	return func(config *SnapshotterConfig) {
-		config.fsMergeThreshold = v
-	}
-}
-
 // WithRemapIDs enables kernel ID-mapped mounts for user namespace support
 func WithRemapIDs() Opt {
 	return func(config *SnapshotterConfig) {
@@ -114,16 +102,15 @@ type MetaStore interface {
 }
 
 type snapshotter struct {
-	root             string
-	ms               *storage.MetaStore
-	ovlOptions       []string
-	enableFsverity   bool
-	setImmutable     bool
-	defaultWritable  int64
-	blockMode        bool
-	fsMergeThreshold uint
-	remapIDs         bool
-	dmverityMode     string
+	root            string
+	ms              *storage.MetaStore
+	ovlOptions      []string
+	enableFsverity  bool
+	setImmutable    bool
+	defaultWritable int64
+	blockMode       bool
+	remapIDs        bool
+	dmverityMode    string
 }
 
 // NewSnapshotter returns a Snapshotter which uses EROFS+OverlayFS. The layers
@@ -191,16 +178,15 @@ func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) {
 	}
 
 	return &snapshotter{
-		root:             root,
-		ms:               ms,
-		ovlOptions:       config.ovlOptions,
-		enableFsverity:   config.enableFsverity,
-		setImmutable:     config.setImmutable,
-		defaultWritable:  config.defaultSize,
-		blockMode:        config.defaultSize > 0,
-		fsMergeThreshold: config.fsMergeThreshold,
-		remapIDs:         config.remapIDs,
-		dmverityMode:     config.dmverityMode,
+		root:            root,
+		ms:              ms,
+		ovlOptions:      config.ovlOptions,
+		enableFsverity:  config.enableFsverity,
+		setImmutable:    config.setImmutable,
+		defaultWritable: config.defaultSize,
+		blockMode:       config.defaultSize > 0,
+		remapIDs:        config.remapIDs,
+		dmverityMode:    config.dmverityMode,
 	}, nil
 }
 
@@ -448,12 +434,10 @@ func (s *snapshotter) mounts(snap storage.Snapshot, info snapshots.Info) ([]moun
 	for i := range snap.ParentIDs {
 		// If a merged fsmeta is valid for this layer, skip the remaining bottom layers.
 		// Why? Because bottom layers have been flattened with the thin fsmeta.
-		if s.fsMergeThreshold > 0 {
-			if m, ok := s.mountFsMeta(snap, i); ok {
-				mounts = append(mounts, m)
-				first = len(mounts) - 1
-				break
-			}
+		if m, ok := s.mountFsMeta(snap, i); ok {
+			mounts = append(mounts, m)
+			first = len(mounts) - 1
+			break
 		}
 
 		layerBlob, err := s.lowerPath(snap.ParentIDs[i])
@@ -601,11 +585,6 @@ func (s *snapshotter) createSnapshot(ctx context.Context, kind snapshots.Kind, k
 		return nil, err
 	}
 
-	// Generate fsmeta outside of the transaction since it's unnecessary.
-	// Also ignore all errors since it's a nice-to-have stuff.
-	if !strings.Contains(key, snapshots.UnpackKeyPrefix) {
-		s.generateFsMeta(ctx, snap.ParentIDs)
-	}
 	return s.mounts(snap, info)
 }
 
@@ -654,47 +633,6 @@ func (s *snapshotter) commitBlock(ctx context.Context, layerBlob string, id stri
 	return nil
 }
 
-// generate a metadata-only EROFS fsmeta.erofs if all EROFS layer blobs are valid
-func (s *snapshotter) generateFsMeta(ctx context.Context, snapIDs []string) {
-	var blobs []string
-
-	if s.fsMergeThreshold == 0 || uint(len(snapIDs)) <= s.fsMergeThreshold {
-		return
-	}
-
-	t1 := time.Now()
-	mergedMeta := s.fsMetaPath(snapIDs[0])
-	// If the empty placeholder cannot be created (mainly due to os.IsExist), just return
-	if _, err := os.OpenFile(mergedMeta, os.O_CREATE|os.O_EXCL, 0644); err != nil {
-		return
-	}
-
-	for i := len(snapIDs) - 1; i >= 0; i-- {
-		blob := s.layerBlobPath(snapIDs[i])
-		if _, err := os.Stat(blob); err != nil {
-			return
-		}
-		blobs = append(blobs, blob)
-	}
-	tmpMergedMeta := mergedMeta + ".tmp"
-	args := append([]string{"--aufs", "--ovlfs-strip=1", "--quiet", tmpMergedMeta}, blobs...)
-	log.G(ctx).Infof("merging layers with mkfs.erofs %v", args)
-	cmd := exec.CommandContext(ctx, "mkfs.erofs", args...)
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		log.G(ctx).Warnf("failed to generate merged fsmeta for %v: %q: %v", snapIDs[0], string(out), err)
-		return
-	}
-	// Atomically replace the fsmeta with the generated file
-	if err = os.Rename(tmpMergedMeta, mergedMeta); err != nil {
-		log.G(ctx).Errorf("failed to rename fsmeta: %v", err)
-		return
-	}
-	log.G(ctx).WithFields(log.Fields{
-		"d": time.Since(t1),
-	}).Infof("merged fsmeta for %v generated", snapIDs[0])
-}
-
 func (s *snapshotter) Commit(ctx context.Context, name, key string, opts ...snapshots.Opt) error {
 	var layerBlob string
 	var id string

plugins/snapshots/erofs/erofs_linux_test.go

@@ -703,3 +703,74 @@ func TestApplyDmverityPolicy(t *testing.T) {
 		assert.Equal(t, "X-containerd.dmverity="+expectedPath, opt)
 	})
 }
+
+func TestMountFsMeta(t *testing.T) {
+	root := t.TempDir()
+	s := &snapshotter{root: root}
+
+	parents := []string{"p0", "p1", "p2"}
+	for _, id := range parents {
+		require.NoError(t, os.MkdirAll(filepath.Join(root, "snapshots", id), 0755))
+	}
+
+	writeMeta := func(t *testing.T, id string, contents []byte) {
+		t.Helper()
+		require.NoError(t, os.WriteFile(s.fsMetaPath(id), contents, 0644))
+	}
+	removeMeta := func(t *testing.T, id string) {
+		t.Helper()
+		err := os.Remove(s.fsMetaPath(id))
+		if err != nil && !os.IsNotExist(err) {
+			t.Fatal(err)
+		}
+	}
+
+	snap := storage.Snapshot{ParentIDs: parents}
+
+	t.Run("missing fsmeta returns false", func(t *testing.T) {
+		for _, id := range parents {
+			removeMeta(t, id)
+		}
+		_, ok := s.mountFsMeta(snap, 0)
+		assert.False(t, ok)
+	})
+
+	t.Run("empty fsmeta returns false", func(t *testing.T) {
+		writeMeta(t, "p0", nil)
+		t.Cleanup(func() { removeMeta(t, "p0") })
+
+		_, ok := s.mountFsMeta(snap, 0)
+		assert.False(t, ok)
+	})
+
+	t.Run("non-empty fsmeta on top parent returns mount with all device options", func(t *testing.T) {
+		writeMeta(t, "p0", []byte("merged"))
+		t.Cleanup(func() { removeMeta(t, "p0") })
+
+		m, ok := s.mountFsMeta(snap, 0)
+		require.True(t, ok)
+		assert.Equal(t, "erofs", m.Type)
+		assert.Equal(t, s.fsMetaPath("p0"), m.Source)
+		// Devices appended in reverse parent order from len-1 down to id.
+		assert.Equal(t, []string{
+			"ro", "loop",
+			"device=" + s.layerBlobPath("p2"),
+			"device=" + s.layerBlobPath("p1"),
+			"device=" + s.layerBlobPath("p0"),
+		}, m.Options)
+	})
+
+	t.Run("non-empty fsmeta on intermediate parent only references parents at or below id", func(t *testing.T) {
+		writeMeta(t, "p1", []byte("merged"))
+		t.Cleanup(func() { removeMeta(t, "p1") })
+
+		m, ok := s.mountFsMeta(snap, 1)
+		require.True(t, ok)
+		assert.Equal(t, s.fsMetaPath("p1"), m.Source)
+		assert.Equal(t, []string{
+			"ro", "loop",
+			"device=" + s.layerBlobPath("p2"),
+			"device=" + s.layerBlobPath("p1"),
+		}, m.Options)
+	})
+}

plugins/snapshots/erofs/plugin/plugin.go

@@ -52,9 +52,6 @@ type Config struct {
 	// DefaultSize is the default size of a writable layer in string
 	DefaultSize string `toml:"default_size"`
 
-	// MaxUnmergedLayers (>0) enables fsmerge when the number of image layers exceeds this value.
-	MaxUnmergedLayers uint `toml:"max_unmerged_layers"`
-
 	// DmverityMode controls dm-verity behavior: "auto" (use if available), "on" (require), "off" (disable)
 	// Linux only
 	DmverityMode string `toml:"dmverity_mode"`
@@ -99,10 +96,6 @@ func init() {
 				opts = append(opts, erofs.WithDefaultSize(size))
 			}
 
-			if config.MaxUnmergedLayers > 0 {
-				opts = append(opts, erofs.WithFsMergeThreshold(config.MaxUnmergedLayers))
-			}
-
 			if config.DmverityMode != "" {
 				opts = append(opts, erofs.WithDmverityMode(config.DmverityMode))
 			}