On Mon, May 3, 2021 at 9:43 PM Vyacheslav Yurkov
<Vyacheslav.Yurkov@xxxxxxxxxx> 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@xxxxxxxxxx>
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
>
