[RFC PATCH v6 00/15] overlayfs: Delayed copy up of data

[RFC PATCH v6 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

Hi,

Here is the V6 of patches. I have taken care of most of the comments
from previous iteration and also rebased patches to overlayfs-next
branch of miklos's tree.

These patches are still in RFC stage and very lightly tested. First
I want to make sure that I have addressed all design concerns. Once that
is done, will do more extensive testing.

Following are changes from V5.

- Rebased on top of overlafs-next branch
- Fixed races in ovl_getattr() w.r.t metacopy only file.
- Fixed issues related to encryped and compressed files in ovl_getattr() 
- Added bunch of helper functions so that code is little more clean.
- Miscellaneous code cleanup as suggested by Amir.

Amir, I am still setting UPPERDATA only if file needs it. I am really
not convinced that we should do it for all files. Code is not that
much extra. Please have another look and see how do you feel about it
now.

Vivek

Vivek Goyal (15):
  ovl: Create origin xattr on copy up for all files
  ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check
  ovl: Provide a mount option metacopy=on/off for metadata copyup
  ovl: During copy up, first copy up metadata and then data
  ovl: Copy up only metadata during copy up where it makes sense
  ovl: Add helper ovl_already_copied_up()
  ovl: A new xattr OVL_XATTR_METACOPY for file on upper
  ovl: Fix ovl_getattr() to get number of blocks from lower
  ovl: Set OVL_UPPERDATA flag during ovl_lookup()
  ovl: Return lower dentry if only metadata copy up took place
  ovl: Do not expose metacopy only upper dentry
  ovl: Fix encryption status of a metacopy only file
  ovl: Fix compression status of a metacopy only file
  ovl: Introduce read/write barriers around metacopy flag update
  ovl: Enable metadata only feature

 fs/overlayfs/Kconfig     |   8 ++++
 fs/overlayfs/copy_up.c   | 115 +++++++++++++++++++++++++++++------------------
 fs/overlayfs/inode.c     |  35 +++++++++++++--
 fs/overlayfs/namei.c     |  40 +++++++++++++++++
 fs/overlayfs/overlayfs.h |   9 +++-
 fs/overlayfs/ovl_entry.h |   1 +
 fs/overlayfs/super.c     |  61 ++++++++++++++++++++++---
 fs/overlayfs/util.c      | 107 ++++++++++++++++++++++++++++++++++++++-----
 8 files changed, 311 insertions(+), 65 deletions(-)

-- 
2.13.6

[PATCH v6 01/15] ovl: Create origin xattr on copy up for all files

[Vivek Goyal]

Right now my understanding is that origin xattr is created for all copied
up files if index=on. And if index=off, then we create it for all type
of files except hardlinks (nlink != 1).

With metadata only copy up, I will still require origin xattr to copy up
data later, so create it even for hardlinks even with index=off.

Given ->origin is always true now, get rid of this field from
"struct ovl_copy_up_ctx".

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index eb3b8d39fb61..f55bece3688e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -326,7 +326,6 @@ struct ovl_copy_up_ctx {
 	struct qstr destname;
 	struct dentry *workdir;
 	bool tmpfile;
-	bool origin;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -469,15 +468,10 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	/*
 	 * Store identifier of lower inode in upper inode xattr to
 	 * allow lookup of the copy up origin inode.
-	 *
-	 * Don't set origin when we are breaking the association with a lower
-	 * hard link.
 	 */
-	if (c->origin) {
-		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-		if (err)
-			return err;
-	}
+	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -542,9 +536,6 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 	    c->stat.nlink > 1)
 		indexed = true;
 
-	if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || indexed)
-		c->origin = true;
-
 	if (indexed) {
 		c->destdir = ovl_indexdir(c->dentry->d_sb);
 		err = ovl_get_index_name(c->lowerpath.dentry, &c->destname);
-- 
2.13.6

Re: [PATCH v6 01/15] ovl: Create origin xattr on copy up for all files

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now my understanding is that origin xattr is created for all copied
> up files if index=on. And if index=off, then we create it for all type
> of files except hardlinks (nlink != 1).
>
> With metadata only copy up, I will still require origin xattr to copy up
> data later, so create it even for hardlinks even with index=off.
>
> Given ->origin is always true now, get rid of this field from
> "struct ovl_copy_up_ctx".
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

[PATCH v6 02/15] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check

[Vivek Goyal]

At mount time we check if upper supports xattr or not and set ofs->noxattr
field accordingly. But in ovl_check_setxattr() still seems to have logic
which expects that ovl_do_setxattr() can return -EOPNOTSUPP. Not sure when
and how can we hit this code. Feels redundant to me. So I am removing
this code.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/util.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index d6bb1c9f5e7a..77dc00438d54 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -365,21 +365,12 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 		       const char *name, const void *value, size_t size,
 		       int xerr)
 {
-	int err;
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
 	if (ofs->noxattr)
 		return xerr;
 
-	err = ovl_do_setxattr(upperdentry, name, value, size, 0);
-
-	if (err == -EOPNOTSUPP) {
-		pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
-		ofs->noxattr = true;
-		return xerr;
-	}
-
-	return err;
+	return ovl_do_setxattr(upperdentry, name, value, size, 0);
 }
 
 int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
-- 
2.13.6

[PATCH v6 03/15] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

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, we verify on mount that upper root is not being
reused with a different lower root. This hopes to get the configuration
right and detect the copied layers use case. 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.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/Kconfig     |  8 ++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 42 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 49 insertions(+), 2 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 93eb6a044dd2..2720ed21ad53 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;
 };
 
 struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index a8f7b26c65c0..84b8f6dc0f61 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -46,6 +46,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
 		dput(oe->lowerstack[i].dentry);
 }
 
+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;
@@ -319,6 +324,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s",
 			   ofs->config.index ? "on" : "off");
+	if (ofs->config.metacopy != ovl_metacopy_def)
+		seq_printf(m, ",metacopy=%s",
+			   ofs->config.metacopy ? "on" : "off");
 	return 0;
 }
 
@@ -352,6 +360,8 @@ enum {
 	OPT_REDIRECT_DIR_OFF,
 	OPT_INDEX_ON,
 	OPT_INDEX_OFF,
+	OPT_METACOPY_ON,
+	OPT_METACOPY_OFF,
 	OPT_ERR,
 };
 
@@ -364,6 +374,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}
 };
 
@@ -444,6 +456,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;
@@ -657,9 +677,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);
+
 		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;
@@ -1162,6 +1189,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	ofs->config.redirect_dir = ovl_redirect_dir_def;
 	ofs->config.index = ovl_index_def;
+	ofs->config.metacopy = ovl_metacopy_def;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
@@ -1207,6 +1235,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
 		ofs->same_sb = NULL;
 
+	if (!(ovl_force_readonly(ofs)) && ofs->config.metacopy) {
+		/* Verify lower root is upper root origin */
+		err = ovl_verify_origin(upperpath.dentry,
+					oe->lowerstack[0].dentry, false, true);
+		if (err) {
+			pr_err("overlayfs: failed to verify upper root origin\n");
+                        goto out_free_oe;
+		}
+	}
+
 	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
 		err = ovl_get_indexdir(ofs, oe, &upperpath);
 		if (err)
-- 
2.13.6

Re: [PATCH v6 03/15] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, 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, we verify on mount that upper root is not being
> reused with a different lower root. This hopes to get the configuration
> right and detect the copied layers use case. 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.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Fine, as long as Miklos agrees not to enforce exclusive dir lock,
it's fine by me.

Please take a look at my ovl-features patches and say what you think.
It is time we started to enforce some order with all the inter-dependencies
between features.
With my proposal, it will make mounting an overlay with metacopy object
on older kernel slightly harder (but not impossible), so it can help admins
from tripping over themselves.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>


[...]

> -       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);
> +
>                 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;
>         }
>

So you didn't like the less granular but shorter warning I used in my
verify_dir patches, e.g.:
                 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;
+               pr_warn("overlayfs: fs on '%s' does not support file
handles, falling back to index=off,metacopy=off.\n", name);

Doesn't matter much to me,

Amir.

Re: [PATCH v6 03/15] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Fri, Nov 10, 2017 at 9:07 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Nov 9, 2017 at 10:50 PM, 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, we verify on mount that upper root is not being
>> reused with a different lower root. This hopes to get the configuration
>> right and detect the copied layers use case. 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.
>>
>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>
> Fine, as long as Miklos agrees not to enforce exclusive dir lock,
> it's fine by me.
>
> Please take a look at my ovl-features patches and say what you think.
> It is time we started to enforce some order with all the inter-dependencies
> between features.
> With my proposal, it will make mounting an overlay with metacopy object
> on older kernel slightly harder (but not impossible), so it can help admins
> from tripping over themselves.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>

On second though, hold that Reviewed-by, there is one more thing
that this patch should check, see path:
"ovl: disable redirect_dir and index when no xattr support"
from my verify_dir patches:
https://github.com/amir73il/linux/commits/ovl-verify-dir

You may cherry-pick it at beginning of your series and then this
patch should also disable metacopy on mount with noxattr with a warning to user,
instead of silently not doing the metacopies, but claiming in mount options that
metacopy is on.

Thanks,
Amir.

Re: [PATCH v6 03/15] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Fri, Nov 10, 2017 at 09:14:49AM +0200, Amir Goldstein wrote:
> On Fri, Nov 10, 2017 at 9:07 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Thu, Nov 9, 2017 at 10:50 PM, 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, we verify on mount that upper root is not being
> >> reused with a different lower root. This hopes to get the configuration
> >> right and detect the copied layers use case. 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.
> >>
> >> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >
> > Fine, as long as Miklos agrees not to enforce exclusive dir lock,
> > it's fine by me.
> >
> > Please take a look at my ovl-features patches and say what you think.
> > It is time we started to enforce some order with all the inter-dependencies
> > between features.
> > With my proposal, it will make mounting an overlay with metacopy object
> > on older kernel slightly harder (but not impossible), so it can help admins
> > from tripping over themselves.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> 
> On second though, hold that Reviewed-by, there is one more thing
> that this patch should check, see path:
> "ovl: disable redirect_dir and index when no xattr support"
> from my verify_dir patches:
> https://github.com/amir73il/linux/commits/ovl-verify-dir
> 
> You may cherry-pick it at beginning of your series and then this
> patch should also disable metacopy on mount with noxattr with a warning to user,
> instead of silently not doing the metacopies, but claiming in mount options that
> metacopy is on.

Makes sense. Will do.

Vivek

Re: [PATCH v6 03/15] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Fri, Nov 10, 2017 at 09:07:04AM +0200, Amir Goldstein wrote:
> On Thu, Nov 9, 2017 at 10:50 PM, 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, we verify on mount that upper root is not being
> > reused with a different lower root. This hopes to get the configuration
> > right and detect the copied layers use case. 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.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Fine, as long as Miklos agrees not to enforce exclusive dir lock,
> it's fine by me.
> 
> Please take a look at my ovl-features patches and say what you think.
> It is time we started to enforce some order with all the inter-dependencies
> between features.
> With my proposal, it will make mounting an overlay with metacopy object
> on older kernel slightly harder (but not impossible), so it can help admins
> from tripping over themselves.

I am trying to understand all that discussion but I realiaze that metacopy
feature will have issues if mounted with older kernel. Some of the files
will be metacopy only and old kernel will think that these files have
been truncated.

Turning metacopy=off with new kernel should be fine though. This just
means that new copy up operations will not be metacopy only and files
which are metacopy only will be copied up when opened for WRITE.

So we need some mechanism where old kernel can detect a new incomatible
feature in fs metadata and either refuse to mount.

Right now we don't seem to have any mechanism to do that in overlayfs.
Having something like that will make it harder to make mistakes.


[..]
> > -       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);
> > +
> >                 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;
> >         }
> >
> 
> So you didn't like the less granular but shorter warning I used in my
> verify_dir patches, e.g.:
>                  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;
> +               pr_warn("overlayfs: fs on '%s' does not support file
> handles, falling back to index=off,metacopy=off.\n", name);
> 
> Doesn't matter much to me,

Actually, I do like shorter warning with single line. I think I just forgot
to make this change. I will do it in V7.

Vivek

[PATCH v6 04/15] ovl: During copy up, first copy up metadata and then data

[Vivek Goyal]

Just a little re-ordering of code. This helps with next patch where after
copying up metadata, we skip data copying step, if needed.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f55bece3688e..303794bb9127 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -443,6 +443,19 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
 
+	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
+	if (err)
+		return err;
+
+	/*
+	 * Store identifier of lower inode in upper inode xattr to
+	 * allow lookup of the copy up origin inode.
+	 *
+	 */
+	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+	if (err)
+		return err;
+
 	if (S_ISREG(c->stat.mode)) {
 		struct path upperpath;
 
@@ -455,25 +468,11 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
-	if (err)
-		return err;
-
 	inode_lock(temp->d_inode);
 	err = ovl_set_attr(temp, &c->stat);
 	inode_unlock(temp->d_inode);
-	if (err)
-		return err;
-
-	/*
-	 * Store identifier of lower inode in upper inode xattr to
-	 * allow lookup of the copy up origin inode.
-	 */
-	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-	if (err)
-		return err;
 
-	return 0;
+	return err;
 }
 
 static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
-- 
2.13.6

[PATCH v6 05/15] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

If it makes sense to copy up only metadata during copy up, do it. This
is done for regular files which are not opened for WRITE and have origin
being saved.

Right now ->metacopy is set to 0 always. Last patch in the series will
remove the hard coded statement and enable metacopy feature.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 18 +++++++++++-------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 22 ++++++++++++++++++++++
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 303794bb9127..6bb6b1723564 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -305,11 +305,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 			return PTR_ERR(fh);
 	}
 
-	/*
-	 * Do not fail when upper doesn't support xattrs.
-	 */
 	err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
-				 fh ? fh->len : 0, 0);
+				 fh ? fh->len : 0, -EOPNOTSUPP);
 	kfree(fh);
 
 	return err;
@@ -326,6 +323,7 @@ struct ovl_copy_up_ctx {
 	struct qstr destname;
 	struct dentry *workdir;
 	bool tmpfile;
+	bool metacopy;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -453,10 +451,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 *
 	 */
 	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-	if (err)
-		return err;
+	if (err) {
+		if (err != -EOPNOTSUPP)
+			return err;
+		/* Upper does not support xattr. Copy up data as well */
+		c->metacopy = false;
+	}
 
-	if (S_ISREG(c->stat.mode)) {
+	if (S_ISREG(c->stat.mode) && !c->metacopy) {
 		struct path upperpath;
 
 		ovl_path_upper(c->dentry, &upperpath);
@@ -601,6 +603,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	if (err)
 		return err;
 
+	ctx.metacopy = ovl_metaonly_copy_up(dentry, ctx.stat.mode, flags);
+
 	ovl_path_upper(parent, &parentpath);
 	ctx.destdir = parentpath.dentry;
 	ctx.destname = dentry->d_name;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 13eab09a6b6f..9e5aaa4a5db2 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -215,6 +215,7 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
 bool ovl_dentry_has_upper_alias(struct dentry *dentry);
 void ovl_dentry_set_upper_alias(struct dentry *dentry);
+bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags);
 bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 77dc00438d54..c5b62546c613 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -231,6 +231,28 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
 	oe->has_upper = true;
 }
 
+bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+
+	/* TODO: Will enable metacopy in last patch of series */
+	return false;
+
+	if (!ofs->config.metacopy)
+		return false;
+
+	if (!S_ISREG(mode))
+		return false;
+
+	if (flags && (flags & O_TRUNC))
+		return false;
+
+	if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))
+		return false;
+
+	return true;
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
-- 
2.13.6

Re: [PATCH v6 05/15] ovl: Copy up only metadata during copy up where it makes sense

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If it makes sense to copy up only metadata during copy up, do it. This
> is done for regular files which are not opened for WRITE and have origin
> being saved.
>
> Right now ->metacopy is set to 0 always. Last patch in the series will
> remove the hard coded statement and enable metacopy feature.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/util.c      | 22 ++++++++++++++++++++++
>  3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 303794bb9127..6bb6b1723564 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -305,11 +305,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>                         return PTR_ERR(fh);
>         }
>
> -       /*
> -        * Do not fail when upper doesn't support xattrs.
> -        */
>         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> -                                fh ? fh->len : 0, 0);
> +                                fh ? fh->len : 0, -EOPNOTSUPP);
>         kfree(fh);
>
>         return err;
> @@ -326,6 +323,7 @@ struct ovl_copy_up_ctx {
>         struct qstr destname;
>         struct dentry *workdir;
>         bool tmpfile;
> +       bool metacopy;
>  };
>
>  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> @@ -453,10 +451,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>          *
>          */
>         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
> -       if (err)
> -               return err;
> +       if (err) {
> +               if (err != -EOPNOTSUPP)
> +                       return err;
> +               /* Upper does not support xattr. Copy up data as well */
> +               c->metacopy = false;
> +       }
>
> -       if (S_ISREG(c->stat.mode)) {
> +       if (S_ISREG(c->stat.mode) && !c->metacopy) {
>                 struct path upperpath;
>
>                 ovl_path_upper(c->dentry, &upperpath);
> @@ -601,6 +603,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>         if (err)
>                 return err;
>
> +       ctx.metacopy = ovl_metaonly_copy_up(dentry, ctx.stat.mode, flags);
> +
>         ovl_path_upper(parent, &parentpath);
>         ctx.destdir = parentpath.dentry;
>         ctx.destname = dentry->d_name;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 13eab09a6b6f..9e5aaa4a5db2 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -215,6 +215,7 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
>  void ovl_dentry_set_opaque(struct dentry *dentry);
>  bool ovl_dentry_has_upper_alias(struct dentry *dentry);
>  void ovl_dentry_set_upper_alias(struct dentry *dentry);
> +bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags);
>  bool ovl_redirect_dir(struct super_block *sb);
>  const char *ovl_dentry_get_redirect(struct dentry *dentry);
>  void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 77dc00438d54..c5b62546c613 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -231,6 +231,28 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
>         oe->has_upper = true;
>  }
>
> +bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags)

You picked a strange place to insert this function.
For no better reason, please put it at the end of the file.

The name sounds like an action to me, please use something like:
ovl_need_meta_copy_up(), to match
ovl_open_need_copy_up()
and I think it would be cleaner if you first moved ovl_open_maybe_copy_up()
and ovl_open_need_copy_up() into copy_up.c
and then keep this new helper static near those two.


> +{
> +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> +
> +       /* TODO: Will enable metacopy in last patch of series */
> +       return false;
> +
> +       if (!ofs->config.metacopy)
> +               return false;
> +
> +       if (!S_ISREG(mode))
> +               return false;
> +
> +       if (flags && (flags & O_TRUNC))
> +               return false;
> +
> +       if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))
> +               return false;
> +

if (flags &&

seems a bit redundant in two above cases..
It's probably better to make an inline helper for the write flags
test in ovl_open_need_copy_up() and use it here as well or
just copy the one line test as it is.

Amir.

[PATCH v6 06/15] ovl: Add helper ovl_already_copied_up()

[Vivek Goyal]

There are couple of places where we need to know if file is already copied
up (in lockless manner). Right now its open coded and there are only
two conditions to check. Soon this patch series will introduce another
condition to check and Amir wants to introduce one more. So introduce
a helper instead to check this so that code is easier to read.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 16 +---------------
 fs/overlayfs/inode.c     |  3 +--
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 24 +++++++++++++++++++++++-
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 6bb6b1723564..23122bdf0b14 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -651,21 +651,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent;
 
-		/*
-		 * Check if copy-up has happened as well as for upper alias (in
-		 * case of hard links) is there.
-		 *
-		 * Both checks are lockless:
-		 *  - false negatives: will recheck under oi->lock
-		 *  - false positives:
-		 *    + ovl_dentry_upper() uses memory barriers to ensure the
-		 *      upper dentry is up-to-date
-		 *    + ovl_dentry_has_upper_alias() relies on locking of
-		 *      upper parent i_rwsem to prevent reordering copy-up
-		 *      with rename.
-		 */
-		if (ovl_dentry_upper(dentry) &&
-		    ovl_dentry_has_upper_alias(dentry))
+		if (ovl_already_copied_up(dentry))
 			break;
 
 		next = dget(dentry);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 00b6b294272a..537dc25f84c6 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -343,8 +343,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
-	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	if (ovl_already_copied_up(dentry))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9e5aaa4a5db2..a6fdc2f0da6d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -228,6 +228,7 @@ bool ovl_is_whiteout(struct dentry *dentry);
 struct file *ovl_path_open(struct path *path, int flags);
 int ovl_copy_up_start(struct dentry *dentry);
 void ovl_copy_up_end(struct dentry *dentry);
+bool ovl_already_copied_up(struct dentry *dentry);
 bool ovl_check_origin_xattr(struct dentry *dentry);
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index c5b62546c613..f4e3e011a097 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -342,7 +342,7 @@ int ovl_copy_up_start(struct dentry *dentry)
 	int err;
 
 	err = mutex_lock_interruptible(&oi->lock);
-	if (!err && ovl_dentry_has_upper_alias(dentry)) {
+	if (!err && ovl_already_copied_up(dentry)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
@@ -368,6 +368,28 @@ bool ovl_check_origin_xattr(struct dentry *dentry)
 	return false;
 }
 
+bool ovl_already_copied_up(struct dentry *dentry)
+{
+	/*
+	 * Check if copy-up has happened as well as for upper alias (in
+	 * case of hard links) is there.
+	 *
+	 * Both checks are lockless:
+	 *  - false negatives: will recheck under oi->lock
+	 *  - false positives:
+	 *    + ovl_dentry_upper() uses memory barriers to ensure the
+	 *      upper dentry is up-to-date
+	 *    + ovl_dentry_has_upper_alias() relies on locking of
+	 *      upper parent i_rwsem to prevent reordering copy-up
+	 *      with rename.
+	 */
+	if (ovl_dentry_upper(dentry) &&
+	    ovl_dentry_has_upper_alias(dentry))
+		return true;
+
+	return false;
+}
+
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
 {
 	int res;
-- 
2.13.6

Re: [PATCH v6 06/15] ovl: Add helper ovl_already_copied_up()

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> There are couple of places where we need to know if file is already copied
> up (in lockless manner). Right now its open coded and there are only
> two conditions to check. Soon this patch series will introduce another
> condition to check and Amir wants to introduce one more. So introduce
> a helper instead to check this so that code is easier to read.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

I like it!

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

[PATCH v6 07/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

Now we will have the capability to have upper inodes which might be only
metadata copy up and data is still on lower inode. So add a new xattr
OVL_XATTR_METACOPY to distinguish between two cases.

Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata
only and and data will be copied up later from lower origin.
So this xattr is set when a metadata copy takes place and cleared when
data copy takes place.

We also use a bit in ovl_inode->flags to cache OVL_UPPERDATA which reflects
whether ovl inode has data or not (as opposed to metadata only copy up).

Note, OVL_UPPERDATA is set only on those upper inodes which can possibly
be metacopy only inodes. For example, inode should be a regular file and
there needs to be a lower dentry.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 53 +++++++++++++++++++++++++++++++++++++++++++++---
 fs/overlayfs/inode.c     |  2 +-
 fs/overlayfs/overlayfs.h |  9 ++++++--
 fs/overlayfs/util.c      | 46 +++++++++++++++++++++++++++++++++++++----
 4 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 23122bdf0b14..977ece7d3055 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -195,6 +195,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	return error;
 }
 
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+	struct iattr attr = {
+		.ia_valid = ATTR_SIZE,
+		.ia_size = stat->size,
+	};
+
+	return notify_change(upperdentry, &attr, NULL);
+}
+
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
 	struct iattr attr = {
@@ -437,6 +447,28 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	goto out;
 }
 
+/* Copy up data of an inode which was copied up metadata only in the past. */
+static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
+{
+	struct path upperpath;
+	int err;
+
+	ovl_path_upper(c->dentry, &upperpath);
+	if (WARN_ON(upperpath.dentry == NULL))
+		return -EIO;
+
+	err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+	if (err)
+		return err;
+
+	err= vfs_removexattr(upperpath.dentry, OVL_XATTR_METACOPY);
+	if (err)
+		return err;
+
+	ovl_set_upperdata(c->dentry);
+	return err;
+}
+
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
@@ -470,8 +502,19 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
+	if (c->metacopy) {
+		err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+					 NULL, 0, -EOPNOTSUPP);
+		if (err)
+			return err;
+	}
+
+
 	inode_lock(temp->d_inode);
-	err = ovl_set_attr(temp, &c->stat);
+	if (c->metacopy)
+		err = ovl_set_size(temp, &c->stat);
+	if (!err)
+		err = ovl_set_attr(temp, &c->stat);
 	inode_unlock(temp->d_inode);
 
 	return err;
@@ -503,6 +546,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto out_cleanup;
 
+	if (!c->metacopy)
+		ovl_set_upperdata(c->dentry);
 	inode = d_inode(c->dentry);
 	ovl_inode_update(inode, newdentry);
 	if (S_ISDIR(inode->i_mode))
@@ -625,7 +670,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	}
 	ovl_do_check_copy_up(ctx.lowerpath.dentry);
 
