storage: fsync staging directory before atomic rename

[asahay19]

PR Summary

Add fsync of overlay staging directory contents before the atomic rename that
commits a new layer during image pull/unpack.

Replaces auto-closed containers/storage#2397
after the storage library moved into this monorepo.

Problem

This gap was reported against real customer deployments. Edge and
disconnected SNO customers (vehicle-mounted, tactical, and air-gapped
deployments with unstable power) hit readlink: invalid argument errors and
CRI-O crashes after hard resets, and requested a fix, since these nodes cannot
simply re-pull images from a registry that isn't reachable.
Root cause: when CRI-O (via go.podman.io/storage) pulls an image, layer data
is written into a staging directory and then committed with os.Rename().
Nothing forces that data to physical disk before the rename makes it visible
at its final path.
If power is lost in that window, the layer isn't just missing — it's present
but corrupted: binaries can end up truncated, and overlay l/ symlinks can be
left invalid, producing exactly the readlink: invalid argument error reported
above. On restart, CRI-O detects the unclean shutdown and internal_repair
can't tell which layer is actually bad, so it falls back to a broad,
destructive repair — on a disconnected node, that means unrecoverable image
loss.

Solution

Guarantee durability before the rename ever exposes the new layer, so that
window can't exist regardless of sync=filesystem configuration:

1. fdatasync() every file under the staging directory
2. fsync() every directory under the staging tree, bottom-up
3. Only then perform the atomic os.Rename()
   This mirrors the pattern already used by atomicFileWriter
   (storage/pkg/ioutils/fswriters_linux.go) for single files, extended to a
   whole staging tree. It's scoped to just the layer being committed — much
   cheaper than Syncfs()'s whole-filesystem flush

Changes

File	Change
storage/pkg/ioutils/sync_directory_linux.go	New SyncDirectoryContents() helper (Linux)
storage/pkg/ioutils/sync_directory_linux_test.go	Unit test
storage/drivers/overlay/overlay.go	Call sync before rename in ApplyDiffFromStagingDirectory()

Testing

 asahay@asahay-ubuntu:~/container-libs/storage$ go test ./pkg/ioutils/ -run TestSyncDirectoryContents -v
=== RUN   TestSyncDirectoryContents
--- PASS: TestSyncDirectoryContents (0.00s)
PASS
ok  	go.podman.io/storage/pkg/ioutils	0

[mtrmac]

https://github.com/podman-container-tools/container-libs/blob/main/CONTRIBUTING.md#sign-your-prs please, we can’t really even look at contributions with unclear copyright status.

---

We now have sync=filesystem via #622 , isn’t that sufficient? What is missing?

[asahay19]

https://github.com/podman-container-tools/container-libs/blob/main/CONTRIBUTING.md#sign-your-prs please, we can’t really even look at contributions with unclear copyright status.

We now have sync=filesystem via #622 , isn’t that sufficient? What is missing?

Thank you @mtrmac for your feedback. sync=filesystem helps, but doesn't fully close this gap: it's opt-in (SyncModeNone is the default), and even when enabled, its Syncfs() call happens after ApplyDiffFromStagingDirectory's os.Rename() (as part of saveFor(layer, true)), so there's still a window where the layer is visible at its final path before it's actually durable. This PR closes that window by syncing before the rename, and works unconditionally regardless of sync=filesystem config. I've updated the PR description with the full details (Problem/Solution sections). Happy to adjust the approach if there's a simpler way to get the same guarantee.

[mtrmac]

We now have sync=filesystem via #622 , isn’t that sufficient? What is missing?

sync=filesystem helps, but doesn't fully close this gap: it's opt-in (SyncModeNone is the default)

*shrug* so opt in.

, and even when enabled, its Syncfs() call happens after ApplyDiffFromStagingDirectory's os.Rename() (as part of saveFor(layer, true)), so there's still a window where the layer is visible at its final path before it's actually durable.

That shouldn’t matter; the store is locked in the meantime, so there should be no concurrent accesses . And in case of power loss or the like, what governs is the layer database in layers.json, and until the Syncfs succeeds, layers.json still records the layer with incompleteFlag, to be removed on the next write. Nothing should observe the unsynced layer contents.

[mtrmac]

What documents that fsync on a directory also syncs all nodes of its children?

[asahay19]

Thanks for the detailed explanation, @mtrmac, that makes sense. If the store lock genuinely prevents any concurrent access to the layer while it's still flagged incomplete, and layers.json is what governs recovery after a crash (not the on-disk layer content itself), then sync=filesystem should indeed close this gap without needing a pre-rename fsync.

I am going to validate that end-to-end (kill -9 mid-pull with sync=filesystem enabled, confirm check --repair cleans up correctly) before I invest further here. If that holds up, I will likely move to adopting/backporting sync=filesystem instead of this approach, and I'll close this PR out. Will report back either way. appreciate you pushing on this.