Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mountinfo.Mounted: optimize by adding fast paths #20

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Jun 29, 2020

Add two fast paths to avoid (slow) mountinfo parsing:

  1. In case path does not exist, it is definitely NOT a mount point.

  2. In case path's st.Dev differs from that of its parent, it is
    definitely a mount point.

In all the other cases (including bind mounts, case of path="/", other
errors from unix.Lstat) the code falls back to the old slow method
of parsing mountinfo.

@kolyshkin kolyshkin mentioned this pull request Jun 29, 2020
mountinfo/mountinfo.go Outdated Show resolved Hide resolved
mountinfo/mountinfo.go Outdated Show resolved Hide resolved
@kolyshkin kolyshkin marked this pull request as draft June 29, 2020 23:54
@kolyshkin
Copy link
Collaborator Author

So, the method is not working for bind mounts, so I changed the commit to try comparing devices first, then falling back to mountinfo parsing. This way, if there is a proper (not bind) mount, we can skip parsing mountinfo.

@kolyshkin kolyshkin marked this pull request as ready for review June 30, 2020 03:12
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts/comments

mountinfo/mountinfo.go Show resolved Hide resolved
mountinfo/mountinfo_unix.go Outdated Show resolved Hide resolved
mountinfo/mountinfo_unix.go Outdated Show resolved Hide resolved
mountinfo/mountinfo_unix.go Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the mounted branch 2 times, most recently from 12490ee to fce2c1a Compare June 30, 2020 18:06
@kolyshkin kolyshkin changed the title mountinfo.Mounted: reimplement w/o parsing mountinfo.Mounted: optimize by adding fast paths Jun 30, 2020
@kolyshkin
Copy link
Collaborator Author

kolyshkin commented Jun 30, 2020

I have reworked it considerably, PTAL @tianon @thaJeztah @cpuguy83

@kolyshkin kolyshkin force-pushed the mounted branch 2 times, most recently from 2ebd59a to 73fb47e Compare June 30, 2020 23:17
@thaJeztah
Copy link
Member

Looks like there's still a duplicate declaration for mounted() on darwin;

file darwin freebsd linux windows
mountinfo_unix -
mountinfo_unsupported - - -
mountinfo_windows - - -
conflict? ⚠️

Add two fast paths to avoid (slow) mountinfo parsing:

1. In case path does not exist, it is definitely NOT a mount point.

2. In case path's st.Dev differs from that of its parent, it is
   definitely a mount point.

In all the other cases (including bind mounts, case of path="/", other
errors from unix.Lstat) the code falls back to the old slow method
of parsing mountinfo.

Signed-off-by: Kir Kolyshkin <[email protected]>
@thaJeztah
Copy link
Member

Cross is failing;

for os in linux freebsd darwin windows; do \
	for arch in amd64 arm arm64 ppc64le s390x; do \
		echo "$os/$arch" | grep -qE '^((freebsd|darwin|windows)/(ppc64le|s390x)|windows/arm64|freebsd/s390x)$' && continue; \
		echo "# building for $os/$arch"; \
		GOOS=$os GOARCH=$arch go build ./...; \
	done; \
done
# building for linux/amd64
##[error]mount/mount.go:9:2: cannot find package "github.com/moby/sys/mountinfo" in any of:
	/opt/hostedtoolcache/go/1.13.12/x64/src/github.com/moby/sys/mountinfo (from $GOROOT)
	/home/runner/go/src/github.com/moby/sys/mountinfo (from $GOPATH)
##[error]mount/flags_linux.go:4:2: cannot find package "golang.org/x/sys/unix" in any of:
	/opt/hostedtoolcache/go/1.13.12/x64/src/golang.org/x/sys/unix (from $GOROOT)
	/home/runner/go/src/golang.org/x/sys/unix (from $GOPATH)
##[error]Makefile:33: recipe for target 'cross' failed
make: *** [cross] Error 1
##[error]Process completed with exit code 2.

Does it need the same trickery as we use for test?

sys/Makefile

Lines 10 to 12 in fb9f8cf

for p in $(PACKAGES); do \
(cd $$p && go test -v .); \
done

Starting to wonder if it would be cleaner to have separate Makefiles in each module, and then call those targets from the main Makefile (also beginning to feel the pain of the submodules 😓 - do you think it causes more issues to have them in the same repository than to have to maintain two repositories?)

@kolyshkin
Copy link
Collaborator Author

Will fix

Starting to wonder if it would be cleaner to have separate Makefiles in each module

It will result in more code (same/similar for in the top makefile, plus go test in every sub-makefile).

@kolyshkin
Copy link
Collaborator Author

kolyshkin commented Jul 1, 2020

OK we need replace in go.mod for such tests, and once replace is there, we can ditch the for p in $(PACKAGES) loop in the top Makefile.

@kolyshkin kolyshkin force-pushed the mounted branch 4 times, most recently from 2cb4baa to 3654e7c Compare July 1, 2020 22:51
Makefile Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Collaborator Author

Separated out cross build to #23

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah
Copy link
Member

@cpuguy83 PTAL

@thaJeztah
Copy link
Member

oh, and @tianon 🤗

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄

switch err {
case unix.ENOENT:
// Nonexistent path, so not a mount point.
return false, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some concern that a "broken" FUSE mount might hit this case, but it returns ENOTCONN. 👍

(Just including a note in case anyone else reading this has the same question/concern. 😅)

@kolyshkin
Copy link
Collaborator Author

@cpuguy83 PTAL

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83 cpuguy83 merged commit 95fd265 into moby:master Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants