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

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

[Vivek Goyal]

Hi,

Please find attached V2 of patches. I think I have taken care of
feedback from V1. Changes from V1 are as follows.

- Made metacopy up feature dependent on index=on.
- Added a patch to provide kernel config option, module option and mount
  option to enable metadata copyup feature.
- Now I am copying up size from lower when metadata only copyup takes
  place.
- I am returning -ESTALE in ovl_lookup() if an inode as METACOPY xattr
  but can find/resolve associated origin.
- Calling ovl_set_attr() after metadata copyup so that mtime on lower
  and upper are same.

I have little concern about access to METACOPY flag of inode stored in
ovl_inode. We are accessing this flag in lockless manner from reader
side and updating this flag under a mutex. My understanding is that
current code will make sure that user will see this flag set only after
METACOPY XATTR has been set (and not before that). Following is code
flow.

ovl_check_setxattr(OVL_XATTR_METACOPY)
smp_wmb(); (From ovl_inode_update())
ovl_set_flag(OVL_METACOPY, d_inode(c->dentry))

But there is no lockless_deference() call while checking this flag in
ovl_test_flag(). So I have two questions. Is this sufficient? And is
this too subtle and prone to breakage.

-------------------

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

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

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

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

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

Any feedback is helpful.

Vivek Goyal (8):
  ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check
  ovl: During copy up, first copy up metadata and then data
  ovl: Provide a mount option metacopy=on/off for metadata copyup
  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 number of blocks from lower

 fs/overlayfs/Kconfig     |   9 ++++
 fs/overlayfs/copy_up.c   | 115 ++++++++++++++++++++++++++++++++++++++---------
 fs/overlayfs/inode.c     |  21 +++++++--
 fs/overlayfs/namei.c     |  38 ++++++++++++++++
 fs/overlayfs/overlayfs.h |   5 ++-
 fs/overlayfs/ovl_entry.h |   1 +
 fs/overlayfs/super.c     |  36 +++++++++++++++
 fs/overlayfs/util.c      |  32 ++++++++-----
 8 files changed, 219 insertions(+), 38 deletions(-)

-- 
2.13.5

[PATCH 1/8] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check

[Vivek Goyal]

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

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

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

[PATCH 2/8] 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 | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index aad97b30d5e6..00e8ae8bb9aa 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -445,6 +445,23 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
 
+	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
+	if (err)
+		return err;
+
+	/*
+	 * Store identifier of lower inode in upper inode xattr to
+	 * allow lookup of the copy up origin inode.
+	 *
+	 * Don't set origin when we are breaking the association with a lower
+	 * hard link.
+	 */
+	if (c->origin) {
+		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+		if (err)
+			return err;
+	}
+
 	if (S_ISREG(c->stat.mode)) {
 		struct path upperpath;
 
@@ -457,29 +474,12 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
-	if (err)
-		return err;
-
 	inode_lock(temp->d_inode);
 	err = ovl_set_attr(temp, &c->stat);
 	inode_unlock(temp->d_inode);
 	if (err)
 		return err;
 
-	/*
-	 * Store identifier of lower inode in upper inode xattr to
-	 * allow lookup of the copy up origin inode.
-	 *
-	 * Don't set origin when we are breaking the association with a lower
-	 * hard link.
-	 */
-	if (c->origin) {
-		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-		if (err)
-			return err;
-	}
-
 	return 0;
 }
 
-- 
2.13.5

[PATCH 3/8] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

By default metadata only copy up is disabled. Provide a mount option so
that users can choose one way or other.

Also metadata copyup is conditional on index=on. If index=off and user
specifies metacopy=on, it goes back to metacopy=off and a warning is
printed.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/Kconfig     |  9 +++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index cbfc196e5dc5..94d4c61719c8 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -43,3 +43,12 @@ config OVERLAY_FS_INDEX
 	  outcomes.  However, mounting the same overlay with an old kernel
 	  read-write and then mounting it again with a new kernel, will have
 	  unexpected results.
+
+config OVERLAY_FS_METACOPY
+	bool "Overlayfs: turn on metadata only copy up feature by default"
+	depends on OVERLAY_FS
+	depends on OVERLAY_FS_INDEX
+	help
+	  If this config option is enabled then overlay filesystems will
+	  copy up only metadata where appropriate and data copy up will
+	  happen when a file is opended for WRITE operation.
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 878a750986dd..a49f61111c79 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -15,6 +15,7 @@ struct ovl_config {
 	bool default_permissions;
 	bool redirect_dir;
 	bool index;
+	bool metacopy;
 };
 
 /* private information held for overlayfs's superblock */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fd5ea4facc62..338fb0c3d345 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
 		 "Default to on or off for the inodes index feature");
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+		 "Default to on or off for the metadata only copy up feature");
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
@@ -302,6 +307,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ufs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s",
 			   ufs->config.index ? "on" : "off");
+	if (ufs->config.metacopy != ovl_metacopy_def)
+		seq_printf(m, ",metacopy=%s",
+			   ufs->config.metacopy ? "on" : "off");
 	return 0;
 }
 
@@ -335,6 +343,8 @@ enum {
 	OPT_REDIRECT_DIR_OFF,
 	OPT_INDEX_ON,
 	OPT_INDEX_OFF,
+	OPT_METACOPY_ON,
+	OPT_METACOPY_OFF,
 	OPT_ERR,
 };
 
@@ -347,6 +357,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_REDIRECT_DIR_OFF,		"redirect_dir=off"},
 	{OPT_INDEX_ON,			"index=on"},
 	{OPT_INDEX_OFF,			"index=off"},
+	{OPT_METACOPY_ON,		"metacopy=on"},
+	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -427,6 +439,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->index = false;
 			break;
 
+		case OPT_METACOPY_ON:
+			config->metacopy = true;
+			break;
+
+		case OPT_METACOPY_OFF:
+			config->metacopy = false;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -441,6 +461,12 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		config->workdir = NULL;
 	}
 
+	/* As of now metacopy=on is dependent on index=on */
+	if (config->metacopy && !config->index) {
+		pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
+		config->metacopy = false;
+	}
+
 	return 0;
 }
 
@@ -846,6 +872,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	ufs->config.redirect_dir = ovl_redirect_dir_def;
 	ufs->config.index = ovl_index_def;
+	if (ovl_metacopy_def && !ovl_index_def) {
+		pr_warn("overlayfs: metadata copy up can not be enabled by default as index feature is not enabled by default.\n");
+		ovl_metacopy_def = false;
+	}
+	ufs->config.metacopy = ovl_metacopy_def;
+
 	err = ovl_parse_opt((char *) data, &ufs->config);
 	if (err)
 		goto out_free_config;
-- 
2.13.5

[PATCH 4/8] 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 | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 00e8ae8bb9aa..a1fb6ccfcc51 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)
@@ -458,11 +456,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);
@@ -594,10 +596,12 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	int err;
 	DEFINE_DELAYED_CALL(done);
 	struct path parentpath;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct ovl_copy_up_ctx ctx = {
 		.parent = parent,
 		.dentry = dentry,
 		.workdir = ovl_workdir(dentry),
+		.metadata_only = ofs->config.metacopy,
 	};
 
 	if (WARN_ON(!ctx.workdir))
@@ -618,9 +622,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 5/8] 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.

ovl_set_size() code has been taken from Amir's patch.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a1fb6ccfcc51..f4d269790080 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -196,6 +196,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	return error;
 }
 
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+	struct iattr attr = {
+		.ia_valid = ATTR_SIZE,
+		.ia_size = stat->size,
+	};
+
+	return notify_change(upperdentry, &attr, NULL);
+}
+
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
 	struct iattr attr = {
@@ -439,9 +449,30 @@ 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;
+	bool set_size = false;
 
 	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
 	if (err)
@@ -476,8 +507,29 @@ 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;
+
+		set_size = true;
+	}
+
 	inode_lock(temp->d_inode);
