[PATCH 0/6] ovl: concurrent copy up

[PATCH 0/6] ovl: concurrent copy up

[Amir Goldstein]

Miklos,

This is my 2nd attempt to implement concurrent copy up
in overlayfs (3rd if you count the DCACHE_DELETE_LOCK flop).

It's not tagged as V2, because there is nothing in common
between the previous attempt and this one.

This work is based on Al's suggestion to use O_TMPFILE
for regular files and your very helpful waitqueue patch.

Patches 1-4 implement copy up of regular file using O_TMPFILE.
Since there is more common then different code wrt O_TMPFILE
vs. workdir+rename, I opted to keep ovl_copy_up_one() and
copy_up_one_locked() functions with a few 'if (tmpfile) else'
sprinkled around. It may be better to clone copy_up_one_locked()?

Patch 5 is the waitqueue patch you sent me. My only additions are
init_waitqueue_head() and wake_up_lock(), so I kept you as author.

Patch 6 puts it all together. Once again, in an attempt to keep
more common code, I added a few 'if (tmpfile)' and kept the locking
code generic, by assigning workdir = upperdir. I thought you may not
approve with this trick, but couldn't resist trying.

Beyond this simple test:
$ touch /lower/{empty,4g}
$ truncate -s 4g /lower/4g
$ touch /mnt/4g &  # takes a while
$ touch /mnt/empty # exits immediately
$ touch /mnt/4g    # blocks until %1 completes and can be interrupted

I also wrote xfstest overlay/021 which exercises concurrent copy up
using 8 processes on 4 directories and 4K files.

Amir.

Amir Goldstein (5):
  vfs: create vfs helper vfs_tmpfile()
  ovl: check if upperdir fs supports O_TMPFILE
  ovl: rearrange code in ovl_copy_up_locked()
  ovl: copy up regular file using O_TMPFILE
  ovl: concurrent copy up of regular files

Miklos Szeredi (1):
  ovl: introduce copy up waitqueue

 fs/namei.c               | 26 +++++++++-------
 fs/overlayfs/copy_up.c   | 78 +++++++++++++++++++++++++++++++++++-------------
 fs/overlayfs/overlayfs.h | 13 ++++++++
 fs/overlayfs/ovl_entry.h |  3 ++
 fs/overlayfs/super.c     | 11 +++++++
 fs/overlayfs/util.c      | 68 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h       |  1 +
 7 files changed, 170 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Amir Goldstein]

Factor out some common vfs bits from do_tmpfile()
to be used by overlayfs for concurrent copy up.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/namei.c         | 26 ++++++++++++++++----------
 include/linux/fs.h |  1 +
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ad74877..4cbaf54 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3353,6 +3353,20 @@ static int do_last(struct nameidata *nd,
 	return error;
 }
 
+int vfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
+{
+	int error;
+
+	/* we want directory to be writable */
+	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
+	if (error)
+		return error;
+	if (!dir->i_op->tmpfile)
+		return -EOPNOTSUPP;
+	return dir->i_op->tmpfile(dir, dentry, mode);
+}
+EXPORT_SYMBOL(vfs_tmpfile);
+
 static int do_tmpfile(struct nameidata *nd, unsigned flags,
 		const struct open_flags *op,
 		struct file *file, int *opened)
@@ -3368,14 +3382,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
 	if (unlikely(error))
 		goto out;
 	dir = path.dentry->d_inode;
-	/* we want directory to be writable */
-	error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
-	if (error)
-		goto out2;
-	if (!dir->i_op->tmpfile) {
-		error = -EOPNOTSUPP;
-		goto out2;
-	}
 	child = d_alloc(path.dentry, &name);
 	if (unlikely(!child)) {
 		error = -ENOMEM;
@@ -3383,8 +3389,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
 	}
 	dput(path.dentry);
 	path.dentry = child;
-	error = dir->i_op->tmpfile(dir, child, op->mode);
-	if (error)
+	error = vfs_tmpfile(dir, child, op->mode);
+	if (unlikely(error))
 		goto out2;
 	audit_inode(nd->name, child, 0);
 	/* Don't check for other permissions, the inode was just created */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba0743..13f56f1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1560,6 +1560,7 @@ extern int vfs_rmdir(struct inode *, struct dentry *);
 extern int vfs_unlink(struct inode *, struct dentry *, struct inode **);
 extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int);
 extern int vfs_whiteout(struct inode *, struct dentry *);
