support more overlayfs layers #2502

fuweid · 2018-07-26T07:42:02Z

Auto-detect longest common dir in lowerdir option and compact it if the
option size is hitting one page size. If does, Use chdir + CLONE to do
mount thing to avoid hitting one page argument buffer in linux kernel
mount.

More information is here #2497

codecov-io · 2018-07-26T10:08:42Z

Codecov Report

Merging #2502 into master will decrease coverage by 0.1%.
The diff coverage is 36.56%.

@@            Coverage Diff             @@
##           master    #2502      +/-   ##
==========================================
- Coverage   45.05%   44.95%   -0.11%     
==========================================
  Files          94       95       +1     
  Lines        9796     9928     +132     
==========================================
+ Hits         4414     4463      +49     
- Misses       4662     4743      +81     
- Partials      720      722       +2

Flag	Coverage Δ
#linux	`48.88% <36.56%> (-0.2%)`	⬇️
#windows	`41.57% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
sys/mount_linux.go	`0% <0%> (ø)`
mount/mount_linux.go	`31.01% <60.49%> (+31.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fb9230...67b54c6. Read the comment docs.

AkihiroSuda

Can we avoid circular dependency on docker/docker?

fuweid · 2018-07-26T11:54:32Z

I see.... the docker/docker/builder using the containerd/containerd/mount.... 🤔

stevvooe · 2018-07-26T22:47:45Z

cmd/containerd-shim/main_unix.go

@@ -41,6 +41,7 @@ import (
 	shimapi "github.com/containerd/containerd/runtime/v1/shim/v1"
 	"github.com/containerd/ttrpc"
 	"github.com/containerd/typeurl"
+	"github.com/docker/docker/pkg/reexec"


Using this as a dependency is a big no no. Either reimplement or find another solution.

DRY is a liability here.

Yes @stevvooe. I will find other way to handle this. 😄

Was looking around for a suggestion, but I think @dmcgowan covered the options in #2497 (comment).

Found a way to create a Fmountat which we could use here dmcgowan@d8f6d09

@stevvooe yes. the option provided by @dmcgowan works! I have changed the code. Please take a look.

dmcgowan · 2018-07-27T23:22:09Z

mount/mount_linux.go

+		newdirs = append(newdirs, dir[len(commondir):])
+	}
+
+	m.Options = append(m.Options[:idx], m.Options[idx+1:]...)


Make a copy of m before altering

dmcgowan · 2018-07-27T23:25:59Z

mount/mount_linux.go

+
+	// NOTE: the snapshot id is based on digits. in order to avoid to get
+	// snapshots/x, need to back to parent snapshots dir. however, there is
+	// assumption that the common dir is ${root}/io.containerd.v1.overlayfs.


should be ${root}/io.containerd.v1.overlayfs/snapshots

AkihiroSuda · 2018-07-30T13:43:49Z

sys/subprocess_unsafe_linux.go

+	_ "unsafe" // required for go:linkname.
+)
+
+//go:linkname beforeFork syscall.runtime_BeforeFork


Is this officially supported across different versions of Go?

# https://github.com/golang/go/blob/release-branch.go1.8/src/syscall/exec_linux.go#L46 func runtime_BeforeFork() func runtime_AfterFork()

# https://github.com/golang/go/blob/release-branch.go1.9/src/syscall/exec_linux.go#L52 func runtime_BeforeFork() func runtime_AfterFork() func runtime_AfterForkInChild()

1.9 or higher supports the three features. But, I think go1.10 can work well than go1.9 since the runtime.LockOSThread has been improved.

more input from https://golang.org/doc/go1.10#runtime

dmcgowan · 2018-07-30T18:25:11Z

mount/mount_linux.go

+	)
+
+	// avoid hitting one page limit of mount argument buffer
+	if m.Type == "overlay" {


There should still be a page size check in here, we need to offload that to a linux only section though

dmcgowan · 2018-07-30T18:25:51Z

mount/mount_linux.go

+
+	// avoid hitting one page limit of mount argument buffer
+	if m.Type == "overlay" {
+		options = copyOptions(options)


This copy should be done as part of the next function, and only when necessary

dmcgowan · 2018-07-30T18:42:23Z

sys/mount_linux.go

+	}
+
+	// read exit status from child thread.
+	n, err = readlen(p[0], (*byte)(unsafe.Pointer(&errno)), int(unsafe.Sizeof(errno)))


Is there no other way to do this without a raw syscall? That should only be necessary in the child process

crosbymichael · 2018-07-30T18:43:03Z

sys/mount_linux.go

+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
+	if _, errno = forkAndMountat(dirfd,


I think you are still missing a waitpid or else we will have zombie babies

crosbymichael · 2018-07-30T19:07:16Z

sys/mount_linux.go

+
+	// open pipe to receive exit status from other thread
+	p[0], p[1] = -1, -1
+	if err = syscall.Pipe(p[:]); err != nil {


I actually don't think you need pipes where, just waitpid(wait4) the exit status and use that. You can always wait on a process, running or not, that you forked off.

I make it complicated and it can cause the zombie process. I have updated the code. Please take a look.

crosbymichael · 2018-07-31T16:15:26Z

sys/mount_linux.go

+		return errors.Wrapf(err, "failed to find pid=%d process", pid)
+	}
+
+	if ps, err = p.Wait(); err != nil {


i would almost recommend using unix.Wait4 here to follow the rest of the theme of low level syscalls in this function

make senses. I will update it.

thanks for all your help

should we add loop to retry if the Wait4 return EINTR? it is possible?

ya, good idea

dmcgowan · 2018-07-31T18:13:09Z

mount/mount_linux.go

+
+// optionsSize returns the byte size of options of mount.
+func optionsSize(opts []string) int {
+	size := 0


We should still set some buffer room here, maybe start size at 512

the buffer will be used in pagesize check. have updated it.

crosbymichael · 2018-08-01T14:38:14Z

mount code LGTM

fuweid · 2018-08-03T15:00:46Z

ping @dmcgowan @stevvooe @AkihiroSuda

please take a look~ thanks

kolyshkin · 2018-08-06T19:20:52Z

mount/mount_linux.go

+	//
+	// NOTE: 512 is a buffer during pagesize check.
+	if m.Type == "overlay" && optionsSize(options) >= pagesize-512 {
+		chdir, options = compactLowerdirOption(options)


Looks like there should be another check here that after compacting the options do fit a page, i.e. something like

if optionsSize(options) >= pagesize-512 { return errors.Errorf("can't mount: lower layers list too long") }

Yet better, move this check to after parseMountOptions() as it's simpler and more precise in there:

flags, data := parseMountOptions(options) if len(data) >= pagesize { return errors.Errorf("can't mount: mount options string too long") } ``` or smth like that

