[RFC PATCH v7 00/14] overlayfs: Delayed copy up of data

[RFC PATCH v7 00/14] overlayfs: Delayed copy up of data

[Vivek Goyal]

Hi,

Here is the V7 of the patches. These patches apply on top of
overlayfs-next branch of miklos's tree.

I have taken care of most of the comments from previous iteration.

These patches are 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 V6.

- Cherry picked Amir's patch to disable redirect_dir and index feature
  if upper does not support xattr. 
- Disable metacopy if upper does not support xattr.
- Merged encryption and compression patch.
- Merged barriers patch into core patch.
- Now OVL_UPPERDATA is set for all kind of dentries. Read side of 
  OVL_UPPERDATA first checks whether dentry in question should have
  this flag set or not.  
- Now smp_rmb() is called only when checking OVL_UPPERDATA from lockless
  path. 
- Miscellaneous code cleanus as asked by Amir.

Original Description
--------------------
In one of the recent converstions, people mentioned that chown/chmod
lead to copy up files as well as data. We could optimize it so that
only metadata is copied up during chown/chmod and data is copied up when
file is opened for WRITE.

This optimization potentially could be useful with containers and user
namespaces. In popular scenario, people end up doing chown() on whole
image directory tree based on container mappings. And this chown copies
up everything, breaking sharing of page cache between containers.

With these patches, only metadat is copied up during chown() and if file
is opened for READ, d_real() returns lower dentry/inode. That way,
different containers can still continue to use page cache. That's the
use case I have in mind.

Basically, I am relying on storing OVL_XATTR_ORIGIN in upper inode
during copy up. I use that information to get to lower inode later and
do data copy up when necessary.

I also store OVL_XATTR_METACOPY in upper inode to mark that only
metadata has been copied up and data copy up still might be required.

Any feedback is helpful.

Thanks
Vivek

Amir Goldstein (1):
  ovl: disable redirect_dir and index when no xattr support

Vivek Goyal (13):
  ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check
  ovl: Create origin xattr on copy up for all files
  ovl: Provide a mount option metacopy=on/off for metadata copyup
  ovl: During copy up, first copy up metadata and then data
  ovl: Move couple of copy up functions in copy_up.c
  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: Do not expose metacopy only upper dentry from d_real()
  ovl: Fix encryption/compression status of a metacopy only file
  ovl: Enable metadata only feature

 fs/overlayfs/Kconfig     |   8 +++
 fs/overlayfs/copy_up.c   | 161 ++++++++++++++++++++++++++++++++++-------------
 fs/overlayfs/dir.c       |   1 +
 fs/overlayfs/inode.c     |  58 ++++++++---------
 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 +++++++++++++++++++++++++++----
 9 files changed, 352 insertions(+), 94 deletions(-)

-- 
2.13.6

[PATCH v7 01/14] ovl: disable redirect_dir and index when no xattr support

[Vivek Goyal]

From: Amir Goldstein <amir73il@gmail.com>

Overlayfs falls back to index=off if lower/upper fs does not support
file handles. We should do the same if upper fs does not support xattr.

The redirect_dir feature is implicitly disabled when upper fs does not
support xattr via the check in ovl_redirect_dir(). Make the feature
explicitly disabled in this case by emitting a warning at mount time
and setting redirect_dir to off so its true state is visible in
/proc/mounts.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index be03578181d2..ee41bde87b14 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -921,7 +921,9 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
 	if (err) {
 		ofs->noxattr = true;
-		pr_warn("overlayfs: upper fs does not support xattr.\n");
+		ofs->config.redirect_dir = false;
+		ofs->config.index = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
-- 
2.13.6

[PATCH v7 02/14] 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 v7 03/14] 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>
Reviewed-by: Amir Goldstein <amir73il@gmail.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

[PATCH v7 04/14] 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     | 40 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 46 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 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 ee41bde87b14..d8b6b4d80d78 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,11 @@ 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)) {
+		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off and 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;
@@ -923,7 +945,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 		ofs->noxattr = true;
 		ofs->config.redirect_dir = false;
 		ofs->config.index = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
+		ofs->config.metacopy = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off, metacopy=off and no opaque dir.\n");
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
@@ -1164,6 +1187,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;
@@ -1209,6 +1233,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

[PATCH v7 05/14] 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 v7 06/14] ovl: Move couple of copy up functions in copy_up.c

[Vivek Goyal]

Right now two copy up related functions are part of inode.c. Amir suggested
it might be better to move these to copy_up.c. Hence this patch does that.
There will one more related function which will come in later patch.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c   | 30 ------------------------------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 303794bb9127..f84ba12c9b05 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
+static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
+{
+	if (ovl_dentry_upper(dentry) &&
+	    ovl_dentry_has_upper_alias(dentry))
+		return false;
+
+	if (special_file(d_inode(dentry)->i_mode))
+		return false;
+
+	if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+		return false;
+
+	return true;
+}
+
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
+{
+	int err = 0;
+
+	if (ovl_open_need_copy_up(dentry, file_flags)) {
+		err = ovl_want_write(dentry);
+		if (!err) {
+			err = ovl_copy_up_flags(dentry, file_flags);
+			ovl_drop_write(dentry);
+		}
+	}
+
+	return err;
+}
+
 struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper)
 {
 	struct ovl_fh *fh;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 00b6b294272a..e94a89e8a0f4 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -341,36 +341,6 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
-{
-	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
-		return false;
-
-	if (special_file(d_inode(dentry)->i_mode))
-		return false;
-
-	if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
-		return false;
-
-	return true;
-}
-
-int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
-{
-	int err = 0;
-
-	if (ovl_open_need_copy_up(dentry, file_flags)) {
-		err = ovl_want_write(dentry);
-		if (!err) {
-			err = ovl_copy_up_flags(dentry, file_flags);
-			ovl_drop_write(dentry);
-		}
-	}
-
-	return err;
-}
-
 int ovl_update_time(struct inode *inode, struct timespec *ts, int flags)
 {
 	struct dentry *alias;
-- 
2.13.6

[PATCH v7 07/14] 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 | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f84ba12c9b05..82d66e96be0f 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -262,6 +262,26 @@ int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
 	return err;
 }
 
+static bool ovl_need_meta_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 && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
+		return false;
+
+	return true;
+}
+
 struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper)
 {
 	struct ovl_fh *fh;
@@ -335,11 +355,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;
@@ -356,6 +373,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)
@@ -483,10 +501,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);
@@ -631,6 +653,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	if (err)
 		return err;
 
