erofs: add fsync_on_commit to sync a layer before its commit

Commit records a snapshot in the metadata store, which syncs its own
file, while layer.erofs and the snapshot directory may still be in the
page cache. A power loss shortly after a pull can then leave a committed
snapshot whose blob is corrupt or whose directory is missing. A later
pull does nothing because the chain ID exists, and running the image
fails to mount the layer.

With fsync_on_commit = true, Commit fsyncs everything in the snapshot
directory, the directory itself and snapshots/ before the snapshot is
recorded. Blobs served from a layer content cache are not synced. The
option is off by default and lives in the snapshotter because the
differ's SyncFs does not reach Commit, where the walking differ's blob
is created.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Mykhaylo Sul <mykhaylo.sul@oss.qualcomm.com>

docs/snapshotters/erofs.md

@@ -141,6 +141,13 @@ forget to restart containerd after changing the configuration.
       # Enable fsverity support for EROFS layers, default is false
       enable_fsverity = true
 
+      # fsync the snapshot directory, everything in it and its entry in
+      # snapshots/ before a snapshot is committed, so a power loss cannot
+      # leave a committed snapshot without its blob or directory. Blobs served
+      # from layer_content_caches are not synced. Costs a few fsyncs per layer,
+      # default is false
+      fsync_on_commit = true
+
       # Optional: Additional mount options for overlayfs
       ovl_mount_options = []
 

plugins/snapshots/erofs/erofs.go

@@ -51,6 +51,9 @@ type SnapshotterConfig struct {
 	enableFsverity bool
 	// setImmutable enables IMMUTABLE_FL file attribute for EROFS layers
 	setImmutable bool
+	// fsyncOnCommit makes Commit sync the layer blob and the snapshot
+	// directories before the snapshot is recorded as committed
+	fsyncOnCommit bool
 	// defaultSize creates a default size writable layer for active snapshots
 	defaultSize int64
 	remapIDs    bool
@@ -88,6 +91,14 @@ func WithImmutable() Opt {
 	}
 }
 
