storage: new sync option for overlay/vfs by giuseppe · Pull Request #622 · containers/container-libs

giuseppe · 2026-02-02T10:56:37Z

Add a new driver-specific sync configuration option that controls filesystem synchronization during layer operations. When set to "filesystem", the driver ensures all pending writes are flushed to the file system before marking a layer as complete.

This helps prevent data corruption in scenarios where the system crashes or loses power before the filesystem has finished writing layer data.

giuseppe · 2026-02-02T10:56:55Z

@mrunalp PTAL

mtrmac

just an initial skim.

storage/docs/containers-storage.conf.5.md

storage/pkg/system/syncfs_windows.go

mtrmac · 2026-02-02T12:18:39Z

storage/pkg/config/config.go

 			doptions = append(doptions, fmt.Sprintf("%s.use_composefs=%s", driverName, options.Overlay.UseComposefs))
 		}
+		if options.Overlay.SyncPolicy != "" {
+			doptions = append(doptions, fmt.Sprintf("%s.sync_policy=%s", driverName, options.Overlay.SyncPolicy))


I seriously dislike this kind of turning of structured data back into strings.

I realize that’s not the fault of this PR, but can we stop doing that some time? There will never be a convenient time.

I suppose OptionsConfig, or something, could be stored in StoreOptions, and perhaps added to drivers.Options.

Also it would be nice to parse the value into an enum already here.

storage/drivers/vfs/driver.go

storage/drivers/driver.go

mrunalp · 2026-02-03T19:30:47Z

👍 to adding this option.

mrunalp · 2026-02-03T21:02:10Z

@nalind thoughts?

Signed-off-by: Ayato Tokubi <atokubi@redhat.com>

Luap99

not really fully review just some drive by comments I might be missing the context of the origin of this.

Luap99 · 2026-02-05T17:59:22Z

storage/docs/containers-storage.conf.5.md

+**sync**=""
+  Filesystem synchronization mode for layer creation. (default: "")


default: "" does not say what the default actually is, it should say none or filesystem.

I think it default to none? Is that the right default? I mean practically speaking should we not do the safe thing as default. I guess the concern would be performance? But the people needing that could opt. Without sync we need to worry about more store corruption which seems worse for us?

Luap99 · 2026-02-05T18:02:08Z

storage/drivers/overlay/overlay.go

-					logrus.Debugf("Error Syncfs(%s) - %v", mountpoint, err)
-				}
-				unix.Close(fd)
+			if err := system.Syncfs(mountpoint); err != nil {


is this correct to not use the syncmode setting here and always sync?

yes, this was already done before, I've only reused the new function now.

If you kill a FUSE file system, all the data in memory will be lost, even if the system didn't crash. This is a fallback path taken when doing it correctly through fusermount failed.

The syncfs here prevents that when fusermount failed (or it is not present), we do not lose data on fuse-overlayfs when we unmount it.

cgwalters · 2026-02-10T23:01:07Z

Of reference too composefs/composefs-rs#185 (comment) Basically ostree keeps track of the boot id optionally - another option here is to have a periodic "walk layer dirs and fsync()" which is syscall expensive (without io-uring) but avoids flushing unrelated I/O.

This of course relates to "containers-storage on top of composefs(rs)" in that this option would today always be on by default...

giuseppe · 2026-02-16T09:51:53Z

@mrunalp @bitoku got any feedback on this feature?

bitoku · 2026-02-16T10:22:54Z

Can we keep it open for a while until we get any feedback?

bitoku · 2026-02-19T10:45:14Z

we haven't got any feedback from the customer, but they'd like us not to wait for them to get it released.
If it doesn't hurt, can we proceed with this change?

giuseppe · 2026-02-19T10:47:46Z

@containers/storage-maintainers PTAL

giuseppe · 2026-03-04T13:44:28Z

@mtrmac @kolyshkin @Luap99 PTAL

Luap99

LGTM

mtrmac

ACK overall.

storage/docs/containers-storage.conf.5.md

storage/drivers/driver.go

storage/drivers/vfs/vfs_test.go

storage/pkg/config/config_test.go

storage/drivers/driver.go

storage/layers.go

storage/drivers/vfs/driver.go

mtrmac · 2026-03-06T14:08:33Z

storage/pkg/config/config_test.go

+		t.Fatalf("Expected to find overlay sync option in %v", doptions)
+	}
+
+	// Make sure no sync option when not configured


These tests already start with “empty should mean 0 options”, this is unnecessary. Same with VFS.

mtrmac · 2026-03-06T14:10:36Z

storage/layers.go

 			modifiedLocations |= layer.location
 		}
-		if err := r.saveLayers(modifiedLocations); err != nil {
+		if err := r.saveLayers(modifiedLocations, true); err != nil {


This one is after deleting layers and BigData… I am really unsure whether we need to sync.

I suppose any future operation will sync that?!

At least, it is not “metadata only” so we might need a more precise naming of the option/situation.

mtrmac · 2026-03-06T14:11:22Z

storage/layers.go


 // saveFor saves the contents of the store relevant for modifiedLayer to disk.
+// If metadataOnly is true, only the layer metadata (db) is being changed (e.g. setting a flag,
+// changing names), and the sync of the storage filesystem is skipped.


(After the API settles) please document this also in the other save… methods.

mtrmac · 2026-03-06T14:12:42Z

storage/layers.go

-		}
-	}
-
+	// Write the tar-split file before UpdateLayerIDMap.


