[RFC PATCH 0/7] overlayfs: Delayed copy up of data

[RFC PATCH 0/7] overlayfs: Delayed copy up of data

[Vivek Goyal]

Hi,

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. I have not tested it though.

So here are very crude RFC patch. I have done bare minimal testing on
these and there are many unaddressed issues I can see. 

Before I go any further, I wanted to send these out for some feedback
and see if I am moving in right direction or this appraoch is completely
broken.

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

 
Vivek Goyal (7):
  Create origin xattr on copy up for all files
  ovl: During copy up, first copy up metadata and then data
  ovl: Copy up only metadata during copy up where it makes sense
  ovl: Set xattr OVL_XATTR_METACOPY on upper file
  ovl: Set OVL_METACOPY flag during ovl_lookup()
  ovl: Return lower dentry if only metadata copy up took place
  ovl: Fix ovl_getattr() to get size from lower

 fs/overlayfs/copy_up.c   | 87 +++++++++++++++++++++++++++++++++++++-----------
 fs/overlayfs/inode.c     | 13 +++++++-
 fs/overlayfs/namei.c     | 34 +++++++++++++++++++
 fs/overlayfs/overlayfs.h |  5 ++-
 fs/overlayfs/super.c     |  4 +++
 fs/overlayfs/util.c      | 21 ++++++++++--
 6 files changed, 140 insertions(+), 24 deletions(-)

-- 
2.13.5

[PATCH 1/7] 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. I am
hoping it does not break anything because we do OVL_INDEX test before
using inode number of origin.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index aad97b30d5e6..f1b15e5c37d3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -538,9 +538,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);
@@ -598,6 +595,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 		.parent = parent,
 		.dentry = dentry,
 		.workdir = ovl_workdir(dentry),
+		.origin = true,
 	};
 
 	if (WARN_ON(!ctx.workdir))
-- 
2.13.5

[PATCH 2/7] ovl: During copy up, first copy up metadata and then data

[Vivek Goyal]

This just helps with later patches where after copying up metadata, we
skip data copying step, if needed.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f1b15e5c37d3..cdc4d79a1249 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -445,18 +445,6 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
 
-	if (S_ISREG(c->stat.mode)) {
-		struct path upperpath;
-
-		ovl_path_upper(c->dentry, &upperpath);
-		BUG_ON(upperpath.dentry != NULL);
-		upperpath.dentry = temp;
-
-		err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
-		if (err)
-			return err;
-	}
-
 	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
 	if (err)
 		return err;
@@ -480,6 +468,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
+	if (S_ISREG(c->stat.mode)) {
+		struct path upperpath;
+
+		ovl_path_upper(c->dentry, &upperpath);
+		BUG_ON(upperpath.dentry != NULL);
+		upperpath.dentry = temp;
+
+		err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
-- 
2.13.5

[PATCH 3/7] 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.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index cdc4d79a1249..b2d9ed81e9ff 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -306,11 +306,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;
@@ -328,6 +325,7 @@ struct ovl_copy_up_ctx {
 	struct dentry *workdir;
 	bool tmpfile;
 	bool origin;
+	bool metadata_only;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -464,11 +462,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 */
 	if (c->origin) {
 		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->metadata_only = false;
+		}
 	}
 
-	if (S_ISREG(c->stat.mode)) {
+	if (S_ISREG(c->stat.mode) && !c->metadata_only) {
 		struct path upperpath;
 
 		ovl_path_upper(c->dentry, &upperpath);
@@ -596,6 +598,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 		.dentry = dentry,
 		.workdir = ovl_workdir(dentry),
 		.origin = true,
+		.metadata_only = true,
 	};
 
 	if (WARN_ON(!ctx.workdir))
@@ -616,9 +619,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	if (err)
 		return err;
 
+	if (!S_ISREG(ctx.stat.mode))
+		ctx.metadata_only = false;
+
 	/* maybe truncate regular file. this has no effect on dirs */
-	if (flags & O_TRUNC)
+	if (flags & O_TRUNC) {
 		ctx.stat.size = 0;
+		ctx.metadata_only = false;
+	}
+
+	if (flags & (OPEN_FMODE(flags) & FMODE_WRITE))
+		ctx.metadata_only = false;
 
 	if (S_ISLNK(ctx.stat.mode)) {
 		ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
-- 
2.13.5

[PATCH 4/7] ovl: Set xattr OVL_XATTR_METACOPY on upper file

[Vivek Goyal]

Presence of OVL_XATTR_METACOPY reflects that file has been copied up
with metadata only and data needs to be copied up later from lower.
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_METACOPY which reflects
whether ovl inode has only metadata copied up.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
 fs/overlayfs/inode.c     |  3 ++-
 fs/overlayfs/overlayfs.h |  5 ++++-
 fs/overlayfs/util.c      | 21 +++++++++++++++++++--
 4 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b2d9ed81e9ff..d76a4272b43e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -439,6 +439,26 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	goto out;
 }
 
+static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
+{
+	struct path upperpath;
+	int err;
+
+	ovl_path_upper(c->dentry, &upperpath);
+	BUG_ON(upperpath.dentry == NULL);
+
+	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_clear_flag(OVL_METACOPY, d_inode(c->dentry));
+	return err;
+}
+
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
@@ -482,6 +502,13 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
+	if (c->metadata_only) {
+		err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+					 NULL, 0, -EOPNOTSUPP);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -511,6 +538,9 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 		goto out_cleanup;
 
 	ovl_inode_update(d_inode(c->dentry), newdentry);
+	if (c->metadata_only) {
+		ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
+	}
 out:
 	dput(temp);
 	return err;
@@ -638,7 +668,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)
@@ -648,6 +678,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			err = ovl_do_copy_up(&ctx);
 		if (!err && !ovl_dentry_has_upper_alias(dentry))
 			err = ovl_link_up(&ctx);
+		if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
+			err = ovl_copy_up_data_inode(&ctx);
 		ovl_copy_up_end(dentry);
 	}
 	do_delayed_call(&done);
@@ -659,6 +691,10 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 {
 	int err = 0;
 	const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
+	bool data_copy_up = false;
+
+	if (flags & (OPEN_FMODE(flags) & FMODE_WRITE))
+                data_copy_up = true;
 
 	while (!err) {
 		struct dentry *next;
@@ -678,8 +714,10 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		 *      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)) {
 			break;
+		}
 
 		next = dget(dentry);
 		/* find the topmost dentry not yet copied up */
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index a619addecafc..e5825b8948e0 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -320,7 +320,8 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
 	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	    ovl_dentry_has_upper_alias(dentry) &&
+	    !ovl_dentry_needs_data_copy_up(dentry, flags))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d4e8c1a08fb0..28969d64af0d 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -26,10 +26,12 @@ 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 {
 	OVL_IMPURE,
 	OVL_INDEX,
+	OVL_METACOPY,
 };
 
 /*
@@ -211,6 +213,7 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
 bool ovl_dentry_has_upper_alias(struct dentry *dentry);
 void ovl_dentry_set_upper_alias(struct dentry *dentry);
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags);
 bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
@@ -221,7 +224,7 @@ 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_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 117794582f9f..946eec488a17 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -227,6 +227,22 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
 	oe->has_upper = true;
 }
 
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return false;
+
+	if (!flags)
+		return false;
+
+	if (!(OPEN_FMODE(flags) & FMODE_WRITE))
+		return false;
+
+	if (!ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
+		return false;
+
+	return true;
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
@@ -310,13 +326,14 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
-int ovl_copy_up_start(struct dentry *dentry)
+int ovl_copy_up_start(struct dentry *dentry, int flags)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(dentry));
 	int err;
 
 	err = mutex_lock_interruptible(&oi->lock);
-	if (!err && ovl_dentry_has_upper_alias(dentry)) {
+	if (!err && ovl_dentry_has_upper_alias(dentry) &&
+	    !ovl_dentry_needs_data_copy_up(dentry, flags)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
-- 
2.13.5

[PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Vivek Goyal]

During lookup, check for presence of OVL_XATTR_METACOPY and if present,
set OVL_METACOPY bit in flags.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index c3addd1114f1..9b6f9afa4f75 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -26,6 +26,24 @@ 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;
+
+	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)
 {
@@ -591,6 +609,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,
@@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 					       roe->numlower, &stack, &ctr);
 			if (err)
 				goto out;
+
+			err = ovl_check_metacopy(upperdentry);
+			if (err < 0)
+				goto out;
+			if (err == 1)
+				metacopy = true;
 		}
 
 		if (d.redirect) {
@@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		OVL_I(inode)->redirect = upperredirect;
 		if (index)
 			ovl_set_flag(OVL_INDEX, inode);
+
+		if (metacopy) {
+			/*
+			 * TODO: What happens we if find metacopy xattr but
+			 * could not find/resolve origin.
+			 */
+			WARN_ON(!ctr);
+			ovl_set_flag(OVL_METACOPY, inode);
+		}
 	}
 
 	revert_creds(old_cred);
-- 
2.13.5

[PATCH 6/7] ovl: Return lower dentry if only metadata copy up took place

[Vivek Goyal]

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

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fd5ea4facc62..79da8276ae82 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -96,10 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 			err = ovl_check_append_only(d_inode(real), open_flags);
 			if (err)
 				return ERR_PTR(err);
+
+			if (ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
+				goto lower;
 		}
 		return real;
 	}
 
+lower:
 	real = ovl_dentry_lower(dentry);
 	if (!real)
 		goto bug;
-- 
2.13.5

[PATCH 7/7] ovl: Fix ovl_getattr() to get size from lower

[Vivek Goyal]

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

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index e5825b8948e0..1b676deacc2d 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -140,6 +140,16 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	if (!is_dir && ovl_test_flag(OVL_INDEX, d_inode(dentry)))
 		stat->nlink = dentry->d_inode->i_nlink;
 
+	if (ovl_test_flag(OVL_METACOPY, d_inode(dentry))) {
+		struct kstat lowerstat;
+		u32 lowermask = STATX_SIZE;
+
+		ovl_path_lower(dentry, &realpath);
+		err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
+		if (err)
+			goto out;
+		stat->size = lowerstat.size;
+	}
 out:
 	revert_creds(old_cred);
 
-- 
2.13.5

Re: [RFC PATCH 0/7] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Hi,
>
> 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. I have not tested it though.
>
> So here are very crude RFC patch. I have done bare minimal testing on
> these and there are many unaddressed issues I can see.
>
> Before I go any further, I wanted to send these out for some feedback
> and see if I am moving in right direction or this appraoch is completely
> broken.
>

I like the direction this is going :)
Beyond the important use case you listed, this could be useful also for:
1. copy up of lower hardlink in ovl_nlink_start(), just to have a place holder
    inode for OVL_XATTR_NLINK
2. similar case as above needed for NFS export of lower hardlink
3. possible starting point for consistent ro/rw file descriptor, see POC:
    https://github.com/amir73il/linux/commits/ovl-rocopyup
    your patches actually take off where my patches stop

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

Your feature is relying on OVL_XATTR_ORIGIN, and so does index feature.
There are several places in your patches were you wonder what happens
in cases there is no index or there is an index.
Why not make life simpler and make METACOPY depend on index?
METACOPY is not backward compat, not even readonly backward compat.
It may be easy for you to base on my index=all patches:
https://github.com/amir73il/linux/commits/ovl-index-all
and make the life cycle of copy up go through the following stages:
- create metadata copy index
- copy data to index
- link index to upper

AFAICT there is never any reason to actually have an upper alias as
a result of metadata copy up.

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

And that is not backward compat so need a new opt-in config option.
I don't like it so much that we keep adding config options and complicate
the compatibility matrix, that is why I prefer if we bundle several new
functionalities into a single new opt-in config option if possible, but
Miklos seems to feel differently about this.

Cheers,
Amir.

Re: [PATCH 1/7] Create origin xattr on copy up for all files

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now my understanding is that origin xattr is created for all copied
> up files if index=on. And if index=off, then we create it for all type
> of files except hardlinks (nlink != 1).
>
> With metadata only copy up, I will still require origin xattr to copy up
> data later, so create it even for hardlinks even with index=off. I am
> hoping it does not break anything because we do OVL_INDEX test before
> using inode number of origin.

It does break index consistency if 2 different upper inodes point to the same
lower ORIGIN.
If you must support METACOPY with index=off (which I prefer not to)
then you will have to set a different xattr to follow to data copy up
source or set a BROKENHARDLINK xattr for this case so index=on lookup
knows it cannot use ORIGIN as index key.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index aad97b30d5e6..f1b15e5c37d3 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -538,9 +538,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);
> @@ -598,6 +595,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                 .parent = parent,
>                 .dentry = dentry,
>                 .workdir = ovl_workdir(dentry),
> +               .origin = true,
>         };
>
>         if (WARN_ON(!ctx.workdir))
> --
> 2.13.5
>

Re: [PATCH 2/7] ovl: During copy up, first copy up metadata and then data

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> This just helps with later patches where after copying up metadata, we
> skip data copying step, if needed.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index f1b15e5c37d3..cdc4d79a1249 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -445,18 +445,6 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>  {
>         int err;
>
> -       if (S_ISREG(c->stat.mode)) {
> -               struct path upperpath;
> -
> -               ovl_path_upper(c->dentry, &upperpath);
> -               BUG_ON(upperpath.dentry != NULL);
> -               upperpath.dentry = temp;
> -
> -               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> -               if (err)
> -                       return err;
> -       }
> -
>         err = ovl_copy_xattr(c->lowerpath.dentry, temp);
>         if (err)
>                 return err;
> @@ -480,6 +468,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>                         return err;
>         }
>
> +       if (S_ISREG(c->stat.mode)) {
> +               struct path upperpath;
> +
> +               ovl_path_upper(c->dentry, &upperpath);
> +               BUG_ON(upperpath.dentry != NULL);
> +               upperpath.dentry = temp;
> +
> +               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> +               if (err)
> +                       return err;
> +       }
> +

You moved copy_up_data to after ovl_set_attr() -> ovl_set_timestamps()
Does this break the copied up mtime? I'm not sure.
I didn't see that do_splice_direct() or vfs_clone_file_range() change
mtime directly, but inside file system, they might.

>         return 0;
>  }
>
> --
> 2.13.5
>

Re: [PATCH 3/7] ovl: Copy up only metadata during copy up where it makes sense

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If it makes sense to copy up only metadata during copy up, do it. This
> is done for regular files which are not opened for WRITE and have origin
> being saved.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index cdc4d79a1249..b2d9ed81e9ff 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -306,11 +306,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;
> @@ -328,6 +325,7 @@ struct ovl_copy_up_ctx {
>         struct dentry *workdir;
>         bool tmpfile;
>         bool origin;
> +       bool metadata_only;
>  };
>
>  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> @@ -464,11 +462,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>          */
>         if (c->origin) {
>                 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->metadata_only = false;
> +               }
>         }
>
> -       if (S_ISREG(c->stat.mode)) {
> +       if (S_ISREG(c->stat.mode) && !c->metadata_only) {
>                 struct path upperpath;
>

I would set the meta inode size here. see:
https://github.com/amir73il/linux/commits/ovl-rocopyup

>                 ovl_path_upper(c->dentry, &upperpath);
> @@ -596,6 +598,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                 .dentry = dentry,
>                 .workdir = ovl_workdir(dentry),
>                 .origin = true,
> +               .metadata_only = true,
>         };
>
>         if (WARN_ON(!ctx.workdir))
> @@ -616,9 +619,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>         if (err)
>                 return err;
>
> +       if (!S_ISREG(ctx.stat.mode))
> +               ctx.metadata_only = false;
> +
>         /* maybe truncate regular file. this has no effect on dirs */
> -       if (flags & O_TRUNC)
> +       if (flags & O_TRUNC) {
>                 ctx.stat.size = 0;
> +               ctx.metadata_only = false;
> +       }
> +
> +       if (flags & (OPEN_FMODE(flags) & FMODE_WRITE))
> +               ctx.metadata_only = false;
>
>         if (S_ISLNK(ctx.stat.mode)) {
>                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
> --
> 2.13.5
>

Re: [PATCH 4/7] ovl: Set xattr OVL_XATTR_METACOPY on upper file

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Presence of OVL_XATTR_METACOPY reflects that file has been copied up
> with metadata only and data needs to be copied up later from lower.
> 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_METACOPY which reflects
> whether ovl inode has only metadata copied up.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
>  fs/overlayfs/inode.c     |  3 ++-
>  fs/overlayfs/overlayfs.h |  5 ++++-
>  fs/overlayfs/util.c      | 21 +++++++++++++++++++--
>  4 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b2d9ed81e9ff..d76a4272b43e 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -439,6 +439,26 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
>         goto out;
>  }
>
> +static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
> +{
> +       struct path upperpath;
> +       int err;
> +
> +       ovl_path_upper(c->dentry, &upperpath);
> +       BUG_ON(upperpath.dentry == NULL);
> +
> +       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_clear_flag(OVL_METACOPY, d_inode(c->dentry));
> +       return err;
> +}
> +
>  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>  {
>         int err;
> @@ -482,6 +502,13 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>                         return err;
>         }
>
> +       if (c->metadata_only) {
> +               err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
> +                                        NULL, 0, -EOPNOTSUPP);
> +               if (err)
> +                       return err;
> +       }
> +
>         return 0;
>  }
>
> @@ -511,6 +538,9 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
>                 goto out_cleanup;
>
>         ovl_inode_update(d_inode(c->dentry), newdentry);
> +       if (c->metadata_only) {
> +               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
> +       }
>  out:
>         dput(temp);
>         return err;
> @@ -638,7 +668,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)
> @@ -648,6 +678,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                         err = ovl_do_copy_up(&ctx);
>                 if (!err && !ovl_dentry_has_upper_alias(dentry))
>                         err = ovl_link_up(&ctx);
> +               if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
> +                       err = ovl_copy_up_data_inode(&ctx);

I think it would simplify life cycle of copy up if ovl_copy_up_data_inode()
is always before ovl_link_up() and that ovl_dentry_has_upper_alias()
means "has a concrete upper alias with data".

index feature already supports an upper index without any upper aliases
(a property that I use in the ro-copyup patches).

Amir.

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> During lookup, check for presence of OVL_XATTR_METACOPY and if present,
> set OVL_METACOPY bit in flags.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index c3addd1114f1..9b6f9afa4f75 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -26,6 +26,24 @@ 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;
> +
> +       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)
>  {
> @@ -591,6 +609,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,
> @@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                                roe->numlower, &stack, &ctr);
>                         if (err)
>                                 goto out;
> +
> +                       err = ovl_check_metacopy(upperdentry);
> +                       if (err < 0)
> +                               goto out;
> +                       if (err == 1)
> +                               metacopy = true;
>                 }
>
>                 if (d.redirect) {
> @@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 OVL_I(inode)->redirect = upperredirect;
>                 if (index)
>                         ovl_set_flag(OVL_INDEX, inode);
> +
> +               if (metacopy) {
> +                       /*
> +                        * TODO: What happens we if find metacopy xattr but
> +                        * could not find/resolve origin.
> +                        */

Hint: if metacopy could also exist only on an index entry, with no
upper aliases,
this index entry would be detected as stale on mount (cannot resolve origin) and
cleaned, so you won't get to this problem.

Amir.

Re: [PATCH 7/7] ovl: Fix ovl_getattr() to get size from lower

[Amir Goldstein]

On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If an inode has been copied up metadata only, then we need to query the
> size from lower and fill up the stat->size.

So you won't need this patch if you copy st_size on metadata_copy,
which seems like the logical thing to do.
Thinking very far into the future when METACOPY and ORIGIN will
be able to share page mappings, they will have to start with the same size.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index e5825b8948e0..1b676deacc2d 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -140,6 +140,16 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>         if (!is_dir && ovl_test_flag(OVL_INDEX, d_inode(dentry)))
>                 stat->nlink = dentry->d_inode->i_nlink;
>
> +       if (ovl_test_flag(OVL_METACOPY, d_inode(dentry))) {
> +               struct kstat lowerstat;
> +               u32 lowermask = STATX_SIZE;
> +
> +               ovl_path_lower(dentry, &realpath);
> +               err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
> +               if (err)
> +                       goto out;
> +               stat->size = lowerstat.size;
> +       }
>  out:
>         revert_creds(old_cred);
>
> --
> 2.13.5
>

Re: [RFC PATCH 0/7] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:03:13PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Hi,
> >
> > 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. I have not tested it though.
> >
> > So here are very crude RFC patch. I have done bare minimal testing on
> > these and there are many unaddressed issues I can see.
> >
> > Before I go any further, I wanted to send these out for some feedback
> > and see if I am moving in right direction or this appraoch is completely
> > broken.
> >
> 
> I like the direction this is going :)
> Beyond the important use case you listed, this could be useful also for:
> 1. copy up of lower hardlink in ovl_nlink_start(), just to have a place holder
>     inode for OVL_XATTR_NLINK
> 2. similar case as above needed for NFS export of lower hardlink
> 3. possible starting point for consistent ro/rw file descriptor, see POC:
>     https://github.com/amir73il/linux/commits/ovl-rocopyup
>     your patches actually take off where my patches stop
> 
> > 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.
> 
> Your feature is relying on OVL_XATTR_ORIGIN, and so does index feature.
> There are several places in your patches were you wonder what happens
> in cases there is no index or there is an index.
> Why not make life simpler and make METACOPY depend on index?

Hi Amir,

I am not sure. Both index and METACOPY rely on OVL_XATTR_ORIGIN.
But conceptually metacopy does not seem to depend on index feature. We
could very well have index disabled while still having metacopy enabled.

> METACOPY is not backward compat, not even readonly backward compat.

Do you mean forward compatible? IIUC, I can take existing overlay
directories and mount them with newer kernel with metadata copy up
enabled. Just that metadata copy up will apply to only copy ups which
happen with new kernels. Files which have already been copied up
with old kernels will remain unaffected. So this way it is backward
compatible.

But once a metadata copy up has taken up, I can't go back to old kernel
and expect it to work. It will show an upper file of zero size. So this
looks like a forward compatibility issue. Metadata created by newer
version of software is not expected to be handled by older version of
software. 

Am I misunderstanding the issue?


> It may be easy for you to base on my index=all patches:
> https://github.com/amir73il/linux/commits/ovl-index-all
> and make the life cycle of copy up go through the following stages:
> - create metadata copy index

So this is same index which you are creating for hardlink with index=on?
With my pathces, now when copy up happens, only metadata will be copied
up.

> - copy data to index

As of now, this will happen when file is opened for WRITE.

> - link index to upper

So this step happens after first one. We create index with metadata copy
up and then index is hard linked to upper (alias).

> 
> AFAICT there is never any reason to actually have an upper alias as
> a result of metadata copy up.

I am not able to understand this point. So if a file foo.txt is hard
linked and I do a "chown vivek foo.txt", then I would like to have.

- Index created with metadata copy up only.
- Create upper alias. 

Isn't it?

IOW why not create upper alias with metadata copy up.

> 
> >
> > 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.
> >
> 
> And that is not backward compat so need a new opt-in config option.
> I don't like it so much that we keep adding config options and complicate
> the compatibility matrix, that is why I prefer if we bundle several new
> functionalities into a single new opt-in config option if possible, but
> Miklos seems to feel differently about this.

Making user opt-in for this feature is fine. It especially makes sense
because user can't downgrade its kernel now to older version. So those
who are used to downgrading (because upgraded kernel had issues), will
complain, saying we did not ask for this optimzation and don't break
our downgrade.

So a mount option "-o metacopy=on/off" sounds reasonable?

Vivek
> 
> Cheers,
> Amir.

Re: [RFC PATCH 0/7] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Mon, Oct 2, 2017 at 10:48 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Oct 02, 2017 at 10:03:13PM +0300, Amir Goldstein wrote:
>> On Mon, Oct 2, 2017 at 4:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Hi,
>> >
>> > 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. I have not tested it though.
>> >
>> > So here are very crude RFC patch. I have done bare minimal testing on
>> > these and there are many unaddressed issues I can see.
>> >
>> > Before I go any further, I wanted to send these out for some feedback
>> > and see if I am moving in right direction or this appraoch is completely
>> > broken.
>> >
>>
>> I like the direction this is going :)
>> Beyond the important use case you listed, this could be useful also for:
>> 1. copy up of lower hardlink in ovl_nlink_start(), just to have a place holder
>>     inode for OVL_XATTR_NLINK
>> 2. similar case as above needed for NFS export of lower hardlink
>> 3. possible starting point for consistent ro/rw file descriptor, see POC:
>>     https://github.com/amir73il/linux/commits/ovl-rocopyup
>>     your patches actually take off where my patches stop
>>
>> > 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.
>>
>> Your feature is relying on OVL_XATTR_ORIGIN, and so does index feature.
>> There are several places in your patches were you wonder what happens
>> in cases there is no index or there is an index.
>> Why not make life simpler and make METACOPY depend on index?
>
> Hi Amir,
>
> I am not sure. Both index and METACOPY rely on OVL_XATTR_ORIGIN.
> But conceptually metacopy does not seem to depend on index feature. We
> could very well have index disabled while still having metacopy enabled.

*conceptually* you may be right, but as I pointed out in on some of the
METACOPY patches, supporting metacopy=on,index=off gets you into
an implementation corner that I think would be best to avoid.

We need to ask ourselves why someone would *want* to have the feature
combination metacopy=on,index=off. It only makes sense if index=on
brings extra constrains that metacopy=on doesn't have, but does it?
index=on requires:
1. upper xattr support
2. lower file handle support
3. upper file handle support
4. no reuse of upperdir with new lowerdir
5. no reuse of workdir with new upperdir

METACOPY requires most of the above, except 3 and 5,
but these extra constrains are so easy to meet, so compared to the
tradeoff of simplifying the implementation corners of metacopy=on,index=off
relying on index=on seems like a win to me.

I can't imagine that feature wise someone would *want* to break hardlinks
with metacopy=on is an argument.

In theory, METACOPY could be made safe to migrate to new filesystem
and then won't require prereq 2 and 4 above if OVL_XATTR_REDIRECT
would be set on regular files as well and then it would make sense to decouple
it from index=on, but I am not sure if this is a direction worth pursuing.

My plan for the future is to have a userland migration tool for layer+index that
sets   OVL_XATTR_REDIRECT from OVL_XATTR_ORIGIN before export to
tarball and restores uptodate  OVL_XATTR_ORIGIN from
OVL_XATTR_REDIRECT after import layers+index from tarball.

>
>> METACOPY is not backward compat, not even readonly backward compat.
>
> Do you mean forward compatible? IIUC, I can take existing overlay
> directories and mount them with newer kernel with metadata copy up
> enabled. Just that metadata copy up will apply to only copy ups which
> happen with new kernels. Files which have already been copied up
> with old kernels will remain unaffected. So this way it is backward
> compatible.
>
> But once a metadata copy up has taken up, I can't go back to old kernel
> and expect it to work. It will show an upper file of zero size. So this
> looks like a forward compatibility issue. Metadata created by newer
> version of software is not expected to be handled by older version of
> software.
>
> Am I misunderstanding the issue?
>

Not misunderstanding just mixing up the terms as they are used in file systems:
Backward compat = new on-disk format understood by old kernels
Forward compat = old on-disk format understood by new kernel

>
>> It may be easy for you to base on my index=all patches:
>> https://github.com/amir73il/linux/commits/ovl-index-all
>> and make the life cycle of copy up go through the following stages:
>> - create metadata copy index
>
> So this is same index which you are creating for hardlink with index=on?
> With my pathces, now when copy up happens, only metadata will be copied
> up.
>

Correct.

>> - copy data to index
>
> As of now, this will happen when file is opened for WRITE.
>
>> - link index to upper
>
> So this step happens after first one. We create index with metadata copy
> up and then index is hard linked to upper (alias).
>
>>
>> AFAICT there is never any reason to actually have an upper alias as
>> a result of metadata copy up.
>
> I am not able to understand this point. So if a file foo.txt is hard
> linked and I do a "chown vivek foo.txt", then I would like to have.
>
> - Index created with metadata copy up only.
> - Create upper alias.
>
> Isn't it?
>
> IOW why not create upper alias with metadata copy up.
>

Because chown/chmod is a property of the inode, not the directory entry,
so if you have
ln foo.txt bar.txt
chown vivek foo.txt

The optimal result is that both foo.txt and bar.txt are now owned by vivek

Specifically, with index=on and hardlinks not broken on copy up
ovl_dentry_upper(dentry) of bar.txt will return the index denrty
with !ovl_has_upper_alias(dentry) and overlay inode as well as
ovl_inode_real() will have the new ownership for all union aliases
(bar.txt and foo.txt) whether they have upper alias or not.

>>
>> >
>> > 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.
>> >
>>
>> And that is not backward compat so need a new opt-in config option.
>> I don't like it so much that we keep adding config options and complicate
>> the compatibility matrix, that is why I prefer if we bundle several new
>> functionalities into a single new opt-in config option if possible, but
>> Miklos seems to feel differently about this.
>
> Making user opt-in for this feature is fine. It especially makes sense
> because user can't downgrade its kernel now to older version. So those
> who are used to downgrading (because upgraded kernel had issues), will
> complain, saying we did not ask for this optimzation and don't break
> our downgrade.
>
> So a mount option "-o metacopy=on/off" sounds reasonable?
>

Sounds reasonable to me, but I campaign for have mount reject
metacopy=on or fallback to metacopy=off if neither index=on or
CONFIG_OVERLAY_FS_INDEX=y are set.

Amir.

Re: [RFC PATCH 0/7] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Tue, Oct 03, 2017 at 08:36:07AM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 10:48 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Oct 02, 2017 at 10:03:13PM +0300, Amir Goldstein wrote:
> >> On Mon, Oct 2, 2017 at 4:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Hi,
> >> >
> >> > 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. I have not tested it though.
> >> >
> >> > So here are very crude RFC patch. I have done bare minimal testing on
> >> > these and there are many unaddressed issues I can see.
> >> >
> >> > Before I go any further, I wanted to send these out for some feedback
> >> > and see if I am moving in right direction or this appraoch is completely
> >> > broken.
> >> >
> >>
> >> I like the direction this is going :)
> >> Beyond the important use case you listed, this could be useful also for:
> >> 1. copy up of lower hardlink in ovl_nlink_start(), just to have a place holder
> >>     inode for OVL_XATTR_NLINK
> >> 2. similar case as above needed for NFS export of lower hardlink
> >> 3. possible starting point for consistent ro/rw file descriptor, see POC:
> >>     https://github.com/amir73il/linux/commits/ovl-rocopyup
> >>     your patches actually take off where my patches stop
> >>
> >> > 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.
> >>
> >> Your feature is relying on OVL_XATTR_ORIGIN, and so does index feature.
> >> There are several places in your patches were you wonder what happens
> >> in cases there is no index or there is an index.
> >> Why not make life simpler and make METACOPY depend on index?
> >
> > Hi Amir,
> >
> > I am not sure. Both index and METACOPY rely on OVL_XATTR_ORIGIN.
> > But conceptually metacopy does not seem to depend on index feature. We
> > could very well have index disabled while still having metacopy enabled.
> 
> *conceptually* you may be right, but as I pointed out in on some of the
> METACOPY patches, supporting metacopy=on,index=off gets you into
> an implementation corner that I think would be best to avoid.

Ok, that's fine. Right now, I don't have any requirement which says
that I need metadata copy up but index=off. So I will make it
conditional on index=on. And if somebody has such requirement later,
we can look into, how to decouple two.

> 
> We need to ask ourselves why someone would *want* to have the feature
> combination metacopy=on,index=off. It only makes sense if index=on
> brings extra constrains that metacopy=on doesn't have, but does it?
> index=on requires:
> 1. upper xattr support
> 2. lower file handle support
> 3. upper file handle support
> 4. no reuse of upperdir with new lowerdir
> 5. no reuse of workdir with new upperdir
> 
> METACOPY requires most of the above, except 3 and 5,
> but these extra constrains are so easy to meet, so compared to the
> tradeoff of simplifying the implementation corners of metacopy=on,index=off
> relying on index=on seems like a win to me.

Right. I just now fixed 1 and 2 for meta copyup. It was trivial. I did
not take care of 4 though. So index=on, will get me that as well.

