ovl: return error on mount if metacopy cannot be enabled

Consider a user who wants to enable metadata only copy-up with metacopy=on. If this feature can't be enabled, we disable metacopy=off and just leave a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) or redirect_dir=follow (for non-upper mount).

As user does not see mount failure, he/she assumes metadata only copy-up has been enabled but that's not the case.

So instead of disabling metacopy, return an error to user and leave a message in logs. That will allow user to fix mount options and retry. This is done only if user specified metacopy=on in mount options. If metacopy is enabled as default either through module command line or kernel Kconfig, that's not enforced and it can be disabled automatically if system configuration does not permit it.

Reported-by Daniel Walsh <[email protected]>

Suggested-by Vivek Goyal <[email protected]>

Fixes d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")

Cc <[email protected]> # v4.19

Signed-off-by Amir Goldstein <[email protected]>

---

Miklos, Vivek,

I think you will find this patch more pleasant to the eye than Vivek's v1.

I am working on followup patches to implicitly enforce the 'strict' behavior on *all* features if metacopy=on was provided and to add strict behavior as an opt-in Kconfig/module/mount option regardless of metacopy.

The basic idea used to reduce complexity and increase consistency among features, is that if user provides the *new* metacopy=on we can safely enable strict=on on all other features as well without generating backward compat issues.

I chose to separate this patch from the rest for ease of backporting to stable.

Also my feeling is that the option dependency thing is still too
complicated. Why can't we just implicitly enable redirects if
metacopy is enabled? Example: foo=off option is conflicting with foo=on, yet we just take the one which came later, don't warn or error out. Similarly metacopy=on should just imply redirect_dir=on and redirect_dir=off should imply metacopy=off. And document this dependency, of course.

Quoted 7 lines · Miklos Szeredi

Also my feeling is that the option dependency thing is still too
complicated. Why can't we just implicitly enable redirects if
metacopy is enabled? Example: foo=off option is conflicting with foo=on, yet we just take the one which came later, don't warn or error out. Similarly metacopy=on should just imply redirect_dir=on and redirect_dir=off should imply metacopy=off. And document this dependency, of course.

Something like this? Untested, and incomplete:

- show options needs to kill the implied ones

- what's with "metacopy=on,redirect=follow"?

Thanks,
Miklos

---
 fs/overlayfs/super.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

fs/overlayfs/super.c +1 -1

