[v2] overlay2: test for and report metacopy status#43557
[v2] overlay2: test for and report metacopy status#43557thaJeztah merged 2 commits intomoby:masterfrom
Conversation
thaJeztah
added
area/storage/overlay
status/2-code-review
kind/enhancement
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! Had a first glance over the changes; out of interest; this currently only does the detection; should (if supported) this detection also be used by the actual driver?
| } else { | ||
| logger.Warnf("overlay test mount did not indicate whether or not metacopy is being used: %v", err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Given that this information currently appears to only be for "informational" purposes, we should not return an error here; do we know under what circumstances this may fail? Effectively, this is a "best effort" attempt to detect the feature (nothing should fail if we fail to detect), so I'm inclined to;
- in case of failure, log the error, and consider it unsupported ("false")?
- consider moving all logging into
doesMetacopy()/supportsMetaCopy(), and make it just return a boolean.
There was a problem hiding this comment.
That sounds reasonable to me. There are situations in which the mount could fail, but we mostly should not reach those as the overlay2 driver wouldn't be supported/loaded.
There was a problem hiding this comment.
Okay, I'm looking at this again and thinking that we should still bubble any errors up out of doesMetacopy() -- specifically, any failure to Mkdir/Chmod/manipulate the filesystem in any way should be bubbled up instead of swallowed by doesMetacopy().
While yes, it is optional/informational, I don't think it makes sense to silently ignore errors that may indicate an underlying system problem (like being unable to create a temporary directory, or being unable to mount a directory).
neersighted
force-pushed
the
overlay2-report-metacopy
branch
2 times, most recently
from
a80ad77 to
9af74c6
Compare
|
I've had a go at addressing review feedback -- please let me know if any of the points I pushed back on are problematic. Regarding this:
I don't think there is anything else to be done in the daemon at this time. We don't currently request metacopy (on, or off) in our mounts, so this is merely meant to report the status of metacopy to aid in debugging issues with the daemon/driver in the field. Typically metacopy will be off, as that is the upstream default, but it could be turned on by a custom kernel, a kernel command line, or writing to In theory we could add configuration to force metacopy on/off and change this check to detect support -- however there is currently no drive to do so. However it does give a good jumping off point if such a feature is ever desirable for some reason. |
neersighted
force-pushed
the
overlay2-report-metacopy
branch
from
9af74c6 to
35f5577
Compare
neersighted
changed the title
This is a first, naive implementation, that does not account for userxattr/UserNS. Signed-off-by: Bjorn Neergaard <[email protected]>
neersighted
force-pushed
the
overlay2-report-metacopy
branch
from
35f5577 to
da9ef2c
Compare
| func usingMetacopy(d string) (bool, error) { | ||
| userxattr := false | ||
| if userns.RunningInUserNS() { | ||
| needed, err := overlayutils.NeedsUserXAttr(d) |
There was a problem hiding this comment.
Note for reviewers: this results in us doing a mount to check if we need userxattr twice -- however, I didn't want to attach a major refactor to this PR.
The cost is negligible as this is only done at driver Init. There will be a follow-up PR that restructures this to avoid the duplicate work.
|
@neersighted looks like a linter is failing; |
Signed-off-by: Bjorn Neergaard <[email protected]>
Fixed, thanks! |
neersighted
force-pushed
the
overlay2-report-metacopy
branch
from
da9ef2c to
ce3e2d1
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
only wondering if the two commits should be squashed (if the first commit is broken, they probably should be squashed)
thaJeztah
dismissed
AkihiroSuda’s stale review
Looks like this was addressed in the second commit
4 participants
This is helpful for debugging issues with the overlay2 storage driver.
Metacopy may be enabled by CONFIG_OVERLAY_FS_METACOPY, the overlay.metacopy parameter, or the metacopy mount option. See Documentation/filesystems/overlayfs.txt in the kernel source tree for details.
Backported from containers/storage@05c69f1b2a.