> 
> I can't imagine that feature wise someone would *want* to break hardlinks
> with metacopy=on is an argument.
> 
> In theory, METACOPY could be made safe to migrate to new filesystem
> and then won't require prereq 2 and 4 above if OVL_XATTR_REDIRECT
> would be set on regular files as well and then it would make sense to decouple
> it from index=on, but I am not sure if this is a direction worth pursuing.
> 
> My plan for the future is to have a userland migration tool for layer+index that
> sets   OVL_XATTR_REDIRECT from OVL_XATTR_ORIGIN before export to
> tarball and restores uptodate  OVL_XATTR_ORIGIN from
> OVL_XATTR_REDIRECT after import layers+index from tarball.

Sounds very interesting.

> 
> >
> >> METACOPY is not backward compat, not even readonly backward compat.
> >
> > Do you mean forward compatible? IIUC, I can take existing overlay
> > directories and mount them with newer kernel with metadata copy up
> > enabled. Just that metadata copy up will apply to only copy ups which
> > happen with new kernels. Files which have already been copied up
> > with old kernels will remain unaffected. So this way it is backward
> > compatible.
> >
> > But once a metadata copy up has taken up, I can't go back to old kernel
> > and expect it to work. It will show an upper file of zero size. So this
> > looks like a forward compatibility issue. Metadata created by newer
> > version of software is not expected to be handled by older version of
> > software.
> >
> > Am I misunderstanding the issue?
> >
> 
> Not misunderstanding just mixing up the terms as they are used in file systems:
> Backward compat = new on-disk format understood by old kernels
> Forward compat = old on-disk format understood by new kernel

Ok, got it. 

> 
> >
> >> It may be easy for you to base on my index=all patches:
> >> https://github.com/amir73il/linux/commits/ovl-index-all
> >> and make the life cycle of copy up go through the following stages:
> >> - create metadata copy index
> >
> > So this is same index which you are creating for hardlink with index=on?
> > With my pathces, now when copy up happens, only metadata will be copied
> > up.
> >
> 
> Correct.
> 
> >> - copy data to index
> >
> > As of now, this will happen when file is opened for WRITE.
> >
> >> - link index to upper
> >
> > So this step happens after first one. We create index with metadata copy
> > up and then index is hard linked to upper (alias).
> >
> >>
> >> AFAICT there is never any reason to actually have an upper alias as
> >> a result of metadata copy up.
> >
> > I am not able to understand this point. So if a file foo.txt is hard
> > linked and I do a "chown vivek foo.txt", then I would like to have.
> >
> > - Index created with metadata copy up only.
> > - Create upper alias.
> >
> > Isn't it?
> >
> > IOW why not create upper alias with metadata copy up.
> >
> 
> Because chown/chmod is a property of the inode, not the directory entry,
> so if you have
> ln foo.txt bar.txt
> chown vivek foo.txt
> 
> The optimal result is that both foo.txt and bar.txt are now owned by vivek
> 
> Specifically, with index=on and hardlinks not broken on copy up
> ovl_dentry_upper(dentry) of bar.txt will return the index denrty
> with !ovl_has_upper_alias(dentry) and overlay inode as well as
> ovl_inode_real() will have the new ownership for all union aliases
> (bar.txt and foo.txt) whether they have upper alias or not.

I understand this part, that with index=on, both bar.txt and foo.txt
will show "vivek" as new owner.

What I was trying to say is that with index=off, hardlinks are broken
over copyup and metadata copyup does not make it any worse.

Anyway, now with index=on, being pre-condition, hopefully, this is
take care of.

> 
> >>
> >> >
> >> > 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.
> >> >
> >>
> >> And that is not backward compat so need a new opt-in config option.
> >> I don't like it so much that we keep adding config options and complicate
> >> the compatibility matrix, that is why I prefer if we bundle several new
> >> functionalities into a single new opt-in config option if possible, but
> >> Miklos seems to feel differently about this.
> >
> > Making user opt-in for this feature is fine. It especially makes sense
> > because user can't downgrade its kernel now to older version. So those
> > who are used to downgrading (because upgraded kernel had issues), will
> > complain, saying we did not ask for this optimzation and don't break
> > our downgrade.
> >
> > So a mount option "-o metacopy=on/off" sounds reasonable?
> >
> 
> Sounds reasonable to me, but I campaign for have mount reject
> metacopy=on or fallback to metacopy=off if neither index=on or
> CONFIG_OVERLAY_FS_INDEX=y are set.

Ok, in V2, I will make metacopy conditional on index=on.

Thanks
Vivek

Re: [PATCH 2/7] ovl: During copy up, first copy up metadata and then data

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:13:24PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > This just helps with later patches where after copying up metadata, we
> > skip data copying step, if needed.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index f1b15e5c37d3..cdc4d79a1249 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -445,18 +445,6 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >  {
> >         int err;
> >
> > -       if (S_ISREG(c->stat.mode)) {
> > -               struct path upperpath;
> > -
> > -               ovl_path_upper(c->dentry, &upperpath);
> > -               BUG_ON(upperpath.dentry != NULL);
> > -               upperpath.dentry = temp;
> > -
> > -               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> > -               if (err)
> > -                       return err;
> > -       }
> > -
> >         err = ovl_copy_xattr(c->lowerpath.dentry, temp);
> >         if (err)
> >                 return err;
> > @@ -480,6 +468,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >                         return err;
> >         }
> >
> > +       if (S_ISREG(c->stat.mode)) {
> > +               struct path upperpath;
> > +
> > +               ovl_path_upper(c->dentry, &upperpath);
> > +               BUG_ON(upperpath.dentry != NULL);
> > +               upperpath.dentry = temp;
> > +
> > +               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> 
> You moved copy_up_data to after ovl_set_attr() -> ovl_set_timestamps()
> Does this break the copied up mtime? I'm not sure.
> I didn't see that do_splice_direct() or vfs_clone_file_range() change
> mtime directly, but inside file system, they might.

Hi Amir,

That's a good point. I will move ovl_set_attr() call after data copy.

Thanks
Vivek

Re: [PATCH 3/7] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:21:05PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If it makes sense to copy up only metadata during copy up, do it. This
> > is done for regular files which are not opened for WRITE and have origin
> > being saved.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index cdc4d79a1249..b2d9ed81e9ff 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -306,11 +306,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;
> > @@ -328,6 +325,7 @@ struct ovl_copy_up_ctx {
> >         struct dentry *workdir;
> >         bool tmpfile;
> >         bool origin;
> > +       bool metadata_only;
> >  };
> >
> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > @@ -464,11 +462,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >          */
> >         if (c->origin) {
> >                 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->metadata_only = false;
> > +               }
> >         }
> >
> > -       if (S_ISREG(c->stat.mode)) {
> > +       if (S_ISREG(c->stat.mode) && !c->metadata_only) {
> >                 struct path upperpath;
> >
> 
> I would set the meta inode size here. see:
> https://github.com/amir73il/linux/commits/ovl-rocopyup
> 

Hi Amir,

Ok. So instead of having a zero size file in upper, set the size to same
as lower file. Just that there will not be any data blocks in upper file
yet? Will do.

Vivek

> >                 ovl_path_upper(c->dentry, &upperpath);
> > @@ -596,6 +598,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> >                 .dentry = dentry,
> >                 .workdir = ovl_workdir(dentry),
> >                 .origin = true,
> > +               .metadata_only = true,
> >         };
> >
> >         if (WARN_ON(!ctx.workdir))
> > @@ -616,9 +619,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> >         if (err)
> >                 return err;
> >
> > +       if (!S_ISREG(ctx.stat.mode))
> > +               ctx.metadata_only = false;
> > +
> >         /* maybe truncate regular file. this has no effect on dirs */
> > -       if (flags & O_TRUNC)
> > +       if (flags & O_TRUNC) {
> >                 ctx.stat.size = 0;
> > +               ctx.metadata_only = false;
> > +       }
> > +
> > +       if (flags & (OPEN_FMODE(flags) & FMODE_WRITE))
> > +               ctx.metadata_only = false;
> >
> >         if (S_ISLNK(ctx.stat.mode)) {
> >                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
> > --
> > 2.13.5
> >

Re: [PATCH 4/7] ovl: Set xattr OVL_XATTR_METACOPY on upper file

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:28:30PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Presence of OVL_XATTR_METACOPY reflects that file has been copied up
> > with metadata only and data needs to be copied up later from lower.
> > 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_METACOPY which reflects
> > whether ovl inode has only metadata copied up.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
> >  fs/overlayfs/inode.c     |  3 ++-
> >  fs/overlayfs/overlayfs.h |  5 ++++-
> >  fs/overlayfs/util.c      | 21 +++++++++++++++++++--
> >  4 files changed, 65 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index b2d9ed81e9ff..d76a4272b43e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -439,6 +439,26 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
> >         goto out;
> >  }
> >
> > +static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
> > +{
> > +       struct path upperpath;
> > +       int err;
> > +
> > +       ovl_path_upper(c->dentry, &upperpath);
> > +       BUG_ON(upperpath.dentry == NULL);
> > +
> > +       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_clear_flag(OVL_METACOPY, d_inode(c->dentry));
> > +       return err;
> > +}
> > +
> >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >  {
> >         int err;
> > @@ -482,6 +502,13 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >                         return err;
> >         }
> >
> > +       if (c->metadata_only) {
> > +               err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
> > +                                        NULL, 0, -EOPNOTSUPP);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -511,6 +538,9 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
> >                 goto out_cleanup;
> >
> >         ovl_inode_update(d_inode(c->dentry), newdentry);
> > +       if (c->metadata_only) {
> > +               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
> > +       }
> >  out:
> >         dput(temp);
> >         return err;
> > @@ -638,7 +668,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)
> > @@ -648,6 +678,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> >                         err = ovl_do_copy_up(&ctx);
> >                 if (!err && !ovl_dentry_has_upper_alias(dentry))
> >                         err = ovl_link_up(&ctx);
> > +               if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
> > +                       err = ovl_copy_up_data_inode(&ctx);
> 
> I think it would simplify life cycle of copy up if ovl_copy_up_data_inode()
> is always before ovl_link_up() and that ovl_dentry_has_upper_alias()
> means "has a concrete upper alias with data".