+	ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags);
+
 	ovl_path_upper(parent, &parentpath);
 	ctx.destdir = parentpath.dentry;
 	ctx.destname = dentry->d_name;
-- 
2.13.6

[PATCH v7 08/14] 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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 19 ++-----------------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 82d66e96be0f..b6fa5830c4c6 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -234,8 +234,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 
 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))
@@ -701,21 +700,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/overlayfs.h b/fs/overlayfs/overlayfs.h
index 13eab09a6b6f..7f9a79b5a2b5 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -227,6 +227,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 77dc00438d54..572bf46e9f2e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -314,13 +314,35 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
+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;
+}
+
 int ovl_copy_up_start(struct dentry *dentry)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(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);
 	}
-- 
2.13.6

[PATCH v7 09/14] 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).

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 can be
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_open_maybe_copy_up()                ovl_copy_up_data()
  open_open_need_copy_up()		 vfs_removexattr()
   ovl_already_copied_up()
    ovl_dentry_needs_data_copy_up()	 ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_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 the effects
of preceeding operations (ex. upper that is not fully copied up), it will 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.

Note, we now set OVL_UPPERDATA on all kind of dentires and check
OVL_UPPERDATA only where it makes sense. If a file is created on upper,
we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
path again. This time ordering dependency is w.r.t oe->__upperdata. We
need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
visible. Otherwise following code path might try to copy up an upper
only file.

ovl_d_real()
 ovl_open_maybe_copy_up()
  open_open_need_copy_up()
   ovl_already_copied_up()
    ovl_dentry_needs_data_copy_up()
     ovl_test_flag(OVL_UPPERDATA)
         <-- OVL_UPPERDATA is not visible yet. copy up file.

I have not introduced any barrier to take care of this. There is already
a check ovl_should_check_upperdata() to make sure lower dentry is there
otherwise return false (even if OVL_UPPERDATA is not visible yet).

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 55 +++++++++++++++++++++++++++++++---
 fs/overlayfs/dir.c       |  1 +
 fs/overlayfs/overlayfs.h | 10 +++++--
 fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 134 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b6fa5830c4c6..3f59b98340f7 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 = {
@@ -234,7 +244,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 
 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))
@@ -486,6 +496,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;
@@ -519,8 +551,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;
@@ -552,6 +595,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))
@@ -674,7 +719,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)
@@ -684,6 +729,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_locked(dentry, flags))
+			err = ovl_copy_up_meta_inode_data(&ctx);
 		ovl_copy_up_end(dentry);
 	}
 	do_delayed_call(&done);
@@ -700,7 +747,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/dir.c b/fs/overlayfs/dir.c
index e13921824c70..fb3cd73c3693 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
 	ovl_dentry_version_inc(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
 	if (!hardlink) {
+		ovl_set_flag(OVL_UPPERDATA, inode);
 		ovl_inode_update(inode, newdentry);
 		ovl_copyattr(newdentry->d_inode, inode);
 	} else {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 7f9a79b5a2b5..d4890e59b881 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,10 @@ 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_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags);
+bool ovl_has_upperdata(struct dentry *dentry);
+void ovl_set_upperdata(struct dentry *dentry);
 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);
@@ -225,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 572bf46e9f2e..73578bfe9f4b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -231,6 +231,64 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
 	oe->has_upper = 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;
+
+	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) {
+	/*
+	 * 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));
+}
+
+static inline bool open_for_write(int flags)
+{
+	if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))
+		return true;
+
+	return false;
+}
+
+/* Caller should hold ovl_entry->locked */
+bool ovl_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags) {
+	if (!open_for_write(flags))
+		return false;
+
+	return !ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
+	if (!open_for_write(flags))
+		return false;
+
+	return !ovl_has_upperdata(dentry);
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
@@ -314,7 +372,18 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
-bool ovl_already_copied_up(struct dentry *dentry)
+/* Caller should hold ovl_inode->lock */
+static bool ovl_already_copied_up_locked(struct dentry *dentry, int flags)
+{
+	if (ovl_dentry_upper(dentry) &&
+	    ovl_dentry_has_upper_alias(dentry) &&
+	    !ovl_dentry_needs_data_copy_up_locked(dentry, flags))
+		return true;
+
+	return false;
+}
+
+bool ovl_already_copied_up(struct dentry *dentry, int flags)
 {
 	/*
 	 * Check if copy-up has happened as well as for upper alias (in
@@ -330,19 +399,20 @@ 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;
 }
 
-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_locked(dentry, flags)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
-- 
2.13.6

[PATCH v7 10/14] 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.

We need to be careful about races where we are doing stat on one cpu and
data copy up is taking place on other cpu. We want to return stat->st_blocks
either from lower or stable upper and not something in between. Hence,
ovl_has_upperdata() is called first to figure out whether block reporting
will take place from lower or upper.

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 e94a89e8a0f4..7af8a292d54d 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

[PATCH v7 11/14] 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..00f3ca8a3e58 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 && !metacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	revert_creds(old_cred);
-- 
2.13.6

[PATCH v7 12/14] ovl: Do not expose metacopy only upper dentry from d_real()

[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 expose metacopy only dentry/inode to vfs using d_real().

For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
dentry inode is metacopy only and does not have data, return lower dentry.

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 | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d8b6b4d80d78..b15d4c4a68f2 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))
@@ -101,14 +107,21 @@ 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;
+		} else if (unlikely(metacopy))
+			goto bug;
+
 		return real;
 	}
 
+lower:
 	real = ovl_dentry_lower(dentry);
 	if (!real)
 		goto bug;
-- 
2.13.6

[PATCH v7 13/14] ovl: Fix encryption/compression 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).

Similarly if lower is compressed, report that in stat for a metacopy
only file.

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 7af8a292d54d..fe2d62cc07a7 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -66,6 +66,22 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 	return err;
 }
 
+#define OVL_STATX_ATTR_MASK	(STATX_ATTR_ENCRYPTED | STATX_ATTR_COMPRESSED)
+static void ovl_stat_fix_attributes(struct kstat *ustat, struct kstat *lstat) {
+	unsigned int attr_mask, attr;
+
+	attr_mask = lstat->attributes_mask & OVL_STATX_ATTR_MASK;
+	if (!attr_mask)
+		return;
+
+	attr = lstat->attributes & attr_mask;
+	if (!attr)
+		return;
+
+	ustat->attributes_mask |= attr_mask;
+	ustat->attributes |= attr;
+}
+
 int ovl_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int flags)
 {
@@ -123,8 +139,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_fix_attributes(stat, &lowerstat);
+			}
 		}
 		if (samefs) {
 			/*
-- 
2.13.6

[PATCH v7 14/14] 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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 3f59b98340f7..c70f5dbddd3b 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -276,9 +276,6 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 {
 	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 v7 06/14] ovl: Move couple of copy up functions in copy_up.c

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now two copy up related functions are part of inode.c. Amir suggested
> it might be better to move these to copy_up.c. Hence this patch does that.

IMO it is better to phrase simply "Move the copy up helpers to copy_up.c."

> There will one more related function which will come in later patch.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
>  fs/overlayfs/inode.c   | 30 ------------------------------
>  2 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 303794bb9127..f84ba12c9b05 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
>         return err;
>  }
>
> +static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
> +{
> +       if (ovl_dentry_upper(dentry) &&
> +           ovl_dentry_has_upper_alias(dentry))
> +               return false;
> +
> +       if (special_file(d_inode(dentry)->i_mode))
> +               return false;
> +
> +       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
> +               return false;
> +
> +       return true;
> +}
> +
> +int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
> +{
> +       int err = 0;
> +
> +       if (ovl_open_need_copy_up(dentry, file_flags)) {
> +               err = ovl_want_write(dentry);
> +               if (!err) {
> +                       err = ovl_copy_up_flags(dentry, file_flags);
> +                       ovl_drop_write(dentry);
> +               }
> +       }
> +
> +       return err;
> +}
> +

I would put there at the bottom ,below ovl_copy_up_flags, where they belong
logically (this file is sorted bottom up.

And also need to move the declaration of ovl_open_maybe_copy_up() in
overlayfs.h to the respective location.

Thanks,
Amir.

Re: [PATCH v7 13/14] ovl: Fix encryption/compression status of a metacopy only file

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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).
>
> Similarly if lower is compressed, report that in stat for a metacopy
> only file.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

> ---
>  fs/overlayfs/inode.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7af8a292d54d..fe2d62cc07a7 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -66,6 +66,22 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>         return err;
>  }
>
> +#define OVL_STATX_ATTR_MASK    (STATX_ATTR_ENCRYPTED | STATX_ATTR_COMPRESSED)

nit: add newline

> +static void ovl_stat_fix_attributes(struct kstat *ustat, struct kstat *lstat) {
> +       unsigned int attr_mask, attr;
> +
> +       attr_mask = lstat->attributes_mask & OVL_STATX_ATTR_MASK;
> +       if (!attr_mask)
> +               return;
> +
> +       attr = lstat->attributes & attr_mask;
> +       if (!attr)
> +               return;
> +
> +       ustat->attributes_mask |= attr_mask;
> +       ustat->attributes |= attr;
> +}
> +
>  int ovl_getattr(const struct path *path, struct kstat *stat,
>                 u32 request_mask, unsigned int flags)
>  {
> @@ -123,8 +139,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_fix_attributes(stat, &lowerstat);
> +                       }
>                 }
>                 if (samefs) {
>                         /*
> --
> 2.13.6
>

Re: [PATCH v7 10/14] ovl: Fix ovl_getattr() to get number of blocks from lower

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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.
>
> We need to be careful about races where we are doing stat on one cpu and
> data copy up is taking place on other cpu. We want to return stat->st_blocks
> either from lower or stable upper and not something in between. Hence,
> ovl_has_upperdata() is called first to figure out whether block reporting
> will take place from lower or upper.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

Re: [PATCH v7 11/14] ovl: Set OVL_UPPERDATA flag during ovl_lookup()

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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.

It is not completely clear that "this is only done" refers to the
xattr check and
not to setting the flag. Suggest to add text like this:

"... For the rest of the files, OVL_UPPERDATA flag is set unconditionally
if upper inode exists."

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

Re: [PATCH v7 04/14] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 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, 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     | 40 +++++++++++++++++++++++++++++++++++++---
>  3 files changed, 46 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 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 ee41bde87b14..d8b6b4d80d78 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,11 @@ 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)) {
> +               pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off and 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;
> @@ -923,7 +945,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>                 ofs->noxattr = true;
>                 ofs->config.redirect_dir = false;
>                 ofs->config.index = false;
> -               pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
> +               ofs->config.metacopy = false;
> +               pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off, metacopy=off and no opaque dir.\n");
>         } else {
>                 vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
>         }
> @@ -1164,6 +1187,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;
> @@ -1209,6 +1233,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;
> +               }
> +       }
> +
>

Fs can have upper but no workdir and you still need to verify lower
If metacopy is enabled

Amir.

Re: [PATCH v7 06/14] ovl: Move couple of copy up functions in copy_up.c

[Vivek Goyal]

On Fri, Nov 17, 2017 at 11:06:12AM +0200, Amir Goldstein wrote:
> On Fri, Nov 17, 2017 at 12:03 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Right now two copy up related functions are part of inode.c. Amir suggested
> > it might be better to move these to copy_up.c. Hence this patch does that.
> 
> IMO it is better to phrase simply "Move the copy up helpers to copy_up.c."
> 
> > There will one more related function which will come in later patch.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
> >  fs/overlayfs/inode.c   | 30 ------------------------------
> >  2 files changed, 30 insertions(+), 30 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 303794bb9127..f84ba12c9b05 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
> >         return err;
> >  }
> >
> > +static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
> > +{
> > +       if (ovl_dentry_upper(dentry) &&
> > +           ovl_dentry_has_upper_alias(dentry))
> > +               return false;
> > +
> > +       if (special_file(d_inode(dentry)->i_mode))
> > +               return false;
> > +
> > +       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> > +int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
> > +{
> > +       int err = 0;
> > +
> > +       if (ovl_open_need_copy_up(dentry, file_flags)) {
> > +               err = ovl_want_write(dentry);
> > +               if (!err) {
> > +                       err = ovl_copy_up_flags(dentry, file_flags);
> > +                       ovl_drop_write(dentry);
> > +               }
> > +       }
> > +
> > +       return err;
> > +}
> > +
> 
> I would put there at the bottom ,below ovl_copy_up_flags, where they belong
> logically (this file is sorted bottom up.

Ok, I will keep these functions below ovl_copy_up_flag().

But ovl_copy_up_meta_inode_data() is called by ovl_copy_up_one(). So will
keep that above ovl_copy_up_one().

> 
> And also need to move the declaration of ovl_open_maybe_copy_up() in
> overlayfs.h to the respective location.

Ok, will do.

Vivek

Re: [PATCH v7 07/14] ovl: Copy up only metadata during copy up where it makes sense

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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>

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

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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).
>
> 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 can be
> 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_open_maybe_copy_up()                ovl_copy_up_data()
>   open_open_need_copy_up()               vfs_removexattr()
>    ovl_already_copied_up()
>     ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
>      ovl_test_flag(OVL_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 the effects
> of preceeding operations (ex. upper that is not fully copied up), it will 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.
>

Vivek,

I like this version a lot more, but IMO it could still be even simpler.

> Note, we now set OVL_UPPERDATA on all kind of dentires and check
> OVL_UPPERDATA only where it makes sense. If a file is created on upper,
> we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
> path again. This time ordering dependency is w.r.t oe->__upperdata. We
> need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
> visible. Otherwise following code path might try to copy up an upper
> only file.

Why all this explanations?
Just use ovl_set_upperdata() when creating upper and be done with it.
Creating new upper is expensive anyway, so I don't think we should
care about an unneeded smp_wmb() and probably Miklos will know to
tell why it is not needed anyway.

It is very easy to make sure that the  OVL_UPPERDATA is always set
for the pure upper and non regular file cases and then we have no need
for ovl_should_check_upperdata().
Simple is better.

There is just one more thing you need to do before killing
ovl_should_check_upperdata() - set the OVL_UPPERDATA on the root
inode in ovl_fill_super().

>
> ovl_d_real()
>  ovl_open_maybe_copy_up()
>   open_open_need_copy_up()
>    ovl_already_copied_up()
>     ovl_dentry_needs_data_copy_up()
>      ovl_test_flag(OVL_UPPERDATA)
>          <-- OVL_UPPERDATA is not visible yet. copy up file.
>
> I have not introduced any barrier to take care of this. There is already
> a check ovl_should_check_upperdata() to make sure lower dentry is there
> otherwise return false (even if OVL_UPPERDATA is not visible yet).
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c   | 55 +++++++++++++++++++++++++++++++---
>  fs/overlayfs/dir.c       |  1 +
>  fs/overlayfs/overlayfs.h | 10 +++++--
>  fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 134 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b6fa5830c4c6..3f59b98340f7 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 = {
> @@ -234,7 +244,7 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
>
>  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))
> @@ -486,6 +496,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;
> @@ -519,8 +551,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;
> @@ -552,6 +595,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))
> @@ -674,7 +719,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)
> @@ -684,6 +729,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_locked(dentry, flags))
> +                       err = ovl_copy_up_meta_inode_data(&ctx);
>                 ovl_copy_up_end(dentry);
>         }
>         do_delayed_call(&done);
> @@ -700,7 +747,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/dir.c b/fs/overlayfs/dir.c
> index e13921824c70..fb3cd73c3693 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
>         ovl_dentry_version_inc(dentry->d_parent, false);
>         ovl_dentry_set_upper_alias(dentry);
>         if (!hardlink) {
> +               ovl_set_flag(OVL_UPPERDATA, inode);

Call ovl_set_upperdata() instead?

>                 ovl_inode_update(inode, newdentry);
>                 ovl_copyattr(newdentry->d_inode, inode);
>         } else {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 7f9a79b5a2b5..d4890e59b881 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,10 @@ 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_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags);
> +bool ovl_has_upperdata(struct dentry *dentry);
> +void ovl_set_upperdata(struct dentry *dentry);
>  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);
> @@ -225,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 572bf46e9f2e..73578bfe9f4b 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -231,6 +231,64 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
>         oe->has_upper = 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;
> +


IMO we don't need this check, but you may leave it as
if (WARN_ON(!ovl_should_check_upperdata(dentry)
after ovl_test_flag() to be on the safe side.


> +       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> +               return false;
> +       /*
> +        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure

To be consistent, you should update the comment to say pairs with... in
ovl_set_upperdata(), although it may be useful to leave the information that
the main user of ovl_set_upperdata() is ovl_copy_up_meta_inode_data().

> +        * 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) {
> +       /*
> +        * 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));
> +}
> +
> +static inline bool open_for_write(int flags)

Need ovl_ namespace prefix
and the name sounds like an action (i.e. a request to open for write)

> +{
> +       if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))

What I meant in previous review is that you make a static inline helper
in overlayfs.h to be used by this and ovl_open_need_copy_up().
Your helper is missing the O_TRUNC test, i.e.

return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));

It may sound like we don't *need* to copy up data on O_TRUNC,
but in fact, if we don't call copy up functions, we won't be cleaning the
metacopy xattr/flag on truncate.

The thing is that the result of O_TRUNC|O_RDONLY is not even well
defined by man page ("... Otherwise, the effect of O_TRUNC is unspecified"),
but we shouldn't change the way overlayfs treats this strange combination.

> +               return true;
> +
> +       return false;
> +}
> +
> +/* Caller should hold ovl_entry->locked */

...should hold ovl_inode->lock

Eventually, Miklos may decide that the _locked variants are not needed
and that it is
not expensive to call the lockless variants in locked path, but I am happy we
can see how this looks like. It looks cleaner to me and not over complicated.

Thanks,
Amir.

Re: [PATCH v7 12/14] ovl: Do not expose metacopy only upper dentry from d_real()

[Amir Goldstein]

On Fri, Nov 17, 2017 at 12:03 AM, 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 expose metacopy only dentry/inode to vfs using d_real().
>
> For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
> dentry inode is metacopy only and does not have data, return lower dentry.
>
> 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>

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

Re: [PATCH v7 06/14] ovl: Move couple of copy up functions in copy_up.c

[Amir Goldstein]

On Fri, Nov 17, 2017 at 5:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Nov 17, 2017 at 11:06:12AM +0200, Amir Goldstein wrote:
>> On Fri, Nov 17, 2017 at 12:03 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Right now two copy up related functions are part of inode.c. Amir suggested
>> > it might be better to move these to copy_up.c. Hence this patch does that.
>>
>> IMO it is better to phrase simply "Move the copy up helpers to copy_up.c."
>>
>> > There will one more related function which will come in later patch.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/copy_up.c | 30 ++++++++++++++++++++++++++++++
>> >  fs/overlayfs/inode.c   | 30 ------------------------------
>> >  2 files changed, 30 insertions(+), 30 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > index 303794bb9127..f84ba12c9b05 100644
>> > --- a/fs/overlayfs/copy_up.c
>> > +++ b/fs/overlayfs/copy_up.c
>> > @@ -232,6 +232,36 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
>> >         return err;
>> >  }
>> >
>> > +static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
>> > +{
>> > +       if (ovl_dentry_upper(dentry) &&
>> > +           ovl_dentry_has_upper_alias(dentry))
>> > +               return false;
>> > +
>> > +       if (special_file(d_inode(dentry)->i_mode))
>> > +               return false;
>> > +
>> > +       if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
>> > +               return false;
>> > +
>> > +       return true;
>> > +}
>> > +
>> > +int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
>> > +{
>> > +       int err = 0;
>> > +
>> > +       if (ovl_open_need_copy_up(dentry, file_flags)) {
>> > +               err = ovl_want_write(dentry);
>> > +               if (!err) {
>> > +                       err = ovl_copy_up_flags(dentry, file_flags);
>> > +                       ovl_drop_write(dentry);
>> > +               }
>> > +       }
>> > +
>> > +       return err;
>> > +}
>> > +
>>
>> I would put there at the bottom ,below ovl_copy_up_flags, where they belong
>> logically (this file is sorted bottom up.
>
> Ok, I will keep these functions below ovl_copy_up_flag().
>
> But ovl_copy_up_meta_inode_data() is called by ovl_copy_up_one(). So will
> keep that above ovl_copy_up_one().
>

Of course, it is an inner function and those are outer functions.

Amir.

Re: [PATCH v7 04/14] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Fri, Nov 17, 2017 at 01:23:58PM +0200, Amir Goldstein wrote:

[..]
> > @@ -1164,6 +1187,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;
> > @@ -1209,6 +1233,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;
> > +               }
> > +       }
> > +
> >
> 
> Fs can have upper but no workdir and you still need to verify lower
> If metacopy is enabled

Hmm..., trying to understand this. This probably is more involved.

So first use case is that if metacopy is being enabled, verify lower
root is upper root origin. (Even if it is read-only fs).

if (ofs->upper_mnt && ofs->config.metacopy)
	ovl_verify_origin().

But this does not cover the case of same fs being remounted with
metacopy=off. In that case we will not do metacopy only copy ups but
existing metacopy inodes will still require that lower does not change.

Will it make sense to set OVL_METACOPY xattr on upper when metacopy=on
is done first time on mount. And later in subsequent mounts, if METACOPY
is set on upper, make sure to verify origin and make sure lower supports
file handles etc.

Thanks
Vivek

Re: [PATCH v7 04/14] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Fri, Nov 17, 2017 at 6:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Nov 17, 2017 at 01:23:58PM +0200, Amir Goldstein wrote:
>
> [..]
>> > @@ -1164,6 +1187,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;
>> > @@ -1209,6 +1233,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;
>> > +               }
>> > +       }
>> > +
>> >
>>
>> Fs can have upper but no workdir and you still need to verify lower
>> If metacopy is enabled
>
> Hmm..., trying to understand this. This probably is more involved.
>
> So first use case is that if metacopy is being enabled, verify lower
> root is upper root origin. (Even if it is read-only fs).
>
> if (ofs->upper_mnt && ofs->config.metacopy)
>         ovl_verify_origin().

Yes, this is needed in case workdir/work dir creation fails and fs is mounted
read-only. You should not allow it with metacopy on unverified lower.

>
> But this does not cover the case of same fs being remounted with
> metacopy=off. In that case we will not do metacopy only copy ups but
> existing metacopy inodes will still require that lower does not change.
>
> Will it make sense to set OVL_METACOPY xattr on upper when metacopy=on
> is done first time on mount. And later in subsequent mounts, if METACOPY
> is set on upper, make sure to verify origin and make sure lower supports
> file handles etc.
>

This is an related to what we discussed at length on the ovl-features
patches.

Yes, we SHOULD mark that the metacopy feature was enabled, at least
on first meta_copy_up().

No, we should not do that by marking root upper with metacopy xattr.
this would be very confusing and inconsistent.
We can either set trusted.overlay.features on upper root as Miklos suggested
or incompat_feature/ directory as I suggested, but we need to decide on
a consistent scheme we can use for other features as well.

I guess metacopy must have this infrastructure in place, because as you
correctly noted, we should not mount an overlay with metacopied inodes
without verifying lower and file handle support.

Amir.

Re: [PATCH v7 04/14] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Fri, Nov 17, 2017 at 06:32:52PM +0200, Amir Goldstein wrote:
> On Fri, Nov 17, 2017 at 6:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Nov 17, 2017 at 01:23:58PM +0200, Amir Goldstein wrote:
> >
> > [..]
> >> > @@ -1164,6 +1187,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;
> >> > @@ -1209,6 +1233,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;
> >> > +               }
> >> > +       }
> >> > +
> >> >
> >>
> >> Fs can have upper but no workdir and you still need to verify lower
> >> If metacopy is enabled
> >
> > Hmm..., trying to understand this. This probably is more involved.
> >
> > So first use case is that if metacopy is being enabled, verify lower
> > root is upper root origin. (Even if it is read-only fs).
> >
> > if (ofs->upper_mnt && ofs->config.metacopy)
> >         ovl_verify_origin().
> 
> Yes, this is needed in case workdir/work dir creation fails and fs is mounted
> read-only. You should not allow it with metacopy on unverified lower.
> 
> >
> > But this does not cover the case of same fs being remounted with
> > metacopy=off. In that case we will not do metacopy only copy ups but
> > existing metacopy inodes will still require that lower does not change.
> >
> > Will it make sense to set OVL_METACOPY xattr on upper when metacopy=on
> > is done first time on mount. And later in subsequent mounts, if METACOPY
> > is set on upper, make sure to verify origin and make sure lower supports
> > file handles etc.
> >
> 
> This is an related to what we discussed at length on the ovl-features
> patches.
> 
> Yes, we SHOULD mark that the metacopy feature was enabled, at least
> on first meta_copy_up().
> 
> No, we should not do that by marking root upper with metacopy xattr.
> this would be very confusing and inconsistent.
> We can either set trusted.overlay.features on upper root as Miklos suggested
> or incompat_feature/ directory as I suggested, but we need to decide on
> a consistent scheme we can use for other features as well.

