chunked: use reflinks for chunk deduplication by giuseppe · Pull Request #892 · podman-container-tools/container-libs

giuseppe · 2026-06-05T15:20:38Z

When reusing chunks from other layers, reflink the source file into a temporary directory under the staging dir. The reflinked copy shares data blocks (CoW, O(1)) but is a separate inode, so itsurvives concurrent deletion of the source layer.

If the filesystem does not support reflinks, chunk deduplication is skipped and all chunks are fetched from the network.

Follow-up for #890

mtrmac

I think this would work individually and cleanly resolve the race, but I don’t think it scales: a layer can be thousands or millions of files (i.e. possibly even an order of magnitude more roll-sum chunks), and the process has a file descriptor limit.

(I didn’t review the details of the implementation.)

mtrmac · 2026-06-05T15:27:08Z

 	srcDirfd, err := unix.Open(source, unix.O_RDONLY|unix.O_CLOEXEC, 0)
 	if err != nil {
+		// The source layer may have been deleted concurrently.
+		if errors.Is(err, unix.ENOENT) || errors.Is(err, unix.ENOTDIR) {


Why ENOTDIR?

wrong assumption that it could happen when a parent directory is deleted. That still maps to ENOENT.

mtrmac

This is a literally one-minute skim, but looks reasonable — and actually much smaller than I expected. Nice!

giuseppe · 2026-06-05T17:27:34Z

I've tested this manually and seems to work fine

cgwalters · 2026-06-05T19:57:33Z

Code changes offhand look sane to me.

One thing I don't understand - why can't we lock the source layer?

Side note: this is very different in a composefs-rs, because we don't have unpacked files per layer at all, just an object store.

giuseppe · 2026-06-06T09:36:25Z

One thing I don't understand - why can't we lock the source layer?

the network code calls into containers/image that in turn can call back into c/storage so we might end up with some deadlocks.

On top of that, the network calls could be slow and they would block any write access to the store in the meanwhile (possibly this could be abused)

mtrmac · 2026-06-09T19:25:00Z


 	wg.Wait()

+	chunkRefsDir := filepath.Join(filepath.Dir(dest), "chunk-refs")


This would really benefit from a comment documenting why we bother with all of this. (Yes, it is now documented in the commit message, but finding that is more work.)

mtrmac · 2026-06-09T19:25:19Z

+		defer os.RemoveAll(chunkRefsDir)
+	}
+	type reflinkKey struct{ root, path string }
+	reflinkMap := make(map[reflinkKey]string)


(Documenting the key/value semantics would be nice.)

mtrmac · 2026-06-09T19:47:06Z


 	wg.Wait()

+	chunkRefsDir := filepath.Join(filepath.Dir(dest), "chunk-refs")


[1] … Given the signature of the function, a caller would not expect anything to happen outside of dest.

driver.Differ is explicitly an unstable interface, so let’s make the design explicit, please.

Because overlay already hard-codes the structure of the staging directory (overlay.Driver.ApplyDiffWithDiffer sets out.Target, and later overlay.Driver.CleanupStagingDirectory uses filepath.Dir(stagingDirectory), chunkedDiffer.ApplyDiff can’t just be given the top-level staging directory to create the Target wherever it wants.

So, ApplyDiffWithDiffer could explicitly provide a “destination directory” and an “empty scratch directory” siblings to chunkedDiffer.ApplyDiff (OTOH conceptually ApplyDiffWithDiffer does not know a scratch will always be necessary); or chunkedDiffer.ApplyDiff could get a “destination directory” and “staging area” and with the obligation to use MkdirTemp to allocate the reflink directory within the staging area.

mtrmac · 2026-06-09T19:48:31Z

+						if isReflinkNotSupported(err) {
+							reflinkSupported = false
+						}
+						break


Should this explicitly check for ENOENT and fail on unexpected errors? I’m not really sure it’s necessary, when we have the option to get the data from the network.

At least a comment highlighting that ENOENT can legitimately happen here might be useful.

Add a Reflink() function that attempts a CoW file clone without falling back to io.Copy. Callers that need to know whether the filesystem supports reflinks can use this instead of ReflinkOrCopy. ReflinkOrCopy is refactored to call Reflink internally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2026-06-10T05:25:59Z

@mtrmac thanks, I've addressed the comments

Honny1

LGTM, just one non-blocking comment.

Honny1 · 2026-06-10T08:27:54Z

+	// entirely and fetch everything from the network.
+	var chunkRefsDir string
+	if differOpts != nil && differOpts.StagingDirectory != "" {
+		d, err := os.MkdirTemp(differOpts.StagingDirectory, "chunk-refs-")


Non-blocking: I would log an error at least as debug.

thanks.

Thinking more about it, I think it should be a hard error. If we fail to create a temporary directory, we need to report that

When reusing chunks from other layers, reflink the source file into a temporary directory under the staging dir. The reflinked copy shares data blocks (CoW, O(1)) but is a separate inode, so it survives concurrent deletion of the source layer. If the filesystem does not support reflinks, chunk deduplication is skipped and all chunks are fetched from the network. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

github-actions Bot added the storage Related to "storage" package label Jun 5, 2026

giuseppe mentioned this pull request Jun 5, 2026

Skip reuse of removed layers in copyFileFromOtherLayer #890

Merged

mtrmac requested changes Jun 5, 2026

View reviewed changes

giuseppe force-pushed the chunked-preopen-files branch from 89cac1b to a09e9f6 Compare June 5, 2026 16:52

giuseppe marked this pull request as ready for review June 5, 2026 17:24

giuseppe changed the title ~~[WIP] chunked: handle concurrent layer deletion during dedup~~ chunked: follow-up for https://github.com/podman-container-tools/container-libs/pull/890 Jun 5, 2026

mtrmac reviewed Jun 5, 2026

View reviewed changes

Comment thread storage/pkg/chunked/storage_linux.go Outdated

giuseppe changed the title ~~chunked: follow-up for https://github.com/podman-container-tools/container-libs/pull/890~~ chunked: use reflinks for chunk deduplication Jun 5, 2026

mtrmac reviewed Jun 9, 2026

View reviewed changes

giuseppe force-pushed the chunked-preopen-files branch 2 times, most recently from 48e0860 to 23867e1 Compare June 10, 2026 05:24

Honny1 reviewed Jun 10, 2026

View reviewed changes

giuseppe force-pushed the chunked-preopen-files branch from 23867e1 to 838d9bf Compare June 10, 2026 08:44


		wg.Wait()

		chunkRefsDir := filepath.Join(filepath.Dir(dest), "chunk-refs")

Conversation

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

Uh oh!

Uh oh!

mtrmac 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!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giuseppe commented Jun 5, 2026

Uh oh!

cgwalters commented Jun 5, 2026

Uh oh!

giuseppe commented Jun 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Jun 10, 2026

Uh oh!

Honny1 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giuseppe commented Jun 5, 2026 •

edited

Loading