@@ -312,7 +312,7 @@ static bool ovl_force_readonly(struct ov
 
 static const char *ovl_redirect_mode_def(void)
 {
-       return ovl_redirect_dir_def ? "on" : "off";
+       return ovl_redirect_dir_def || ovl_metacopy_def ? "on" : "off";
 }
 
 enum {

fs/overlayfs/super.c +0 -0

                        break;
 
                case OPT_INDEX_ON:

@@ -548,6 +553,12 @@ static int ovl_parse_opt(char *opt, stru
 

On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <[email protected]> wrote:

My 'strict' patch did not try to deal with implicitly enabling redirect_dir Vivek posted a different patch to deal with that.

fs/overlayfs/super.c static int ovl_parse_opt(char *opt, stru · line 523

Quoted 8 lines

                        config->redirect_mode = match_strdup(&args[0]);
                        if (!config->redirect_mode)
                                return -ENOMEM;

fs/overlayfs/super.c +5 -0

+
+                       /* "redirect_dir=off" implies "metacopy=off" */
+                       if (strcmp(config->redirect_mode, "off") == 0 ||
+                           strcmp(config->redirect_mode, "nofollow") == 0)
+                               config->metacopy = false;

This look completely counter to the 'strict' behavior definition. If user specified metacopy=on, we are not allowed to end up with metacopy=off.

fs/overlayfs/super.c static int ovl_parse_opt(char *opt, stru · line 563

Quoted 12 lines

fs/overlayfs/super.c +6 -0

@@ -548,6 +553,12 @@ static int ovl_parse_opt(char *opt, stru
 
                case OPT_METACOPY_ON:
                        config->metacopy = true;
+
+                       /* "metacopy=on" implies "redirect_dir=on" */
+                       kfree(config->redirect_mode);
+                       config->redirect_mode = kstrdup("on", GFP_KERNEL);
+                       if (!config->redirect_mode)
+                               return -ENOMEM;
                        break;
 

Helper please. from my "deprecated upper fs" v1 patch:

Quoted 9 lines

Quoted 8 lines

                        config->redirect_mode = match_strdup(&args[0]);
                        if (!config->redirect_mode)
                                return -ENOMEM;

fs/overlayfs/super.c +5 -0

+
+                       /* "redirect_dir=off" implies "metacopy=off" */
+                       if (strcmp(config->redirect_mode, "off") == 0 ||
+                           strcmp(config->redirect_mode, "nofollow") == 0)
+                               config->metacopy = false;

Quoted 2 lines

This look completely counter to the 'strict' behavior definition. If user specified metacopy=on, we are not allowed to end up with metacopy=off.

What about "metacopy=on,metacopy=off"?

What I'm saying is that "metacopy=on,redirect_dir=off" should be exactly equivalent to the above, if taking the implicit disable rule into account.

Quoted 12 lines · Miklos Szeredi

Quoted 11 lines

fs/overlayfs/super.c static int ovl_parse_opt(char *opt, stru · line 523

Quoted 8 lines

                        config->redirect_mode = match_strdup(&args[0]);
                        if (!config->redirect_mode)
                                return -ENOMEM;

fs/overlayfs/super.c +5 -0

+
+                       /* "redirect_dir=off" implies "metacopy=off" */
+                       if (strcmp(config->redirect_mode, "off") == 0 ||
+                           strcmp(config->redirect_mode, "nofollow") == 0)
+                               config->metacopy = false;

This look completely counter to the 'strict' behavior definition. If user specified metacopy=on, we are not allowed to end up with metacopy=off.

Quoted 5 lines

What about "metacopy=on,metacopy=off"?

What I'm saying is that "metacopy=on,redirect_dir=off" should be exactly equivalent to the above, if taking the implicit disable rule into account.

In other words: it's a nonsense combination of options, just like the former. If documented, then I don't think this would cause problems.

Quoted 4 lines · Amir Goldstein

On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <[email protected]> wrote:

My 'strict' patch did not try to deal with implicitly enabling redirect_dir Vivek posted a different patch to deal with that.

What I mean, is that sections of your patch dealing with the redirect_dir conflict and sections of Vivek's patch dealing with _enforce are completely unnecessary if we just define the rules of implicit enable/disable between metacopy and redirect_dir.

Quoted 9 lines

Quoted 8 lines

                        config->redirect_mode = match_strdup(&args[0]);
                        if (!config->redirect_mode)
                                return -ENOMEM;

fs/overlayfs/super.c +5 -0

+
+                       /* "redirect_dir=off" implies "metacopy=off" */
+                       if (strcmp(config->redirect_mode, "off") == 0 ||
+                           strcmp(config->redirect_mode, "nofollow") == 0)
+                               config->metacopy = false;

Quoted 2 lines

This look completely counter to the 'strict' behavior definition. If user specified metacopy=on, we are not allowed to end up with metacopy=off.

What about "metacopy=on,metacopy=off"?

What I'm saying is that "metacopy=on,redirect_dir=off" should be exactly equivalent to the above, if taking the implicit disable rule into account.

Quoted 22 lines · Amir Goldstein

Consider a user who wants to enable metadata only copy-up with metacopy=on. If this feature can't be enabled, we disable metacopy=off and just leave a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) or redirect_dir=follow (for non-upper mount).

As user does not see mount failure, he/she assumes metadata only copy-up has been enabled but that's not the case.

So instead of disabling metacopy, return an error to user and leave a message in logs. That will allow user to fix mount options and retry. This is done only if user specified metacopy=on in mount options. If metacopy is enabled as default either through module command line or kernel Kconfig, that's not enforced and it can be disabled automatically if system configuration does not permit it.

Reported-by Daniel Walsh <[email protected]>

Suggested-by Vivek Goyal <[email protected]>

Fixes d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")

Cc <[email protected]> # v4.19

Signed-off-by Amir Goldstein <[email protected]>

---

Quoted 7 lines

Miklos, Vivek,

I think you will find this patch more pleasant to the eye than Vivek's v1.

I am working on followup patches to implicitly enforce the 'strict' behavior on *all* features if metacopy=on was provided and to add strict behavior as an opt-in Kconfig/module/mount option regardless of metacopy.

Sounds good.

One other note: I count extactly 6 filesystems (tmpfs, ext4, xfs, btrfs, f2fs, ubifs) that properly support being an upper layer. Why? Because only these ones have RENAME_WHITEOUT, which is the only one operation specifically implemented *for* overlayfs. So if a filesystem implements this, it very very likely implements all the other requirements. Perhaps we should add a check for RENAME_WHITEOUT at mount time, like we do for xattr and tmpfile and d_type.

Quoted 8 lines

The basic idea used to reduce complexity and increase consistency among features, is that if user provides the *new* metacopy=on we can safely enable strict=on on all other features as well without generating backward compat issues.

I chose to separate this patch from the rest for ease of backporting to stable.

I find the precompiler magic a bit too heavy for this. Let's
generalize when that's actually needed...

Quoted 34 lines · Miklos Szeredi

Quoted 22 lines · Amir Goldstein

Consider a user who wants to enable metadata only copy-up with metacopy=on. If this feature can't be enabled, we disable metacopy=off and just leave a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) or redirect_dir=follow (for non-upper mount).

As user does not see mount failure, he/she assumes metadata only copy-up has been enabled but that's not the case.

So instead of disabling metacopy, return an error to user and leave a message in logs. That will allow user to fix mount options and retry. This is done only if user specified metacopy=on in mount options. If metacopy is enabled as default either through module command line or kernel Kconfig, that's not enforced and it can be disabled automatically if system configuration does not permit it.

Reported-by Daniel Walsh <[email protected]>

Suggested-by Vivek Goyal <[email protected]>

Fixes d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")

Cc <[email protected]> # v4.19

Signed-off-by Amir Goldstein <[email protected]>

---

Quoted 7 lines

Miklos, Vivek,

I think you will find this patch more pleasant to the eye than Vivek's v1.

I am working on followup patches to implicitly enforce the 'strict' behavior on *all* features if metacopy=on was provided and to add strict behavior as an opt-in Kconfig/module/mount option regardless of metacopy.

Sounds good.

Quoted 8 lines

One other note: I count extactly 6 filesystems (tmpfs, ext4, xfs, btrfs, f2fs, ubifs) that properly support being an upper layer. Why? Because only these ones have RENAME_WHITEOUT, which is the only one operation specifically implemented *for* overlayfs. So if a filesystem implements this, it very very likely implements all the other requirements. Perhaps we should add a check for RENAME_WHITEOUT at mount time, like we do for xattr and tmpfile and d_type.

Yes, I though about this while working on disable-new-features-for-deprecated-upper-fs
Now I wish to deprecate "unsupported upper fs" with strict=on, so adding a check for RENAME_WHITEOUT makes sense - I will do that. Per your previous comment, I was contemplating whether or not to allow upper fs with no O_TMPFILE support, as there is no feature that actually depends on it.
But since there is no fs with RENAME_WHITEOUT and without O_TMPFILE I guess there is no harm in making O_TMPFILE a 'strict' requirement.

So "strict" will change behavior. That is where we think configuration is not right/suboptimal, we will fail mount?

I feel little odd about enabling "strict" implicitly just because "metacopy" has been passed in. To me, for all new mount options, "strict" should be default implementation (and does not require strict to be on as such).

For old options, users are already happy with what they are seeing as of now. Those who want strict behavior, they should pass in "strict=on" and then behavior of old knobs will change without breaking backward compatibility.

On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <[email protected]> wrote:
...

Quoted 9 lines

So "strict" will change behavior. That is where we think configuration is not right/suboptimal, we will fail mount?

I feel little odd about enabling "strict" implicitly just because "metacopy" has been passed in. To me, for all new mount options, "strict" should be default implementation (and does not require strict to be on as such).

Quoted 5 lines

For old options, users are already happy with what they are seeing as of now. Those who want strict behavior, they should pass in "strict=on" and then behavior of old knobs will change without breaking backward compatibility.

We need to think of users and documentation and real life use cases. We need to think of overlayfs code maintenance and overlayfs developers. We need to come up with a compromise that is the best of all worlds.

Maintaining different behavior per feature complicates the code and IMO brings no real value to users.

If you think there is a concrete real world problem with metacopy=on implying strict=on, please present the case.

Are you interested in making metacopy=on work on sub-optimal upper file systems? Which one?
Are you interested in making index=on,metacopy=on fallback to index=off,metacopy=on on underlying fs without file handle support? Why?
What is the real life use case for which you wish to preserve those behaviors that are mostly there because we made mistakes in the past?

Quoted 27 lines · Amir Goldstein

On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <[email protected]> wrote:
...

Quoted 9 lines

So "strict" will change behavior. That is where we think configuration is not right/suboptimal, we will fail mount?

I feel little odd about enabling "strict" implicitly just because "metacopy" has been passed in. To me, for all new mount options, "strict" should be default implementation (and does not require strict to be on as such).

Quoted 5 lines

For old options, users are already happy with what they are seeing as of now. Those who want strict behavior, they should pass in "strict=on" and then behavior of old knobs will change without breaking backward compatibility.

We need to think of users and documentation and real life use cases. We need to think of overlayfs code maintenance and overlayfs developers. We need to come up with a compromise that is the best of all worlds.

Maintaining different behavior per feature complicates the code and IMO brings no real value to users.

If you think there is a concrete real world problem with metacopy=on implying strict=on, please present the case.

Quoted 8 lines

Are you interested in making metacopy=on work on sub-optimal upper file systems? Which one?
Are you interested in making index=on,metacopy=on fallback to index=off,metacopy=on on underlying fs without file handle support? Why?
What is the real life use case for which you wish to preserve those behaviors that are mostly there because we made mistakes in the past?

I don't have a good example. But, following is my thought process that why I think turning on strict with metacopy=on is not very good.

- Turning on strict on, can make a working configuration fail due to unrelated reasons. Say d_type is not supported and user is fine with that and wants metacopy=on. Now that will not work and it will be confusing to understand that what metacopy has to do with d_type.

- Are you 100% convinced that all the users who are not complaining about current behavior, will not want that behavior for old knobs with metacopy=on. If you decouple strict from metacopy, then it allows you to support these users if somebody comes back.

Quoted 38 lines · Vivek Goyal

Quoted 27 lines · Amir Goldstein

On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <[email protected]> wrote:
...

Quoted 9 lines

So "strict" will change behavior. That is where we think configuration is not right/suboptimal, we will fail mount?

I feel little odd about enabling "strict" implicitly just because "metacopy" has been passed in. To me, for all new mount options, "strict" should be default implementation (and does not require strict to be on as such).

Quoted 5 lines

For old options, users are already happy with what they are seeing as of now. Those who want strict behavior, they should pass in "strict=on" and then behavior of old knobs will change without breaking backward compatibility.

We need to think of users and documentation and real life use cases. We need to think of overlayfs code maintenance and overlayfs developers. We need to come up with a compromise that is the best of all worlds.

Maintaining different behavior per feature complicates the code and IMO brings no real value to users.

If you think there is a concrete real world problem with metacopy=on implying strict=on, please present the case.

Quoted 8 lines

Are you interested in making metacopy=on work on sub-optimal upper file systems? Which one?
Are you interested in making index=on,metacopy=on fallback to index=off,metacopy=on on underlying fs without file handle support? Why?
What is the real life use case for which you wish to preserve those behaviors that are mostly there because we made mistakes in the past?

Quoted 8 lines

I don't have a good example. But, following is my thought process that why I think turning on strict with metacopy=on is not very good.

- Turning on strict on, can make a working configuration fail due to unrelated reasons. Say d_type is not supported and user is fine with that and wants metacopy=on. Now that will not work and it will be confusing to understand that what metacopy has to do with d_type.

It is not in the best of our (overlayfs developers) interest to allow users to shoot their own feet. We allow that when we have no choice (backward compat), but we really have no reason to do that with new features. In fact, new features is our *only* opportunity to deprecate old code that is sub-optimal and only exists because of backward compat reasons.

To use a very concrete example, index=on enables the exclusive upperdir workdir checks. Not because index=on needs exclusive upperdir/workdir more than index=off, but just because we can. And now docker cannot deal with exclusive upperdir/workdir because docker has mount leak bugs, so docker sets index=off to workaround its own bugs.

If ever docker would want to provide sane hardlink behavior to its users, docker will have to fix its mount leaks bugs first. We keep looking backwards, but general direction should be forward!

Ok. So basically strict and metacopy has no connection but we want to take this opportunity to enable more sane overlayfs behavior by default without breaking backward compatibility.

And if a corner case user does not like implicit strict=on, they can specifically do "metacopy=on,strict=off".

I am fine with this. Thanks.

So every new mount option now will enable strict=on implicitly?

Quoted 4 lines

Ok. So basically strict and metacopy has no connection but we want to take this opportunity to enable more sane overlayfs behavior by default without breaking backward compatibility.

Quoted 3 lines

And if a corner case user does not like implicit strict=on, they can specifically do "metacopy=on,strict=off".

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Quoted 5 lines

I am fine with this. Thanks.

So every new mount option now will enable strict=on implicitly?

Correct. For example:

(quoted diff) +1 -0

@@ -109,6 +109,7 @@ config OVERLAY_FS_METACOPY
        bool "Overlayfs: turn on metadata only copy up feature by default"
        depends on OVERLAY_FS
        select OVERLAY_FS_REDIRECT_DIR
+       select OVERLAY_FS_STRICT
        help
          If this config option is enabled then overlay filesystems will
          copy up only metadata where appropriate and data copy up will
 
...

Quoted 5 lines

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Did not understand this. So strict=on will be in stable or not? I mean, all the behavior strict=on enforces will be in 4.19 or not.

If it is not, then it will become backward compatibility issue by the time 4.20 releases.

If strict=on is not part of 4.19 stable, then you can't switch it on in 4.20.

What am I missing.

Review

Quoted 5 lines

(quoted diff) +4 -0

+       /* We must honor user explicit request to enable metacopy */
+       if (config->metacopy && !ovl_metacopy_def)
+               config->strict = true;
+

So if a user loads module with metacopy=on and also specifies metacopy=on mount option, then strict will not be enforced?

Quoted 1 line · Vivek Goyal

...

Quoted 9 lines

Quoted 5 lines

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Did not understand this. So strict=on will be in stable or not? I mean, all the behavior strict=on enforces will be in 4.19 or not.

Quoted 8 lines

If it is not, then it will become backward compatibility issue by the time 4.20 releases.

If strict=on is not part of 4.19 stable, then you can't switch it on in 4.20.

What am I missing.

I guess I haven't thought this through. Let's see:

1.a. In 4.19, specifying metacopy=on will result in metacopy=on OR -EINVAL (a.k.a. "the bug fix") [*]

1.b. In 4.19, user will not be able to mount with metacopy=on with an upper fs that does not support xattr/d_type/RENAME_WHITEOUT.

1.d. In 4.19, mount will fail if given mount option metacopy=on and nfs_export is enabled (even if enabled by default) or if redirect_dir is disabled (even if disabled by default).

1.d. In 4.19, mount will fail if given mount option metacopy=on, index is enabled (even if enabled by default) and underlying fs does not support file handles.

[*] I intentionally reduced the scope of the bug fix to not infer
redirect_dir=on and the like, to keep the stable fix small and stay away from the discussion about conflicting explicit mount options. Kconfig already has the correct inter dependencies between metacopy, redirect_dir and nfs_export. mount options can implement those inter dependencies in 4.20 to reduce the cases of mount failure.

2.a. In 4.20, user will be able to configure strict=on by default to always enforce xattr and RENAME_WHITEOUT support on upper fs

2.b. In 4.20, user will be able to use strict=off to mount metacopy=on without RENAME_WHITEOUT/d_type requirements on upper fs

2.c. In 4.20, metacopy=on will set redirect_dir=on and nfs_export=off unless otherwise specified explicitly by user, in which case mount will fail.

An alternative to 2.c. is what Miklos proposed - conflicting options are allowed - last option among conflicting options is the effective one.

...

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+       /* We must honor user explicit request to enable metacopy */
+       if (config->metacopy && !ovl_metacopy_def)
+               config->strict = true;
+

So if a user loads module with metacopy=on and also specifies metacopy=on mount option, then strict will not be enforced?

Mmm, right. I guess the previous version I posted was better:

(quoted diff) +1 -1

@@ -548,6 +586,7 @@ static int ovl_parse_opt(char *opt, struct
 ovl_config *config)
 
                case OPT_METACOPY_ON:
                        config->metacopy = true;
+                       config->strict = true;
                        break;
 
---

I was trying to avoid having to do:

(quoted diff) +2 -1

@@@ -524,6 -519,6 +560,8 @@@ static int ovl_parse_opt(char *opt, str
                        continue;
 
                token = match_token(p, ovl_tokens, args);
+              if (token >= 0 && token < OPT_ERR)
+                      config->user_opts |= 1UL << token;
                switch (token) {
                case OPT_UPPERDIR:
---

At the moment, the only way I see to avoid it is by adopting Miklos' concept of last option is effective one.

Seeing that it is getting late, I will try to post patch(es) for my 4.19 stable proposal and the 4.20 strict=on config patch.

I'll probably end up leaving the redirect_dir=on logic for you to handle. It's up to you if you want to promote it as stable patch as well - I don't see the point, because it is ok for something that didn't work in 4.19 (redirect_dir_def=off,metacopy=on) to start working in 4.20. and users of metacopy can always "work around" this behavior from userspace by explicitly providing redirect_dir=on,metacopy=on.

Thanks,
Amir.

Quoted 21 lines · Amir Goldstein

Quoted 1 line · Vivek Goyal

...

Quoted 9 lines

Quoted 5 lines

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Did not understand this. So strict=on will be in stable or not? I mean, all the behavior strict=on enforces will be in 4.19 or not.

Quoted 8 lines

If it is not, then it will become backward compatibility issue by the time 4.20 releases.

If strict=on is not part of 4.19 stable, then you can't switch it on in 4.20.

What am I missing.

Quoted 7 lines

I guess I haven't thought this through. Let's see:

1.a. In 4.19, specifying metacopy=on will result in metacopy=on OR -EINVAL (a.k.a. "the bug fix") [*]

1.b. In 4.19, user will not be able to mount with metacopy=on with an upper fs that does not support xattr/d_type/RENAME_WHITEOUT.

Ok, so metacopy will trigger strict=on internally. And that in-turn will trigger all the checks on upper fs (xattr/d_type/RENAME_WHITEOUT).

Quoted 4 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on and nfs_export is enabled (even if enabled by default) or if redirect_dir is disabled (even if disabled by default).

Ok, so no automatic enable/disable of redirect_dir/nfs_export options.

Quoted 1 line

Quoted 3 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on, index is enabled (even if enabled by default) and underlying fs does not support file handles.

Ok, so index might be enabled by default but we will not disable it because metacopy=on has triggered strict behavior.

Quoted 41 lines · Vivek Goyal

Quoted 21 lines · Amir Goldstein

Quoted 1 line · Vivek Goyal

...

Quoted 9 lines

Quoted 5 lines

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Did not understand this. So strict=on will be in stable or not? I mean, all the behavior strict=on enforces will be in 4.19 or not.

Quoted 8 lines

If it is not, then it will become backward compatibility issue by the time 4.20 releases.

If strict=on is not part of 4.19 stable, then you can't switch it on in 4.20.

What am I missing.

Quoted 7 lines

I guess I haven't thought this through. Let's see:

1.a. In 4.19, specifying metacopy=on will result in metacopy=on OR -EINVAL (a.k.a. "the bug fix") [*]

1.b. In 4.19, user will not be able to mount with metacopy=on with an upper fs that does not support xattr/d_type/RENAME_WHITEOUT.

Ok, so metacopy will trigger strict=on internally. And that in-turn will trigger all the checks on upper fs (xattr/d_type/RENAME_WHITEOUT).

Quoted 4 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on and nfs_export is enabled (even if enabled by default) or if redirect_dir is disabled (even if disabled by default).

Ok, so no automatic enable/disable of redirect_dir/nfs_export options.

Quoted 1 line

Quoted 6 lines

Quoted 3 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on, index is enabled (even if enabled by default) and underlying fs does not support file handles.

Ok, so index might be enabled by default but we will not disable it because metacopy=on has triggered strict behavior.

Hi Amir,

Thinking more about it. What does strict=on mean for default behavior of a knob (configured using module param or Kconfig). Will it mean that default value of knob can't be changed either (disabled if need be).

IOW, strict=on will mean that all values of options will be static and can't be changed. It does not matter where these came from. (Override ordering between Kconfig/module/mount option will still be there).

If that's the case, then metacopy=on (hence strict=on) and auto enabling of redirect_dir are contradictory. Given strict is on, we are not supposed to change the default value as it came from Kconfig/module?

Similarly, we can't disable nfs_export if metacopy=on.

IOW, If we limit "strict=on" to values passed as part of mount options, then it will allow to change default values of other options as needed. But if "strict=on" means default values can't be changed, then auto enabling/disabling of redirect_dir/nfs_export is not possible?

Quoted 43 lines · Vivek Goyal

Quoted 41 lines · Vivek Goyal

Quoted 21 lines · Amir Goldstein

Quoted 1 line · Vivek Goyal

...

Quoted 9 lines

Quoted 5 lines

Yes, but keep in mind that strict=off will NOT be applied to stable v4.19.y *unless* a real user really reports a bug, a-priori, we assume that implicit metacopy=on => redirect_dir=on,strict=on is sufficient to solve the bug, so it is sufficient for stable.

Did not understand this. So strict=on will be in stable or not? I mean, all the behavior strict=on enforces will be in 4.19 or not.

Quoted 8 lines

If it is not, then it will become backward compatibility issue by the time 4.20 releases.

If strict=on is not part of 4.19 stable, then you can't switch it on in 4.20.

What am I missing.

Quoted 7 lines

I guess I haven't thought this through. Let's see:

1.a. In 4.19, specifying metacopy=on will result in metacopy=on OR -EINVAL (a.k.a. "the bug fix") [*]

1.b. In 4.19, user will not be able to mount with metacopy=on with an upper fs that does not support xattr/d_type/RENAME_WHITEOUT.

Ok, so metacopy will trigger strict=on internally. And that in-turn will trigger all the checks on upper fs (xattr/d_type/RENAME_WHITEOUT).

Quoted 4 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on and nfs_export is enabled (even if enabled by default) or if redirect_dir is disabled (even if disabled by default).

Ok, so no automatic enable/disable of redirect_dir/nfs_export options.

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

1.d. In 4.19, mount will fail if given mount option metacopy=on, index is enabled (even if enabled by default) and underlying fs does not support file handles.

Ok, so index might be enabled by default but we will not disable it because metacopy=on has triggered strict behavior.

Hi Amir,

First, we got the versions a bit mixed up.
I am not sure strict=on is 4.20 material, this late in the cycle. Maybe. More likely we get the 4.19 material in 4.20 and backported to 4.19. strict=on, unless someone ends up needing it soon, is more likely 4.21 material.

Quoted 4 lines

Thinking more about it. What does strict=on mean for default behavior of a knob (configured using module param or Kconfig). Will it mean that default value of knob can't be changed either (disabled if need be).

It means that features cannot be disabled to resolve unmet dependencies. It doesn't mean that features cannot be enabled to meet dependencies.

config OVERLAY_FS_STRICT
bool "Overlayfs: turn on strict requirements by default" depends on OVERLAY_FS
help
The overlayfs filesystem is fully functional and optimal when the underlying filesystems support extended attributes, d_type values in readdir and RENAME_WHITEOUT. In addition, some overlay filesystem features require that underlying filesystems support exporting NFS file handles.

If this config option is enabled, then overlay filesystem mount will fail if any of the requirements from underlying filesystems are not met. When strict requirements are enabled, overlay filesystem mount will also fail if any of the mount options are conflicting, for example: "direct_dir=off,metacopy=on".

If this config option is enabled, it is still possible to turn off strict requirements globally with the "strict=off" module option or on a filesystem instance basis with the "strict=off" mount option.

If unsure, say N.

Quoted 5 lines

IOW, strict=on will mean that all values of options will be static and can't be changed. It does not matter where these came from. (Override ordering between Kconfig/module/mount option will still be there).

That's right. It's a very 'strict' policy ;-)

Quoted 4 lines

If that's the case, then metacopy=on (hence strict=on) and auto enabling of redirect_dir are contradictory. Given strict is on, we are not supposed to change the default value as it came from Kconfig/module?

'strict' does not forbid us to enable redirect_dir.
If we want to forbid enabling redirect_dir when user explicitly requested redirect_dir=nofollow that is a different story and not related to 'strict' (yet).

Quoted 2 lines

Similarly, we can't disable nfs_export if metacopy=on.

No, we cannot. That is exactly what this config means:
CONFIG_OVERLAY_FS_NFS_EXPORT=y CONFIG_OVERLAY_FS_STRICT=y

It means all overlay mounts by default support NFS export or fail. On failure, user can opt-out of NFS export, for example with nfs_export=off,metacopy=on.

I feel definition of "strict" has been expanding as we are discussing.

- Initilaly, I thought that it only meant that any options specified as mount option can'e be disabled and if system configuration does not support it (for older mount options), we will fail mount.

- Now it also means that Kconfig/module can lock down value of a varibale and it can't be disabled from mount option (until and unless user passes in strict=off).

Have a quick question. Does backward compatibility applies even if both upper/ and workdir/ are empty. IOW, can't we automatically enable "strict" when we know that this is the first time overlayfs mount is being created (otherwise both upper and workdir will not have been empty) and we can enable strict policy without breaking backward compatibility.

/me is assuming that if a overlay mount has been successfuly mounted once, workdir will atleast have a subdir named "work".