[PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[Vivek Goyal]

Assume a user wanted to enable metadata only copy-up and passes 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.

Reported-by: Daniel Walsh <dwalsh@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/super.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/super.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/super.c	2018-10-26 09:07:46.474353806 -0400
+++ rhvgoyal-linux/fs/overlayfs/super.c	2018-10-26 09:28:23.195422393 -0400
@@ -574,11 +574,11 @@ static int ovl_parse_opt(char *opt, stru
 
 	/* 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;
+		pr_err("overlayfs: metadata only copy up requires \"redirect_dir=on\".\n");
+		return -EINVAL;
 	} 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;
+		pr_err("overlayfs: metadata only copy up requires either \"redirect_dir=follow\" or \"redirect_dir=on\" on non-upper mount.\n");
+		return -EINVAL;
 	}
 
 	return 0;

Re: [PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[Miklos Szeredi]

On Fri, Oct 26, 2018 at 3:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Assume a user wanted to enable metadata only copy-up and passes 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).

What's the reason we can't enable these for metacopy=on automatically?

I mean, the simplest and least surprising behavior would be if
metacopy would simply imply redirect_dir, no?

Thanks,
Miklos

Re: [PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[Vivek Goyal]

On Fri, Oct 26, 2018 at 04:10:27PM +0200, Miklos Szeredi wrote:
> On Fri, Oct 26, 2018 at 3:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Assume a user wanted to enable metadata only copy-up and passes 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).
> 
> What's the reason we can't enable these for metacopy=on automatically?
> 
> I mean, the simplest and least surprising behavior would be if
> metacopy would simply imply redirect_dir, no?

redirect_dir=on is a significant change. And it might be better if
users passes it in (instead of enabling it automatically).

For example, when redirect_dir is enabled, container runtime software
(docker, podman), their logic to generate image does not work and
they have to fall back to *slower* method of diff.

https://github.com/moby/moby/pull/34342

IOW, enabling redirect_dir automatically has performance implications
for container image building process. Given that, it might be better
to fail and let user conciously make that choise and in the process
understand that there can be downsides to enabling metacopy.

But I am not very particular about it. Automatically enabling redirect_dir
sounds better than disabling metacopy automatically.

Thanks
Vivek

Re: [PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[Vivek Goyal]

On Fri, Oct 26, 2018 at 10:16:40AM -0400, Vivek Goyal wrote:
> On Fri, Oct 26, 2018 at 04:10:27PM +0200, Miklos Szeredi wrote:
> > On Fri, Oct 26, 2018 at 3:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > Assume a user wanted to enable metadata only copy-up and passes 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).
> > 
> > What's the reason we can't enable these for metacopy=on automatically?
> > 
> > I mean, the simplest and least surprising behavior would be if
> > metacopy would simply imply redirect_dir, no?
> 
> redirect_dir=on is a significant change. And it might be better if
> users passes it in (instead of enabling it automatically).
> 
> For example, when redirect_dir is enabled, container runtime software
> (docker, podman), their logic to generate image does not work and
> they have to fall back to *slower* method of diff.
> 
> https://github.com/moby/moby/pull/34342

Having said that, metacopy=on will have same issue and user need to
fall back to slower naivediff interface for image generation.

So enabling redirect_dir automatically probably is good. We could
just drop a message in logs that it has been enabled automatically.

IOW, I can go either way depending on how others feel about it.

Thanks
Vivek

Re: [PATCH] overlayfs: Return error if metadata only copy-up can't be enabled

[Daniel Walsh]

On 10/26/18 10:28 AM, Vivek Goyal wrote:
> On Fri, Oct 26, 2018 at 10:16:40AM -0400, Vivek Goyal wrote:
>> On Fri, Oct 26, 2018 at 04:10:27PM +0200, Miklos Szeredi wrote:
>>> On Fri, Oct 26, 2018 at 3:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>> Assume a user wanted to enable metadata only copy-up and passes 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).
>>> What's the reason we can't enable these for metacopy=on automatically?
>>>
>>> I mean, the simplest and least surprising behavior would be if
>>> metacopy would simply imply redirect_dir, no?
>> redirect_dir=on is a significant change. And it might be better if
>> users passes it in (instead of enabling it automatically).
>>
>> For example, when redirect_dir is enabled, container runtime software
>> (docker, podman), their logic to generate image does not work and
>> they have to fall back to *slower* method of diff.
>>
>> https://github.com/moby/moby/pull/34342
> Having said that, metacopy=on will have same issue and user need to
> fall back to slower naivediff interface for image generation.
>
> So enabling redirect_dir automatically probably is good. We could
> just drop a message in logs that it has been enabled automatically.
>
> IOW, I can go either way depending on how others feel about it.
>
> Thanks
> Vivek

I think turning them both on and DOCUMENTING the behavior would be good.