Hmm..., I think I don't understand the suggestion.  So will upper alias will
not be created until and unless data copy up happens?

IOW, say I have file lower-file.txt in lower/ and also a hard link, say
lower-link.txt

lower-file.txt
lower-link.txt

Now if do "chown vivek lower-file.txt", then lower-file.txt will be copied
up metadata only and an alias will be created. So are you saying that we
should not create an alias till actual data copy happens?

What about regular files with no links.

Vivek

> 
> index feature already supports an upper index without any upper aliases
> (a property that I use in the ro-copyup patches).
> 
> Amir.

Re: [PATCH 7/7] ovl: Fix ovl_getattr() to get size from lower

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:40:07PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If an inode has been copied up metadata only, then we need to query the
> > size from lower and fill up the stat->size.
> 
> So you won't need this patch if you copy st_size on metadata_copy,
> which seems like the logical thing to do.
> Thinking very far into the future when METACOPY and ORIGIN will
> be able to share page mappings, they will have to start with the same size.

Ok, Will set upper size same as lower during metacopy operation.

Vivek

> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index e5825b8948e0..1b676deacc2d 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -140,6 +140,16 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >         if (!is_dir && ovl_test_flag(OVL_INDEX, d_inode(dentry)))
> >                 stat->nlink = dentry->d_inode->i_nlink;
> >
> > +       if (ovl_test_flag(OVL_METACOPY, d_inode(dentry))) {
> > +               struct kstat lowerstat;
> > +               u32 lowermask = STATX_SIZE;
> > +
> > +               ovl_path_lower(dentry, &realpath);
> > +               err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
> > +               if (err)
> > +                       goto out;
> > +               stat->size = lowerstat.size;
> > +       }
> >  out:
> >         revert_creds(old_cred);
> >
> > --
> > 2.13.5
> >

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:31:42PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > During lookup, check for presence of OVL_XATTR_METACOPY and if present,
> > set OVL_METACOPY bit in flags.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index c3addd1114f1..9b6f9afa4f75 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -26,6 +26,24 @@ 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;
> > +
> > +       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)
> >  {
> > @@ -591,6 +609,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,
> > @@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                                                roe->numlower, &stack, &ctr);
> >                         if (err)
> >                                 goto out;
> > +
> > +                       err = ovl_check_metacopy(upperdentry);
> > +                       if (err < 0)
> > +                               goto out;
> > +                       if (err == 1)
> > +                               metacopy = true;
> >                 }
> >
> >                 if (d.redirect) {
> > @@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 OVL_I(inode)->redirect = upperredirect;
> >                 if (index)
> >                         ovl_set_flag(OVL_INDEX, inode);
> > +
> > +               if (metacopy) {
> > +                       /*
> > +                        * TODO: What happens we if find metacopy xattr but
> > +                        * could not find/resolve origin.
> > +                        */
> 
> Hint: if metacopy could also exist only on an index entry, with no
> upper aliases,
> this index entry would be detected as stale on mount (cannot resolve origin) and
> cleaned, so you won't get to this problem.

Hi Amir,

Sorry, I am getting lost here. Can you please explain a bit more.

My understanding is that index entry is created during copy up only for
files with nlink > 1. If a regular file is copied up there will not be
any index entry.

And during mount time, you are going through all index entries and
verifying there are no stale entries and cleaning up stale entries.
(ovl_indexdir_cleanup()).

So is the idea that with metacopyup we create index entries even for
regular files with nlink=1 and do similar cleanup? Sorry, I am little
lost here.

Vivek

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Amir Goldstein]

On Tue, Oct 3, 2017 at 6:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Oct 02, 2017 at 10:31:42PM +0300, Amir Goldstein wrote:
>> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > During lookup, check for presence of OVL_XATTR_METACOPY and if present,
>> > set OVL_METACOPY bit in flags.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/namei.c | 34 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 34 insertions(+)
>> >
>> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> > index c3addd1114f1..9b6f9afa4f75 100644
>> > --- a/fs/overlayfs/namei.c
>> > +++ b/fs/overlayfs/namei.c
>> > @@ -26,6 +26,24 @@ 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;
>> > +
>> > +       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)
>> >  {
>> > @@ -591,6 +609,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,
>> > @@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> >                                                roe->numlower, &stack, &ctr);
>> >                         if (err)
>> >                                 goto out;
>> > +
>> > +                       err = ovl_check_metacopy(upperdentry);
>> > +                       if (err < 0)
>> > +                               goto out;
>> > +                       if (err == 1)
>> > +                               metacopy = true;
>> >                 }
>> >
>> >                 if (d.redirect) {
>> > @@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> >                 OVL_I(inode)->redirect = upperredirect;
>> >                 if (index)
>> >                         ovl_set_flag(OVL_INDEX, inode);
>> > +
>> > +               if (metacopy) {
>> > +                       /*
>> > +                        * TODO: What happens we if find metacopy xattr but
>> > +                        * could not find/resolve origin.
>> > +                        */
>>
>> Hint: if metacopy could also exist only on an index entry, with no
>> upper aliases,
>> this index entry would be detected as stale on mount (cannot resolve origin) and
>> cleaned, so you won't get to this problem.
>
> Hi Amir,
>
> Sorry, I am getting lost here. Can you please explain a bit more.
>
> My understanding is that index entry is created during copy up only for
> files with nlink > 1. If a regular file is copied up there will not be
> any index entry.
>
> And during mount time, you are going through all index entries and
> verifying there are no stale entries and cleaning up stale entries.
> (ovl_indexdir_cleanup()).
>
> So is the idea that with metacopyup we create index entries even for
> regular files with nlink=1

Yes. please cherry-pick 982a78ebd5cd ovl: create ovl_need_index() helper
from https://github.com/amir73il/linux/commits/ovl-index-all
and then in the patch that introduces metacopy up, return true from
ovl_need_index() helper for regular files if ofs->config.metacopy is set

> and do similar cleanup? Sorry, I am little
> lost here.
>

Your patches need not do anything more.
The cleanup will happen with current ovl_indexdir_cleanup() code.
You ask in the comment "What happens we if find metacopy xattr but
could not find/resolve origin?"
If you follow my suggestions, then metacopy xattr can exist only on
index entry *before* it was linked to upper dir.
So if such an index entry exists that its origin is not available or
cannot be resolved, this index will be cleaned on mount time
and you shall not find it on lookup.

Amir.

Re: [PATCH 4/7] ovl: Set xattr OVL_XATTR_METACOPY on upper file

[Amir Goldstein]

On Tue, Oct 3, 2017 at 6:10 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Oct 02, 2017 at 10:28:30PM +0300, Amir Goldstein wrote:
>> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Presence of OVL_XATTR_METACOPY reflects that file has been copied up
>> > with metadata only and data needs to be copied up later from lower.
>> > 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_METACOPY which reflects
>> > whether ovl inode has only metadata copied up.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/copy_up.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
>> >  fs/overlayfs/inode.c     |  3 ++-
>> >  fs/overlayfs/overlayfs.h |  5 ++++-
>> >  fs/overlayfs/util.c      | 21 +++++++++++++++++++--
>> >  4 files changed, 65 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > index b2d9ed81e9ff..d76a4272b43e 100644
>> > --- a/fs/overlayfs/copy_up.c
>> > +++ b/fs/overlayfs/copy_up.c
>> > @@ -439,6 +439,26 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
>> >         goto out;
>> >  }
>> >
>> > +static int ovl_copy_up_data_inode(struct ovl_copy_up_ctx *c)
>> > +{
>> > +       struct path upperpath;
>> > +       int err;
>> > +
>> > +       ovl_path_upper(c->dentry, &upperpath);
>> > +       BUG_ON(upperpath.dentry == NULL);
>> > +
>> > +       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_clear_flag(OVL_METACOPY, d_inode(c->dentry));
>> > +       return err;
>> > +}
>> > +
>> >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>> >  {
>> >         int err;
>> > @@ -482,6 +502,13 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>> >                         return err;
>> >         }
>> >
>> > +       if (c->metadata_only) {
>> > +               err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
>> > +                                        NULL, 0, -EOPNOTSUPP);
>> > +               if (err)
>> > +                       return err;
>> > +       }
>> > +
>> >         return 0;
>> >  }
>> >
>> > @@ -511,6 +538,9 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
>> >                 goto out_cleanup;
>> >
>> >         ovl_inode_update(d_inode(c->dentry), newdentry);
>> > +       if (c->metadata_only) {
>> > +               ovl_set_flag(OVL_METACOPY, d_inode(c->dentry));
>> > +       }
>> >  out:
>> >         dput(temp);
>> >         return err;
>> > @@ -638,7 +668,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)
>> > @@ -648,6 +678,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>> >                         err = ovl_do_copy_up(&ctx);
>> >                 if (!err && !ovl_dentry_has_upper_alias(dentry))
>> >                         err = ovl_link_up(&ctx);
>> > +               if (!err && ovl_dentry_needs_data_copy_up(dentry, flags))
>> > +                       err = ovl_copy_up_data_inode(&ctx);
>>
>> I think it would simplify life cycle of copy up if ovl_copy_up_data_inode()
>> is always before ovl_link_up() and that ovl_dentry_has_upper_alias()
>> means "has a concrete upper alias with data".
>
> Hmm..., I think I don't understand the suggestion.  So will upper alias will
> not be created until and unless data copy up happens?
>
> IOW, say I have file lower-file.txt in lower/ and also a hard link, say
> lower-link.txt
>
> lower-file.txt
> lower-link.txt
>
> Now if do "chown vivek lower-file.txt", then lower-file.txt will be copied
> up metadata only and an alias will be created. So are you saying that we
> should not create an alias till actual data copy happens?