+extern int vfs_tmpfile(struct inode *, struct dentry *, umode_t);
 
 /*
  * VFS file helper functions.
-- 
2.7.4

[PATCH 2/6] ovl: check if upperdir fs supports O_TMPFILE

[Amir Goldstein]

This is needed for choosing between concurrent copyup
using O_TMPFILE and legacy copyup using workdir+rename.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  9 +++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 10 ++++++++++
 fs/overlayfs/util.c      | 18 ++++++++++++++++++
 4 files changed, 38 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 8af450b..190b03f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -127,6 +127,14 @@ static inline int ovl_do_whiteout(struct inode *dir, struct dentry *dentry)
 	return err;
 }
 
+static inline int ovl_do_tmpfile(struct inode *dir, struct dentry *dentry,
+				 umode_t mode)
+{
+	int err = vfs_tmpfile(dir, dentry, mode);
+	pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err);
+	return err;
+}
+
 static inline struct inode *ovl_inode_real(struct inode *inode, bool *is_upper)
 {
 	unsigned long x = (unsigned long) READ_ONCE(inode->i_private);
@@ -169,6 +177,7 @@ void ovl_dentry_version_inc(struct dentry *dentry);
 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);
+struct dentry *ovl_alloc_tmpfile(struct dentry *parent, umode_t mode);
 
 /* namei.c */
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index d14bca1..65f24000 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -27,6 +27,7 @@ struct ovl_fs {
 	struct ovl_config config;
 	/* creds of process who forced instantiation of super block */
 	const struct cred *creator_cred;
+	bool tmpfile;
 };
 
 /* private information held for every overlayfs dentry */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 20f48ab..a6c42f5 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -825,6 +825,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		 * creation of workdir in previous step.
 		 */
 		if (ufs->workdir) {
+			struct dentry *temp;
+
 			err = ovl_check_d_type_supported(&workpath);
 			if (err < 0)
 				goto out_put_workdir;
@@ -836,6 +838,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			 */
 			if (!err)
 				pr_warn("overlayfs: upper fs needs to support d_type.\n");
+
+			/* Check if upper/work fs supports O_TMPFILE */
+			temp = ovl_alloc_tmpfile(ufs->workdir, S_IFREG | 0);
+			ufs->tmpfile = !IS_ERR(temp);
+			if (ufs->tmpfile)
+				dput(temp);
+			else
+				pr_warn("overlayfs: upper fs does not support tmpfile.\n");
 		}
 	}
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 952286f..e6facb1 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -263,3 +263,21 @@ struct file *ovl_path_open(struct path *path, int flags)
 {
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
+
+struct dentry *ovl_alloc_tmpfile(struct dentry *parent, umode_t mode)
+{
+	static const struct qstr name = QSTR_INIT("/", 1);
+	struct dentry *temp;
+	struct inode *dir = parent->d_inode;
+	int err;
+
+	temp = d_alloc(parent, &name);
+	if (unlikely(!temp))
+		return ERR_PTR(-ENOMEM);
+
+	err = ovl_do_tmpfile(dir, temp, mode);
+	if (unlikely(err))
+		return ERR_PTR(err);
+
+	return temp;
+}
-- 
2.7.4

[PATCH 3/6] ovl: rearrange code in ovl_copy_up_locked()

[Amir Goldstein]

As preparation to implementing copy up with O_TMPFILE,
name the variable for dentry final rename 'temp' and
assign it to 'newdentry' only after rename.

Also lookup upper dentry before looking up temp dentry,
because that is more convenient for upcoming change.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f57043d..429990a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -238,6 +238,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	struct inode *udir = upperdir->d_inode;
 	struct dentry *newdentry = NULL;
 	struct dentry *upper = NULL;
+	struct dentry *temp = NULL;
 	int err;
 	const struct cred *old_creds = NULL;
 	struct cred *new_creds = NULL;
@@ -248,25 +249,25 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 		.link = link
 	};
 
-	newdentry = ovl_lookup_temp(workdir, dentry);
-	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
-		goto out;
-
 	upper = lookup_one_len(dentry->d_name.name, upperdir,
 			       dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
-		goto out1;
+		goto out;
 
 	err = security_inode_copy_up(dentry, &new_creds);
 	if (err < 0)
-		goto out2;
+		goto out1;
 
 	if (new_creds)
 		old_creds = override_creds(new_creds);
 
-	err = ovl_create_real(wdir, newdentry, &cattr, NULL, true);
+	temp = ovl_lookup_temp(workdir, dentry);
+	err = PTR_ERR(temp);
+	if (IS_ERR(temp))
+		goto out1;
+
+	err = ovl_create_real(wdir, temp, &cattr, NULL, true);
 
 	if (new_creds) {
 		revert_creds(old_creds);
@@ -281,39 +282,39 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 
 		ovl_path_upper(dentry, &upperpath);
 		BUG_ON(upperpath.dentry != NULL);
-		upperpath.dentry = newdentry;
+		upperpath.dentry = temp;
 
 		err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
 		if (err)
 			goto out_cleanup;
 	}
 
-	err = ovl_copy_xattr(lowerpath->dentry, newdentry);
+	err = ovl_copy_xattr(lowerpath->dentry, temp);
 	if (err)
 		goto out_cleanup;
 
-	inode_lock(newdentry->d_inode);
-	err = ovl_set_attr(newdentry, stat);
-	inode_unlock(newdentry->d_inode);
+	inode_lock(temp->d_inode);
+	err = ovl_set_attr(temp, stat);
+	inode_unlock(temp->d_inode);
 	if (err)
 		goto out_cleanup;
 
-	err = ovl_do_rename(wdir, newdentry, udir, upper, 0);
+	err = ovl_do_rename(wdir, temp, udir, upper, 0);
 	if (err)
 		goto out_cleanup;
 
+	newdentry = dget(temp);
 	ovl_dentry_update(dentry, newdentry);
 	ovl_inode_update(d_inode(dentry), d_inode(newdentry));
-	newdentry = NULL;
 out2:
-	dput(upper);
+	dput(temp);
 out1:
-	dput(newdentry);
+	dput(upper);
 out:
 	return err;
 
 out_cleanup:
-	ovl_cleanup(wdir, newdentry);
+	ovl_cleanup(wdir, temp);
 	goto out2;
 }
 
