[PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[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 <dwalsh@redhat.com>
Suggested-by: Vivek Goyal <vgoyal@redhat.com>
Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

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.

Thoughts?

Thanks,
Amir.

 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 77 ++++++++++++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..fb819aec46c0 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@ struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool strict;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 22ffb23ea44d..69ced0aa96de 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -56,6 +56,49 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
 MODULE_PARM_DESC(ovl_xino_auto_def,
 		 "Auto enable xino feature");
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+		 "Default to on or off for the metadata only copy up feature");
+
+/*
+ * Check dependencies between features.
+ *
+ * @enabled is a boolean variable that depends on the boolean @required
+ * variable.
+ *
+ * If @strict is false, the feature is disabled when requirement is not met.
+ * If @strict is true, return error when requirement is not met.
+ *
+ * @feature is the name of the feature and @requirement is the description of
+ * the requirement for the error/warning message.
+ */
+static int ovl_bool_feature_requires(bool *enabled, bool required, bool strict,
+				     const char *feature,
+				     const char *requirement)
+{
+	/* If feature is enabled, required condition should be met */
+	if (!*enabled || required)
+		return 0;
+
+	if (strict) {
+		pr_err("overlayfs: %s requires %s.\n", feature, requirement);
+		return -EINVAL;
+	}
+
+	pr_warn("overlayfs: %s requies %s, disabling %s feature.\n", feature,
+		requirement, feature);
+	*enabled = false;
+
+	return 0;
+}
+
+#define ovl_feature_requires(config, type, name, required,	\
+			     feature, requirement)		\
+ovl_##type##_feature_requires(&(config)->name, required,	\
+			      (config)->strict, feature,	\
+			      requirement)
+
 static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;
@@ -64,11 +107,6 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
 		dput(oe->lowerstack[i].dentry);
 }
 
-static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
-module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
-MODULE_PARM_DESC(ovl_metacopy_def,
-		 "Default to on or off for the metadata only copy up feature");
-
 static void ovl_dentry_release(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
@@ -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;
 
 		case OPT_METACOPY_OFF:
@@ -573,15 +612,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		return err;
 
 	/* metacopy feature with upper requires redirect_dir=on */
-	if (config->upperdir && config->metacopy && !config->redirect_dir) {
-		pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=on\", falling back to metacopy=off.\n");
-		config->metacopy = false;
-	} else if (config->metacopy && !config->redirect_follow) {
-		pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
-		config->metacopy = false;
+	if (config->upperdir) {
+		err = ovl_feature_requires(config, bool, metacopy,
+					   config->redirect_dir,
+					   "metadata only copy up",
+					   "\"redirect_dir=on\"");
+	} else {
+		err = ovl_feature_requires(config, bool, metacopy,
+					   config->redirect_follow,
+					   "metadata only copy up",
+					   "\"redirect_dir=follow\" on non-upper mount");
 	}
 
-	return 0;
+	return err;
 }
 
 #define OVL_WORKDIR_NAME "work"