+// WithFsyncOnCommit makes Commit sync the layer blob and the snapshot
+// directories to disk before the snapshot is recorded as committed
+func WithFsyncOnCommit() Opt {
+	return func(config *SnapshotterConfig) {
+		config.fsyncOnCommit = true
+	}
+}
+
 // WithDmverityMode sets the dm-verity mode: "auto" (default), "on" (required), or "off" (disabled)
 func WithDmverityMode(mode string) Opt {
 	return func(config *SnapshotterConfig) {
@@ -131,6 +142,7 @@ type snapshotter struct {
 	ovlOptions         []string
 	enableFsverity     bool
 	setImmutable       bool
+	fsyncOnCommit      bool
 	defaultWritable    int64
 	blockMode          bool
 	remapIDs           bool
@@ -234,6 +246,7 @@ func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) {
 		ovlOptions:         config.ovlOptions,
 		enableFsverity:     config.enableFsverity,
 		setImmutable:       config.setImmutable,
+		fsyncOnCommit:      config.fsyncOnCommit,
 		defaultWritable:    config.defaultSize,
 		blockMode:          config.defaultSize > 0,
 		remapIDs:           config.remapIDs,
@@ -283,6 +296,33 @@ func (s *snapshotter) layerBlobPath(id string) string {
 	return filepath.Join(s.root, "snapshots", id, "layer.erofs")
 }
 
+// syncDir fsyncs the immediate entries of dir, then dir itself and its parent,
+// so that the files in dir, its entry list and its own entry in the parent are
+// on disk before the caller records them elsewhere. It does not recurse.
+func syncDir(dir string) error {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return fmt.Errorf("failed to read %s for sync: %w", dir, err)
+	}
+	paths := make([]string, 0, len(entries)+2)
+	for _, e := range entries {
+		paths = append(paths, filepath.Join(dir, e.Name()))
+	}
+	paths = append(paths, dir, filepath.Dir(dir))
+	for _, p := range paths {
+		f, err := os.Open(p)
+		if err != nil {
+			return fmt.Errorf("failed to open %s for sync: %w", p, err)
+		}
+		err = f.Sync()
+		f.Close()
+		if err != nil {
+			return fmt.Errorf("failed to sync %s: %w", p, err)
+		}
+	}
+	return nil
+}
+
 func (s *snapshotter) fsMetaPath(id string) string {
 	return filepath.Join(s.root, "snapshots", id, "fsmeta.erofs")
 }
@@ -963,6 +1003,22 @@ func (s *snapshotter) Commit(ctx context.Context, name, key string, opts ...snap
 
 	// Note: dm-verity formatting is handled by the EROFS differ, not here
 
+	// The metadata store syncs its own file when the snapshot is committed, so
+	// with fsync_on_commit enabled the layer is made durable first; otherwise
+	// a power loss shortly after a successful pull can leave a committed
+	// snapshot whose blob is truncated or whose directory does not exist, and
+	// nothing re-pulls a layer whose chain ID already exists. Everything in
+	// the snapshot directory is synced rather than a list of known files, then
+	// the directory and its entry in snapshots/, which was created by a rename
+	// in Prepare and may still be unwritten. A blob served from the layer
+	// content cache lives elsewhere and is not synced; whoever populates the
+	// cache must make it durable.
+	if s.fsyncOnCommit {
+		if err := syncDir(filepath.Join(s.root, "snapshots", id)); err != nil {
+			return err
+		}
+	}
+
 	return s.ms.WithTransaction(ctx, true, func(ctx context.Context) error {
 		if _, err := os.Stat(layerBlob); err != nil {
 			return fmt.Errorf("failed to get the converted erofs blob: %w", err)

plugins/snapshots/erofs/erofs_linux_test.go

@@ -143,6 +143,21 @@ func TestGetCleanupDirectoriesSkipsSnapshotTempDirs(t *testing.T) {
 	assert.Equal(t, []string{orphanDir}, cleanup)
 }
 
+// TestSyncDir checks that a snapshot directory holding files and
+// subdirectories can be synced, and that a directory that cannot be read
+// returns an error instead of being skipped.
+func TestSyncDir(t *testing.T) {
+	root := t.TempDir()
+	snapshotDir := filepath.Join(root, "snapshots", "1")
+	require.NoError(t, os.MkdirAll(filepath.Join(snapshotDir, "fs"), 0700))
+	require.NoError(t, os.WriteFile(filepath.Join(snapshotDir, "layer.erofs"), []byte("erofs"), 0644))
+
+	require.NoError(t, syncDir(snapshotDir))
+
+	err := syncDir(filepath.Join(root, "snapshots", "missing"))
+	require.ErrorIs(t, err, os.ErrNotExist)
+}
+
 // TestWritableSize exercises the LabelSnapshotMaxSize override that the
 // block-mode mkfs path passes to X-containerd.mkfs.size. Covers the
 // happy path (label overrides default), fallback cases (missing, empty,

plugins/snapshots/erofs/plugin/plugin.go

@@ -51,6 +51,13 @@ type Config struct {
 	// If `SetImmutable` is enabled, IMMUTABLE_FL will be set on layer blobs.
 	SetImmutable bool `toml:"set_immutable"`
 
+	// FsyncOnCommit makes Commit fsync the snapshot directory, everything in
+	// it and its entry in snapshots/ before the snapshot is recorded, so that
+	// a power loss cannot leave a committed snapshot without its blob or
+	// directory. Blobs served from a layer content cache are not synced. Off
+	// by default, like the diff service's sync_fs.
+	FsyncOnCommit bool `toml:"fsync_on_commit"`
+
 	// DefaultSize is the default size of a writable layer in string
 	DefaultSize string `toml:"default_size"`
 
@@ -114,6 +121,10 @@ func init() {
 				opts = append(opts, erofs.WithImmutable())
 			}
 
+			if config.FsyncOnCommit {
+				opts = append(opts, erofs.WithFsyncOnCommit())
+			}
+
 			if config.DefaultSize != "" {
 				size, err := units.RAMInBytes(config.DefaultSize)
 				if err != nil {