+	if (set_size) {
+		err = ovl_set_size(temp, &c->stat);
+		if (err) {
+			inode_unlock(temp->d_inode);
+			return err;
+		}
+	}
+
 	err = ovl_set_attr(temp, &c->stat);
+	if (err) {
+		inode_unlock(temp->d_inode);
+		return err;
+	}
 	inode_unlock(temp->d_inode);
 	if (err)
 		return err;
@@ -511,6 +563,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;
@@ -641,7 +696,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)
@@ -651,6 +706,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);
@@ -681,8 +738,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 792a5f5f27e0..cfcbf1e522f0 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 6/8] 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 | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index c3addd1114f1..f139596b13f0 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,19 @@ 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) {
+			/*
+			 * Found an upper with metacopy set but at the same
+			 * time there is no lower dentry. Something is not
+			 * right.
+			 */
+			if (!ctr) {
+				err = -ESTALE;
+				goto out_free_oe;
+			}
+			ovl_set_flag(OVL_METACOPY, inode);
+		}
 	}
 
 	revert_creds(old_cred);
-- 
2.13.5

[PATCH 7/8] 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 338fb0c3d345..0877b4baf14b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -101,10 +101,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 8/8] ovl: Fix ovl_getattr() to get number of blocks from lower

[Vivek Goyal]

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

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index e5825b8948e0..18007634ea9a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -67,6 +67,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	const struct cred *old_cred;
 	bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
 	int err;
