[PATCH] ovl: do not set overlay.opaque for new directories

[PATCH] ovl: do not set overlay.opaque for new directories

[Vyacheslav Yurkov]

This optimization breaks existing use case when a lower layer directory
appears after directory was created on a merged layer. If overlay.opaque
is applied, new files on lower layer are not visible.

Consider the following scenario:
- /lower and /upper are mounted to /merged
- directory /merged/new-dir is created with a file test1
- overlay is unmounted
- directory /lower/new-dir is created with a file test2
- overlay is mounted again

If opaque is applied by default, file test2 is not going to be visible
without explicitly clearing the overlay.opaque attribute

Signed-off-by: Vyacheslav Yurkov <Vyacheslav.Yurkov@bruker.com>
---
 fs/overlayfs/dir.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 93efe7048a77..f66f96dd9f0c 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -338,11 +338,6 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(newdentry))
 		goto out_unlock;
 
-	if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) {
-		/* Setting opaque here is just an optimization, allow to fail */
-		ovl_set_opaque(dentry, newdentry);
-	}
-
 	err = ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
 	if (err)
 		goto out_cleanup;
-- 
2.25.1

Re: [PATCH] ovl: do not set overlay.opaque for new directories

[Amir Goldstein]

On Mon, May 3, 2021 at 9:43 PM Vyacheslav Yurkov
<Vyacheslav.Yurkov@bruker.com> wrote:
>
> This optimization breaks existing use case when a lower layer directory
> appears after directory was created on a merged layer. If overlay.opaque
> is applied, new files on lower layer are not visible.
>
> Consider the following scenario:
> - /lower and /upper are mounted to /merged
> - directory /merged/new-dir is created with a file test1
> - overlay is unmounted
> - directory /lower/new-dir is created with a file test2
> - overlay is mounted again
>
> If opaque is applied by default, file test2 is not going to be visible
> without explicitly clearing the overlay.opaque attribute
>
> Signed-off-by: Vyacheslav Yurkov <Vyacheslav.Yurkov@bruker.com>

Hi Vyacheslav,

Sorry for the late reply.
Is the described regression really happening in real deployments?
I would like to avoid removing the optimization if possible.

In any case, if we have to support existing deployments that use this practice,
the optimization should be removed only for the case where the user did not
opt-in to any of the new features, quoting overlayfs.rst:
'
  Offline changes to the lower tree are only allowed if the
  "metadata only copy up", "inode index", "xino" and "redirect_dir" features
  have not been used. If the lower tree is modified and any of these
  features has been used, the behavior of the overlay is undefined,
  though it will not result in a crash or deadlock.
'

This means putting this check from ovl_lower_uuid_ok() into a helper:

static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
{
        /*
         * To avoid regressions in existing setups with overlay lower offline
         * changes, we allow lower changes only if none of the new features
         * are used.
         */
        return (!ofs->config.index && !ofs->config.metacopy &&
                    !ofs>config.redirect_dir && ofs->config.xino !=
OVL_XINO_ON);
}

Note that ovl_lower_uuid_ok() does not currently check the redirect_dir
feature, but it would be better to use the same helper in that case as well.

Thanks,
Amir.

> ---
>  fs/overlayfs/dir.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 93efe7048a77..f66f96dd9f0c 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -338,11 +338,6 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         if (IS_ERR(newdentry))
>                 goto out_unlock;
>
> -       if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) {
> -               /* Setting opaque here is just an optimization, allow to fail */
> -               ovl_set_opaque(dentry, newdentry);
> -       }
> -
>         err = ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
>         if (err)
>                 goto out_cleanup;
> --
> 2.25.1
>

RE: [PATCH] ovl: do not set overlay.opaque for new directories

[Yurkov, Vyacheslav]

Hi Amir,
Thanks for the follow-up.

Yes, this is existing problem I solved with a proposed patch on our devices. Of course having a proper solution would be better.
The only option I see enabled in my config is CONFIG_OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW.

Took me some time to backport proposed changes to my kernel, but they worked fine. Will send a v2 shortly

The


- confidential -

> -----Original Message-----
> From: Amir Goldstein <amir73il@gmail.com>
> Sent: Tuesday, May 25, 2021 10:32
> To: Yurkov, Vyacheslav <Vyacheslav.Yurkov@bruker.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>; overlayfs <linux-
> unionfs@vger.kernel.org>
> Subject: Re: [PATCH] ovl: do not set overlay.opaque for new directories
> 
> **EXTERNAL EMAIL**
> 
> On Mon, May 3, 2021 at 9:43 PM Vyacheslav Yurkov
> <Vyacheslav.Yurkov@bruker.com> wrote:
> >
> > This optimization breaks existing use case when a lower layer directory
> > appears after directory was created on a merged layer. If overlay.opaque
> > is applied, new files on lower layer are not visible.
> >
> > Consider the following scenario:
> > - /lower and /upper are mounted to /merged
> > - directory /merged/new-dir is created with a file test1
> > - overlay is unmounted
> > - directory /lower/new-dir is created with a file test2
> > - overlay is mounted again
> >
> > If opaque is applied by default, file test2 is not going to be visible
> > without explicitly clearing the overlay.opaque attribute
> >
> > Signed-off-by: Vyacheslav Yurkov <Vyacheslav.Yurkov@bruker.com>
> 
> Hi Vyacheslav,
> 
> Sorry for the late reply.
> Is the described regression really happening in real deployments?
> I would like to avoid removing the optimization if possible.
> 
> In any case, if we have to support existing deployments that use this
> practice,
> the optimization should be removed only for the case where the user did not
> opt-in to any of the new features, quoting overlayfs.rst:
> '
>   Offline changes to the lower tree are only allowed if the
>   "metadata only copy up", "inode index", "xino" and "redirect_dir" features
>   have not been used. If the lower tree is modified and any of these
>   features has been used, the behavior of the overlay is undefined,
>   though it will not result in a crash or deadlock.
> '
> 
> This means putting this check from ovl_lower_uuid_ok() into a helper:
> 
> static inline bool ovl_allow_offline_changes(struct ovl_fs *ofs)
> {
>         /*
>          * To avoid regressions in existing setups with overlay lower offline
>          * changes, we allow lower changes only if none of the new features
>          * are used.
>          */
>         return (!ofs->config.index && !ofs->config.metacopy &&
>                     !ofs>config.redirect_dir && ofs->config.xino !=
> OVL_XINO_ON);
> }
> 
> Note that ovl_lower_uuid_ok() does not currently check the redirect_dir
> feature, but it would be better to use the same helper in that case as well.
> 
> Thanks,
> Amir.
> 
> > ---
> >  fs/overlayfs/dir.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 93efe7048a77..f66f96dd9f0c 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -338,11 +338,6 @@ static int ovl_create_upper(struct dentry *dentry,
> struct inode *inode,
> >         if (IS_ERR(newdentry))
> >                 goto out_unlock;
> >
> > -       if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) {
> > -               /* Setting opaque here is just an optimization, allow to fail */
> > -               ovl_set_opaque(dentry, newdentry);
> > -       }
> > -
> >         err = ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
> >         if (err)
> >                 goto out_cleanup;
> > --
> > 2.25.1
> >