-	err = ovl_copy_up_start(dentry);
+	err = ovl_copy_up_start(dentry, flags);
 	/* err < 0: interrupted, err > 0: raced with another copy-up */
 	if (unlikely(err)) {
 		if (err > 0)
@@ -635,6 +680,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			err = ovl_do_copy_up(&ctx);
 		if (!err && !ovl_dentry_has_upper_alias(dentry))
 			err = ovl_link_up(&ctx);
+		if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
+			err = ovl_copy_up_meta_inode_data(&ctx);
 		ovl_copy_up_end(dentry);
 	}
 	do_delayed_call(&done);
@@ -651,7 +698,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent;
 
-		if (ovl_already_copied_up(dentry))
+		if (ovl_already_copied_up(dentry, flags))
 			break;
 
 		next = dget(dentry);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 537dc25f84c6..405a8c34471b 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -343,7 +343,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
-	if (ovl_already_copied_up(dentry))
+	if (ovl_already_copied_up(dentry, flags))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index a6fdc2f0da6d..6962fab08012 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -27,6 +27,7 @@ enum ovl_path_type {
 #define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
 #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
+#define OVL_XATTR_METACOPY OVL_XATTR_PREFIX "metacopy"
 
 enum ovl_flag {
 	/* Pure upper dir that may contain non pure upper entries */
@@ -34,6 +35,7 @@ enum ovl_flag {
 	/* Non-merge dir that may contain whiteout entries */
 	OVL_WHITEOUTS,
 	OVL_INDEX,
+	OVL_UPPERDATA,
 };
 
 /*
@@ -215,6 +217,9 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
 bool ovl_dentry_has_upper_alias(struct dentry *dentry);
 void ovl_dentry_set_upper_alias(struct dentry *dentry);
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags);
+bool ovl_has_upperdata(struct dentry *dentry);
+void ovl_set_upperdata(struct dentry *dentry);
 bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags);
 bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
@@ -226,9 +231,9 @@ void ovl_dentry_version_inc(struct dentry *dentry, bool impurity);
 u64 ovl_dentry_version_get(struct dentry *dentry);
 bool ovl_is_whiteout(struct dentry *dentry);
 struct file *ovl_path_open(struct path *path, int flags);
-int ovl_copy_up_start(struct dentry *dentry);
+int ovl_copy_up_start(struct dentry *dentry, int flags);
 void ovl_copy_up_end(struct dentry *dentry);
-bool ovl_already_copied_up(struct dentry *dentry);
+bool ovl_already_copied_up(struct dentry *dentry, int flags);
 bool ovl_check_origin_xattr(struct dentry *dentry);
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f4e3e011a097..f70eea77c5c7 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -253,6 +253,43 @@ bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags)
 	return true;
 }
 
+static bool ovl_should_check_upperdata(struct dentry *dentry) {
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return false;
+
+	if (!ovl_dentry_lower(dentry))
+		return false;
+
+	return true;
+}
+
+bool ovl_has_upperdata(struct dentry *dentry) {
+	if (!ovl_should_check_upperdata(dentry))
+		return true;
+
+	return ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+void ovl_set_upperdata(struct dentry *dentry) {
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return;
+
+	ovl_set_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
+	if (!flags)
+		return false;
+
+	if (!(OPEN_FMODE(flags) & FMODE_WRITE))
+		return false;
+
+	if (ovl_has_upperdata(dentry))
+		return false;
+
+	return true;
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
@@ -336,13 +373,13 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
-int ovl_copy_up_start(struct dentry *dentry)
+int ovl_copy_up_start(struct dentry *dentry, int flags)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(dentry));
 	int err;
 
 	err = mutex_lock_interruptible(&oi->lock);
-	if (!err && ovl_already_copied_up(dentry)) {
+	if (!err && ovl_already_copied_up(dentry, flags)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
@@ -368,7 +405,7 @@ bool ovl_check_origin_xattr(struct dentry *dentry)
 	return false;
 }
 
-bool ovl_already_copied_up(struct dentry *dentry)
+bool ovl_already_copied_up(struct dentry *dentry, int flags)
 {
 	/*
 	 * Check if copy-up has happened as well as for upper alias (in
@@ -384,7 +421,8 @@ bool ovl_already_copied_up(struct dentry *dentry)
 	 *      with rename.
 	 */
 	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	    ovl_dentry_has_upper_alias(dentry) &&
+	    !ovl_dentry_needs_data_copy_up(dentry, flags))
 		return true;
 
 	return false;
-- 
2.13.6

Re: [PATCH v6 07/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Now we will have the capability to have upper inodes which might be only
> metadata copy up and data is still on lower inode. So add a new xattr
> OVL_XATTR_METACOPY to distinguish between two cases.
>
> Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata
> only and and data will be copied up later from lower origin.
> So this xattr is set when a metadata copy takes place and cleared when
> data copy takes place.
>
> We also use a bit in ovl_inode->flags to cache OVL_UPPERDATA which reflects
> whether ovl inode has data or not (as opposed to metadata only copy up).
>
> Note, OVL_UPPERDATA is set only on those upper inodes which can possibly
> be metacopy only inodes. For example, inode should be a regular file and
> there needs to be a lower dentry.
>

Sorry, not buying and I bet Miklos won't buy it either.
I just introduced OVL_WHITEOUTS for directories that are non-merge
but have origin xattr, to know that we need to create dir cache using
ovl_dir_read_merged() to filter out whiteouts.

Miklos tweaked my patch to set OVL_WHITEOUTS on all merge dirs
on copy up and on lookup, so the test is uniform and doesn't need to check
if dir is eligible to OVL_WHITEOUTS.

Same logic applied to OVL_UPPERDATA: if flag is not set, need to
copy up data. If that requires another memory barrier when creating
pure upper then it is not hurting performance to add it, but I doubt that
it is needed anyway.

Amir.

[PATCH v6 08/15] ovl: Fix ovl_getattr() to get number of blocks from lower

[Vivek Goyal]

If an inode has been copied up metadata only, then we need to query the
number of blocks from lower and fill up the stat->st_blocks.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 405a8c34471b..7ba19a97a8da 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -76,6 +76,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
 	bool samefs = ovl_same_sb(dentry->d_sb);
 	int err;
+	bool metacopy = false;
+
+	if (ovl_dentry_upper(dentry) && !ovl_has_upperdata(dentry))
+		metacopy = true;
 
 	type = ovl_path_real(dentry, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -93,7 +97,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	if (!is_dir || samefs) {
 		if (OVL_TYPE_ORIGIN(type)) {
 			struct kstat lowerstat;
-			u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+			u32 lowermask = STATX_INO | STATX_BLOCKS |
+					(!is_dir ? STATX_NLINK : 0);
 
 			ovl_path_lower(dentry, &realpath);
 			err = vfs_getattr(&realpath, &lowerstat,
@@ -117,6 +122,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 				WARN_ON_ONCE(stat->dev != lowerstat.dev);
 			else
 				stat->dev = ovl_get_pseudo_dev(dentry);
+
+			if (metacopy)
+				stat->blocks = lowerstat.blocks;
 		}
 		if (samefs) {
 			/*
-- 
2.13.6

Re: [PATCH v6 08/15] ovl: Fix ovl_getattr() to get number of blocks from lower

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If an inode has been copied up metadata only, then we need to query the
> number of blocks from lower and fill up the stat->st_blocks.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 405a8c34471b..7ba19a97a8da 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -76,6 +76,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>         bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
>         bool samefs = ovl_same_sb(dentry->d_sb);
>         int err;
> +       bool metacopy = false;
> +
> +       if (ovl_dentry_upper(dentry) && !ovl_has_upperdata(dentry))
> +               metacopy = true;
>
>         type = ovl_path_real(dentry, &realpath);
>         old_cred = ovl_override_creds(dentry->d_sb);
> @@ -93,7 +97,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>         if (!is_dir || samefs) {
>                 if (OVL_TYPE_ORIGIN(type)) {
>                         struct kstat lowerstat;
> -                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
> +                       u32 lowermask = STATX_INO | STATX_BLOCKS |
> +                                       (!is_dir ? STATX_NLINK : 0);
>
>                         ovl_path_lower(dentry, &realpath);
>                         err = vfs_getattr(&realpath, &lowerstat,
> @@ -117,6 +122,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                                 WARN_ON_ONCE(stat->dev != lowerstat.dev);
>                         else
>                                 stat->dev = ovl_get_pseudo_dev(dentry);
> +
> +                       if (metacopy)
> +                               stat->blocks = lowerstat.blocks;

OVL_TYPE_ORIGIN(type) implies ovl_dentry_upper(dentry), so you don't
need the boolean
var, you can test !ovl_has_upperdata(dentry) here instead.
To be honest, I am not fond of the patches adding memory barriers
being separate from
that patches that introduce the races, even though technically races
cannot happen
until last patch enables metacopy.

Amir.

Re: [PATCH v6 08/15] ovl: Fix ovl_getattr() to get number of blocks from lower

[Vivek Goyal]

On Fri, Nov 10, 2017 at 10:43:18AM +0200, Amir Goldstein wrote:
> On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If an inode has been copied up metadata only, then we need to query the
> > number of blocks from lower and fill up the stat->st_blocks.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 405a8c34471b..7ba19a97a8da 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -76,6 +76,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >         bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> >         bool samefs = ovl_same_sb(dentry->d_sb);
> >         int err;
> > +       bool metacopy = false;
> > +
> > +       if (ovl_dentry_upper(dentry) && !ovl_has_upperdata(dentry))
> > +               metacopy = true;
> >
> >         type = ovl_path_real(dentry, &realpath);
> >         old_cred = ovl_override_creds(dentry->d_sb);
> > @@ -93,7 +97,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >         if (!is_dir || samefs) {
> >                 if (OVL_TYPE_ORIGIN(type)) {
> >                         struct kstat lowerstat;
> > -                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
> > +                       u32 lowermask = STATX_INO | STATX_BLOCKS |
> > +                                       (!is_dir ? STATX_NLINK : 0);
> >
> >                         ovl_path_lower(dentry, &realpath);
> >                         err = vfs_getattr(&realpath, &lowerstat,
> > @@ -117,6 +122,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >                                 WARN_ON_ONCE(stat->dev != lowerstat.dev);
> >                         else
> >                                 stat->dev = ovl_get_pseudo_dev(dentry);
> > +
> > +                       if (metacopy)
> > +                               stat->blocks = lowerstat.blocks;
> 
> OVL_TYPE_ORIGIN(type) implies ovl_dentry_upper(dentry), so you don't
> need the boolean
> var, you can test !ovl_has_upperdata(dentry) here instead.

Actually the real reason I checked ovl_has_upperdata() early, is to
tackle the possible race with data copy up happening on separate
cpu. For example, say CPU1 is executing ovl_getattr() while CPU2
is opening same file for WRITE and that has triggered a data
copy up.

Now we will do vfs_getattr(&realpath) which will get stat from upper
and some block numbers (depending on how much data has been copied up).
Say by the time we check for ovl_has_upperdata(), data copy up is
complete, so we will return stat->blocks from upper. And this is not
correct stat as upper data was not stable when we did vfs_getattr().

So I wanted to call ovl_has_upperdata() early (with a smp_rmb() if needed)
and make sure if OVL_UPPERDATA is visible, then upper blocks are stable
and can be reported to user space otherwise report number of blocks
from lower.

> To be honest, I am not fond of the patches adding memory barriers
> being separate from
> that patches that introduce the races, even though technically races
> cannot happen
> until last patch enables metacopy.

I will try to rework my patch series and introduce barriers along with 
where races are introduced.

Vivek

[PATCH v6 09/15] ovl: Set OVL_UPPERDATA flag during ovl_lookup()

[Vivek Goyal]

During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
set OVL_UPPERDATA bit in flags. This is only done for upper inode with a
corresponding lower dentry and file needs to be a regular file. Basically
any file which is eligible for metadata only copy up.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 625ed8066570..74c15d001d68 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -25,6 +25,28 @@ struct ovl_lookup_data {
 	char *redirect;
 };
 
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+static int ovl_check_metacopy(struct dentry *dentry)
+{
+	int res;
+
+	/* Only regular files can have metacopy xattr */
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return 0;
+
+	res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+	if (res < 0) {
+		if (res == -ENODATA || res == -EOPNOTSUPP)
+			return 0;
+		goto out;
+	}
+
+	return 1;
+out:
+	pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+	return res;
+}
+
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 			      size_t prelen, const char *post)
 {
@@ -602,6 +624,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct dentry *this;
 	unsigned int i;
 	int err;
+	bool metacopy = false;
 	struct ovl_lookup_data d = {
 		.name = dentry->d_name,
 		.is_dir = false,
@@ -642,6 +665,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 					       roe->numlower, &stack, &ctr);
 			if (err)
 				goto out_put_upper;
+
+			err = ovl_check_metacopy(upperdentry);
+			metacopy = err;
+			if (err < 0)
+				goto out_put_upper;
+			if (metacopy && !ctr) {
+				/*
+				 * Found an upper with metacopy set but
+				 * at the same time there is no lower
+				 * dentry. Something is not right.
+				 */
+				err = -ESTALE;
+				goto out_put_upper;
+			}
 		}
 
 		if (d.redirect) {
@@ -726,6 +763,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		OVL_I(inode)->redirect = upperredirect;
 		if (index)
 			ovl_set_flag(OVL_INDEX, inode);
+
+		if (upperdentry && ctr && S_ISREG(inode->i_mode) && !metacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	revert_creds(old_cred);
-- 
2.13.6

Re: [PATCH v6 09/15] ovl: Set OVL_UPPERDATA flag during ovl_lookup()

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
> set OVL_UPPERDATA bit in flags. This is only done for upper inode with a
> corresponding lower dentry and file needs to be a regular file. Basically
> any file which is eligible for metadata only copy up.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 625ed8066570..74c15d001d68 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -25,6 +25,28 @@ struct ovl_lookup_data {
>         char *redirect;
>  };
>
> +/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> +static int ovl_check_metacopy(struct dentry *dentry)
> +{
> +       int res;
> +
> +       /* Only regular files can have metacopy xattr */
> +       if (!S_ISREG(d_inode(dentry)->i_mode))
> +               return 0;
> +
> +       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> +       if (res < 0) {
> +               if (res == -ENODATA || res == -EOPNOTSUPP)
> +                       return 0;
> +               goto out;
> +       }
> +
> +       return 1;
> +out:
> +       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> +       return res;
> +}
> +
>  static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
>                               size_t prelen, const char *post)
>  {
> @@ -602,6 +624,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct dentry *this;
>         unsigned int i;
>         int err;
> +       bool metacopy = false;
>         struct ovl_lookup_data d = {
>                 .name = dentry->d_name,
>                 .is_dir = false,
> @@ -642,6 +665,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                                roe->numlower, &stack, &ctr);
>                         if (err)
>                                 goto out_put_upper;
> +
> +                       err = ovl_check_metacopy(upperdentry);
> +                       metacopy = err;
> +                       if (err < 0)
> +                               goto out_put_upper;
> +                       if (metacopy && !ctr) {
> +                               /*
> +                                * Found an upper with metacopy set but
> +                                * at the same time there is no lower
> +                                * dentry. Something is not right.
> +                                */
> +                               err = -ESTALE;
> +                               goto out_put_upper;
> +                       }
>                 }
>
>                 if (d.redirect) {
> @@ -726,6 +763,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 OVL_I(inode)->redirect = upperredirect;
>                 if (index)
>                         ovl_set_flag(OVL_INDEX, inode);
> +
> +               if (upperdentry && ctr && S_ISREG(inode->i_mode) && !metacopy)
> +                       ovl_set_flag(OVL_UPPERDATA, inode);

I don't buy the justification for this test not being if (upperdentry
&& !metacopy)

Amir.

[PATCH v6 10/15] ovl: Return lower dentry if only metadata copy up took place

[Vivek Goyal]

Upper dentry inode does not have data. So return lower dentry if upper
is only a metadata copy.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/super.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 84b8f6dc0f61..5cb19781763d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -101,14 +101,19 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
 	real = ovl_dentry_upper(dentry);
 	if (real && (!inode || inode == d_inode(real))) {
+		bool metacopy = !ovl_has_upperdata(dentry);
 		if (!inode) {
 			err = ovl_check_append_only(d_inode(real), open_flags);
 			if (err)
 				return ERR_PTR(err);
+
+			if (unlikely(metacopy))
+				goto lower;
 		}
 		return real;
 	}
 
+lower:
 	real = ovl_dentry_lower(dentry);
 	if (!real)
 		goto bug;
-- 
2.13.6

Re: [PATCH v6 10/15] ovl: Return lower dentry if only metadata copy up took place

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Upper dentry inode does not have data. So return lower dentry if upper
> is only a metadata copy.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/super.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 84b8f6dc0f61..5cb19781763d 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -101,14 +101,19 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>
>         real = ovl_dentry_upper(dentry);
>         if (real && (!inode || inode == d_inode(real))) {
> +               bool metacopy = !ovl_has_upperdata(dentry);
>                 if (!inode) {
>                         err = ovl_check_append_only(d_inode(real), open_flags);
>                         if (err)
>                                 return ERR_PTR(err);
> +
> +                       if (unlikely(metacopy))

I don't see why boolean var is needed and I prefer if memory barrier
was introduced
already in this patch.

Amir.

[PATCH v6 11/15] ovl: Do not expose metacopy only upper dentry

[Vivek Goyal]

d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
This is something new and vfs layer does not know that this inode contains
only metadata and not data. And this could break things.

So to be safe, do not export metacopy only dentry/inode to vfs using d_real().

If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
it has data (flag OVL_UPPERDATA is set).

Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
dentry/inode does not have OVL_UPPERDATA set. This should not happen as
we never made this metacopy inode visible to vfs so nobody should be calling
overlayfs back with inode=metacopy_inode.

I scanned the code and I don't think it breaks any of the existing code.
There are two users of D_REAL_UPPER. may_write_real() and
update_ovl_inode_times().

may_write_real(), will get an NULL dentry if upper inode is metacopy only
and it will return -EPERM. Effectively, we are disallowing modifications
to metacopy only inode from this interface. Though there is opportunity
to improve it. (Allow chattr on metacopy inodes).

update_ovl_inode_times() gets inode mtime and ctime from real inode. It
should not be broken for metacopy inode as well for following reasons.

- For any metadata operations (setattr, acl etc), overlay always calls
  ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
  need to update mtime and ctime int his case. Its already updated.

- For metadata inode, mtime should be same as lower and not change. (data
  can't be modified on metadata inode without copyup).

- For file writes, ctime and mtime will be updated. But in that case
  first data will be copied up and this will not be a metadata inode
  anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
  inode and new mtime and ctime will be obtainable.

So atime updates should work just fine for metacopy inodes.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5cb19781763d..f1d7d38b747f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -84,8 +84,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	struct dentry *real;
 	int err;
 
-	if (flags & D_REAL_UPPER)
-		return ovl_dentry_upper(dentry);
+	if (flags & D_REAL_UPPER) {
+		real = ovl_dentry_upper(dentry);
+		if (!real)
+			return NULL;
+		if (!ovl_has_upperdata(dentry))
+			return NULL;
+		return real;
+	}
 
 	if (!d_is_reg(dentry)) {
 		if (!inode || inode == d_inode(dentry))
@@ -109,7 +115,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
 			if (unlikely(metacopy))
 				goto lower;
-		}
+		} else if (unlikely(metacopy))
+			goto bug;
+
 		return real;
 	}
 
-- 
2.13.6

Re: [PATCH v6 11/15] ovl: Do not expose metacopy only upper dentry

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
> This is something new and vfs layer does not know that this inode contains
> only metadata and not data. And this could break things.
>
> So to be safe, do not export metacopy only dentry/inode to vfs using d_real().
>
> If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
> it has data (flag OVL_UPPERDATA is set).
>
> Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
> dentry/inode does not have OVL_UPPERDATA set. This should not happen as
> we never made this metacopy inode visible to vfs so nobody should be calling
> overlayfs back with inode=metacopy_inode.
>
> I scanned the code and I don't think it breaks any of the existing code.
> There are two users of D_REAL_UPPER. may_write_real() and
> update_ovl_inode_times().
>
> may_write_real(), will get an NULL dentry if upper inode is metacopy only
> and it will return -EPERM. Effectively, we are disallowing modifications
> to metacopy only inode from this interface. Though there is opportunity
> to improve it. (Allow chattr on metacopy inodes).
>
> update_ovl_inode_times() gets inode mtime and ctime from real inode. It
> should not be broken for metacopy inode as well for following reasons.
>
> - For any metadata operations (setattr, acl etc), overlay always calls
>   ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
>   need to update mtime and ctime int his case. Its already updated.
>
> - For metadata inode, mtime should be same as lower and not change. (data
>   can't be modified on metadata inode without copyup).
>
> - For file writes, ctime and mtime will be updated. But in that case
>   first data will be copied up and this will not be a metadata inode
>   anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
>   inode and new mtime and ctime will be obtainable.
>
> So atime updates should work just fine for metacopy inodes.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/super.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 5cb19781763d..f1d7d38b747f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -84,8 +84,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>         struct dentry *real;
>         int err;
>
> -       if (flags & D_REAL_UPPER)
> -               return ovl_dentry_upper(dentry);
> +       if (flags & D_REAL_UPPER) {
> +               real = ovl_dentry_upper(dentry);
> +               if (!real)
> +                       return NULL;
> +               if (!ovl_has_upperdata(dentry))
> +                       return NULL;
> +               return real;
> +       }
>
>         if (!d_is_reg(dentry)) {
>                 if (!inode || inode == d_inode(dentry))
> @@ -109,7 +115,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>
>                         if (unlikely(metacopy))
>                                 goto lower;
> -               }
> +               } else if (unlikely(metacopy))
> +                       goto bug;
> +


Now I see why you needed the boolean var.
IMO, this patch should be better squashed with patch 10
for dealing with all cases of d_real()
Maybe title should be:
ovl: Do not expose metacopy only upper dentry from d_real()

Because that is the only thing that this patch takes care of

Amir.

[PATCH v6 12/15] ovl: Fix encryption status of a metacopy only file

[Vivek Goyal]

If file is metacopy only, it is possible that lower is encrypted while
other is not. In that case, report file as encrypted (despite the fact
that only data is encrypted while metadata is not).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 7ba19a97a8da..15713d4ac2dd 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -66,6 +66,16 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 	return err;
 }
 
+static void ovl_stat_set_encryption(struct kstat *ustat, struct kstat *lstat) {
+	if (!((lstat->attributes_mask & STATX_ATTR_ENCRYPTED) &&
+	    (lstat->attributes & STATX_ATTR_ENCRYPTED)))
+		return;
+
+	ustat->attributes |= STATX_ATTR_ENCRYPTED;
+	ustat->attributes_mask |= STATX_ATTR_ENCRYPTED;
+}
+
+
 int ovl_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int flags)
 {
@@ -123,8 +133,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			else
 				stat->dev = ovl_get_pseudo_dev(dentry);
 
-			if (metacopy)
+			if (metacopy) {
 				stat->blocks = lowerstat.blocks;
+				ovl_stat_set_encryption(stat, &lowerstat);
+			}
 		}
 		if (samefs) {
 			/*
-- 
2.13.6

Re: [PATCH v6 12/15] ovl: Fix encryption status of a metacopy only file

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If file is metacopy only, it is possible that lower is encrypted while
> other is not. In that case, report file as encrypted (despite the fact
> that only data is encrypted while metadata is not).

Better consult ext4 guys or find out which user tools care about this
flag and what they could do in response to this flag.

When I commented that we need to see what do to about all these
flags I just gave encrypted flag as an example.

On a hunch I would say that we need a mask of statx flags that are
readonly attributes of the data itself and that mask should probably
contain encrypted and compressed to begin with.
I don't see a reason for 2 separate patches and certainly not for
2 separate helpers.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7ba19a97a8da..15713d4ac2dd 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -66,6 +66,16 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>         return err;
>  }
>
> +static void ovl_stat_set_encryption(struct kstat *ustat, struct kstat *lstat) {
> +       if (!((lstat->attributes_mask & STATX_ATTR_ENCRYPTED) &&
> +           (lstat->attributes & STATX_ATTR_ENCRYPTED)))
> +               return;
> +
> +       ustat->attributes |= STATX_ATTR_ENCRYPTED;
> +       ustat->attributes_mask |= STATX_ATTR_ENCRYPTED;
> +}
> +


This looks buggy. you set STATX_ATTR_ENCRYPTED even if lower
doesn't have STATX_ATTR_ENCRYPTED in attributes nor in attributes_mask.

Amir.

Re: [PATCH v6 12/15] ovl: Fix encryption status of a metacopy only file

[Vivek Goyal]

On Fri, Nov 10, 2017 at 11:09:26AM +0200, Amir Goldstein wrote:
> On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If file is metacopy only, it is possible that lower is encrypted while
> > other is not. In that case, report file as encrypted (despite the fact
> > that only data is encrypted while metadata is not).
> 
> Better consult ext4 guys or find out which user tools care about this
> flag and what they could do in response to this flag.

[ CCing ext4 maling list ]

> 
> When I commented that we need to see what do to about all these
> flags I just gave encrypted flag as an example.
> 
> On a hunch I would say that we need a mask of statx flags that are
> readonly attributes of the data itself and that mask should probably
> contain encrypted and compressed to begin with.

I thought encrypted flag is representing metadata also (and not just 
data). Is that not the case.

> I don't see a reason for 2 separate patches and certainly not for
> 2 separate helpers.

I will merge the two patches. I am not sure about the mask thing though.
We can probably start with encryption and compression flags for now and
make it more generic down the line (using that mask).

> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 7ba19a97a8da..15713d4ac2dd 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -66,6 +66,16 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> >         return err;
> >  }
> >
> > +static void ovl_stat_set_encryption(struct kstat *ustat, struct kstat *lstat) {
> > +       if (!((lstat->attributes_mask & STATX_ATTR_ENCRYPTED) &&
> > +           (lstat->attributes & STATX_ATTR_ENCRYPTED)))
> > +               return;
> > +
> > +       ustat->attributes |= STATX_ATTR_ENCRYPTED;
> > +       ustat->attributes_mask |= STATX_ATTR_ENCRYPTED;
> > +}
> > +
> 
> 
> This looks buggy. you set STATX_ATTR_ENCRYPTED even if lower
> doesn't have STATX_ATTR_ENCRYPTED in attributes nor in attributes_mask.

Hmm.., I am not able to see where is the bug. Did you notice the "!"
in if condition.

We will set STATX_ATTR_ENCRYPTED in attributes_mask and attributes 
only if lower has STATX_ATTR_ENCRYPTED set both in ->attributes_mask
and ->attributes.

Vivek

[PATCH v6 13/15] ovl: Fix compression status of a metacopy only file

[Vivek Goyal]

If file is metacopy only, it is possible that lower is compressed while
upper is not. In that case, report file as compressed (despite the fact
that only data is compressed while metadata is not).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 15713d4ac2dd..dc7573ed947a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -75,6 +75,15 @@ static void ovl_stat_set_encryption(struct kstat *ustat, struct kstat *lstat) {
 	ustat->attributes_mask |= STATX_ATTR_ENCRYPTED;
 }
 
+static void ovl_stat_set_compressed(struct kstat *ustat, struct kstat *lstat) {
+	if (!((lstat->attributes_mask & STATX_ATTR_COMPRESSED) &&
+	    (lstat->attributes & STATX_ATTR_COMPRESSED)))
+		return;
+
+	ustat->attributes |= STATX_ATTR_COMPRESSED;
+	ustat->attributes_mask |= STATX_ATTR_COMPRESSED;
+}
+
 
 int ovl_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int flags)
@@ -136,6 +145,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			if (metacopy) {
 				stat->blocks = lowerstat.blocks;
 				ovl_stat_set_encryption(stat, &lowerstat);
+				ovl_stat_set_compressed(stat, &lowerstat);
 			}
 		}
 		if (samefs) {
-- 
2.13.6

Re: [PATCH v6 13/15] ovl: Fix compression status of a metacopy only file

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If file is metacopy only, it is possible that lower is compressed while
> upper is not. In that case, report file as compressed (despite the fact
> that only data is compressed while metadata is not).
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Unless I am missing something, please unify the encrypted and compressed
logic to a mask of "readonly data attributes"

Amir.

[PATCH v6 14/15] ovl: Introduce read/write barriers around metacopy flag update

[Vivek Goyal]

If a file is copied up metadata only and later when same file is opened
for WRITE, then data copy up takes place. We copy up data, remove METACOPY
xattr and then set the UPPERDATA flag in ovl_entry->flags. While all
these operations happen with oi->lock held, read side of oi->flags is
lockless. That is another thread on another cpu can check if UPPERDATA
flag is set or not.

So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if
another cpu sees UPPERDATA flag set, then it should be guaranteed that
effects of data copy up and remove xattr operations are also visible.

For example.

	CPU1				CPU2
ovl_d_real()				acquire(oi->lock)
  ovl_copy_up_flags()			 ovl_copy_up_data()
   ovl_dentry_needs_data_copy_up()	 vfs_removexattr()
     ovl_test_flag(OVL_UPPERDATA)        ovl_set_flag(OVK_UPPERDATA)
					release(oi->lock)

Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if
CPU1 perceives the effects of setting UPPERDATA flag but not effects of
preceeding operations, that would be a problem.

Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation
and smp_rmb() on UPPERDATA flag test operation.

May be some other lock or barrier is already covering it. But I am not sure
what that is and is it obvious enough that we will not break it in future.

So hence trying to be safe here and introducing barriers explicitly for
UPPERDATA flag/bit.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/util.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f70eea77c5c7..81b307c20a8d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -267,13 +267,26 @@ bool ovl_has_upperdata(struct dentry *dentry) {
 	if (!ovl_should_check_upperdata(dentry))
 		return true;
 
-	return ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
+	if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
+		return false;
+	/*
+	 * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
+	 * if setting of OVL_UPPERDATA is visible, then effects of writes
+	 * before that are visible too.
+	 */
+	smp_rmb();
+	return true;
 }
 
 void ovl_set_upperdata(struct dentry *dentry) {
 	if (!S_ISREG(d_inode(dentry)->i_mode))
 		return;
-
+	/*
+	 * Pairs with smp_rmb() in ovl_has_upperdata(). Make sure
+	 * if OVL_UPPERDATA flag is visible, then effects of write operations
+	 * before it are visible as well.
+	 */
+	smp_wmb();
 	ovl_set_flag(OVL_UPPERDATA, d_inode(dentry));
 }
 
-- 
2.13.6

Re: [PATCH v6 14/15] ovl: Introduce read/write barriers around metacopy flag update

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If a file is copied up metadata only and later when same file is opened
> for WRITE, then data copy up takes place. We copy up data, remove METACOPY
> xattr and then set the UPPERDATA flag in ovl_entry->flags. While all
> these operations happen with oi->lock held, read side of oi->flags is
> lockless. That is another thread on another cpu can check if UPPERDATA
> flag is set or not.
>
> So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if
> another cpu sees UPPERDATA flag set, then it should be guaranteed that
> effects of data copy up and remove xattr operations are also visible.
>
> For example.
>
>         CPU1                            CPU2
> ovl_d_real()                            acquire(oi->lock)
>   ovl_copy_up_flags()                    ovl_copy_up_data()
>    ovl_dentry_needs_data_copy_up()       vfs_removexattr()
>      ovl_test_flag(OVL_UPPERDATA)        ovl_set_flag(OVK_UPPERDATA)
>                                         release(oi->lock)

typo OVK_UPPERDATA

CPU1:
 ovl_d_real()
  ovl_open_maybe_copy_up()
    ovl_open_need_copy_up()
      ovl_dentry_needs_data_copy_up()
        ovl_test_flag(OVL_UPPERDATA)

That's the execution path I meant you should specify, the execution path
that could expose incomplete upper to vfs from d_real().

>
> Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if
> CPU1 perceives the effects of setting UPPERDATA flag but not effects of
> preceeding operations, that would be a problem.
>

Instead of "that would be a problem", you can say that upper that is
not fully copied
up is exposed to vfs.

> Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation
> and smp_rmb() on UPPERDATA flag test operation.
>
> May be some other lock or barrier is already covering it. But I am not sure
> what that is and is it obvious enough that we will not break it in future.
>
> So hence trying to be safe here and introducing barriers explicitly for
> UPPERDATA flag/bit.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/util.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f70eea77c5c7..81b307c20a8d 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -267,13 +267,26 @@ bool ovl_has_upperdata(struct dentry *dentry) {
>         if (!ovl_should_check_upperdata(dentry))
>                 return true;
>
> -       return ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
> +       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> +               return false;
> +       /*
> +        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
> +        * if setting of OVL_UPPERDATA is visible, then effects of writes
> +        * before that are visible too.
> +        */
> +       smp_rmb();
> +       return true;
>  }
>

I think it is not right to have smp_rmb() is the locked path where it
is not needed
too costly.

We should probably have a different variant for ovl_already_copied_up_locked()
and if OVL_UPPERDATA is set for all non-eligible cases, ovl_has_upperdata()
can have the smp_rmb() and locked variant can just can check the flag directly.

Amir.

Re: [PATCH v6 14/15] ovl: Introduce read/write barriers around metacopy flag update

[Vivek Goyal]

On Fri, Nov 10, 2017 at 11:43:36AM +0200, Amir Goldstein wrote:
[..]
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -267,13 +267,26 @@ bool ovl_has_upperdata(struct dentry *dentry) {
> >         if (!ovl_should_check_upperdata(dentry))
> >                 return true;
> >
> > -       return ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
> > +       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> > +               return false;
> > +       /*
> > +        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
> > +        * if setting of OVL_UPPERDATA is visible, then effects of writes
> > +        * before that are visible too.
> > +        */
> > +       smp_rmb();
> > +       return true;
> >  }
> >
> 
> I think it is not right to have smp_rmb() is the locked path where it
> is not needed
> too costly.
> 
> We should probably have a different variant for ovl_already_copied_up_locked()
> and if OVL_UPPERDATA is set for all non-eligible cases, ovl_has_upperdata()
> can have the smp_rmb() and locked variant can just can check the flag directly.

How about adding a parameter "bool locked" to ovl_already_copied_up() and
ovl_has_upperdata(). So if locked is true, smp_rmb() will be skipped by
ovl_has_upperdata().

Vivek

[PATCH v6 15/15] ovl: Enable metadata only feature

[Vivek Goyal]

All the bits are in patches before this. So it is time to enable the
metadata only copy up feature.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/util.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 81b307c20a8d..9bda6ff611d4 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -235,9 +235,6 @@ bool ovl_metaonly_copy_up(struct dentry *dentry, umode_t mode, int flags)
 {
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
-	/* TODO: Will enable metacopy in last patch of series */
-	return false;
-
 	if (!ofs->config.metacopy)
 		return false;
 
-- 
2.13.6

Re: [PATCH v6 15/15] ovl: Enable metadata only feature

[Amir Goldstein]

On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> All the bits are in patches before this. So it is time to enable the
> metadata only copy up feature.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>