fs: add DiffDirChanges function to get changeset fast by fuweid · Pull Request #145 · containerd/continuity

fuweid · 2019-11-23T15:08:56Z

Since AUFS/OverlayFS can persist changeset in diff directory,
DiffDirChanges function can retrieve layer changeset from diff directory
without walking the whole rootfs directory.

Signed-off-by: Wei Fu [email protected]

fuweid · 2019-11-23T15:20:33Z

The original issue is from moby/buildkit#1192. The docker uses graphdriver to retrieve the layer diff based on AUFS/OverlayFS and it is fast than containerd Compare API which walks the whole rootfs directory.

The github.com/containerd/continuity/fs package provides the Changes function, and it is hard to get relationship between two rootfs paths without original mount information, since the overlayfs mount info might be changed by Chdir if there are too many lowerdirs. Therefore, add new function named by DiffDirChanges and containerd Compare API can choose the efficient way to retrieve diff layer.

@dmcgowan @tonistiigi @AkihiroSuda

ktock

@fuweid Are you still working on this?
Fixing slow diff has been gathering demand in the community (moby/buildkit#1704, moby/buildkit#1192).
Can we move this forward?

ktock · 2021-06-14T02:35:03Z

fs/diff.go

+	case DiffSourceAUFS:
+		o = &diffDirOptions{
+			skipChange: skipAUFSMetadata,
+		}


AUFS snapshotter is deprecated since containerd v1.5. Could we focus on overlayfs in this PR?

fuweid · 2021-06-15T02:50:42Z

@fuweid Are you still working on this?

Fixing slow diff has been gathering demand in the community (moby/buildkit#1704, moby/buildkit#1192).

Can we move this forward?

yeah. will update it in days～ Thanks！

pankajkumar229 · 2024-01-23T16:26:28Z

Why was this never merged?

AkihiroSuda · 2024-01-23T16:49:38Z

I think this can be just merged after rebasing and removing AUFS codes

fuweid · 2024-01-24T01:13:01Z

oops. Will update it later. Thanks for remider

AkihiroSuda · 2024-01-24T13:56:31Z

fs/fstest/file_windows.go

+// CreateDeviceFile provides creates devices Applier.
+func CreateDeviceFile(name string, mode os.FileMode, maj, min int) Applier {
+	return applyFn(func(root string) error {
+		return errors.New("Not implemented")


Nit: should we use errdefs.ErrNotImplemented?

Good catch. Let me handle it in the follow-up. (Use containerd/errdefs repo to handle this).

AkihiroSuda · 2024-01-24T13:56:55Z

fs/diff_test.go

 }

+func testDiffDirChange(base, diff fstest.Applier, source DiffSource, expected []TestChange) error {
+	baseTmp, err := ioutil.TempDir("", "fast-diff-base-")


ioutil is deprecated

AkihiroSuda · 2024-01-24T13:57:30Z

fs/diff_test.go

+func (tcs TestChanges) Len() int           { return len(tcs) }
+func (tcs TestChanges) Less(i, j int) bool { return tcs[i].Path < tcs[j].Path }
+func (tcs TestChanges) Swap(i, j int)      { tcs[i], tcs[j] = tcs[j], tcs[i] }
+


Can we just use sort.Slice?

AkihiroSuda · 2024-01-24T13:59:45Z

fs/diff.go

+
+	if f.IsDir() {
+		originalPath := filepath.Join(diffDir, path)
+		opaque, err := sysx.LGetxattr(originalPath, "trusted.overlay.opaque")


user.overlay.* has to be checked too
torvalds/linux@2d2f2d7

Updated. Please take a look.

AkihiroSuda · 2024-01-24T14:00:16Z

fs/diff.go

+		originalPath := filepath.Join(diffDir, path)
+		opaque, err := sysx.LGetxattr(originalPath, "trusted.overlay.opaque")
+		if err != nil {
+			if err == unix.ENODATA {


errors.Is is more preferable over directly using the = operator

pankajkumar229 · 2024-01-24T16:09:53Z

Many Thanks for the quick turnaround.

ktock · 2024-01-24T16:50:30Z

fs/diff.go

+		deletedFile := false
+
+		if o.deleteChange != nil {
+			deletedFile, err = o.deleteChange(diffDir, path, f, changeFn)


nilt: We don't need to mark this file as "deleted" if this file doesn't exist in the base dir.

good catch. Let me think how to handle this

ktock · 2024-01-24T16:59:47Z

fs/diff_linux.go

+		// FIXME(fuweid): In spite of that .wh..wh..opq might not be
+		// existing, mark it as faked delete change so that changeFn
+		// will add whiteout prefix for this faked file. It requires
+		// that changeFn callback must not read the os.FileInfo
+		// parameter.


So the convention introduced here is that if 3rd arg is nil then changeFn must make the dir opaque, right? Could we add this explanation to godoc of DiffDirChanges?

Good catch. Sorry for late reply. Let me think about how to remove FIXME

If we don't want to introduce a new convention, another approach might be starting a new doubleWalkDiffer on the opaque directory to make changeFn opaque-agnostic.

Updated. Please take a look.

For deleted file, 3rd one is nil. FYI

pankajkumar229 · 2024-01-25T08:37:22Z

Is it possible to get this change to a previous version of containerd when it gets released?

AkihiroSuda · 2024-01-25T08:38:49Z

Is it possible to get this change to a previous version of containerd when it gets released?

Unlikely, due to potential risk of regression.

pankajkumar229 · 2024-01-25T08:47:52Z

Is it possible to merge this soon? It will help us compile our own containerd. OTherwise I think we will need some code changes in containerd to use this branch or another fork.

Thanks again for the quick turnaround.

fs/diff.go

pankajkumar229 · 2024-01-27T10:31:59Z

We still see the slowness in commit. It is possibly because "nerdctl commit" is calling the function Compare inside containerd and it seems to create a two temporary mounts: upperRoot and lowerRoot. Both seem to have a full copy of the file system. Now when compare calls writeDiff, even if continuity can check for overlayfs, it is still going through a lot of files and taking forever to do "nerdctl commit". What would be a good way to fix this?

Although this belongs to the containerd repository , I thought I would ask here since I saw Mr. Kohei Tokunaga's name in the relevant code and it is possibly better discussed in one place.

Since AUFS/OverlayFS can persist changeset in diff directory, DiffDirChanges function can retrieve layer changeset from diff directory without walking the whole rootfs directory. Signed-off-by: Wei Fu <[email protected]>

Unlike the walking differ, which implements a generic method to accommodate all kinds of snapshotters, the EROFS differ is just implemented for EROFS and EROFS snapshotter so it can utilize the recent DiffDirChanges() [1] to avoid traversing the entire rootfs directory in order to improve `nerdctl commit` performance. Additionally, I think `baseDir` is unnecessary too (in principle, only `upperdir` is useful for OCI format convention). However, addressing this requires more work, so left as is for now. It's also useful to implement a customized Compare() method for EROFS differ so that we can dump the native EROFS-formatted blob to the content store later. [1] containerd/continuity#145 Signed-off-by: Gao Xiang <[email protected]>

fuweid changed the title ~~fs: add DiffDirChanges function to get changeset fast~~ [RFC] fs: add DiffDirChanges function to get changeset fast Nov 23, 2019

fuweid force-pushed the me-support-diff-change-walk branch 2 times, most recently from ade3c4b to 3214e3b Compare November 30, 2019 09:12

fuweid mentioned this pull request Dec 2, 2019

buildkit import and push image is much slower than docker(enable buildkit) moby/buildkit#1192

Closed

fuweid changed the title ~~[RFC] fs: add DiffDirChanges function to get changeset fast~~ [WIP|RFC] fs: add DiffDirChanges function to get changeset fast Dec 12, 2019

tonistiigi mentioned this pull request Dec 28, 2020

use diff dir changes when using overlay moby/buildkit#1918

Closed

ktock reviewed Jun 14, 2021

View reviewed changes

dmcgowan marked this pull request as draft November 30, 2022 01:07

AkihiroSuda mentioned this pull request Jan 23, 2024

nerdctl commit taking long time possibly because of fs.Changes walking through all files #236

Open

fuweid force-pushed the me-support-diff-change-walk branch from 3214e3b to b9af6a9 Compare January 24, 2024 13:54

fuweid changed the title ~~[WIP|RFC] fs: add DiffDirChanges function to get changeset fast~~ fs: add DiffDirChanges function to get changeset fast Jan 24, 2024

fuweid marked this pull request as ready for review January 24, 2024 13:54

AkihiroSuda reviewed Jan 24, 2024

View reviewed changes

fuweid force-pushed the me-support-diff-change-walk branch from b9af6a9 to 349dea6 Compare January 24, 2024 15:01

AkihiroSuda approved these changes Jan 24, 2024

View reviewed changes

AkihiroSuda requested a review from ktock January 24, 2024 15:03

fuweid force-pushed the me-support-diff-change-walk branch 2 times, most recently from 7e2b2a4 to f700d27 Compare January 24, 2024 15:37

fuweid force-pushed the me-support-diff-change-walk branch 4 times, most recently from bcc5706 to 2545354 Compare January 24, 2024 16:06

fuweid force-pushed the me-support-diff-change-walk branch from 2545354 to 19f4218 Compare January 24, 2024 16:11

ktock reviewed Jan 24, 2024

View reviewed changes

pankajkumar229 reviewed Jan 26, 2024

View reviewed changes

fs/diff.go Show resolved Hide resolved

pankajkumar229 mentioned this pull request Jan 27, 2024

Only mounting lower path for getting a small commit containerd/containerd#9699

Closed

fuweid force-pushed the me-support-diff-change-walk branch 2 times, most recently from 6a68270 to d3a8156 Compare January 29, 2024 13:01

fs: add DiffDirChanges function to get changeset fast

8b312bd

Since AUFS/OverlayFS can persist changeset in diff directory, DiffDirChanges function can retrieve layer changeset from diff directory without walking the whole rootfs directory. Signed-off-by: Wei Fu <[email protected]>

fuweid force-pushed the me-support-diff-change-walk branch from d3a8156 to 8b312bd Compare January 29, 2024 13:33

fuweid requested a review from dmcgowan January 29, 2024 14:39

dmcgowan approved these changes Jan 31, 2024

View reviewed changes

ktock approved these changes Jan 31, 2024

View reviewed changes

fuweid merged commit 386dc78 into containerd:main Jan 31, 2024

fuweid deleted the me-support-diff-change-walk branch January 31, 2024 07:21

dmcgowan mentioned this pull request Jan 9, 2025

[release/1.7] Prepare release notes for v1.7.25 containerd/containerd#11243

Merged

hsiangkao mentioned this pull request Mar 25, 2025

erofs-differ: implement fast differ with DiffDirChanges() containerd/containerd#11603

Merged

Conversation

fuweid commented Nov 23, 2019

Uh oh!

fuweid commented Nov 23, 2019

Uh oh!

ktock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid commented Jun 15, 2021

Uh oh!

pankajkumar229 commented Jan 23, 2024

Uh oh!

AkihiroSuda commented Jan 23, 2024

Uh oh!

fuweid commented Jan 24, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pankajkumar229 commented Jan 24, 2024

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!

pankajkumar229 commented Jan 25, 2024

Uh oh!

AkihiroSuda commented Jan 25, 2024

Uh oh!

pankajkumar229 commented Jan 25, 2024

Uh oh!

Uh oh!

pankajkumar229 commented Jan 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants