If the parameter is read successfully, could it just return here?

Yes, reasonable, will fix. Thank you!

Maybe use iota here? Or do we need it to be these specific numbers?

EDIT: as I mentioned in the other comment, I think SupportsMetacopy can just return a bool instead of these. If that happens to be true, ignore this comment :)

Would it make sense to log inside overlayutils.SupportsMetacopy() instead of here?

Yes, reasonable, will fix.

Do we really need both here? Can't we just save a bool here that is: config.metacopy && metacopySupported and change metacopyState to return true/false to signal if metacopy is supported or not?

Maybe I'm being overly simplistic and missing something? I think if metacopy is supported, then we can always append the metacopy=on/off to the mount point, and if it is not supported then we don't append anything.

Yes, we can do like this. In case of CONFIG_OVERLAY_FS_METACOPY=y the metacopy=on option is just ignored and not shown in mount options, the same for CONFIG_OVERLAY_FS_METACOPY=n the metacopy=off. I'll update this place.

Oh, sounds great then! :)

One note here, I think that we need to keep three states for metacopy field (Enabled, Disabled, Not supported). We need this because of in case of old kernels the mount operation can fail due to metacopy=on/off option is unsupported.

But if metacopy is unsupported, we won't add the option, right? What is the issue, then? I don't follow

I'm saying we don't need both fields in this struct. We need just one bool that is: config.Metacopy && metacopySupported. IOW, this bool is true only if metacopy is requested in the config and supported in the kernel. Othweise is false

We need to know if the metacopy was enabled in the config and we need to know if the kernel supports metacopy. But we don't need to store both things as different bools here, we just need to store one bool with the AND of both.

Is it clear what I say now? Am I missing something?

First case:
Metacopy supported by kernel and CONFIG_OVERLAY_FS_METACOPY=Y.
In this case overlayfs mounts will be created with metacopy support even if we don't pass metacopy=on option to mount() system call.
config.Metacopy = false.
As result:
config.Metacopy = false && metacopySupported = true == false -> mount with metacopy=off-> ok, metacopy disabled for this mount point

Second case:
Metacopy is not supported by kernel.
config.Metacopy = false.
As result:
config.Metacopy = false && metacopySupported = false == false -> mount with metacopy=off -> mount failed

We need third state to distinguish between these situations.
In your approach if CONFIG_OVERLAY_FS_METACOPY=Y and config.Metacopy = false we just don't add metacopy=on/off option to mount system call and metacopy will be silently enabled for containers without containerd request.

In case of assumption that nobody set CONFIG_OVERLAY_FS_METACOPY=Y, yes, we no need third state.

Oh, right, brain fart here (it was implied in the "f metacopy is supported" that I was wrong, as we lost that data combining the bools).

I do think, though, that we don't need the enum state. We need to know if it is supported and if it was enabled, then we can just add the on/off unconditionally. IMHO it seems simpler (less things to know and reason about) and avoids races like the sysfs value changed just between when we read it and when the overlayfs was mounted.

Does it make sense?

In fact the first iteration use two booleans, one for on/off and one for supported/unsupported. From my point of view it doesn't matter enum or booleans, for me enum is more clear just maybe because of I am mostly C programmer ). I'll just compare what option looks better and publish better version.
Regarding sysfs, we can just use stat for /sys/modules/overlay/parameters/metacopy to check supported or not and yes, add on/off unconditionally. This should be race-free.

SGTM

Just curious, in which case can we fail to check via the sysfs files and we want to do a test mount to see if it is available? When sysfs is not mounted? Is there some other case?

I'm ignorant here, but do we care about sysfs not being mounted? Maybe it is common in some scenarios that I don't know about?

Yes, this is for the cases when there is no sysfs mounted or there are some specific LSM policies. But I also not sure that this is the case. So, maybe I will simplify this check to use sysfs only and move this to integration test.

I don't have a strong opinion here, not sure if in other parts of the code we already rely on sysfs maybe? I'd let others weight in. Maybe @dmcgowan that also reviewed this?

Note, however, if you are adding an early return here as @dmcgowan suggested, it would not check the LSM and those things you mentioned. Right?

Yes, in this case it will work like this:
check via sysfs is ok? -> return, otherwise -> slowpath, check via xattrs

I don't have a strong opinion here, not sure if in other parts of the code we already rely on sysfs maybe? I'd let others weight in. Maybe @dmcgowan that also reviewed this?
Another argument for simplification is the next:
overlay snapshotter already use sysfs to check overlay "index=off" option.

containerd/snapshots/overlay/overlay.go

Line 569 in 681aaf6

func supportsIndex() bool {

@artqzn right, I think that is what I was thinking about that we already use it. My vote is to remove all of the rest, then.

Ok, great, will do like this.

It seems the stat is not used. Can't we just remove the first if to stat, and try to read the file? If it doesn't exist, then it should return an error

Hmm, or maybe we don't need to read the file and just stat it with the other simplifications. If the file exists, we can clearly use metacopy in the mounts and that is all we need to know, we don't need to know the content (after the simplifications are done). Right?

If we don't want to support the case when metacopy enabled by default in Linux kernel (CONFIG_OVERLAY_FS_METACOPY=Y) and we would like to disable it by passing metacopy=off to overlay mount point we can do like this.

I think this will become more clear once you push the next iteration. We can discuss it then, if needed :)

If we are keeping the enum, I think this should not be an int but it's own type? I don't know, int seems weird, but no strong opinion.

