* [PATCH] ovl: do not set overlay.opaque for new directories @ 2021-05-03 18:42 Vyacheslav Yurkov 2021-05-25 8:31 ` Amir Goldstein 0 siblings, 1 reply; 3+ messages in thread From: Vyacheslav Yurkov @ 2021-05-03 18:42 UTC (permalink / raw) To: Miklos Szeredi, linux-unionfs Cc: linux-kernel, Amir Goldstein, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ovl: do not set overlay.opaque for new directories 2021-05-03 18:42 [PATCH] ovl: do not set overlay.opaque for new directories Vyacheslav Yurkov @ 2021-05-25 8:31 ` Amir Goldstein 2021-05-27 12:18 ` Yurkov, Vyacheslav 0 siblings, 1 reply; 3+ messages in thread From: Amir Goldstein @ 2021-05-25 8:31 UTC (permalink / raw) To: Vyacheslav Yurkov; +Cc: Miklos Szeredi, overlayfs 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] ovl: do not set overlay.opaque for new directories 2021-05-25 8:31 ` Amir Goldstein @ 2021-05-27 12:18 ` Yurkov, Vyacheslav 0 siblings, 0 replies; 3+ messages in thread From: Yurkov, Vyacheslav @ 2021-05-27 12:18 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs 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 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-27 12:26 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-03 18:42 [PATCH] ovl: do not set overlay.opaque for new directories Vyacheslav Yurkov 2021-05-25 8:31 ` Amir Goldstein 2021-05-27 12:18 ` Yurkov, Vyacheslav
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).