make senses. will update it.

@kolyshkin I add another check after compact and show the error if the mount option is too long.
if we got empty chdir or the data is too long, return the mount options is too long. It is readable to user instead of failed to mount: no such file or directory.

Auto-detect longest common dir in lowerdir option and compact it if the option size is hitting one page size. If does, Use chdir + CLONE to do mount thing to avoid hitting one page argument buffer in linux kernel mount. Signed-off-by: Wei Fu <[email protected]>

dmcgowan · 2018-08-07T18:39:59Z

LGTM

crosbymichael · 2018-08-09T14:31:01Z

LGTM

fuweid changed the title ~~support more overlayfs layers~~ [WIP] support more overlayfs layers Jul 26, 2018

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch 2 times, most recently from 6b8f114 to 47d8a50 Compare July 26, 2018 09:52

fuweid changed the title ~~[WIP] support more overlayfs layers~~ support more overlayfs layers Jul 26, 2018

fuweid mentioned this pull request Jul 26, 2018

[bug & proposal] overlayfs support more-layers-image #2497

Closed

AkihiroSuda reviewed Jul 26, 2018

View reviewed changes

stevvooe requested changes Jul 26, 2018

View reviewed changes

fuweid changed the title ~~support more overlayfs layers~~ [WIP] support more overlayfs layers Jul 27, 2018

dmcgowan reviewed Jul 27, 2018

View reviewed changes

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch 7 times, most recently from fd89efb to b41352b Compare July 30, 2018 13:27

fuweid changed the title ~~[WIP] support more overlayfs layers~~ support more overlayfs layers Jul 30, 2018