Yes, that is what I was suggesting.
There is no need for an upper alias for the purpose of copy up
from ovl_setattr(), ovl_xattr_set() and ovl_nlink_start().

However, the call sites ovl_copy_up(old) and ovl_copy_up(new) from
ovl_link() and ovl_rename() do need to create an upper alias,
so either they out-in to data copy (as they behave today) with
ovl_copy_up_flags(dentry, O_RWONLY) or they could be converted
to ovl_copy_up_flags(dentry, O_PATH) as special indication to opt-in
for metacopy + link_up with no data copy.

>
> What about regular files with no links.
>

When you chmod a regular file with or without links,
an index is created with its metadata.
Any future lookup/stat/permissions will see the i_mode/st_mode
of the upper index.

Amir.

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Vivek Goyal]

On Tue, Oct 03, 2017 at 06:42:48PM +0300, Amir Goldstein wrote:
> On Tue, Oct 3, 2017 at 6:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Oct 02, 2017 at 10:31:42PM +0300, Amir Goldstein wrote:
> >> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > During lookup, check for presence of OVL_XATTR_METACOPY and if present,
> >> > set OVL_METACOPY bit in flags.
> >> >
> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> > ---
> >> >  fs/overlayfs/namei.c | 34 ++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 34 insertions(+)
> >> >
> >> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >> > index c3addd1114f1..9b6f9afa4f75 100644
> >> > --- a/fs/overlayfs/namei.c
> >> > +++ b/fs/overlayfs/namei.c
> >> > @@ -26,6 +26,24 @@ 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;
> >> > +
> >> > +       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)
> >> >  {
> >> > @@ -591,6 +609,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,
> >> > @@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >> >                                                roe->numlower, &stack, &ctr);
> >> >                         if (err)
> >> >                                 goto out;
> >> > +
> >> > +                       err = ovl_check_metacopy(upperdentry);
> >> > +                       if (err < 0)
> >> > +                               goto out;
> >> > +                       if (err == 1)
> >> > +                               metacopy = true;
> >> >                 }
> >> >
> >> >                 if (d.redirect) {
> >> > @@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >> >                 OVL_I(inode)->redirect = upperredirect;
> >> >                 if (index)
> >> >                         ovl_set_flag(OVL_INDEX, inode);
> >> > +
> >> > +               if (metacopy) {
> >> > +                       /*
> >> > +                        * TODO: What happens we if find metacopy xattr but
> >> > +                        * could not find/resolve origin.
> >> > +                        */
> >>
> >> Hint: if metacopy could also exist only on an index entry, with no
> >> upper aliases,
> >> this index entry would be detected as stale on mount (cannot resolve origin) and
> >> cleaned, so you won't get to this problem.
> >
> > Hi Amir,
> >
> > Sorry, I am getting lost here. Can you please explain a bit more.
> >
> > My understanding is that index entry is created during copy up only for
> > files with nlink > 1. If a regular file is copied up there will not be
> > any index entry.
> >
> > And during mount time, you are going through all index entries and
> > verifying there are no stale entries and cleaning up stale entries.
> > (ovl_indexdir_cleanup()).
> >
> > So is the idea that with metacopyup we create index entries even for
> > regular files with nlink=1
> 
> Yes. please cherry-pick 982a78ebd5cd ovl: create ovl_need_index() helper
> from https://github.com/amir73il/linux/commits/ovl-index-all
> and then in the patch that introduces metacopy up, return true from
> ovl_need_index() helper for regular files if ofs->config.metacopy is set

Hi Amir,

Ok. Before I dive into details of that patch, I have a concern about
creating index of all regular files and verifying them on mount time. Will
this lead to excessive mount time overhead for a large index. I mean
in container land, people do mount/unmount of container roots very
frequently. And paying this verification cost on every mount, can soon
start showing up and become a concern.

With chown(), this will be common case for container use case. All the
files in the image will be metadata copy up.

Also, is it fine to just cleanup upper files automatically during mount
time, without any input from users?

Vivek

> 
> > and do similar cleanup? Sorry, I am little
> > lost here.
> >
> 
> Your patches need not do anything more.
> The cleanup will happen with current ovl_indexdir_cleanup() code.
> You ask in the comment "What happens we if find metacopy xattr but
> could not find/resolve origin?"
> If you follow my suggestions, then metacopy xattr can exist only on
> index entry *before* it was linked to upper dir.

> So if such an index entry exists that its origin is not available or
> cannot be resolved, this index will be cleaned on mount time
> and you shall not find it on lookup.
> 
> Amir.

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Amir Goldstein]

On Tue, Oct 3, 2017 at 9:20 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 06:42:48PM +0300, Amir Goldstein wrote:
>> On Tue, Oct 3, 2017 at 6:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Mon, Oct 02, 2017 at 10:31:42PM +0300, Amir Goldstein wrote:
>> >> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > During lookup, check for presence of OVL_XATTR_METACOPY and if present,
>> >> > set OVL_METACOPY bit in flags.
>> >> >
>> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> >> > ---
>> >> >  fs/overlayfs/namei.c | 34 ++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 34 insertions(+)
>> >> >
>> >> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> >> > index c3addd1114f1..9b6f9afa4f75 100644
>> >> > --- a/fs/overlayfs/namei.c
>> >> > +++ b/fs/overlayfs/namei.c
>> >> > @@ -26,6 +26,24 @@ 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;
>> >> > +
>> >> > +       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)
>> >> >  {
>> >> > @@ -591,6 +609,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,
>> >> > @@ -631,6 +650,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> >> >                                                roe->numlower, &stack, &ctr);
>> >> >                         if (err)
>> >> >                                 goto out;
>> >> > +
>> >> > +                       err = ovl_check_metacopy(upperdentry);
>> >> > +                       if (err < 0)
>> >> > +                               goto out;
>> >> > +                       if (err == 1)
>> >> > +                               metacopy = true;
>> >> >                 }
>> >> >
>> >> >                 if (d.redirect) {
>> >> > @@ -716,6 +741,15 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> >> >                 OVL_I(inode)->redirect = upperredirect;
>> >> >                 if (index)
>> >> >                         ovl_set_flag(OVL_INDEX, inode);
>> >> > +
>> >> > +               if (metacopy) {
>> >> > +                       /*
>> >> > +                        * TODO: What happens we if find metacopy xattr but
>> >> > +                        * could not find/resolve origin.
>> >> > +                        */
>> >>
>> >> Hint: if metacopy could also exist only on an index entry, with no
>> >> upper aliases,
>> >> this index entry would be detected as stale on mount (cannot resolve origin) and
>> >> cleaned, so you won't get to this problem.
>> >
>> > Hi Amir,
>> >
>> > Sorry, I am getting lost here. Can you please explain a bit more.
>> >
>> > My understanding is that index entry is created during copy up only for
>> > files with nlink > 1. If a regular file is copied up there will not be
>> > any index entry.
>> >
>> > And during mount time, you are going through all index entries and
>> > verifying there are no stale entries and cleaning up stale entries.
>> > (ovl_indexdir_cleanup()).
>> >
>> > So is the idea that with metacopyup we create index entries even for
>> > regular files with nlink=1
>>
>> Yes. please cherry-pick 982a78ebd5cd ovl: create ovl_need_index() helper
>> from https://github.com/amir73il/linux/commits/ovl-index-all
>> and then in the patch that introduces metacopy up, return true from
>> ovl_need_index() helper for regular files if ofs->config.metacopy is set
>
> Hi Amir,
>
> Ok. Before I dive into details of that patch, I have a concern about
> creating index of all regular files and verifying them on mount time. Will
> this lead to excessive mount time overhead for a large index. I mean
> in container land, people do mount/unmount of container roots very
> frequently. And paying this verification cost on every mount, can soon
> start showing up and become a concern.

Vivek,

That is a valid concern.
Counter proposal:

Have metacopy depend on index=on but not index every regular file.

This way you still get the assurance that you cannot set inconsistent
origins to 2 broken hardlinks of the same origin, which would break
future mount with index=on.

So counter to what I wrote earlier, has_upper_alias AND !index will
be a valid state.
If you find in lookup an upper with METACOPY and no/stale origin
you return ESTALE.
Note there are several cases where ovl_lookup_index() will return
EIO from lookup for inconsistent index.

>
> With chown(), this will be common case for container use case. All the
> files in the image will be metadata copy up.
>
> Also, is it fine to just cleanup upper files automatically during mount
> time, without any input from users?
>

upper files - probably not without explicit opt-in config, index files - yes.
With index=on, a stale index (i.e. original lower is gone), index may
still have upper aliases and they will not be cleaned up.
The only case where cleaning an index MAY result in data loss for
user is:
- foo and bar are lower hardlinks
- user modifies foo (index is created with new data and linked to upper foo)
- user unlinks foo (upper foo unlinked but index remains)
- reading bar still reads the new data from upper index

Now the index has nlink == 1 and if that index is cleaned on mount, the data
of bar is lost.
However, the only way to get from lower bar to index is by encoding bar
file handle, but if index is stale, it means that bar cannot be found
by file handle.
So unless NFS export ops of lower fs are broken, the stale index is useless
and may be safely cleaned on mount.

Hope this example explains more that it confuses ;)

Amir.

Re: [PATCH 7/7] ovl: Fix ovl_getattr() to get size from lower

[Vivek Goyal]

On Mon, Oct 02, 2017 at 10:40:07PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If an inode has been copied up metadata only, then we need to query the
> > size from lower and fill up the stat->size.
> 
> So you won't need this patch if you copy st_size on metadata_copy,
> which seems like the logical thing to do.
> Thinking very far into the future when METACOPY and ORIGIN will
> be able to share page mappings, they will have to start with the same size.

Hi Amir,

What about "st_blocks"? After metadata copy up, doing stat on overlay file
shows "st_blocks" as 0. I think I still will need to intervene and get
st_blocks from lower file instead?

Vivek

> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index e5825b8948e0..1b676deacc2d 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -140,6 +140,16 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >         if (!is_dir && ovl_test_flag(OVL_INDEX, d_inode(dentry)))
> >                 stat->nlink = dentry->d_inode->i_nlink;
> >
> > +       if (ovl_test_flag(OVL_METACOPY, d_inode(dentry))) {
> > +               struct kstat lowerstat;
> > +               u32 lowermask = STATX_SIZE;
> > +
> > +               ovl_path_lower(dentry, &realpath);
> > +               err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
> > +               if (err)
> > +                       goto out;
> > +               stat->size = lowerstat.size;
> > +       }
> >  out:
> >         revert_creds(old_cred);
> >
> > --
> > 2.13.5
> >

Re: [PATCH 5/7] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Vivek Goyal]

