From b7f8c3510d9135ad56277140f9e5aa86eb0e9a66 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Wed, 29 Apr 2026 15:14:24 -0700 Subject: [PATCH] erofs: remove fsmerge threshold from snapshotter Currently the metadata may be generated after a snapshot is committed, causing a difference in the overlayfs mount that may cause ESTALE errors. The committed snapshot must be immutable and should always return the same set of mounts after commit, even if configuration is changed. This setting may be added back later to be performed before commit, either as part of the unpack or as a step before commit. This is getting pulled out from the upcoming release. Signed-off-by: Derek McGowan --- plugins/snapshots/erofs/erofs.go | 108 +++++--------------- plugins/snapshots/erofs/erofs_linux_test.go | 71 +++++++++++++ plugins/snapshots/erofs/plugin/plugin.go | 7 -- 3 files changed, 94 insertions(+), 92 deletions(-) diff --git a/plugins/snapshots/erofs/erofs.go b/plugins/snapshots/erofs/erofs.go index 47402f73ba..586d16583c 100644 --- a/plugins/snapshots/erofs/erofs.go +++ b/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 diff --git a/plugins/snapshots/erofs/erofs_linux_test.go b/plugins/snapshots/erofs/erofs_linux_test.go index 3a13533bb9..07e1600890 100644 --- a/plugins/snapshots/erofs/erofs_linux_test.go +++ b/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) + }) +} diff --git a/plugins/snapshots/erofs/plugin/plugin.go b/plugins/snapshots/erofs/plugin/plugin.go index ff2a1b4b00..834767f714 100644 --- a/plugins/snapshots/erofs/plugin/plugin.go +++ b/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)) }