Agreed. We need a generic sheme to mark what features have been enabled
and what are incomapt features and which ones are rocompat etc and then
all current and future overlayfs features can make use of that generic
infrastructure.

> 
> I guess metacopy must have this infrastructure in place, because as you
> correctly noted, we should not mount an overlay with metacopied inodes
> without verifying lower and file handle support.

Ok, so for now, I will just verify lower when metacopy is enabled. For
other case, will wait for something generic to be merged upstream.

Vivek

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

On Fri, Nov 17, 2017 at 06:07:24PM +0200, Amir Goldstein wrote:
> On Fri, Nov 17, 2017 at 12:03 AM, 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).
> >
> > 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 can be
> > 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_open_maybe_copy_up()                ovl_copy_up_data()
> >   open_open_need_copy_up()               vfs_removexattr()
> >    ovl_already_copied_up()
> >     ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
> >      ovl_test_flag(OVL_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 the effects
> > of preceeding operations (ex. upper that is not fully copied up), it will 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.
> >
> 
> Vivek,
> 

[..]
> I like this version a lot more, but IMO it could still be even simpler.
> 
> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
> > visible. Otherwise following code path might try to copy up an upper
> > only file.
> 
> Why all this explanations?
> Just use ovl_set_upperdata() when creating upper and be done with it.

Just using ovl_set_upperdata() will not do away with ordering dependency
right? I mean, setting OVL_UPPERDATA in file creation path has different
ordering requirement (same is the case of >has_upper). And I wanted to
highlight that ordering requirement in changelogs.

I can get rid of it. But this seems such a subtle requirement, I think
its a good idea to talk about it explicitly.

> Creating new upper is expensive anyway, so I don't think we should
> care about an unneeded smp_wmb() and probably Miklos will know to
> tell why it is not needed anyway.

Ok, I can call ovl_set_upperdata() in creation path and not worry about
extra unneeded smp_wmb().

> 
> It is very easy to make sure that the  OVL_UPPERDATA is always set
> for the pure upper and non regular file cases and then we have no need
> for ovl_should_check_upperdata().
> Simple is better.

It avoids lots of smp_rmb() calls on files which should not have
OVL_UPPERDATA. Now I don't know what is more expensive. smp_rmb() or
calling ovl_should_check_upperdata().

Also, it calls ovl_dentry_lower() and that covers the possible race
with setting of OVL_UPPERDATA on file creation and setting of
oi->__upperdentry.

So if I remove this check, in-theory we open that race.

> 
> There is just one more thing you need to do before killing
> ovl_should_check_upperdata() - set the OVL_UPPERDATA on the root
> inode in ovl_fill_super().

That's a good point. Will do.

[..]
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index e13921824c70..fb3cd73c3693 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
> >         ovl_dentry_version_inc(dentry->d_parent, false);
> >         ovl_dentry_set_upper_alias(dentry);
> >         if (!hardlink) {
> > +               ovl_set_flag(OVL_UPPERDATA, inode);
> 
> Call ovl_set_upperdata() instead?


Will do.

[..]
> > +bool ovl_has_upperdata(struct dentry *dentry) {
> > +       if (!ovl_should_check_upperdata(dentry))
> > +               return true;
> > +
> 
> 
> IMO we don't need this check, but you may leave it as
> if (WARN_ON(!ovl_should_check_upperdata(dentry)
> after ovl_test_flag() to be on the safe side.

I primarily kept it as it avoided smp_rmb() for directories and
non-regular files.

Also not sure how can I use it as WARN_ON(). Now OVL_UPPERDATA is set
on all files/dir. So we will hit WARN_ON() all the time?

> 
> 
> > +       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> > +               return false;
> > +       /*
> > +        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
> 
> To be consistent, you should update the comment to say pairs with... in
> ovl_set_upperdata(), although it may be useful to leave the information that
> the main user of ovl_set_upperdata() is ovl_copy_up_meta_inode_data().
> 

Ok.

> > +        * 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) {
> > +       /*
> > +        * 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));
> > +}
> > +
> > +static inline bool open_for_write(int flags)
> 
> Need ovl_ namespace prefix
> and the name sounds like an action (i.e. a request to open for write)

ovl_opened_for_write_or_trunc()?

> 
> > +{
> > +       if (flags && (OPEN_FMODE(flags) & FMODE_WRITE))
> 
> What I meant in previous review is that you make a static inline helper
> in overlayfs.h to be used by this and ovl_open_need_copy_up().
> Your helper is missing the O_TRUNC test, i.e.
> 
> return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
> 
> It may sound like we don't *need* to copy up data on O_TRUNC,
> but in fact, if we don't call copy up functions, we won't be cleaning the
> metacopy xattr/flag on truncate.
> 
> The thing is that the result of O_TRUNC|O_RDONLY is not even well
> defined by man page ("... Otherwise, the effect of O_TRUNC is unspecified"),
> but we shouldn't change the way overlayfs treats this strange combination.

Hmm.., I had not thought about O_TRUNC|O_RDONLY for a metacopy only file.
Will check for  it as well.

Vivek

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Amir Goldstein]

On Fri, Nov 17, 2017 at 10:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Nov 17, 2017 at 06:07:24PM +0200, Amir Goldstein wrote:
>> On Fri, Nov 17, 2017 at 12:03 AM, 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).
>> >
>> > 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 can be
>> > 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_open_maybe_copy_up()                ovl_copy_up_data()
>> >   open_open_need_copy_up()               vfs_removexattr()
>> >    ovl_already_copied_up()
>> >     ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
>> >      ovl_test_flag(OVL_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 the effects
>> > of preceeding operations (ex. upper that is not fully copied up), it will 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.
>> >
>>
>> Vivek,
>>
>
> [..]
>> I like this version a lot more, but IMO it could still be even simpler.
>>
>> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
>> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
>> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
>> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
>> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
>> > visible. Otherwise following code path might try to copy up an upper
>> > only file.
>>
>> Why all this explanations?
>> Just use ovl_set_upperdata() when creating upper and be done with it.
>
> Just using ovl_set_upperdata() will not do away with ordering dependency
> right? I mean, setting OVL_UPPERDATA in file creation path has different
> ordering requirement (same is the case of >has_upper). And I wanted to
> highlight that ordering requirement in changelogs.
>
> I can get rid of it. But this seems such a subtle requirement, I think
> its a good idea to talk about it explicitly.
>

I am not following. you only need to ovl_set_upperdata() before
ovl_inode_update(), just like in regular copy up. Am I missing something
subtle?

>> Creating new upper is expensive anyway, so I don't think we should
>> care about an unneeded smp_wmb() and probably Miklos will know to
>> tell why it is not needed anyway.
>
> Ok, I can call ovl_set_upperdata() in creation path and not worry about
> extra unneeded smp_wmb().
>
>>
>> It is very easy to make sure that the  OVL_UPPERDATA is always set
>> for the pure upper and non regular file cases and then we have no need
>> for ovl_should_check_upperdata().
>> Simple is better.
>
> It avoids lots of smp_rmb() calls on files which should not have
> OVL_UPPERDATA. Now I don't know what is more expensive. smp_rmb() or
> calling ovl_should_check_upperdata().

You are right. smp_rmb() is potentially more expensive with many CPUs.

>
> Also, it calls ovl_dentry_lower() and that covers the possible race
> with setting of OVL_UPPERDATA on file creation and setting of
> oi->__upperdentry.
>
> So if I remove this check, in-theory we open that race.
>
>>
>> There is just one more thing you need to do before killing
>> ovl_should_check_upperdata() - set the OVL_UPPERDATA on the root
>> inode in ovl_fill_super().
>
> That's a good point. Will do.
>
> [..]
>> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> > index e13921824c70..fb3cd73c3693 100644
>> > --- a/fs/overlayfs/dir.c
>> > +++ b/fs/overlayfs/dir.c
>> > @@ -158,6 +158,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
>> >         ovl_dentry_version_inc(dentry->d_parent, false);
>> >         ovl_dentry_set_upper_alias(dentry);
>> >         if (!hardlink) {
>> > +               ovl_set_flag(OVL_UPPERDATA, inode);
>>
>> Call ovl_set_upperdata() instead?
>
>
> Will do.
>
> [..]
>> > +bool ovl_has_upperdata(struct dentry *dentry) {
>> > +       if (!ovl_should_check_upperdata(dentry))
>> > +               return true;
>> > +
>>
>>
>> IMO we don't need this check, but you may leave it as
>> if (WARN_ON(!ovl_should_check_upperdata(dentry)
>> after ovl_test_flag() to be on the safe side.
>
> I primarily kept it as it avoided smp_rmb() for directories and
> non-regular files.
>

You are right. I forgot.

> Also not sure how can I use it as WARN_ON(). Now OVL_UPPERDATA is set
> on all files/dir. So we will hit WARN_ON() all the time?
>

Right, my bad.

>>
>>
>> > +       if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
>> > +               return false;
>> > +       /*
>> > +        * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
>>
>> To be consistent, you should update the comment to say pairs with... in
>> ovl_set_upperdata(), although it may be useful to leave the information that
>> the main user of ovl_set_upperdata() is ovl_copy_up_meta_inode_data().
>>
>
> Ok.
>
>> > +        * 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) {
>> > +       /*
>> > +        * 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));
>> > +}
>> > +
>> > +static inline bool open_for_write(int flags)
>>
>> Need ovl_ namespace prefix
>> and the name sounds like an action (i.e. a request to open for write)
>
> ovl_opened_for_write_or_trunc()?

OK. maybe ovl_open_flags_need_copy_up() ?

Amir.

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:

[..]
> >> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
> >> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
> >> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
> >> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
> >> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
> >> > visible. Otherwise following code path might try to copy up an upper
> >> > only file.
> >>
> >> Why all this explanations?
> >> Just use ovl_set_upperdata() when creating upper and be done with it.
> >
> > Just using ovl_set_upperdata() will not do away with ordering dependency
> > right? I mean, setting OVL_UPPERDATA in file creation path has different
> > ordering requirement (same is the case of >has_upper). And I wanted to
> > highlight that ordering requirement in changelogs.
> >
> > I can get rid of it. But this seems such a subtle requirement, I think
> > its a good idea to talk about it explicitly.
> >
> 
> I am not following. you only need to ovl_set_upperdata() before
> ovl_inode_update(), just like in regular copy up. Am I missing something
> subtle?

Hi Amir,

Right. This is ordering dependency between setting of OVL_UPPERDATA and
oi->__upperdentry.

So as per barrier documentation file, read/write barrier should look
as follows.

	CPU1				CPU2
     set OVL_UPPERDATA			smp_rmb()
     smp_wmb()				Load oi->__upperdentry
     set oi->__upperdentry

So we need to place smp_wmb() after setting OVL_UPPERDATA and place
smp_rmb() just before load of oi->__upperdentry.

Instead of smp_rmb() we are relying on checking for ovl_dentry_lower().
That is even if OVL_UPPERDATA is not visible on CPU2, but
oi->__upperdentry is visible, then we don't try to copy up a file which
does not have a lower.

So ovl_dentry_lower() is preventing the race which *in-theory* can trigger
trying to copy up file which does not have lower. 

Does this make sense?


[..]
> >> > +static inline bool open_for_write(int flags)
> >>
> >> Need ovl_ namespace prefix
> >> and the name sounds like an action (i.e. a request to open for write)
> >
> > ovl_opened_for_write_or_trunc()?
> 
> OK. maybe ovl_open_flags_need_copy_up() ?

Ok, will use ovl_open_flags_need_copy_up().

Vivek

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Amir Goldstein]

On Mon, Nov 20, 2017 at 4:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:
>
> [..]
>> >> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
>> >> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
>> >> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
>> >> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
>> >> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
>> >> > visible. Otherwise following code path might try to copy up an upper
>> >> > only file.
>> >>
>> >> Why all this explanations?
>> >> Just use ovl_set_upperdata() when creating upper and be done with it.
>> >
>> > Just using ovl_set_upperdata() will not do away with ordering dependency
>> > right? I mean, setting OVL_UPPERDATA in file creation path has different
>> > ordering requirement (same is the case of >has_upper). And I wanted to
>> > highlight that ordering requirement in changelogs.
>> >
>> > I can get rid of it. But this seems such a subtle requirement, I think
>> > its a good idea to talk about it explicitly.
>> >
>>
>> I am not following. you only need to ovl_set_upperdata() before
>> ovl_inode_update(), just like in regular copy up. Am I missing something
>> subtle?
>
> Hi Amir,
>
> Right. This is ordering dependency between setting of OVL_UPPERDATA and
> oi->__upperdentry.
>
> So as per barrier documentation file, read/write barrier should look
> as follows.
>
>         CPU1                            CPU2
>      set OVL_UPPERDATA                  smp_rmb()
>      smp_wmb()                          Load oi->__upperdentry
>      set oi->__upperdentry
>
> So we need to place smp_wmb() after setting OVL_UPPERDATA and place
> smp_rmb() just before load of oi->__upperdentry.
>
> Instead of smp_rmb() we are relying on checking for ovl_dentry_lower().
> That is even if OVL_UPPERDATA is not visible on CPU2, but
> oi->__upperdentry is visible, then we don't try to copy up a file which
> does not have a lower.
>
> So ovl_dentry_lower() is preventing the race which *in-theory* can trigger
> trying to copy up file which does not have lower.
>
> Does this make sense?
>

Maybe. Brain overflow.
To put it shortly, if there is a problem with setting OVL_UPPERDATA on new upper
then there is a problem with setting upper alias as well (as you pointed out).
I don't think that is really the case, but I am not the one to explain why.
It just does not make sense that d_instantiate doesn't make sure that inode
fields are consistent before attaching the inode to dentry.
I think the barriers hide in raw_write_seqcount_begin()/raw_seqcount_begin()
for the lockless dentry cache lookup, but I'm not an expert.

Anyway, I find the commit message text from "If a file is created on upper,..."
very confusing.
I suggest that if you are not sure about this leave a "XXX" comment in the
code path of creating new upper and setting  OVL_UPPERDATA flag or
"TODO" comment if you thing there is something to be done.

It is easy to later apply a patch to remove the XXX comment after explaining it
or remove TODO comment after fixing it.
A confusing text remains in git log forever and get git history
researchers off track.

Amir.

Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

On Mon, Nov 20, 2017 at 05:18:48PM +0200, Amir Goldstein wrote:
> On Mon, Nov 20, 2017 at 4:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:
> >
> > [..]
> >> >> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
> >> >> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
> >> >> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
> >> >> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
> >> >> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
> >> >> > visible. Otherwise following code path might try to copy up an upper
> >> >> > only file.
> >> >>
> >> >> Why all this explanations?
> >> >> Just use ovl_set_upperdata() when creating upper and be done with it.
> >> >
> >> > Just using ovl_set_upperdata() will not do away with ordering dependency
> >> > right? I mean, setting OVL_UPPERDATA in file creation path has different
> >> > ordering requirement (same is the case of >has_upper). And I wanted to
> >> > highlight that ordering requirement in changelogs.
> >> >
> >> > I can get rid of it. But this seems such a subtle requirement, I think
> >> > its a good idea to talk about it explicitly.
> >> >
> >>
> >> I am not following. you only need to ovl_set_upperdata() before
> >> ovl_inode_update(), just like in regular copy up. Am I missing something
> >> subtle?
> >
> > Hi Amir,
> >
> > Right. This is ordering dependency between setting of OVL_UPPERDATA and
> > oi->__upperdentry.
> >
> > So as per barrier documentation file, read/write barrier should look
> > as follows.
> >
> >         CPU1                            CPU2
> >      set OVL_UPPERDATA                  smp_rmb()
> >      smp_wmb()                          Load oi->__upperdentry
> >      set oi->__upperdentry
> >
> > So we need to place smp_wmb() after setting OVL_UPPERDATA and place
> > smp_rmb() just before load of oi->__upperdentry.
> >
> > Instead of smp_rmb() we are relying on checking for ovl_dentry_lower().
> > That is even if OVL_UPPERDATA is not visible on CPU2, but
> > oi->__upperdentry is visible, then we don't try to copy up a file which
> > does not have a lower.
> >
> > So ovl_dentry_lower() is preventing the race which *in-theory* can trigger
> > trying to copy up file which does not have lower.
> >
> > Does this make sense?
> >
> 
> Maybe. Brain overflow.
> To put it shortly, if there is a problem with setting OVL_UPPERDATA on new upper
> then there is a problem with setting upper alias as well (as you pointed out).
> I don't think that is really the case, but I am not the one to explain why.
> It just does not make sense that d_instantiate doesn't make sure that inode
> fields are consistent before attaching the inode to dentry.
> I think the barriers hide in raw_write_seqcount_begin()/raw_seqcount_begin()
> for the lockless dentry cache lookup, but I'm not an expert.
> 
> Anyway, I find the commit message text from "If a file is created on upper,..."
> very confusing.
> I suggest that if you are not sure about this leave a "XXX" comment in the
> code path of creating new upper and setting  OVL_UPPERDATA flag or
> "TODO" comment if you thing there is something to be done.
> 
> It is easy to later apply a patch to remove the XXX comment after explaining it
> or remove TODO comment after fixing it.

Ok, for now, I will just put a comment in code and deal with it later if
need be. Once it happens, it will be a NULL pointer dereference as we might
try to copy up a file which does not have a lower. So we will know if
problem happens. :-)

Vivek

> A confusing text remains in git log forever and get git history
> researchers off track.
> 
> Amir.