+	bool lowerstat_done = false;
+	struct kstat lowerstat;
 
 	type = ovl_path_real(dentry, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -86,8 +88,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	 */
 	if (ovl_same_sb(dentry->d_sb)) {
 		if (OVL_TYPE_ORIGIN(type)) {
-			struct kstat lowerstat;
-			u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+			u32 lowermask = STATX_INO | STATX_BLOCKS |
+					(!is_dir ? STATX_NLINK : 0);
 
 			ovl_path_lower(dentry, &realpath);
 			err = vfs_getattr(&realpath, &lowerstat,
@@ -95,6 +97,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			if (err)
 				goto out;
 
+			lowerstat_done = true;
 			WARN_ON_ONCE(stat->dev != lowerstat.dev);
 			/*
 			 * Lower hardlinks may be broken on copy up to different
@@ -140,6 +143,17 @@ 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))) {
+		u32 lowermask = STATX_BLOCKS;
+
+		if (!lowerstat_done) {
+			ovl_path_lower(dentry, &realpath);
+			err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
+			if (err)
+				goto out;
+		}
+		stat->blocks = lowerstat.blocks;
+	}
 out:
 	revert_creds(old_cred);
 
-- 
2.13.5

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

[Amir Goldstein]

On Fri, Oct 6, 2017 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> If an inode has been copied up metadata only, then we need to query the
> number of blocks from lower and fill up the stat->st_blocks.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index e5825b8948e0..18007634ea9a 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -67,6 +67,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>         const struct cred *old_cred;
>         bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
>         int err;
> +       bool lowerstat_done = false;
> +       struct kstat lowerstat;
>
>         type = ovl_path_real(dentry, &realpath);
>         old_cred = ovl_override_creds(dentry->d_sb);
> @@ -86,8 +88,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>          */
>         if (ovl_same_sb(dentry->d_sb)) {
>                 if (OVL_TYPE_ORIGIN(type)) {
> -                       struct kstat lowerstat;
> -                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
> +                       u32 lowermask = STATX_INO | STATX_BLOCKS |
> +                                       (!is_dir ? STATX_NLINK : 0);
>
>                         ovl_path_lower(dentry, &realpath);
>                         err = vfs_getattr(&realpath, &lowerstat,
> @@ -95,6 +97,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                         if (err)
>                                 goto out;
>
> +                       lowerstat_done = true;
>                         WARN_ON_ONCE(stat->dev != lowerstat.dev);
>                         /*
>                          * Lower hardlinks may be broken on copy up to different
> @@ -140,6 +143,17 @@ 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))) {
> +               u32 lowermask = STATX_BLOCKS;
> +
> +               if (!lowerstat_done) {

I guess lowerstat_done is as result from my comment,
but I did not mean I think vfs_getattr() is expensive,
I just didn't like the duplication of that code block, but maybe
just a matter of personal taste.
Can be sorted out later.

> +                       ovl_path_lower(dentry, &realpath);
> +                       err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
> +                       if (err)
> +                               goto out;
> +               }
> +               stat->blocks = lowerstat.blocks;
> +       }
>  out:
>         revert_creds(old_cred);
>
> --
> 2.13.5
>

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

[Vivek Goyal]

On Fri, Oct 06, 2017 at 09:58:58PM +0300, Amir Goldstein wrote:
> On Fri, Oct 6, 2017 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If an inode has been copied up metadata only, then we need to query the
> > number of blocks from lower and fill up the stat->st_blocks.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/inode.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index e5825b8948e0..18007634ea9a 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -67,6 +67,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >         const struct cred *old_cred;
> >         bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> >         int err;
> > +       bool lowerstat_done = false;
> > +       struct kstat lowerstat;
> >
> >         type = ovl_path_real(dentry, &realpath);
> >         old_cred = ovl_override_creds(dentry->d_sb);
> > @@ -86,8 +88,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >          */
> >         if (ovl_same_sb(dentry->d_sb)) {
> >                 if (OVL_TYPE_ORIGIN(type)) {
> > -                       struct kstat lowerstat;
> > -                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
> > +                       u32 lowermask = STATX_INO | STATX_BLOCKS |
> > +                                       (!is_dir ? STATX_NLINK : 0);
> >
> >                         ovl_path_lower(dentry, &realpath);
> >                         err = vfs_getattr(&realpath, &lowerstat,
> > @@ -95,6 +97,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >                         if (err)
> >                                 goto out;
> >
> > +                       lowerstat_done = true;
> >                         WARN_ON_ONCE(stat->dev != lowerstat.dev);
> >                         /*
> >                          * Lower hardlinks may be broken on copy up to different
> > @@ -140,6 +143,17 @@ 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))) {
> > +               u32 lowermask = STATX_BLOCKS;
> > +
> > +               if (!lowerstat_done) {
> 
> I guess lowerstat_done is as result from my comment,
> but I did not mean I think vfs_getattr() is expensive,
> I just didn't like the duplication of that code block, but maybe
> just a matter of personal taste.
> Can be sorted out later.

Yes, I added this because I thought you wanted to avoid that extra
vfs_getattr() call. I am open to change it whatever way you like.
Personally I did not like this extra boolean and I liked unconditional
vfs_getattr() better. It was not most optimized but was easier to
understand.

Vivek

> 
> > +                       ovl_path_lower(dentry, &realpath);
> > +                       err = vfs_getattr(&realpath, &lowerstat, lowermask, flags);
> > +                       if (err)
> > +                               goto out;
> > +               }
> > +               stat->blocks = lowerstat.blocks;
> > +       }
> >  out:
> >         revert_creds(old_cred);
> >
> > --
> > 2.13.5
> >

Re: [PATCH 3/8] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Fri, Oct 6, 2017 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> By default metadata only copy up is disabled. Provide a mount option so
> that users can choose one way or other.
>
> Also metadata copyup is conditional on index=on. If index=off and user
> specifies metacopy=on, it goes back to metacopy=off and a warning is
> printed.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/Kconfig     |  9 +++++++++
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/super.c     | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+)
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index cbfc196e5dc5..94d4c61719c8 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -43,3 +43,12 @@ config OVERLAY_FS_INDEX
>           outcomes.  However, mounting the same overlay with an old kernel
>           read-write and then mounting it again with a new kernel, will have
>           unexpected results.
> +
> +config OVERLAY_FS_METACOPY
> +       bool "Overlayfs: turn on metadata only copy up feature by default"
> +       depends on OVERLAY_FS
> +       depends on OVERLAY_FS_INDEX
> +       help
> +         If this config option is enabled then overlay filesystems will
> +         copy up only metadata where appropriate and data copy up will
> +         happen when a file is opended for WRITE operation.
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 878a750986dd..a49f61111c79 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -15,6 +15,7 @@ struct ovl_config {
>         bool default_permissions;
>         bool redirect_dir;
>         bool index;
> +       bool metacopy;
>  };
>
>  /* private information held for overlayfs's superblock */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index fd5ea4facc62..338fb0c3d345 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -39,6 +39,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
>  MODULE_PARM_DESC(ovl_index_def,
>                  "Default to on or off for the inodes index feature");
>
> +static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
> +module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
> +MODULE_PARM_DESC(ovl_metacopy_def,
> +                "Default to on or off for the metadata only copy up feature");
> +
>  static void ovl_dentry_release(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = dentry->d_fsdata;
> @@ -302,6 +307,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>         if (ufs->config.index != ovl_index_def)
>                 seq_printf(m, ",index=%s",
>                            ufs->config.index ? "on" : "off");
> +       if (ufs->config.metacopy != ovl_metacopy_def)
> +               seq_printf(m, ",metacopy=%s",
> +                          ufs->config.metacopy ? "on" : "off");
>         return 0;
>  }
>
> @@ -335,6 +343,8 @@ enum {
>         OPT_REDIRECT_DIR_OFF,
>         OPT_INDEX_ON,
>         OPT_INDEX_OFF,
> +       OPT_METACOPY_ON,
> +       OPT_METACOPY_OFF,
>         OPT_ERR,
>  };
>
> @@ -347,6 +357,8 @@ static const match_table_t ovl_tokens = {
>         {OPT_REDIRECT_DIR_OFF,          "redirect_dir=off"},
>         {OPT_INDEX_ON,                  "index=on"},
>         {OPT_INDEX_OFF,                 "index=off"},
> +       {OPT_METACOPY_ON,               "metacopy=on"},
> +       {OPT_METACOPY_OFF,              "metacopy=off"},
>         {OPT_ERR,                       NULL}
>  };
>
> @@ -427,6 +439,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         config->index = false;
>                         break;
>
> +               case OPT_METACOPY_ON:
> +                       config->metacopy = true;
> +                       break;
> +
> +               case OPT_METACOPY_OFF:
> +                       config->metacopy = false;
> +                       break;
> +
>                 default:
>                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>                         return -EINVAL;
> @@ -441,6 +461,12 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                 config->workdir = NULL;
>         }
>
> +       /* As of now metacopy=on is dependent on index=on */
> +       if (config->metacopy && !config->index) {
> +               pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
> +               config->metacopy = false;
> +       }
> +

There are later places in fill_super where index may be falling back to off
due to missing file handle support in lower fs.
Please move this check to much later to make sure metacopy=on and index=off
cannot co-exists.

Thanks,
Amir.

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

[Amir Goldstein]

On Fri, Oct 6, 2017 at 10:01 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Oct 06, 2017 at 09:58:58PM +0300, Amir Goldstein wrote:
>> On Fri, Oct 6, 2017 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > If an inode has been copied up metadata only, then we need to query the
>> > number of blocks from lower and fill up the stat->st_blocks.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/inode.c | 18 ++++++++++++++++--
>> >  1 file changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> > index e5825b8948e0..18007634ea9a 100644
>> > --- a/fs/overlayfs/inode.c
>> > +++ b/fs/overlayfs/inode.c
>> > @@ -67,6 +67,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> >         const struct cred *old_cred;
>> >         bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
>> >         int err;
>> > +       bool lowerstat_done = false;
>> > +       struct kstat lowerstat;
>> >
>> >         type = ovl_path_real(dentry, &realpath);
>> >         old_cred = ovl_override_creds(dentry->d_sb);
>> > @@ -86,8 +88,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> >          */
>> >         if (ovl_same_sb(dentry->d_sb)) {
>> >                 if (OVL_TYPE_ORIGIN(type)) {
>> > -                       struct kstat lowerstat;
>> > -                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
>> > +                       u32 lowermask = STATX_INO | STATX_BLOCKS |
>> > +                                       (!is_dir ? STATX_NLINK : 0);
>> >
>> >                         ovl_path_lower(dentry, &realpath);
>> >                         err = vfs_getattr(&realpath, &lowerstat,
>> > @@ -95,6 +97,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>> >                         if (err)
>> >                                 goto out;
>> >
>> > +                       lowerstat_done = true;
>> >                         WARN_ON_ONCE(stat->dev != lowerstat.dev);
>> >                         /*
>> >                          * Lower hardlinks may be broken on copy up to different
>> > @@ -140,6 +143,17 @@ 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))) {
>> > +               u32 lowermask = STATX_BLOCKS;
>> > +
>> > +               if (!lowerstat_done) {
>>
>> I guess lowerstat_done is as result from my comment,
>> but I did not mean I think vfs_getattr() is expensive,
>> I just didn't like the duplication of that code block, but maybe
>> just a matter of personal taste.
>> Can be sorted out later.
>
> Yes, I added this because I thought you wanted to avoid that extra
> vfs_getattr() call. I am open to change it whatever way you like.
> Personally I did not like this extra boolean and I liked unconditional
> vfs_getattr() better. It was not most optimized but was easier to
> understand.
>

Yeh, I liked it without the boolean better as well.
What I meant is once this patch lands:
https://github.com/chandanr/linux/commit/e9024b0a08a1c725daed548f68ff703305c40124
vfs_getattr() will be called for all non-dir with ORIGIN anyway, so
METACOPY will be able to piggy back STATX_BLOCKS flag on the same call.

Amir.

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

[Vivek Goyal]

On Fri, Oct 06, 2017 at 10:09:38PM +0300, Amir Goldstein wrote:

[..]
> >> > @@ -140,6 +143,17 @@ 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))) {
> >> > +               u32 lowermask = STATX_BLOCKS;
> >> > +
> >> > +               if (!lowerstat_done) {
> >>
> >> I guess lowerstat_done is as result from my comment,
> >> but I did not mean I think vfs_getattr() is expensive,
> >> I just didn't like the duplication of that code block, but maybe
> >> just a matter of personal taste.
> >> Can be sorted out later.
> >
> > Yes, I added this because I thought you wanted to avoid that extra
> > vfs_getattr() call. I am open to change it whatever way you like.
> > Personally I did not like this extra boolean and I liked unconditional
> > vfs_getattr() better. It was not most optimized but was easier to
> > understand.
> >
> 
> Yeh, I liked it without the boolean better as well.
> What I meant is once this patch lands:
> https://github.com/chandanr/linux/commit/e9024b0a08a1c725daed548f68ff703305c40124
> vfs_getattr() will be called for all non-dir with ORIGIN anyway, so
> METACOPY will be able to piggy back STATX_BLOCKS flag on the same call.

Aha, got it. Once these patches are merged, I will rebase my patch on top
of it. 

In the mean time, for the purpose of review and test, I will continue with my
patch.

Vivek

Re: [PATCH 3/8] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Fri, Oct 06, 2017 at 10:01:47PM +0300, Amir Goldstein wrote:

[..]
> > @@ -441,6 +461,12 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >                 config->workdir = NULL;
> >         }
> >
> > +       /* As of now metacopy=on is dependent on index=on */
> > +       if (config->metacopy && !config->index) {
> > +               pr_warn("overlayfs: can not enable metadata only copyup with index=off. Falling back to metacopy=off\n");
> > +               config->metacopy = false;
> > +       }
> > +
> 
> There are later places in fill_super where index may be falling back to off
> due to missing file handle support in lower fs.
> Please move this check to much later to make sure metacopy=on and index=off
> cannot co-exists.

Sure, I missed that. Will fix it.

Vivek