AkihiroSuda reviewed Jul 30, 2018

View reviewed changes

dmcgowan reviewed Jul 30, 2018

View reviewed changes

crosbymichael reviewed Jul 30, 2018

View reviewed changes

dmcgowan added this to the 1.2 milestone Jul 30, 2018

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch 3 times, most recently from 1ca0406 to 7a57b5c Compare July 31, 2018 15:17

crosbymichael reviewed Jul 31, 2018

View reviewed changes

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch from 7a57b5c to b9d7839 Compare July 31, 2018 18:00

dmcgowan reviewed Jul 31, 2018

View reviewed changes

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch from b9d7839 to 3a3c261 Compare August 1, 2018 00:45

kolyshkin reviewed Aug 6, 2018

View reviewed changes

fuweid force-pushed the bugfix_support_more_overlayfs_layers branch from 3a3c261 to 67b54c6 Compare August 7, 2018 03:00

crosbymichael merged commit 9a4c61c into containerd:master Aug 9, 2018

dmcgowan mentioned this pull request Aug 28, 2018

mount: support mounting from relative paths for deep layered images #2153

Closed

estesp added the cherry-pick/1.1.x label Aug 31, 2018

fuweid deleted the bugfix_support_more_overlayfs_layers branch October 23, 2018 02:04

tonistiigi mentioned this pull request Jul 24, 2019

"No such file or directory" error when running lots of copy operations sequentially moby/buildkit#407

Closed

fuweid mentioned this pull request Apr 6, 2020

Creating task failed with "failed to find pid" #4009

Closed

thaJeztah mentioned this pull request Jun 23, 2021

move sys.FMountat() into mount package and un-export #5641

Merged

yuqitao mentioned this pull request May 24, 2022

synchronous: supprt more overlayfs layers containerd/rust-extensions#80

Merged

fuweid mentioned this pull request Apr 9, 2023

RFC: Initial support of idmapped mount points #5890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support more overlayfs layers #2502

support more overlayfs layers #2502

fuweid commented Jul 26, 2018 •

edited

Loading

codecov-io commented Jul 26, 2018 •

edited

Loading

AkihiroSuda left a comment

fuweid commented Jul 26, 2018 •

edited

Loading

stevvooe Jul 26, 2018

fuweid Jul 27, 2018

stevvooe Jul 27, 2018

dmcgowan Jul 27, 2018 •

edited

Loading

fuweid Jul 30, 2018

dmcgowan Jul 27, 2018

dmcgowan Jul 27, 2018

AkihiroSuda Jul 30, 2018

fuweid Jul 30, 2018

dmcgowan Jul 30, 2018

dmcgowan Jul 30, 2018

dmcgowan Jul 30, 2018

crosbymichael Jul 30, 2018

crosbymichael Jul 30, 2018

fuweid Jul 31, 2018

crosbymichael Jul 31, 2018

fuweid Jul 31, 2018

crosbymichael Jul 31, 2018

fuweid Jul 31, 2018 •

edited

Loading

crosbymichael Jul 31, 2018

dmcgowan Jul 31, 2018

fuweid Aug 1, 2018

crosbymichael commented Aug 1, 2018

fuweid commented Aug 3, 2018

kolyshkin Aug 6, 2018

kolyshkin Aug 6, 2018

fuweid Aug 7, 2018

fuweid Aug 7, 2018

dmcgowan commented Aug 7, 2018

crosbymichael commented Aug 9, 2018

support more overlayfs layers #2502

support more overlayfs layers #2502

Conversation

fuweid commented Jul 26, 2018 • edited Loading

codecov-io commented Jul 26, 2018 • edited Loading

Codecov Report

AkihiroSuda left a comment

Choose a reason for hiding this comment

fuweid commented Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Aug 1, 2018

fuweid commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Aug 7, 2018

crosbymichael commented Aug 9, 2018

fuweid commented Jul 26, 2018 •

edited

Loading

codecov-io commented Jul 26, 2018 •

edited

Loading

fuweid commented Jul 26, 2018 •

edited

Loading

dmcgowan Jul 27, 2018 •

edited

Loading

fuweid Jul 31, 2018 •

edited

Loading