Introduce MountedFast function to determine mount points faster. by manugupt1 · Pull Request #97 · moby/sys

manugupt1 · 2021-12-08T18:29:52Z

MountedFast is a method of detecting mount points really fast.
It works for all kinds of mounts (incl. bind mounts) but requires
a recent (v5.6+) Linux kernel. A mount-point check is guaranteed to
be a mount-point or not only when sure is true. When sure is false,
the caller needs to check for other methods (eg: parse /proc/mounts)
to successfully determine if it is a mount-point.

This has been motivated by
kubernetes/kubernetes#105833 (comment)
and
#96 (comment)

Context

The context is that; k8s relies on reading /proc/mounts to determine mount points. It also enforces a consistency check (ie ensure that two reads of proc/mounts are exactly the same) before determining a mount-point which leads to resource contention. This is different than docker where a lock is acquired before reading the entire /proc/mounts.

Now, k8s wont remove the consistency check as it could lead to all sorts of error that Kir demonstrated here (https://github.com/kolyshkin/procfs-test) and the consistency check mitigates that.

To solve for that k8s took inspiration from moby/sys to do its own openat2 call (kubernetes/kubernetes#105833) and when openat2 is not available, fallback on reading /proc/mounts with the consistency check I mentioned above.

During the course of the discussion, Kir suggested it might be better to upstream the change to moby/sys and k8s can take it as a dependency which is what this PR does. It introduces a MountedFast which uses openat2 (for kernel 5.6+) or stat calls to determine a mount-point, it sends back a sure bool to the caller to guarantee the caller if it is a mount-point or not.
k8s will now first call MountedFast when this change is merged and then if sure is false or an error is thrown, it will fall back to reading /proc/mounts. This is beneficial for k8s on newer kernels as they will never read /proc/mounts because of openat2 path and for older kernels only bind mounts will be validated using /proc/mounts.

manugupt1 · 2021-12-08T18:34:01Z

cc @kolyshkin please have a look

kzys · 2021-12-08T23:49:37Z

+// sure is true. When sure is false, the caller needs to check for other '
+// methods (eg: parse /proc/mounts) to successfully determine if it is a
+// mount-point.
+func mountedByOpenat2(path string) (mounted, sure bool, err error) {


How about returning an exported error instead of the sure flag?

Apparently in all error cases, sure=false. So returning an error wouldn't override the original errors.

Generally speaking Go code checks err first and doesn't consume other returning values if err != nil. I want to follow the convention here.

@kolyshkin Do you have thoughts as you suggested this? I think it boils down to preference.

I agree with @kzys here -- no need to change mountedByOpenat2 signature.

Basically, you need to take mounted from mountinfo/mounted_linux.go, add sure bool to it, and remove the fallback to mountinfo parsing. That's all what's needed.

kolyshkin · 2021-12-09T09:25:40Z

Also, please cherry-pick 03cca99 since you're adding more implementations to test, and this becomes ugly -- and this commit is a nice solution to that.

Instead of copy/pasting the code to check individual mountedBy* implementations, add a list and a loop to check them. The only special thing is, we have to check if the name of the function being tested is "mountedByStat", to skip erroring out on bind mounts. Otherwise the code is the same. Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Manu Gupta <[email protected]>

jingxu97 · 2021-12-11T00:12:36Z

+// mount-point or not only when sure is true. When sure is false, the
+// caller needs to check for other methods (eg: parse /proc/mounts)
+// to successfully determine if it is a mount-point.
+func MountedFast(path string) (mounted, sure bool, err error) {


I think in case of "not sure", we can return error indicating cannot figure out whether it is mounted or not.
in this way, we only need return two parameters "bool, error" as before.

jingxu97 · 2021-12-14T20:40:18Z

/assign @jsafrane
/cc @gnufied

overall looks good to me. @jsafrane could you also take a look?

manugupt1 · 2021-12-15T18:33:03Z

Hi @kolyshkin, would you be able to have a look at it? I would like to get it reviewed and address comments if you have any. I have added things that I have found out; but a final review would be great.

jsafrane · 2021-12-16T10:09:11Z

The code looks good to me.

On personal note, running unit tests as root and depending on kernel version is IMHO bad, as different people (and CI) will test different things. It seems moby is fine with that.

kolyshkin · 2021-12-21T23:43:46Z

+				if sure == true {
+					t.Errorf("MountFast: expected sure to be false, got %v", sure)
+				}
+				if mounted != false {


The code would be easier to read if you use the same comparison (either == false or != true) in both cases.

kolyshkin · 2021-12-21T23:44:38Z

+						if sure == true {
+							t.Errorf("MountFast: expected sure to be false, got %v", sure)
+						}
+						if mounted != false {


kolyshkin · 2021-12-21T23:48:03Z

+// mountinfo from procfs. A mount-point check is guaranteed to be a
+// mount-point or not only when sure is true. When sure is false, the


nit: s/mount-point/mount point/. Here "mount point" is a common term, see e.g. man 8 mount.

kolyshkin · 2021-12-21T23:48:18Z

+// mountinfo from procfs. A mount-point check is guaranteed to be a
+// mount-point or not only when sure is true. When sure is false, the
+// caller needs to check for other methods (eg: parse /proc/mounts)
+// to successfully determine if it is a mount-point.


Also, probably makes sense to say this function is only available on Linux.

kolyshkin · 2021-12-21T23:56:38Z

I think we're almost good to go.

Please

merge the last two commits
change "mount-point" to "mount point" everywhere.

MountedFast is a method of detecting mount points ie without reading /proc/mounts. It works for all kinds of mounts (incl. bind mounts) but requires a recent (v5.6+) Linux kernel. A mount-point check is guaranteed to be a mount-point or not only when sure is true. When sure is false, the caller needs to check for other methods (eg: parse /proc/mounts) to successfully determine if it is a mount-point. Signed-off-by: Manu Gupta <[email protected]>

manugupt1 · 2022-01-06T01:24:32Z

@kolyshkin Would you be able to have a final go and check it please?

jingxu97 · 2022-01-13T17:31:07Z

@kolyshkin could you help approve this? Thanks!

kolyshkin · 2022-01-14T23:31:56Z

I started to re-review this, and ended up fixing various nits myself. Please see #100 🙏🏻

thaJeztah · 2022-02-03T10:12:50Z

This was carried in #100 and now merged, so closing this one

thanks for contributing and reviewing, @manugupt1 !

manugupt1 force-pushed the fast-mount branch from 38dd36b to 020afe0 Compare December 8, 2021 18:30

manugupt1 mentioned this pull request Dec 8, 2021

Expose MountedByOpenat2 as a public function so that k8s can consume #96

Closed

kzys reviewed Dec 8, 2021

View reviewed changes

kzys reviewed Dec 9, 2021

View reviewed changes

Comment thread mountinfo/mounted_linux.go

Comment thread mountinfo/mounted_linux.go Outdated

manugupt1 force-pushed the fast-mount branch 2 times, most recently from e3c9d7c to 62d2d10 Compare December 9, 2021 18:20

manugupt1 requested a review from kolyshkin December 9, 2021 18:25

kzys reviewed Dec 9, 2021

View reviewed changes

Comment thread mountinfo/mounted_linux.go

manugupt1 force-pushed the fast-mount branch from 62d2d10 to 1354229 Compare December 9, 2021 23:01

kzys approved these changes Dec 9, 2021

View reviewed changes

jingxu97 reviewed Dec 11, 2021

View reviewed changes

manugupt1 force-pushed the fast-mount branch from 1354229 to 59772ab Compare December 13, 2021 15:58

manugupt1 force-pushed the fast-mount branch from 59772ab to 788a5c1 Compare December 15, 2021 18:25

manugupt1 mentioned this pull request Dec 15, 2021

Determine mount points without relying on /proc/mounts kubernetes/kubernetes#105833

Closed

mkimuram reviewed Dec 15, 2021

View reviewed changes

Comment thread mountinfo/mounted_linux_test.go Outdated

manugupt1 force-pushed the fast-mount branch 5 times, most recently from 74edffe to 5e7a91b Compare December 17, 2021 19:15

kolyshkin reviewed Dec 21, 2021

View reviewed changes

Comment thread mountinfo/mounted_linux_test.go Outdated

kolyshkin reviewed Dec 21, 2021

View reviewed changes

Comment thread mountinfo/mounted_linux.go Outdated

kolyshkin reviewed Dec 21, 2021

View reviewed changes

manugupt1 changed the title ~~Introduce MountedFast function to determine mount-points faster.~~ Introduce MountedFast function to determine mount points faster. Dec 22, 2021

manugupt1 force-pushed the fast-mount branch from 5e7a91b to fbde1ac Compare December 22, 2021 02:10

kolyshkin mentioned this pull request Jan 14, 2022

mountinfo: add MountedFast #100

Merged

thaJeztah closed this Feb 3, 2022

		// mountinfo from procfs. A mount-point check is guaranteed to be a
		// mount-point or not only when sure is true. When sure is false, the

Conversation

manugupt1 commented Dec 8, 2021 • edited by kolyshkin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Uh oh!

manugupt1 commented Dec 8, 2021

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!

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Dec 9, 2021

Uh oh!

Uh oh!

jingxu97 Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jingxu97 commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manugupt1 commented Dec 15, 2021

Uh oh!

Uh oh!

jsafrane commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Dec 21, 2021

Uh oh!

manugupt1 commented Jan 6, 2022

Uh oh!

jingxu97 commented Jan 13, 2022

Uh oh!

kolyshkin commented Jan 14, 2022

Uh oh!

thaJeztah commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

manugupt1 commented Dec 8, 2021 •

edited by kolyshkin

Loading

jingxu97 Dec 11, 2021 •

edited

Loading

jingxu97 commented Dec 14, 2021 •

edited

Loading

jsafrane commented Dec 16, 2021 •

edited

Loading