-- 
2.7.4

[PATCH 4/6] ovl: copy up regular file using O_TMPFILE

[Amir Goldstein]

In preparation for concurrent copy up, implement copy up
of regular file as O_TMPFILE that is linked to upperdir
instead of a file in workdir that is moved to upperdir.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 28 +++++++++++++++++++++-------
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/util.c      | 20 ++++++++++++++++++++
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 429990a..d3b6c15 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -20,6 +20,7 @@
 #include <linux/fdtable.h>
 #include <linux/ratelimit.h>
 #include "overlayfs.h"
+#include "ovl_entry.h"
 
 #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
 
@@ -232,7 +233,8 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 
 static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 			      struct dentry *dentry, struct path *lowerpath,
-			      struct kstat *stat, const char *link)
+			      struct kstat *stat, const char *link,
+			      bool tmpfile)
 {
 	struct inode *wdir = workdir->d_inode;
 	struct inode *udir = upperdir->d_inode;
@@ -262,12 +264,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (new_creds)
 		old_creds = override_creds(new_creds);
 
-	temp = ovl_lookup_temp(workdir, dentry);
+	if (tmpfile)
+		temp = ovl_alloc_tmpfile(upperdir, stat->mode);
+	else
+		temp = ovl_lookup_temp(workdir, dentry);
 	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
 		goto out1;
 
-	err = ovl_create_real(wdir, temp, &cattr, NULL, true);
+	err = 0;
+	if (!tmpfile)
+		err = ovl_create_real(wdir, temp, &cattr, NULL, true);
 
 	if (new_creds) {
 		revert_creds(old_creds);
@@ -299,11 +306,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (err)
 		goto out_cleanup;
 
-	err = ovl_do_rename(wdir, temp, udir, upper, 0);
+	if (tmpfile)
+		err = ovl_link_tmpfile(temp, udir, upper);
+	else
+		err = ovl_do_rename(wdir, temp, udir, upper, 0);
 	if (err)
 		goto out_cleanup;
 
-	newdentry = dget(temp);
+	newdentry = dget(tmpfile ? upper : temp);
 	ovl_dentry_update(dentry, newdentry);
 	ovl_inode_update(d_inode(dentry), d_inode(newdentry));
 out2:
@@ -314,7 +324,8 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	return err;
 
 out_cleanup:
-	ovl_cleanup(wdir, temp);
+	if (!tmpfile)
+		ovl_cleanup(wdir, temp);
 	goto out2;
 }
 
@@ -338,6 +349,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	struct dentry *lowerdentry = lowerpath->dentry;
 	struct dentry *upperdir;
 	const char *link = NULL;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	/* Should we copyup with O_TMPFILE or with workdir? */
+	bool tmpfile = S_ISREG(stat->mode) && ofs->tmpfile;
 
 	if (WARN_ON(!workdir))
 		return -EROFS;
@@ -369,7 +383,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	}
 
 	err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
-				 stat, link);
+				 stat, link, tmpfile);
 	if (!err) {
 		/* Restore timestamps on parent (best effort) */
 		ovl_set_timestamps(upperdir, &pstat);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 190b03f..59d1c38 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -178,6 +178,8 @@ 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);
 struct dentry *ovl_alloc_tmpfile(struct dentry *parent, umode_t mode);
+int ovl_link_tmpfile(struct dentry *temp, struct inode *dir,
+		     struct dentry *dentry);
 
 /* namei.c */
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index e6facb1..0fbf41c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -281,3 +281,23 @@ struct dentry *ovl_alloc_tmpfile(struct dentry *parent, umode_t mode)
 
 	return temp;
 }
+
+int ovl_link_tmpfile(struct dentry *temp, struct inode *dir,
+		     struct dentry *dentry)
+{
+	struct inode *inode = temp->d_inode;
+	int err;
+
+	if (WARN_ON_ONCE(!inode || inode->i_nlink))
+		return -ENOENT;
+
+	/* Make tempfile linkable */
+	spin_lock(&inode->i_lock);
+	if (!inode->i_nlink)
+		inode->i_state |= I_LINKABLE;
+	spin_unlock(&inode->i_lock);
+
+	err = ovl_do_link(temp, dir, dentry, true);
+
+	return err;
+}
-- 
2.7.4