On Wed, Oct 04, 2017 at 10:48:36AM +0300, Amir Goldstein wrote:

[..]
> > Ok. Before I dive into details of that patch, I have a concern about
> > creating index of all regular files and verifying them on mount time. Will
> > this lead to excessive mount time overhead for a large index. I mean
> > in container land, people do mount/unmount of container roots very
> > frequently. And paying this verification cost on every mount, can soon
> > start showing up and become a concern.
> 
> Vivek,
> 
> That is a valid concern.
> Counter proposal:
> 
> Have metacopy depend on index=on but not index every regular file.
> 
> This way you still get the assurance that you cannot set inconsistent
> origins to 2 broken hardlinks of the same origin, which would break
> future mount with index=on.
> 
> So counter to what I wrote earlier, has_upper_alias AND !index will
> be a valid state.
> If you find in lookup an upper with METACOPY and no/stale origin
> you return ESTALE.

Ok, I will post V2 patches and return -ESTALE if metacopy xattr is present
but origin could not be found/resolved during lookup.

Vivek

Re: [PATCH 7/7] ovl: Fix ovl_getattr() to get size from lower

[Amir Goldstein]

On Fri, Oct 6, 2017 at 6:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Oct 02, 2017 at 10:40:07PM +0300, Amir Goldstein wrote:
>> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > If an inode has been copied up metadata only, then we need to query the
>> > size from lower and fill up the stat->size.
>>
>> So you won't need this patch if you copy st_size on metadata_copy,
>> which seems like the logical thing to do.
>> Thinking very far into the future when METACOPY and ORIGIN will
>> be able to share page mappings, they will have to start with the same size.
>
> Hi Amir,
>
> What about "st_blocks"? After metadata copy up, doing stat on overlay file
> shows "st_blocks" as 0. I think I still will need to intervene and get
> st_blocks from lower file instead?
>

Yeh, I guess you would.
BTW, if it were up to me I would prefer a single call to vfs_getattr()
instead of one for STATX_INO and then another for STATX_BLOCKS.
Even though current code queries STATX_INO only for samefs case,
there are patches by Chandan to relax samefs in ovl_getattr().

Amir.