-
Notifications
You must be signed in to change notification settings - Fork 246
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
overlay: try harder with more layers #1375
Conversation
00eb305
to
94f8d8e
Compare
94f8d8e
to
59d3fb8
Compare
This is making me consider if we'd just be better off recording layer IDs in "${layer}/lower" in "${layer}/diff" form and dispensing with the symbolic link farm in "l" entirely. We'd fork/exec for mounting a lot more often. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea and overall implementation LGTM — mostly mild style suggestions.
(Parenthetically, I’m a bit unhappy with overlay.Driver.get
growing even more, to 329 lines now. I suppose splitting that up would be a separate PR…)
drivers/overlay/mount.go
Outdated
// paths, but we don't want to mess with other options. | ||
var upper, work, lower, label, other string | ||
for _, arg := range strings.Split(options.Label, ",") { | ||
if strings.HasPrefix(arg, "upperdir=") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style only:
nv := strings.SplitN(arg, "=")
switch nv[0] {
case "upperdir":
upper = nv[1]
…
}
(and eventually strings.Cut
with Go 1.18.)
Separately: The caller has all of this data. It feels a bit tedious to format it into a single string, and then parse it out again — but changing that would be a rather larger rewrite.
drivers/overlay/mount.go
Outdated
|
||
// Reconstruct the Label field. | ||
options.Label = "" | ||
for _, arg := range []string{upper, work, lower, label, other} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[A] …
something like
for _, pair := range []struct{optionName, value string} {
{ "upperdir=", upper },
…
{ "", other },
}
would allow most of this function to only deal with the individual values instead of the mount syntax.
(Ultimately, this “turn option values into a mount option syntax string” code might be useful to completely factor out…)
If we have enough layers that even using relative paths for symlinks to the diff directories won't make the list of lower directories and options fit in a single page, * open the various lowers for reading * use the descriptor names in the lowerdir= list This is a bit slower, so don't take these extra steps unless we know we have to. Signed-off-by: Nalin Dahyabhai <[email protected]>
59d3fb8
to
7c5964d
Compare
@mtrmac PTAL again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM.
// is true (512 is a buffer for label metadata). | ||
// is true (512 is a buffer for label metadata, 128 is the | ||
// number of lowers we want to be able to use without having | ||
// to use bind mounts to get all the way to the kernel limit). | ||
// ((idLength + len(linkDir) + 1) * maxDepth) <= (pageSize - 512) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the inequality does not hold. s/maxDepth/128/
or maybe … maxDirect = 128 is the …
and * maxDirect
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1392 to do the first one.
If we have enough layers that even using relative paths for symlinks to the diff directories won't make the list of lower directories and options fit in a single page,
create a temporary directorymount a tmpfs therebind mount from the diff directories to subdirectories of the temp directory with short namesmount using those bind-mounted directories as the lower directoriesdetach the temporary directoryremove the temporary directorylowerdir=
listThis is
noticeablyslower, so don't take these extra steps unless we know we absolutely have to.Should help with cri-o/cri-o#6261.