[PATCH 5/6] ovl: introduce copy up waitqueue

[Amir Goldstein]

From: Miklos Szeredi <miklos@szeredi.hu>

The overlay sb 'copyup_wq' and overlay inode 'copying' condition
variable are about to replace the upper sb rename_lock, as finer
grained synchronization objects for concurrent copy up.

Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     |  1 +
 fs/overlayfs/util.c      | 30 ++++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 59d1c38..131efb7 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -180,6 +180,8 @@ struct file *ovl_path_open(struct path *path, int flags);
 struct dentry *ovl_alloc_tmpfile(struct dentry *parent, umode_t mode);
 int ovl_link_tmpfile(struct dentry *temp, struct inode *dir,
 		     struct dentry *dentry);
+int ovl_copy_up_start(struct dentry *dentry);
+void ovl_copy_up_end(struct dentry *dentry);
 
 /* namei.c */
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 65f24000..59614fa 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -28,6 +28,7 @@ struct ovl_fs {
 	/* creds of process who forced instantiation of super block */
 	const struct cred *creator_cred;
 	bool tmpfile;
+	wait_queue_head_t copyup_wq;
 };
 
 /* private information held for every overlayfs dentry */
@@ -39,6 +40,7 @@ struct ovl_entry {
 			u64 version;
 			const char *redirect;
 			bool opaque;
+			bool copying;
 		};
 		struct rcu_head rcu;
 	};
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index a6c42f5..4a9f489 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -708,6 +708,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ufs)
 		goto out;
 
+	init_waitqueue_head(&ufs->copyup_wq);
 	ufs->config.redirect_dir = ovl_redirect_dir_def;
 	err = ovl_parse_opt((char *) data, &ufs->config);
 	if (err)
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0fbf41c..f361b47 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -301,3 +301,33 @@ int ovl_link_tmpfile(struct dentry *temp, struct inode *dir,
 
 	return err;
 }
+
+int ovl_copy_up_start(struct dentry *dentry)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_entry *oe = dentry->d_fsdata;
+	int err;
+
+	spin_lock(&ofs->copyup_wq.lock);
+	err = wait_event_interruptible_locked(ofs->copyup_wq, !oe->copying);
+	if (!err) {
+		if (oe->__upperdentry)
+			err = 1; /* Already copied up */
+		else
+			oe->copying = true;
+	}
+	spin_unlock(&ofs->copyup_wq.lock);
+
+	return err;
+}
+
+void ovl_copy_up_end(struct dentry *dentry)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	spin_lock(&ofs->copyup_wq.lock);
+	oe->copying = false;
+	wake_up_locked(&ofs->copyup_wq);
+	spin_unlock(&ofs->copyup_wq.lock);
+}
-- 
2.7.4

[PATCH 6/6] ovl: concurrent copy up of regular files

[Amir Goldstein]

Now that copy up of regular file is done using O_TMPFILE,
we don't need to hold rename_lock throughout copy up.

Use the copy up waitqueue to synchronize concurrent copy up
of the same file. Different regular files can be copied up
concurrently.

The upper dir inode_lock is taken instead of rename_lock,
because it is needed for lookup and later for linking the
temp file, but it is released while copying up data.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d3b6c15..ddf5e2d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 		BUG_ON(upperpath.dentry != NULL);
 		upperpath.dentry = temp;
 
+		if (tmpfile)
+			inode_unlock(udir);
+
 		err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
+
+		if (tmpfile)
+			inode_lock_nested(udir, I_MUTEX_PARENT);
+
 		if (err)
 			goto out_cleanup;
 	}
@@ -371,6 +378,19 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			return PTR_ERR(link);
 	}
 
+	if (tmpfile) {
+		err = ovl_copy_up_start(dentry);
+		/* err < 0: interrupted, err > 0: raced with another copy-up */
+		if (unlikely(err)) {
+			pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
+			if (err > 0)
+				err = 0;
+			goto out_done;
+		}
+		/* lock_rename/unlock_rename will lock/unlock only upperdir */
+		workdir = upperdir;
+	}
+
 	err = -EIO;
 	if (lock_rename(workdir, upperdir) != NULL) {
 		pr_err("overlayfs: failed to lock workdir+upperdir\n");
@@ -390,6 +410,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	}
 out_unlock:
 	unlock_rename(workdir, upperdir);
+	if (tmpfile)
+		ovl_copy_up_end(dentry);
+out_done:
 	do_delayed_call(&done);
 
 	return err;
-- 
2.7.4

Re: [PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Miklos Szeredi]

On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Factor out some common vfs bits from do_tmpfile()
> to be used by overlayfs for concurrent copy up.

I'm wondering whether the vfs helper should do everything except the
path lookup and the open: d_alloc(), ->tmpfile() and setting
I_LINKABLE.   This will also aid in doing a ->tmpfile() for overlayfs.

