From: Vivek Goyal <vgoyal@redhat.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: overlayfs <linux-unionfs@vger.kernel.org>,
Miklos Szeredi <miklos@szeredi.hu>
Subject: Re: [PATCH 04/11] ovl: Provide a mount option metacopy=on/off for metadata copyup
Date: Wed, 18 Oct 2017 09:03:43 -0400 [thread overview]
Message-ID: <20171018130343.GC3445@redhat.com> (raw)
In-Reply-To: <CAOQ4uximnQT43BK3Ut82Vs0FMDi03cxAkyDK4LthkMiXHii-HQ@mail.gmail.com>
On Wed, Oct 18, 2017 at 07:31:51AM +0300, Amir Goldstein wrote:
> On Wed, Oct 18, 2017 at 12:05 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > By default metadata only copy up is disabled. Provide a mount option so
> > that users can choose one way or other.
> >
> > Also provide a kernel config and module option to enable/disable
> > metacopy feature.
> >
> > Like index feature, when overlay is mounted, on root upper directory we
> > set ORIGIN which points to lower. And at later mount time it is verified
> > again. This hopes to get the configuration right. But this does only so
> > much as we don't verify all the lowers. So it is possible that a lower is
> > missing and later data copy up fails.
>
> Like index feature, please error mount if ovl_inuse_trylock fails.
> As you know, this error is only conditional because of backward
> compatibility, so any new opt-in feature should enforce it.
Hi Amir,
I am not so sure about it. Avoiding leaking any mount point is really
really hard. And I don't think current container runtime have been
modified to make it fool proof.
IMHO, if we really want to enforce something like this, then we need
to have some sort of capability to find existing superblock and reuse it.
(Something like what happens with block devices).
I am afraid that if I start enforcing this, then this feature will not
be used at all because software has not been hardended enough to avoid
mount point leaks completely.
Vivek
>
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/overlayfs/Kconfig | 8 ++++++++
> > fs/overlayfs/ovl_entry.h | 1 +
> > fs/overlayfs/super.c | 38 +++++++++++++++++++++++++++++++++++---
> > 3 files changed, 44 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> > index cbfc196e5dc5..17a0b17ad14c 100644
> > --- a/fs/overlayfs/Kconfig
> > +++ b/fs/overlayfs/Kconfig
> > @@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
> > outcomes. However, mounting the same overlay with an old kernel
> > read-write and then mounting it again with a new kernel, will have
> > unexpected results.
> > +
> > +config OVERLAY_FS_METACOPY
> > + bool "Overlayfs: turn on metadata only copy up feature by default"
> > + depends on OVERLAY_FS
> > + help
> > + If this config option is enabled then overlay filesystems will
> > + copy up only metadata where appropriate and data copy up will
> > + happen when a file is opended for WRITE operation.
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index 25d9b5adcd42..6806f0b0fbc2 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -15,6 +15,7 @@ struct ovl_config {
> > bool default_permissions;
> > bool redirect_dir;
> > bool index;
> > + bool metacopy;
> > };
> >
> > /* private information held for overlayfs's superblock */
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 092d150643c1..32e3d4be1a71 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
> > MODULE_PARM_DESC(ovl_index_def,
> > "Default to on or off for the inodes index 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");
> > +
> > static void ovl_dentry_release(struct dentry *dentry)
> > {
> > struct ovl_entry *oe = dentry->d_fsdata;
> > @@ -303,6 +308,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> > if (ufs->config.index != ovl_index_def)
> > seq_printf(m, ",index=%s",
> > ufs->config.index ? "on" : "off");
> > + if (ufs->config.metacopy != ovl_metacopy_def)
> > + seq_printf(m, ",metacopy=%s",
> > + ufs->config.metacopy ? "on" : "off");
> > return 0;
> > }
> >
> > @@ -336,6 +344,8 @@ enum {
> > OPT_REDIRECT_DIR_OFF,
> > OPT_INDEX_ON,
> > OPT_INDEX_OFF,
> > + OPT_METACOPY_ON,
> > + OPT_METACOPY_OFF,
> > OPT_ERR,
> > };
> >
> > @@ -348,6 +358,8 @@ static const match_table_t ovl_tokens = {
> > {OPT_REDIRECT_DIR_OFF, "redirect_dir=off"},
> > {OPT_INDEX_ON, "index=on"},
> > {OPT_INDEX_OFF, "index=off"},
> > + {OPT_METACOPY_ON, "metacopy=on"},
> > + {OPT_METACOPY_OFF, "metacopy=off"},
> > {OPT_ERR, NULL}
> > };
> >
> > @@ -428,6 +440,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> > config->index = false;
> > break;
> >
> > + case OPT_METACOPY_ON:
> > + config->metacopy = true;
> > + break;
> > +
> > + case OPT_METACOPY_OFF:
> > + config->metacopy = false;
> > + break;
> > +
> > default:
> > pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> > return -EINVAL;
> > @@ -644,9 +664,16 @@ static int ovl_lower_dir(const char *name, struct path *path,
> > * The inodes index feature needs to encode and decode file
> > * handles, so it requires that all layers support them.
> > */
> > - if (ofs->config.index && !ovl_can_decode_fh(path->dentry->d_sb)) {
> > + if ((ofs->config.index || ofs->config.metacopy) &&
> > + !ovl_can_decode_fh(path->dentry->d_sb)) {
> > + if (ofs->config.index)
> > + pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
> > +
> > + if (ofs->config.metacopy)
> > + pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to metacopy=off.\n", name);
> > +
>
> In my verify_dir patches I used the following more compact warning
> style instead of granular warnings:
>
> pr_warn("overlayfs: fs on '%s' does not support file handles,
> falling back to index=off,metacopy=off.\n", name);
>
> It is a bit less informative, but IMO the result in nicer to look at,
> both in the code and in dmesg.
> Others may have a different opinion. Please consider.
> For example, see how the following warning from my verify_dir patches
> has expanded and imagine how
> much more messier it would be if we broke it into separate warnings:
>
> pr_warn("overlayfs: upper fs does not support xattr, falling back
> to redirect_dir=off, index=off, no verify_dir and no opaque dir.\n");
>
>
> > ofs->config.index = false;
> > - pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
> > + ofs->config.metacopy = false;
> > }
> >
> > return 0;
> > @@ -847,6 +874,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >
> > ufs->config.redirect_dir = ovl_redirect_dir_def;
> > ufs->config.index = ovl_index_def;
> > + ufs->config.metacopy = ovl_metacopy_def;
> > +
> > err = ovl_parse_opt((char *) data, &ufs->config);
> > if (err)
> > goto out_free_config;
> > @@ -1057,7 +1086,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> > else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
> > ufs->same_sb = NULL;
> >
> > - if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
> > + if (!(ovl_force_readonly(ufs)) &&
> > + (ufs->config.index || ufs->config.metacopy)) {
> > /* Verify lower root is upper root origin */
> > err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
> > stack[0].dentry, false, true);
> > @@ -1065,7 +1095,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> > pr_err("overlayfs: failed to verify upper root origin\n");
> > goto out_put_lower_mnt;
> > }
> > + }
> >
> > + if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
> > ufs->indexdir = ovl_workdir_create(sb, ufs, workpath.dentry,
> > OVL_INDEXDIR_NAME, true);
> > if (ufs->indexdir) {
> > --
> > 2.13.5
> >
next prev parent reply other threads:[~2017-10-18 13:03 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-17 21:05 [RFC PATCH 00/11][V4] overlayfs: Delayed copy up of data Vivek Goyal
2017-10-17 21:05 ` [PATCH 01/11] ovl: Create origin xattr on copy up for all files Vivek Goyal
2017-10-18 4:09 ` Amir Goldstein
2017-10-18 12:55 ` Vivek Goyal
2017-10-18 13:56 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 02/11] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check Vivek Goyal
2017-10-18 4:11 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 03/11] ovl: During copy up, first copy up metadata and then data Vivek Goyal
2017-10-18 4:13 ` Amir Goldstein
2017-10-18 4:39 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 04/11] ovl: Provide a mount option metacopy=on/off for metadata copyup Vivek Goyal
2017-10-18 4:31 ` Amir Goldstein
2017-10-18 13:03 ` Vivek Goyal [this message]
2017-10-18 14:09 ` Amir Goldstein
2017-10-18 14:26 ` Vivek Goyal
2017-10-18 14:38 ` Amir Goldstein
2017-10-18 14:10 ` Vivek Goyal
2017-10-18 14:26 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 05/11] ovl: Copy up only metadata during copy up where it makes sense Vivek Goyal
2017-10-18 4:46 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 06/11] ovl: Set xattr OVL_XATTR_METACOPY on upper file Vivek Goyal
2017-10-18 4:57 ` Amir Goldstein
2017-10-18 13:30 ` Vivek Goyal
2017-10-17 21:05 ` [PATCH 07/11] ovl: Fix ovl_getattr() to get number of blocks from lower Vivek Goyal
2017-10-18 5:01 ` Amir Goldstein
2017-10-18 13:39 ` Vivek Goyal
2017-10-17 21:05 ` [PATCH 08/11] ovl: Set OVL_METACOPY flag during ovl_lookup() Vivek Goyal
2017-10-18 5:06 ` Amir Goldstein
2017-10-18 13:53 ` Vivek Goyal
2017-10-17 21:05 ` [PATCH 09/11] ovl: Return lower dentry if only metadata copy up took place Vivek Goyal
2017-10-18 5:07 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 10/11] ovl: Introduce read/write barriers around metacopy flag update Vivek Goyal
2017-10-18 5:19 ` Amir Goldstein
2017-10-18 15:32 ` Vivek Goyal
2017-10-18 16:05 ` Amir Goldstein
2017-10-17 21:05 ` [PATCH 11/11] ovl: Put barriers to order oi->__upperdentry and OVL_METACOPY update Vivek Goyal
2017-10-18 5:40 ` Amir Goldstein
2017-10-19 13:00 ` Vivek Goyal
2017-10-19 13:21 ` Amir Goldstein
2017-10-19 14:58 ` Vivek Goyal
2017-10-19 15:08 ` Amir Goldstein
2017-10-19 15:22 ` Vivek Goyal
2017-10-19 15:39 ` Amir Goldstein
2017-10-19 15:59 ` Vivek Goyal
2017-10-19 16:33 ` Amir Goldstein
2017-10-19 20:33 ` Vivek Goyal
2017-10-20 4:09 ` Amir Goldstein
2017-10-20 15:41 ` Vivek Goyal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171018130343.GC3445@redhat.com \
--to=vgoyal@redhat.com \
--cc=amir73il@gmail.com \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.