In any case, I think changing this to return a bool instead seems simpler. Like func metacopySupported() bool (or whatever is a good name, haven't thought about it).

Then, if it is supported, we add the on/off as it was requested on the params. Otherwise, we don't add those params and just log.

Does it make sense?

I've updated the PR. I think that enum is better, especially after comment from @AkihiroSuda #7141 (comment), due to it would be good to hide all checks in one small function that return 3-state metacopy configuration and then use simple switch to provide right mount option. I think it is just more readable and clear.

Good point, SGTM! Thanks!

Thanks for review Rodrigo!

userxattr and metacopy=on cannot be specified together
https://github.com/torvalds/linux/blob/4b0986a3613c92f4ec1bdc7f60ec66fea135991f/fs/overlayfs/super.c#L734

Yes, thanks, I'll add this check

I'm unsure which is is the right behavior: (a) logging and continue or (b) returning an error. But no strong opinion, I lack the context about how userxattr is set

The userxattr option is set in case of we try to create unprivileged mount point of overlayfs, for example in case of containerd is running inside user namespace under user that is not a real root. @AkihiroSuda, please correct me if I am wrong. In this case we will be unable to create overlayfs mount point with metacopy=on.

Regarding error handling I've just following the userxattr option handling approach.
If mounting of rootfs with metacopy=on is impossible, this is not a critical failure. So, we can log warning here.
Dear reviewers @AkihiroSuda @dmcgowan , could you please share your opinion here?

Thanks.

Also, maybe it is necessary to add a sanity check for redirect_dir, due to metacopy=on will not work if redirect_dir={off|nofollow|follow(*)}. This is from https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt.

But currently containerd doesn't touch redirect_dir, due to this fact I drop this check.

Warn looks good

Thanks @AkihiroSuda!

Our recommendation has been to use the system parameters here. If you had previously enabled then upgraded with this change, would it become disabled by default with no configuration change?

Yes, with no configuration change metacopy will be disabled, even it was enabled by CONFIG_OVERLAY_FS_METACOPY=Y in Linux kernel. The CONFIG_OVERLAY_FS_METACOPY option is usually disabled by default, but yes, we can have customized installation or customized overlay.ko that contains CONFIG_OVERLAY_FS_METACOPY = Y.

Possibly we can do like this:
In case of metacopy=true in /etc/containerd/config.toml we add metacopy=on option to overlayfs mount point if metacopy is supported. Otherwise, don't add any option to overlayfs mount point and use system defaults. In this case it will be safe to use default configuration if CONFIG_OVERLAY_FS_METACOPY=Y

With such approach we will be unable to switch metacopy support off for overlayfs snapshotter if metacopy is enabled system-wide. But looks like this is not our goal.

I'll update the logic If such approach is ok.

Maybe we can just have the config field be a *bool instead? We know when it is not set (it is nil) and we don't change anything, or when it is set to true/false and do the right thing.

@rata Good idea, it would be bad to introduce string field in config or something like this to enable 3-state configuration. Will test and update PR. Thanks!

Hi, I've updated the implementation. Now, if there is no metacopy option in /etc/containerd/config.toml the overlayfs module configuration is used, and containerd doesn't override system default.
Also, in case of *bool config option for metacopy support it is better to use booleans in snapshotter config instead of enum, due to it simplify options conversion.

@rata, @AkihiroSuda, @dmcgowan, could you have a look please.

Thank you in advance!

Thanks fixed

I'm not convinced still that making the field in SnapshotterConfig a *bool isn't simpler. We just assign it here (we won't need two "overlay.WithMetacopy..." functions), and then we test for not nil in other parts of the code (instead of metacopyCtr) and that is it.

Just my personal taste (and I'm no maintainer here), definitely not a must.

Here I agree, but I just don't want to change the original behavior. Currently the parameters for func NewSnapshotter(root string, opts ...Opt) (snapshots.Snapshotter, error) function are passed using "ops", like

// WithUpperdirLabel adds as an optional label
// "containerd.io/snapshot/overlay.upperdir". This stores the location
// of the upperdir that contains the changeset between the labelled
// snapshot and its parent.
func WithUpperdirLabel(config *SnapshotterConfig) error {
    config.upperdirLabel = true
    return nil
}

We can pass SnapshotterConfig structure directly to NewSnapshotter, but in this case it should be prepared in plugin.go.
I can do this, not a problem, if there no objections from other reviewers, due to I don't know the story, why current approach was used.

If we are keeping the two bools, then this is redundant. metacopyOn is true if config.metacopyCtrl.

Of course, you can't just change that for if metacopyOn as the else case would be broken (we can add when it is not supported). But it seems like this could be simplified.

Maybe the if should be: if config.metacopyCtrl && supportsMetacopy() and inside that you test for metacopyOn and userxattr?

This is just debug leftover, I'll remove it, thanks!

I think you introduced the bug I mentioned it is easy to introduce here. But let's move the conversation to: https://github.com/containerd/containerd/pull/7141/files#r929864915

But with this change, then, it has the bug that I mentioned here: #7141 (comment)

If metacopy is set to false in the config but the kernel doesn't support it, we will append the metacopy=off option. I'm not sure that is incorrect (the user asked for it), but the true value is only used when it is also supported.

Yes, sorry, fixed.

Current version is ok.
metacopyCtrl := config.metacopyCtrl && supportsMetacopy() - true only if metacopy is supported, so we add metacopy=on/off only in case we have support from overlayfs.

why !os.IsExist here? (Should it be IsNotExist?) And shouldn't we treat it as "failed to check for kernel option" as well?

And shouldn't we treat it as "failed to check for kernel option" as well?

It's reasonable, if a user specified metacopy option then we expect that there's support from the kernel, otherwise just error out. I'll fix this in the next iteration. Thanks!

why can't this be a bool? In other words, is there any difference between metacopyOn == nil and *metacopyOn == false?