Thanks,
Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/namei.c         | 26 ++++++++++++++++----------
>  include/linux/fs.h |  1 +
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index ad74877..4cbaf54 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3353,6 +3353,20 @@ static int do_last(struct nameidata *nd,
>         return error;
>  }
>
> +int vfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
> +{
> +       int error;
> +
> +       /* we want directory to be writable */
> +       error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
> +       if (error)
> +               return error;
> +       if (!dir->i_op->tmpfile)
> +               return -EOPNOTSUPP;
> +       return dir->i_op->tmpfile(dir, dentry, mode);
> +}
> +EXPORT_SYMBOL(vfs_tmpfile);
> +
>  static int do_tmpfile(struct nameidata *nd, unsigned flags,
>                 const struct open_flags *op,
>                 struct file *file, int *opened)
> @@ -3368,14 +3382,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
>         if (unlikely(error))
>                 goto out;
>         dir = path.dentry->d_inode;
> -       /* we want directory to be writable */
> -       error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
> -       if (error)
> -               goto out2;
> -       if (!dir->i_op->tmpfile) {
> -               error = -EOPNOTSUPP;
> -               goto out2;
> -       }
>         child = d_alloc(path.dentry, &name);
>         if (unlikely(!child)) {
>                 error = -ENOMEM;
> @@ -3383,8 +3389,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
>         }
>         dput(path.dentry);
>         path.dentry = child;
> -       error = dir->i_op->tmpfile(dir, child, op->mode);
> -       if (error)
> +       error = vfs_tmpfile(dir, child, op->mode);
> +       if (unlikely(error))
>                 goto out2;
>         audit_inode(nd->name, child, 0);
>         /* Don't check for other permissions, the inode was just created */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba0743..13f56f1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1560,6 +1560,7 @@ extern int vfs_rmdir(struct inode *, struct dentry *);
>  extern int vfs_unlink(struct inode *, struct dentry *, struct inode **);
>  extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int);
>  extern int vfs_whiteout(struct inode *, struct dentry *);
> +extern int vfs_tmpfile(struct inode *, struct dentry *, umode_t);
>
>  /*
>   * VFS file helper functions.
> --
> 2.7.4
>

Re: [PATCH 6/6] ovl: concurrent copy up of regular files

[Miklos Szeredi]

On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Now that copy up of regular file is done using O_TMPFILE,
> we don't need to hold rename_lock throughout copy up.
>
> Use the copy up waitqueue to synchronize concurrent copy up
> of the same file. Different regular files can be copied up
> concurrently.
>
> The upper dir inode_lock is taken instead of rename_lock,
> because it is needed for lookup and later for linking the
> temp file, but it is released while copying up data.
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/copy_up.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index d3b6c15..ddf5e2d 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>                 BUG_ON(upperpath.dentry != NULL);
>                 upperpath.dentry = temp;
>
> +               if (tmpfile)
> +                       inode_unlock(udir);
> +
>                 err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
> +
> +               if (tmpfile)
> +                       inode_lock_nested(udir, I_MUTEX_PARENT);
> +
>                 if (err)
>                         goto out_cleanup;
>         }
> @@ -371,6 +378,19 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                         return PTR_ERR(link);
>         }
>
> +       if (tmpfile) {
> +               err = ovl_copy_up_start(dentry);
> +               /* err < 0: interrupted, err > 0: raced with another copy-up */
> +               if (unlikely(err)) {
> +                       pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
> +                       if (err > 0)
> +                               err = 0;
> +                       goto out_done;
> +               }
> +               /* lock_rename/unlock_rename will lock/unlock only upperdir */
> +               workdir = upperdir;

Unnecessary obfuscation.   Just do lock_inode(); ovl_copy_up_locked();
unlokc_inode();.

Thanks,
Miklos

> +       }
> +
>         err = -EIO;
>         if (lock_rename(workdir, upperdir) != NULL) {
>                 pr_err("overlayfs: failed to lock workdir+upperdir\n");
> @@ -390,6 +410,9 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>         }
>  out_unlock:
>         unlock_rename(workdir, upperdir);
> +       if (tmpfile)
> +               ovl_copy_up_end(dentry);
> +out_done:
>         do_delayed_call(&done);
>
>         return err;
> --
> 2.7.4
>

Re: [PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Amir Goldstein]

On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Factor out some common vfs bits from do_tmpfile()
>> to be used by overlayfs for concurrent copy up.
>
> I'm wondering whether the vfs helper should do everything except the
> path lookup and the open: d_alloc(), ->tmpfile() and setting
> I_LINKABLE.   This will also aid in doing a ->tmpfile() for overlayfs.
>

I started with that, but slowly trimmed it down to this minimal version.
First, mnt_want_write() can't be in there.
Then dentry * return value would be a strange deviation from other vfs_ helpers.
Lastly, all the open related operations are already performed by
ovl_path_open() and I did not want to make an exception,
so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes
a drop-in replacement for ovl_lookup_temp() and set LINKABLE in
ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename().

Re: [PATCH 6/6] ovl: concurrent copy up of regular files

[Amir Goldstein]

