mountinfo: add MountedFast by kolyshkin · Pull Request #100 · moby/sys

kolyshkin · 2022-01-14T23:28:15Z

This is a carry of #97 -- mostly the same code, arranged slightly differently. I have also cleaned up tests a bit.

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]>

manugupt1 · 2022-01-15T00:19:57Z

+	if sure {
+		return mounted, err
 	}


I slightly prefer having this check as (even when we have tests) as an extra check.

if sure && err == nil

Can this change be made?

Sure.

Also, I just realized that if normalizePath returns an error, it does not make sense to continue with parsing mountinfo, so this code needs to be changed anyway.

Will fix.

manugupt1 · 2022-01-15T00:20:30Z

One nit and this looks awesome! thanks!

kolyshkin · 2022-01-17T19:59:58Z

Should we maybe rename this to FastMounted()? @thaJeztah WDYT?

manugupt1 · 2022-01-17T20:16:11Z

Should we maybe rename this to FastMounted()? @thaJeztah WDYT?

I like MountedFast better because it reads as MountedByFastMethods to me; just like MountedByOpenAt2 and MountedByStat in the rest of the code base.

kolyshkin · 2022-01-20T01:24:53Z

@thaJeztah @cpuguy83 PTAL

thaJeztah · 2022-01-27T10:35:02Z

Should we maybe rename this to FastMounted()? @thaJeztah WDYT?

I like MountedFast better because it reads as MountedByFastMethods to me; just like MountedByOpenAt2 and MountedByStat in the rest of the code base.

I agree; let's stick with MountedXXX

thaJeztah

left some comments/suggestions

thaJeztah · 2022-01-27T10:23:31Z