Why? This hints at the order being important but it doesn’t say what to pay attention to.

this is unnecessary now as the sync happens later.

On the other hand, the AtomicWriteFile is not needed here. I think we need to drop it

I think the general principle has been “writes of our metadata always atomic, writes of layer data are atomic only with the new opt-in”. In that sense, tar-split probably counts as “metadata” — OTOH, if either the layer data or the tar-split is corrupted, the outcome is ~the same, so I don’t feel strongly about this.

yes, I agree with the "always write medata atomically". I am only proposing tar-split as an exception because it still needs the data to be meaningful

mtrmac · 2026-03-06T14:14:10Z

storage/layers.go

 		}
+		// If the underlying storage driver is using sync, make sure we sync everything before
+		// saving the layer data, this ensures that all files/directories are properly created and written.
+		if r.driver.SyncMode() != drivers.SyncModeNone {


ACK. For safety, I think I’d prefer a private enum over just a bool. (And I didn’t now carefully review every call site, assuming that it would be easier with the enum.)

there is no reason to sync the tarsplit data if the rest of the layer was not sync'ed. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

save the layer info only after the tarsplit file and big data was written. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

it is a preparatory commit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Add a new driver-specific `sync` configuration option that controls filesystem synchronization during layer operations. When set to "filesystem", the driver ensures all pending writes are flushed to the file system before marking a layer as complete. This helps prevent data corruption in scenarios where the system crashes or loses power before the filesystem has finished writing layer data. Only the vfs and overlay backends support the new flag. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2026-03-06T15:52:30Z

I've left a bool but renamed it to needsSyncfs so the intention is clearer

giuseppe · 2026-03-11T13:01:43Z

@mtrmac is there anything more left or can we move forward with this PR?

github-actions bot added the storage Related to "storage" package label Feb 2, 2026

giuseppe force-pushed the sync-policy branch from 143b43e to e823dba Compare February 2, 2026 11:23

mtrmac reviewed Feb 2, 2026

View reviewed changes

giuseppe force-pushed the sync-policy branch from e823dba to 0c8ddf4 Compare February 2, 2026 15:46

giuseppe changed the title ~~[RFC] new sync_policy option for overlay/vf~~ [RFC] new sync option for overlay/vf Feb 2, 2026

giuseppe changed the title ~~[RFC] new sync option for overlay/vf~~ [RFC] new sync option for overlay/vfs Feb 2, 2026

giuseppe marked this pull request as ready for review February 3, 2026 11:58

giuseppe changed the title ~~[RFC] new sync option for overlay/vfs~~ storage: new sync option for overlay/vfs Feb 3, 2026

bitoku added a commit to bitoku/storage that referenced this pull request Feb 4, 2026

cherry-pick of containers/container-libs#622

0cfac1e

bitoku added a commit to bitoku/storage that referenced this pull request Feb 4, 2026

cherry-pick of containers/container-libs#622

a05b156

Signed-off-by: Ayato Tokubi <atokubi@redhat.com>

Luap99 reviewed Feb 5, 2026

View reviewed changes

giuseppe force-pushed the sync-policy branch from 0c8ddf4 to 7c91d75 Compare February 5, 2026 20:09

giuseppe force-pushed the sync-policy branch 2 times, most recently from 747478c to 2bc826e Compare February 16, 2026 09:41

Luap99 approved these changes Mar 5, 2026

View reviewed changes

mtrmac reviewed Mar 6, 2026

View reviewed changes

giuseppe force-pushed the sync-policy branch 2 times, most recently from 63ba009 to 31330a9 Compare March 6, 2026 09:29

mtrmac reviewed Mar 6, 2026

View reviewed changes

giuseppe added 2 commits March 6, 2026 16:47

layers: do not sync the tarsplit file

b643825

there is no reason to sync the tarsplit data if the rest of the layer was not sync'ed. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

layers: save layer only after tarsplit file

df2ecd5

save the layer info only after the tarsplit file and big data was written. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe added 4 commits March 6, 2026 16:47

system: expose Syncfs

8495084

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

overlay: use system function

b603b69

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

layers: add new needsSyncfs flag to save functions

f85067b

it is a preparatory commit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the sync-policy branch from 31330a9 to 2453705 Compare March 6, 2026 15:52

		sync=""
		Filesystem synchronization mode for layer creation. (default: "")

Conversation

giuseppe commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented Feb 2, 2026

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrunalp commented Feb 3, 2026

Uh oh!

mrunalp commented Feb 3, 2026

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Feb 10, 2026

Uh oh!

giuseppe commented Feb 16, 2026

Uh oh!

bitoku commented Feb 16, 2026

Uh oh!

bitoku commented Feb 19, 2026

Uh oh!

giuseppe commented Feb 19, 2026

Uh oh!

giuseppe commented Mar 4, 2026

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Mar 6, 2026

Uh oh!

giuseppe commented Mar 11, 2026

Uh oh!

Reviewers

giuseppe commented Feb 2, 2026 •

edited

Loading