On Mon, Jan 16, 2017 at 1:05 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Now that copy up of regular file is done using O_TMPFILE,
>> we don't need to hold rename_lock throughout copy up.
>>
>> Use the copy up waitqueue to synchronize concurrent copy up
>> of the same file. Different regular files can be copied up
>> concurrently.
>>
>> The upper dir inode_lock is taken instead of rename_lock,
>> because it is needed for lookup and later for linking the
>> temp file, but it is released while copying up data.
>>
>> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/copy_up.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index d3b6c15..ddf5e2d 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>>                 BUG_ON(upperpath.dentry != NULL);
>>                 upperpath.dentry = temp;
>>
>> +               if (tmpfile)
>> +                       inode_unlock(udir);
>> +
>>                 err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
>> +
>> +               if (tmpfile)
>> +                       inode_lock_nested(udir, I_MUTEX_PARENT);
>> +
>>                 if (err)
>>                         goto out_cleanup;
>>         }
>> @@ -371,6 +378,19 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>>                         return PTR_ERR(link);
>>         }
>>
>> +       if (tmpfile) {
>> +               err = ovl_copy_up_start(dentry);
>> +               /* err < 0: interrupted, err > 0: raced with another copy-up */
>> +               if (unlikely(err)) {
>> +                       pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
>> +                       if (err > 0)
>> +                               err = 0;
>> +                       goto out_done;
>> +               }
>> +               /* lock_rename/unlock_rename will lock/unlock only upperdir */
>> +               workdir = upperdir;
>
> Unnecessary obfuscation.   Just do lock_inode(); ovl_copy_up_locked();
> unlokc_inode();.
>

upperdir still needs to be locked for ovl_set_timestamps(), so I can do:

+                    inode_lock_nested(udir, I_MUTEX_PARENT)
+           } else {
>>         if (lock_rename(workdir, upperdir) != NULL) {
>>                 pr_err("overlayfs: failed to lock workdir+upperdir\n");
...
>>  out_unlock:
+             if (tmpfile)
+                     inode_unlock(udir);
+             else
>>         unlock_rename(workdir, upperdir);
>> +       if (tmpfile)
>> +               ovl_copy_up_end(dentry);
>> +out_done:
>>         do_delayed_call(&done);
>>
>>         return err;
>> --
>> 2.7.4
>>

[PATCH v2 6/6] ovl: concurrent copy up of regular files

[Amir Goldstein]

Now that copy up of regular file is done using O_TMPFILE,
we don't need to hold rename_lock throughout copy up.

Use the copy up waitqueue to synchronize concurrent copy up
of the same file. Different regular files can be copied up
concurrently.

The upper dir inode_lock is taken instead of rename_lock,
because it is needed for lookup and later for linking the
temp file, but it is released while copying up data.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d3b6c15..0d1cb96 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 		BUG_ON(upperpath.dentry != NULL);
 		upperpath.dentry = temp;
 
+		if (tmpfile)
+			inode_unlock(udir);
+
 		err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
+
+		if (tmpfile)
+			inode_lock_nested(udir, I_MUTEX_PARENT);
+
 		if (err)
 			goto out_cleanup;
 	}
@@ -371,15 +378,28 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			return PTR_ERR(link);
 	}
 
-	err = -EIO;
-	if (lock_rename(workdir, upperdir) != NULL) {
-		pr_err("overlayfs: failed to lock workdir+upperdir\n");
-		goto out_unlock;
-	}
-	if (ovl_dentry_upper(dentry)) {
-		/* Raced with another copy-up?  Nothing to do, then... */
-		err = 0;
-		goto out_unlock;
+	if (tmpfile) {
+		err = ovl_copy_up_start(dentry);
+		/* err < 0: interrupted, err > 0: raced with another copy-up */
+		if (unlikely(err)) {
+			pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
+			if (err > 0)
+				err = 0;
+			goto out_done;
+		}
+		/* Lock upper dir for lookup, link tmpfile, set_timestamps */
+		inode_lock_nested(upperdir->d_inode, I_MUTEX_PARENT);
+	} else {
+		err = -EIO;
+		if (lock_rename(workdir, upperdir) != NULL) {
+			pr_err("overlayfs: failed to lock workdir+upperdir\n");
+			goto out_unlock;
+		}
+		if (ovl_dentry_upper(dentry)) {
+			/* Raced with another copy-up?  Nothing to do */
+			err = 0;
+			goto out_unlock;
+		}
 	}
 
 	err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
@@ -389,7 +409,13 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 		ovl_set_timestamps(upperdir, &pstat);
 	}
 out_unlock:
-	unlock_rename(workdir, upperdir);
+	if (tmpfile)
+		inode_unlock(upperdir->d_inode);
+	else
+		unlock_rename(workdir, upperdir);
+	if (tmpfile)
+		ovl_copy_up_end(dentry);
+out_done:
 	do_delayed_call(&done);
 
 	return err;
-- 
2.7.4

Re: [PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Miklos Szeredi]