@@ -1055,9 +1098,13 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	if (err) {
 		ofs->noxattr = true;
 		ofs->config.index = false;
-		ofs->config.metacopy = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
-		err = 0;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
+		err = ovl_feature_requires(&ofs->config, bool, metacopy,
+					   !ofs->noxattr,
+					   "metadata only copy up",
+					   "upper fs xattr support");
+		if (err)
+			goto out;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
-- 
2.17.1

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 12:10 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> 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 <dwalsh@redhat.com>
> Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> 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.


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

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.

Thanks,
Miklos

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 12:48:20PM +0100, Miklos Szeredi wrote:

> 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(-)

--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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 {
@@ -516,6 +516,11 @@ static int ovl_parse_opt(char *opt, stru
 			config->redirect_mode = match_strdup(&args[0]);
 			if (!config->redirect_mode)
 				return -ENOMEM;
+
+			/* "redirect_dir=off" implies "metacopy=off" */
+			if (strcmp(config->redirect_mode, "off") == 0 ||
+			    strcmp(config->redirect_mode, "nofollow") == 0)
+				config->metacopy = false;
 			break;
 
 		case OPT_INDEX_ON:
@@ -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;
 
 		case OPT_METACOPY_OFF:

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 31, 2018 at 12:48:20PM +0100, Miklos Szeredi wrote:
>
> > 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.
>

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

> 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(-)
>
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -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 {
> @@ -516,6 +516,11 @@ static int ovl_parse_opt(char *opt, stru
>                         config->redirect_mode = match_strdup(&args[0]);
>                         if (!config->redirect_mode)
>                                 return -ENOMEM;
> +
> +                       /* "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.

>                         break;
>
>                 case OPT_INDEX_ON:
> @@ -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:

+static int ovl_set_redirect_mode(struct ovl_config *config, const char *mode)
+{
+       kfree(config->redirect_mode);
+       config->redirect_mode = kstrdup(mode, GFP_KERNEL);
+       if (!config->redirect_mode)
+               return -ENOMEM;
+
+       return ovl_parse_redirect_mode(config, mode);
+}
+

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 1:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> 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.

>>                         config->redirect_mode = match_strdup(&args[0]);
>>                         if (!config->redirect_mode)
>>                                 return -ENOMEM;
>> +
>> +                       /* "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.

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.


>> @@ -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:

Yeah, definitely want that one.

Thanks,
Miklos

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Wed, Oct 31, 2018 at 1:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:

>>>                         config->redirect_mode = match_strdup(&args[0]);
>>>                         if (!config->redirect_mode)
>>>                                 return -ENOMEM;
>>> +
>>> +                       /* "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.
>
> 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.

Thanks,
Miklos

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 1:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 31, 2018 at 12:10 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > 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 <dwalsh@redhat.com>
> > Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> > Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
> > Cc: <stable@vger.kernel.org> # v4.19
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > 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.
>

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.

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

You are right. I expanded the macros and it looks better.

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 2:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Oct 31, 2018 at 1:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> >>>                         config->redirect_mode = match_strdup(&args[0]);
> >>>                         if (!config->redirect_mode)
> >>>                                 return -ENOMEM;
> >>> +
> >>> +                       /* "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.
> >
> > 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.
>

I see. That makes sense.
Both metacopy and redirect_dir depend only on upper xattr
which makes this easy.
Although the dependency check of redirect_dir->xattr is missing
so I will look at adding that too.

Will post v2 of 'strict' series later today, with or without your
suggestion about changing inter feature relationship.
Either solution will be sufficient or Vivek can take it from there.

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 03:29:08PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 1:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, Oct 31, 2018 at 12:10 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > 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 <dwalsh@redhat.com>
> > > Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> > > Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
> > > Cc: <stable@vger.kernel.org> # v4.19
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > 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.
> >
> 
> 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.

Just a thought.

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
...
>
> 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.
>

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?

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 01:35:38PM +0100, Miklos Szeredi wrote:
> On Wed, Oct 31, 2018 at 1:26 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> 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.
> 
> >>                         config->redirect_mode = match_strdup(&args[0]);
> >>                         if (!config->redirect_mode)
> >>                                 return -ENOMEM;
> >> +
> >> +                       /* "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.
> 
> What about "metacopy=on,metacopy=off"?

I would think that ordering will matter. metacopy=off will override,
metacopy=on (because its later in the order).

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

I am wondering why do we have to accept nonsense options possibilities.
Why not simply reject them with -EINVAL. IMHO, that behavior is much
simpler to deal with than implicitly disabling metacopy because
redirect_dir=off.

If some options don't make sense together, then it makes sense to return
error and let user understand it and fix it. 

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 04:05:02PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> ...
> >
> > 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.
> >
> 
> 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?

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.

Just a thought. I found it odd to couple metacopy=on and strict=on.
But if you and Miklos like it, I am fine.

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 4:17 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Oct 31, 2018 at 04:05:02PM +0200, Amir Goldstein wrote:
> > On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > ...
> > >
> > > 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.
> > >
> >
> > 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?
>
> 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!

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

If users really want metacopy with no d_type, they can do that with
metacopy=on,strict=off. You see metacopy does not enforce strict=on
it just implies strict=on, just like it implies redirect_dir=on.

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 05:31:33PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 4:17 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Oct 31, 2018 at 04:05:02PM +0200, Amir Goldstein wrote:
> > > On Wed, Oct 31, 2018 at 3:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > ...
> > > >
> > > > 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.
> > > >
> > >
> > > 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?
> >
> > 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?

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

> 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".
>

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.

> I am fine with this. Thanks.
>
> So every new mount option now will enable strict=on implicitly?
>

Correct. For example:


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

...

@@ -560,6 +599,10 @@ static int ovl_parse_opt(char *opt, struct
ovl_config *config)
                }
        }

+       /* We must honor user explicit request to enable metacopy */
+       if (config->metacopy && !ovl_metacopy_def)
+               config->strict = true;
+
        /* Workdir is useless in non-upper mount */
        if (!config->upperdir && config->workdir) {

---

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 06:04:36PM +0200, Amir Goldstein wrote:
> > 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".
> >
> 
> 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.

> 
> > I am fine with this. Thanks.
> >
> > So every new mount option now will enable strict=on implicitly?
> >
> 
> Correct. For example:
> 
> 
> @@ -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
> 
> ...
> 
> @@ -560,6 +599,10 @@ static int ovl_parse_opt(char *opt, struct
> ovl_config *config)
>                 }
>         }
> 
> +       /* 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?

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 6:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
...
> >
> > 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.
>

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.

...
> >
> > +       /* 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:

@@ -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:

@@@ -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.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 08:37:04PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 6:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> ...
> > >
> > > 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.
> >
> 
> 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).

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

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

> 
> [*] 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.

I think that's reasonable. Automatically enabling (redirect_dir) and
disabling (nfs_export) is another level of complexity. So not allowing
it in 4.19 and doing something more smart in 4.20 makes sense.

IOW, in 4.19 you are not introducing user visible option strict=on/off
but internally implementing it which will be triggered by metacopy=on.
That should work as long as we make sure anything existing configuration
which needs to be blocked behind strict, is properly done and nothing
is missed.

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

Ok, so 4.20 will be more about creating user visible strict=on/off and
auto enable/disable of redirect_dir/nfs_export features.

> 
> ...
> > >
> > > +       /* 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:
> 
> @@ -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:
> 
> @@@ -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.

I think "last option is the effective one" makes sense to me. It will
override all the previous instances of same option.

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

I agree that lets get basic stuff in 4.19 stable. (Make sure anything
which needs to be blocked behind strict=on, is taken care of).

All the auto enabling/disabling of dependent options can be taken care
of in 4.20.

Thanks
Vivek
> 
> Thanks,
> Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 03:23:41PM -0400, Vivek Goyal wrote:
> On Wed, Oct 31, 2018 at 08:37:04PM +0200, Amir Goldstein wrote:
> > On Wed, Oct 31, 2018 at 6:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > ...
> > > >
> > > > 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.
> > >
> > 
> > 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).
> 
> > 
> > 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.
> 
> > 
> > 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?

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Oct 31, 2018 at 9:56 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Oct 31, 2018 at 03:23:41PM -0400, Vivek Goyal wrote:
> > On Wed, Oct 31, 2018 at 08:37:04PM +0200, Amir Goldstein wrote:
> > > On Wed, Oct 31, 2018 at 6:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > ...
> > > > >
> > > > > 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.
> > > >
> > >
> > > 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).
> >
> > >
> > > 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.
> >
> > >
> > > 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.

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

> 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 ;-)

> 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).

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


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

I am not ruling out that some day, we can relax strict to know which
options came from where (default or explicit) I just don't want to get into
that right now. If we come to a conclusion that it makes sense to
relax 'strict' on default values and it adds value to users without increasing
code complexity too much, we can change behavior to be more friendly.
Hopefully, if we change behavior only for defaults, nobody will complain.

I will post the 'strict' implementation of 'strict'. You probably have time
until 4.21 (?) to propose several ideas for implementing 'relaxed strict'

Thanks,
Amir.

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Wed, Oct 31, 2018 at 10:24:08PM +0200, Amir Goldstein wrote:
> On Wed, Oct 31, 2018 at 9:56 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Oct 31, 2018 at 03:23:41PM -0400, Vivek Goyal wrote:
> > > On Wed, Oct 31, 2018 at 08:37:04PM +0200, Amir Goldstein wrote:
> > > > On Wed, Oct 31, 2018 at 6:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > ...
> > > > > >
> > > > > > 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.
> > > > >
> > > >
> > > > 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).
> > >
> > > >
> > > > 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.
> > >
> > > >
> > > > 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.
> 
> > 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.
> 
> > 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 ;-)
> 
> > 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).
> 
> > 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).

This is a new requirement. And I am fine if you need it.

But here is an interesting side affect of automatically enabling 
strict based on metacopy=on.

- Assume, a kernel has been compiled with CONFIG_OVERLAY_FS_NFS_EXPORT=y
  and CONFIG_OVERLAY_FS_STRICT=n.

- A user mounts using nfs_export=off and it works.

- Now user mounts with "nfs_export=off,metacopy=on", it does not work. 
  Because metacopy will implicitly enable strict=on and that will
  deny disabling nfs_export from mount option. Only way to take care
  of this is "nfs_export=off,metacopy=on,strict=off". This is atleast
  unintuitive. 

  Now say a user wants to use metacopy but also retain all the new
  behavior of not disabling index, not ignoreing d_type check and
  not ignoring RENAME_WHITEOUT, that's not possible for a system
  built with CONFIG_OVERLAY_FS_NFS_EXPORT=y.

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".

Thanks
Vivek

Re: [PATCH v2] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

> 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).
>

I managed to confuse you. 'strict' doesn't mean that you cannot
change defaults with mount options at all.
It simply means that every instance of pr_warn("...falling back to")
is going to error instead of disabling a feature whose requirements
are not met.

Better wait for patches it will be clearer to read the code than my
emails...

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

I have already discussed this with Zhangyi in the context of "overlay
format v2".
There is not backward compat issue if both upper/work dirs are empty.

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

The check for empty upper & work (including index) dirs should be done after
the cleanup of 'work' dir on mount.

Thanks,
Amir.