+				}
+			} else {
+				if openat2Supported {
+					if mounted != exp {


This test is becoming quite complex; could we remove the local exp variable, and instead use tc.isMount ? (I had to scroll up to learn what exp was)

thaJeztah · 2022-01-27T10:27:55Z

+			// Check the public MountedFast() function as a whole.
+			mounted, sure, err := MountedFast(m)


Also see my other comment about this test becoming quite complex; wondering if we should just move this to a separate (TestMountedFast) test. There will be some boilerplating / code duplication, but perhaps it's a good trade-off compared to the complexity we're ending up into now. i.e. using subtests normally avoids having to include the function name in the t.Errorf, but because we're now combining so many we have to do it again.

I was thinking about it, too, but decided against it, since test setup/cleanup is complex and I'd rather not duplicate it.

We can move mountedFast checks into a function maybe; let me see.

Yes, this looks better. Updated.

kolyshkin · 2022-01-27T19:55:58Z

@manugupt1 @thaJeztah PTAL 🙏🏻

thaJeztah · 2022-01-31T16:15:06Z

looks good to me after @manugupt1's suggestion above has been looked at

Fixes: dbd468b Signed-off-by: Kir Kolyshkin <[email protected]>

MountedFast is a method of detecting a mount point without reading mountinfo from procfs. A caller can only trust the result if no error and sure == true are returned. Otherwise, other methods (e.g. parsing /proc/mounts) have to be used. If unsure, use Mounted instead (which uses MountedFast, but falls back to parsing mountinfo if needed). If a non-existent path is specified, an appropriate error (rather than "not mounted") is returned. In case the caller is not interested in this particular error, it should be handled separately using e.g. errors.Is(err, os.ErrNotExist). This function is only available on Linux. The implementation mostly relies on openat2(2) syscall, available since Linux kernel v5.6. On older kernels, this uses stat(2) syscall which can reliably detect normal (but not bind) mounts. Signed-off-by: Kir Kolyshkin <[email protected]>

1. Do not compare booleans to false and true. 2. Since there are only two values, no sense in printing the actual value. Signed-off-by: Kir Kolyshkin <[email protected]>

For clarity. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

Test both MountedFast and Mounted. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2022-02-03T04:31:31Z

@thaJeztah @moby/moby-maintainers PTAL

kolyshkin · 2022-02-03T04:36:04Z

One thing I failed to document is shadowed mounts.

For example, if /mnt/one is a mount point, but later sysadmin does mount /dev/sda /mnt, the /mnt/one path becomes shadowed by the new mount. The shadowed mount is still visible from mountinfo, but it is impossible to access it (for example, to unmount).

MounedFast returns ENOENT for such shadowed mounts, since the path is inaccessible. Mounted used to return "true" for such mounts, but since commit dbd468b it also returns ENOENT.

I am failing to describe all this in a nice yet compact way.

In any case, this can be done later.

manugupt1

thank you for doing this in such a nice way and helping me. I learnt a lot from you.

thaJeztah

LGTM

we can further improve documentation in follow ups if we see a need 👍

manugupt1 · 2022-02-03T16:09:19Z

@kolyshkin @thaJeztah
Would you be able to cut a release for Mountinfo for with this change?

Thanks

thaJeztah · 2022-02-03T16:16:48Z

Yes, I wanted to prose doing so.

Needed to check again in which order things had to be released (as the mountinfo and mount packages have a dependency on each other)

kolyshkin · 2022-02-03T16:42:27Z

https://github.com/moby/sys/releases/tag/mountinfo%2Fv0.6.0

kolyshkin · 2022-02-03T16:44:31Z

(as the mountinfo and mount packages have a dependency on each other)

Only mount depends on mountinfo, but not vice versa.

AFAIK we don't need to do anything; updating the mount's go.mod to require mountinfo/v.0.6.0 is highly optional (but makes sense to do before its next release, of course).

thaJeztah · 2022-02-03T16:49:15Z

Yes, it's good to update them, so that projects that use them are using the same version

kolyshkin mentioned this pull request Jan 14, 2022

Introduce MountedFast function to determine mount points faster. #97

Closed

manugupt1 reviewed Jan 15, 2022

View reviewed changes

kolyshkin force-pushed the mounted-fast branch 2 times, most recently from bd6e12a to 7cca805 Compare January 17, 2022 21:02

manugupt1 reviewed Jan 17, 2022

View reviewed changes

Comment thread mountinfo/mounted_linux_test.go Outdated

manugupt1 approved these changes Jan 18, 2022

View reviewed changes

thaJeztah requested changes Jan 27, 2022

View reviewed changes

kolyshkin force-pushed the mounted-fast branch from 7cca805 to 5044597 Compare January 27, 2022 19:54

kolyshkin mentioned this pull request Jan 27, 2022

mountinfo: introduce MountedBind #95

Closed

kolyshkin requested a review from thaJeztah January 27, 2022 19:55

kolyshkin force-pushed the mounted-fast branch from 5044597 to 422ecb3 Compare January 27, 2022 21:26

manugupt1 suggested changes Jan 28, 2022

View reviewed changes

Comment thread mountinfo/mounted_linux.go Outdated

kolyshkin and others added 6 commits February 2, 2022 20:28

mountinfo: improve Mounted doc

4c08076

Fixes: dbd468b Signed-off-by: Kir Kolyshkin <[email protected]>

mountinfo: mounted_linux_test: nits

3494465

1. Do not compare booleans to false and true. 2. Since there are only two values, no sense in printing the actual value. Signed-off-by: Kir Kolyshkin <[email protected]>

mountinfo: TestMountedBy: rm exp

9742587

For clarity. Signed-off-by: Kir Kolyshkin <[email protected]>

mountinfo: add tests for MountedFast

303f101

Signed-off-by: Kir Kolyshkin <[email protected]>

mountinfo: add TestMountedRoot

5d09d69

Test both MountedFast and Mounted. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the mounted-fast branch from 422ecb3 to 5d09d69 Compare February 3, 2022 04:31

manugupt1 approved these changes Feb 3, 2022

View reviewed changes

thaJeztah approved these changes Feb 3, 2022

View reviewed changes

thaJeztah merged commit d01e595 into moby:main Feb 3, 2022

		// Check the public MountedFast() function as a whole.
		mounted, sure, err := MountedFast(m)

Conversation

kolyshkin commented Jan 14, 2022

Uh oh!

manugupt1 Jan 15, 2022 • 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!

kolyshkin Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

manugupt1 commented Jan 15, 2022

Uh oh!

kolyshkin commented Jan 17, 2022

Uh oh!

manugupt1 commented Jan 17, 2022

Uh oh!

Uh oh!

kolyshkin commented Jan 20, 2022

Uh oh!

thaJeztah commented Jan 27, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kolyshkin commented Jan 27, 2022

Uh oh!

Uh oh!

thaJeztah commented Jan 31, 2022

Uh oh!

kolyshkin commented Feb 3, 2022

Uh oh!

kolyshkin commented Feb 3, 2022

Uh oh!

manugupt1 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

manugupt1 commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 3, 2022

Uh oh!

kolyshkin commented Feb 3, 2022

Uh oh!

kolyshkin commented Feb 3, 2022

Uh oh!

thaJeztah commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

manugupt1 Jan 15, 2022 •

edited

Loading

manugupt1 commented Feb 3, 2022 •

edited

Loading