On Mon, Jan 16, 2017 at 12:19 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Factor out some common vfs bits from do_tmpfile()
>>> to be used by overlayfs for concurrent copy up.
>>
>> I'm wondering whether the vfs helper should do everything except the
>> path lookup and the open: d_alloc(), ->tmpfile() and setting
>> I_LINKABLE.   This will also aid in doing a ->tmpfile() for overlayfs.
>>
>
> I started with that, but slowly trimmed it down to this minimal version.
> First, mnt_want_write() can't be in there.
> Then dentry * return value would be a strange deviation from other vfs_ helpers.

What about lookup_one_len()?  It's not called vfs_something but that's
beside the point, I think.

> Lastly, all the open related operations are already performed by
> ovl_path_open() and I did not want to make an exception,
> so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes
> a drop-in replacement for ovl_lookup_temp() and set LINKABLE in
> ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename().

The logical place to set I_LINKABLE is in vfs_tmpfile().  You are
arguing about overlayfs code structure, but that's mostly irrelevant
when doing a new vfs interface.

Thanks,
Miklos

Re: [PATCH v2 6/6] ovl: concurrent copy up of regular files

[Miklos Szeredi]

On Mon, Jan 16, 2017 at 12:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Now that copy up of regular file is done using O_TMPFILE,
> we don't need to hold rename_lock throughout copy up.
>
> Use the copy up waitqueue to synchronize concurrent copy up
> of the same file. Different regular files can be copied up
> concurrently.
>
> The upper dir inode_lock is taken instead of rename_lock,
> because it is needed for lookup and later for linking the
> temp file, but it is released while copying up data.
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/copy_up.c | 46 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index d3b6c15..0d1cb96 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -291,7 +291,14 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
>                 BUG_ON(upperpath.dentry != NULL);
>                 upperpath.dentry = temp;
>
> +               if (tmpfile)
> +                       inode_unlock(udir);
> +
>                 err = ovl_copy_up_data(lowerpath, &upperpath, stat->size);
> +
> +               if (tmpfile)
> +                       inode_lock_nested(udir, I_MUTEX_PARENT);
> +
>                 if (err)
>                         goto out_cleanup;
>         }
> @@ -371,15 +378,28 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                         return PTR_ERR(link);
>         }
>
> -       err = -EIO;
> -       if (lock_rename(workdir, upperdir) != NULL) {
> -               pr_err("overlayfs: failed to lock workdir+upperdir\n");
> -               goto out_unlock;
> -       }
> -       if (ovl_dentry_upper(dentry)) {
> -               /* Raced with another copy-up?  Nothing to do, then... */
> -               err = 0;
> -               goto out_unlock;
> +       if (tmpfile) {
> +               err = ovl_copy_up_start(dentry);
> +               /* err < 0: interrupted, err > 0: raced with another copy-up */
> +               if (unlikely(err)) {
> +                       pr_debug("ovl_copy_up_start(%pd2) = %i\n", dentry, err);
> +                       if (err > 0)
> +                               err = 0;
> +                       goto out_done;
> +               }
> +               /* Lock upper dir for lookup, link tmpfile, set_timestamps */
> +               inode_lock_nested(upperdir->d_inode, I_MUTEX_PARENT);
> +       } else {
> +               err = -EIO;
> +               if (lock_rename(workdir, upperdir) != NULL) {
> +                       pr_err("overlayfs: failed to lock workdir+upperdir\n");
> +                       goto out_unlock;
> +               }
> +               if (ovl_dentry_upper(dentry)) {
> +                       /* Raced with another copy-up?  Nothing to do */
> +                       err = 0;
> +                       goto out_unlock;
> +               }
>         }
>
>         err = ovl_copy_up_locked(workdir, upperdir, dentry, lowerpath,
> @@ -389,7 +409,13 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>                 ovl_set_timestamps(upperdir, &pstat);

Or move ovl_set_timestamps() into ovl_copy_up_locked().

Lock and unlock in a different block will be confusing at best.

Thanks,
Miklos

>         }
>  out_unlock:
> -       unlock_rename(workdir, upperdir);
> +       if (tmpfile)
> +               inode_unlock(upperdir->d_inode);
> +       else
> +               unlock_rename(workdir, upperdir);
> +       if (tmpfile)
> +               ovl_copy_up_end(dentry);
> +out_done:
>         do_delayed_call(&done);
>
>         return err;
> --
> 2.7.4
>

Re: [PATCH 2/6] ovl: check if upperdir fs supports O_TMPFILE

[Miklos Szeredi]

On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> This is needed for choosing between concurrent copyup
> using O_TMPFILE and legacy copyup using workdir+rename.

I'm really wondering if we should constrain upper fs to those that:

 - can do RENAME_EXCHANGE and RENAME_WHITEOUT
 - can do ->tmpfile() which is currently a superset of the above
 - can do xattr, again a superset

The question is whether anybody actually using it with an fs that
doesn't have all of the above.  Because if so, we need to keep
supporting them.  Perhaps we should add warnings about deprecation and
if nobody complains we can remove support for non-conformant fs.

Thanks,
Miklos

Re: [PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Amir Goldstein]

On Mon, Jan 16, 2017 at 3:22 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Jan 16, 2017 at 12:19 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Jan 16, 2017 at 1:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Factor out some common vfs bits from do_tmpfile()
>>>> to be used by overlayfs for concurrent copy up.
>>>
>>> I'm wondering whether the vfs helper should do everything except the
>>> path lookup and the open: d_alloc(), ->tmpfile() and setting
>>> I_LINKABLE.   This will also aid in doing a ->tmpfile() for overlayfs.
>>>
>>
>> I started with that, but slowly trimmed it down to this minimal version.
>> First, mnt_want_write() can't be in there.
>> Then dentry * return value would be a strange deviation from other vfs_ helpers.
>
> What about lookup_one_len()?  It's not called vfs_something but that's
> beside the point, I think.
>
>> Lastly, all the open related operations are already performed by
>> ovl_path_open() and I did not want to make an exception,
>> so I resorted to doing d_alloc in ovl_alloc_tmpfile(), which becomes
>> a drop-in replacement for ovl_lookup_temp() and set LINKABLE in
>> ovl_link_tmpfile(), which becomes a drop-in replacement for ovl_do_rename().
>
> The logical place to set I_LINKABLE is in vfs_tmpfile().  You are
> arguing about overlayfs code structure, but that's mostly irrelevant
> when doing a new vfs interface.
>

What I tried to do at first is leave d_alloc inside vfs_tmpfile()
and return dentry, but I did not want this interface to deal with struct file
as well.

so do_tmpfile() is very roughly equivalent to:
path_lookupat()
mnt_want_write()
vfs_tmpfile() (including d_alloc)
dentry_open()
inode->i_state |= I_LINKABLE

So I can set LINKABLE inside vfs_tmpfile() only if I also pass
it open_flag and then it would also happen before open returns
success.

My intention was to keep the VFS patch as boring and uncontroversial
as possible.

I am tempted to say: could you possibly create that extra re-factoring yourself
either before or after this patch set? You are the one who is going to
be selling it to
Al after all...

Either that, or just send me better guidelines and I'll do the leg work myself.

Thanks.

Re: [PATCH 1/6] vfs: create vfs helper vfs_tmpfile()

[Miklos Szeredi]

On Mon, Jan 16, 2017 at 3:04 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> What I tried to do at first is leave d_alloc inside vfs_tmpfile()
> and return dentry, but I did not want this interface to deal with struct file
> as well.

Agreed, I think you should do that.

>
> so do_tmpfile() is very roughly equivalent to:
> path_lookupat()
> mnt_want_write()
> vfs_tmpfile() (including d_alloc)
> dentry_open()
> inode->i_state |= I_LINKABLE
>
> So I can set LINKABLE inside vfs_tmpfile() only if I also pass
> it open_flag

Yes.

> and then it would also happen before open returns
> success.

It doesn't matter.  If open fails the temporary file will be deleted,
dentry, inode and all.

Thanks,
Miklos

Re: [PATCH 2/6] ovl: check if upperdir fs supports O_TMPFILE

[Amir Goldstein]

On Mon, Jan 16, 2017 at 4:02 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Sun, Jan 15, 2017 at 2:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> This is needed for choosing between concurrent copyup
>> using O_TMPFILE and legacy copyup using workdir+rename.
>
> I'm really wondering if we should constrain upper fs to those that:
>
>  - can do RENAME_EXCHANGE and RENAME_WHITEOUT
>  - can do ->tmpfile() which is currently a superset of the above
>  - can do xattr, again a superset

Makes sense to me.
Let me know if you want me to add the rename flag test.

>
> The question is whether anybody actually using it with an fs that
> doesn't have all of the above.  Because if so, we need to keep
> supporting them.  Perhaps we should add warnings about deprecation and
> if nobody complains we can remove support for non-conformant fs.
>


But how exactly do we "support" those fs right now?
Any attempt to use them would result in -EINVAL, because we will
bw requesting RENAME_EXCHANGE and RENAME_WHITEOUT

Re: [PATCH 2/6] ovl: check if upperdir fs supports O_TMPFILE

[Miklos Szeredi]

On Mon, Jan 16, 2017 at 3:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> The question is whether anybody actually using it with an fs that
>> doesn't have all of the above.  Because if so, we need to keep
>> supporting them.  Perhaps we should add warnings about deprecation and
>> if nobody complains we can remove support for non-conformant fs.
>>
>
>
> But how exactly do we "support" those fs right now?
> Any attempt to use them would result in -EINVAL, because we will
> bw requesting RENAME_EXCHANGE and RENAME_WHITEOUT

Not unless whiteout/opaque objects are involved, and in fact those are
not very common, so I can imagine a limited usage that doesn't involve
those and will work fine.  But I don't think this scenario is likely,
so we should try deprecating it.

Thanks,
Miklos