storage: new sync option for overlay/vfs#622
storage: new sync option for overlay/vfs#622giuseppe wants to merge 5 commits intocontainers:mainfrom
Conversation
|
@mrunalp PTAL |
| Use ComposeFS to mount the data layers image. ComposeFS support is experimental and not recommended for production use. | ||
| This is a "string bool": "false"|"true" (cannot be native TOML boolean) | ||
|
|
||
| **sync_policy**="" |
There was a problem hiding this comment.
(“policy” is a unnecessary word here)
storage/pkg/system/syncfs_windows.go
Outdated
| return err | ||
| } | ||
| defer windows.CloseHandle(h) | ||
| return windows.FlushFileBuffers(h) |
There was a problem hiding this comment.
Does this really sync the whole filesystem? A quick skim of the reference documentation suggests it’s just that one file.
There was a problem hiding this comment.
yes, thanks. I don't find any equivalent of syncfs (or sync), so I am just going to return an error on Windows
storage/pkg/config/config.go
Outdated
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also it would be nice to parse the value into an enum already here.
storage/drivers/vfs/driver.go
Outdated
| return err | ||
| } | ||
|
|
||
| return d.syncPolicy.Sync(dir) |
There was a problem hiding this comment.
UpdateLayerIDMap is a very surprising place for this, at a glance. Why is this the structure?
There was a problem hiding this comment.
we chown the entire layer, we need to make sure these files are persisted. For overlay it is only metadata, but for vfs we copy the entire layer
giuseppe
changed the title
giuseppe
changed the title
|
👍 to adding this option. |
giuseppe
changed the title
|
@nalind thoughts? |
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
not really fully review just some drive by comments I might be missing the context of the origin of this.
| **sync**="" | ||
| Filesystem synchronization mode for layer creation. (default: "") |
There was a problem hiding this comment.
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?
| logrus.Debugf("Error Syncfs(%s) - %v", mountpoint, err) | ||
| } | ||
| unix.Close(fd) | ||
| if err := system.Syncfs(mountpoint); err != nil { |
There was a problem hiding this comment.
is this correct to not use the syncmode setting here and always sync?
There was a problem hiding this comment.
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.
|
Of reference too containers/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... |
save the layer info only after the tarsplit file was written. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
preparatory commit for the next patch. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
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
force-pushed
the
sync-policy
branch
from
7c91d75 to
747478c
Compare
5 participants
Add a new driver-specific
syncconfiguration 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.