[RFC][PATH 0/4] Reduce excessive use of lock_rename() in overlayfs

[RFC][PATH 0/4] Reduce excessive use of lock_rename() in overlayfs

[Amir Goldstein]

lock_rename() is a very big hammer used before every call to vfs_rename(),
in order to avoid some horrible corner cases that are described in the
comment above vfs_rename().

lock_rename() taked a super block global mutex before every rename operation
that involved moving to a different parent and forces all such renames in
the file system to be serialized.

That cost may be acceptable to most workloads, because moving files/dirs
between directories are not as common as other file system operations.

Overlayfs uses vfs_rename() more that most file systems do, not only for
user requested renames, but also for every copy-up and every whiteout,
so vfs_rename() can be triggered from user operations open/create/delete,
that are much more common the renames.

It turns out, though, that all the horrible corner cases that require
the big lock_rename() hammer, can be eliminated when setting up the
overlay mount and then lock_rename() can be avoided in many use cases.

I do not have any performance results to show, which is really called
for in this sort of work. But it seems obvious that when running multiple
docker containers, file system operations should not be serialized among
all of them. So I am hoping that someone, with test machines stronger
than mine, can take this code to a test drive.

This code passed regression tests with unionmount-testsuite, pjdfstest
and xfstests -g quick.
It fails xfs tests generic/373 and generic/374 and for a good reason:
those tests try to mount 2 overlayfs mounts with the same upper/work dirs.
I will send patches to fix those 2 tests.

Amir Goldstein (4):
  vfs: reinterpret sillyrename flag as delete lock
  vfs: introduce delete trylock/unlock
  ovl: delete lock upper dir and work dir
  ovl: relax lock_rename when moving files between work and upper dir

 fs/afs/dir.c             |  6 ++---
 fs/btrfs/ioctl.c         |  5 ++--
 fs/namei.c               | 25 ++++++++++++++++---
 fs/overlayfs/copy_up.c   |  7 +++---
 fs/overlayfs/dir.c       | 25 +++++++++++++------
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/super.c     | 63 +++++++++++++++++++++++++++++++++++++++---------
 include/linux/dcache.h   | 35 +++++++++++++++++++++++++++
 include/linux/namei.h    |  2 ++
 9 files changed, 137 insertions(+), 33 deletions(-)

-- 
2.7.4

[RFC][PATH 1/4] vfs: reinterpret sillyrename flag as delete lock

[Amir Goldstein]

The dentry flag DCACHE_NFSFS_RENAMED indicates the the entry
was sillyrenamed, as do some of the comment above may_delete().

It is better if the flag name indicates its true meaning instead
of refering to nfs internal implementation. In fact, afs was using
the flag with no relation to nfs silly rename.

Re-brand the flag as DCACHE_DELETE_LOCK and modify places
in the code that check/set it to use helpers cant_delete()
and dont_delete().

The old flag DCACHE_NFSFS_RENAMED remains and is being used only
in places really related to the nfs silly rename implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/afs/dir.c           |  6 ++----
 fs/btrfs/ioctl.c       |  5 ++---
 fs/namei.c             |  5 ++---
 include/linux/dcache.h | 13 +++++++++++++
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 51a241e..a0d79a7 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -678,9 +678,7 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
 
 	/* the dirent, if it exists, now points to a different vnode */
 not_found:
-	spin_lock(&dentry->d_lock);
-	dentry->d_flags |= DCACHE_NFSFS_RENAMED;
-	spin_unlock(&dentry->d_lock);
+	dont_delete(dentry);
 
 out_bad:
 	_debug("dropping dentry %pd2", dentry);
@@ -701,7 +699,7 @@ static int afs_d_delete(const struct dentry *dentry)
 {
 	_enter("%pd", dentry);
 
-	if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
+	if (cant_delete(dentry))
 		goto zap;
 
 	if (d_really_is_positive(dentry) &&
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7acbd2c..46048e54 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -785,8 +785,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
  *  7. If we were asked to remove a directory and victim isn't one - ENOTDIR.
  *  8. If we were asked to remove a non-directory and victim isn't one - EISDIR.
  *  9. We can't remove a root or mountpoint.
- * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
- *     nfs_async_unlink().
+ * 10. We don't allow removal of delete locked files.
  */
 
 static int btrfs_may_delete(struct inode *dir, struct dentry *victim, int isdir)
@@ -816,7 +815,7 @@ static int btrfs_may_delete(struct inode *dir, struct dentry *victim, int isdir)
 		return -EISDIR;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
-	if (victim->d_flags & DCACHE_NFSFS_RENAMED)
+	if (cant_delete(victim))
 		return -EBUSY;
 	return 0;
 }
diff --git a/fs/namei.c b/fs/namei.c
index 31d04d9..6040d1e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2762,8 +2762,7 @@ EXPORT_SYMBOL(__check_sticky);
  *  8. If we were asked to remove a directory and victim isn't one - ENOTDIR.
  *  9. If we were asked to remove a non-directory and victim isn't one - EISDIR.
  * 10. We can't remove a root or mountpoint.
- * 11. We don't allow removal of NFS sillyrenamed files; it's handled by
- *     nfs_async_unlink().
+ * 11. We don't allow removal of delete locked files.
  */
 static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
 {
@@ -2795,7 +2794,7 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
 		return -EISDIR;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
-	if (victim->d_flags & DCACHE_NFSFS_RENAMED)
+	if (cant_delete(victim))
 		return -EBUSY;
 	return 0;
 }
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 5beed7b..94cd40c 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -180,6 +180,7 @@ struct dentry_operations {
 #define DCACHE_OP_WEAK_REVALIDATE	0x00000800
 
 #define DCACHE_NFSFS_RENAMED		0x00001000
+#define DCACHE_DELETE_LOCK		0x00001000 /* May not delete/rename */
      /* this dentry has been "silly renamed" and has to be deleted on the last
       * dput() */
 #define DCACHE_COOKIE			0x00002000 /* For use by dcookie subsystem */
@@ -350,6 +351,18 @@ static inline void dont_mount(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+static inline int cant_delete(const struct dentry *dentry)
+{
+	return (dentry->d_flags & DCACHE_DELETE_LOCK);
+}
+
+static inline void dont_delete(struct dentry *dentry)
+{
+	spin_lock(&dentry->d_lock);
+	dentry->d_flags |= DCACHE_DELETE_LOCK;
+	spin_unlock(&dentry->d_lock);
+}
+
 extern void __d_lookup_done(struct dentry *);
 
 static inline int d_in_lookup(struct dentry *dentry)
-- 
2.7.4

[RFC][PATH 2/4] vfs: introduce delete trylock/unlock

[Amir Goldstein]

A delete lock prevents a dentry from being unlinked and renamed,
for as long as the dentry remains in cache.

Introduce the helpers delete_trylock() and delete_unlock(), which
use test_and_set and test_and_clear semantics to ser/clear a delete
lock on a dentry.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/dcache.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 94cd40c..90584b9 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -356,11 +356,33 @@ static inline int cant_delete(const struct dentry *dentry)
 	return (dentry->d_flags & DCACHE_DELETE_LOCK);
 }
 
-static inline void dont_delete(struct dentry *dentry)
+static inline int delete_trylock(struct dentry *dentry)
 {
+	int locked;
+
 	spin_lock(&dentry->d_lock);
+	locked = likely(!cant_delete(dentry));
 	dentry->d_flags |= DCACHE_DELETE_LOCK;
 	spin_unlock(&dentry->d_lock);
+
+	return locked;
+}
+
+static inline void dont_delete(struct dentry *dentry)
+{
+	delete_trylock(dentry);
+}
+
+static inline int delete_unlock(struct dentry *dentry)
+{
+	int unlocked;
+
+	spin_lock(&dentry->d_lock);
+	unlocked = likely(cant_delete(dentry));
+	dentry->d_flags &= ~DCACHE_DELETE_LOCK;
+	spin_unlock(&dentry->d_lock);
+
+	return unlocked;
 }
 
 extern void __d_lookup_done(struct dentry *);
-- 
2.7.4

[RFC][PATH 3/4] ovl: delete lock upper dir and work dir

[Amir Goldstein]

Attempt to take a delete lock on both upper dir and work dir
after verifying that they are not ancestors of each other.
Fail the mount with -EBUSY on failure to take the locks, as it
indicates that those directories may be in use by another overlay
mount.

The delete lock guaranties that upper/work dir can't be
moved into each other while the overlay is mounted.
The "work" directory created inside the workdir path
is also delete locked, so it can't be moved into upper dir
while overlay is mounted.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 625fa705..4f4eb16 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -142,8 +142,15 @@ static void ovl_put_super(struct super_block *sb)
 	struct ovl_fs *ufs = sb->s_fs_info;
 	unsigned i;
 
-	dput(ufs->workdir);
-	mntput(ufs->upper_mnt);
+	if (ufs->workdir) {
+		delete_unlock(ufs->workdir->d_parent);
+		delete_unlock(ufs->workdir);
+		dput(ufs->workdir);
+	}
+	if (ufs->upper_mnt) {
+		delete_unlock(ufs->upper_mnt->mnt_root);
+		mntput(ufs->upper_mnt);
+	}
 	for (i = 0; i < ufs->numlower; i++)
 		mntput(ufs->lower_mnt[i]);
 	kfree(ufs->lower_mnt);
@@ -384,6 +391,11 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt,
 		inode_unlock(work->d_inode);
 		if (err)
 			goto out_dput;
+
+		err = -EBUSY;
+		/* Lock work dir to its parent */
+		if (!delete_trylock(work))
+			goto out_dput;
 	}
 out_unlock:
 	inode_unlock(dir);
@@ -490,16 +502,34 @@ static int ovl_lower_dir(const char *name, struct path *path, long *namelen,
 	return err;
 }
 
-/* Workdir should not be subdir of upperdir and vice versa */
-static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
+/*
+ * Workdir should not be subdir of upperdir and vice versa.
+ * Delete lock both upper and workdir to their parent under lock_rename(),
+ * so if they are siblings, they remain siblings throughout the overlay mount
+ */
+static int ovl_workdir_trylock(struct dentry *workdir, struct dentry *upperdir)
 {
-	bool ok = false;
+	int ret;
+
+	if (workdir == upperdir)
+		return -EINVAL;
+
+	ret = -EINVAL;
+	if (lock_rename(workdir, upperdir) != NULL)
+		goto out_unlock_rename;
 
-	if (workdir != upperdir) {
-		ok = (lock_rename(workdir, upperdir) == NULL);
-		unlock_rename(workdir, upperdir);
+	ret = -EBUSY;
+	if (!delete_trylock(upperdir))
+		goto out_unlock_rename;
+	if (!delete_trylock(workdir)) {
+		delete_unlock(upperdir);
+		goto out_unlock_rename;
 	}
-	return ok;
+	ret = 0;
+
+out_unlock_rename:
+	unlock_rename(workdir, upperdir);
+	return ret;
 }
 
 static unsigned int ovl_split_lowerdirs(char *str)
@@ -717,16 +747,20 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			pr_err("overlayfs: workdir and upperdir must reside under the same mount\n");
 			goto out_put_workpath;
 		}
-		if (!ovl_workdir_ok(workpath.dentry, upperpath.dentry)) {
+		err = ovl_workdir_trylock(workpath.dentry, upperpath.dentry);
+		if (err == -EINVAL) {
 			pr_err("overlayfs: workdir and upperdir must be separate subtrees\n");
 			goto out_put_workpath;
+		} else if (err == -EBUSY) {
+			pr_err("overlayfs: workdir/upperdir may be in use by another overlay\n");
+			goto out_put_workpath;
 		}
 		sb->s_stack_depth = upperpath.mnt->mnt_sb->s_stack_depth;
 	}
 	err = -ENOMEM;
 	lowertmp = kstrdup(ufs->config.lowerdir, GFP_KERNEL);
 	if (!lowertmp)
-		goto out_put_workpath;
+		goto out_unlock_workpath;
 
 	err = -EINVAL;
 	stacklen = ovl_split_lowerdirs(lowertmp);
@@ -885,6 +919,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		mntput(ufs->lower_mnt[i]);
 	kfree(ufs->lower_mnt);
 out_put_workdir:
+	if (ufs->workdir)
+		delete_unlock(ufs->workdir);
 	dput(ufs->workdir);
 	mntput(ufs->upper_mnt);
 out_put_lowerpath:
@@ -893,6 +929,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	kfree(stack);
 out_free_lowertmp:
 	kfree(lowertmp);
+out_unlock_workpath:
+	if (workpath.dentry)
+		delete_unlock(workpath.dentry);
+	if (upperpath.dentry)
+		delete_unlock(upperpath.dentry);
 out_put_workpath:
 	path_put(&workpath);
 out_put_upperpath:
-- 
2.7.4

[RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

If work dir and upper dir have a common parent, then it
is safe to move files between them without taking the super block
s_vfs_rename_mutex.

Since both upper dir and work dir (as well as $workdir/work)
are delete locked on overlay mount, they cannot be moved.
Therefore, if upper and work dir have the same parent at mount
time, this cannot change thougout the time that overlay is mounted.

As a result, moving files between work dir and upper dir cannot
change the tree topology and cannot result in loops. For the same
reason, it is safe to take the inode locks on work+upper before
moving files, in any consistent order, as they cannot be descendants
of each other.

Implement a pair of helpers {unlock,lock}_rename_safe() to be used
instead of the full blown {unlock,lock}_rename() pair in the case of
work/upper dir having a common parent.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/namei.c               | 20 ++++++++++++++++++++
 fs/overlayfs/copy_up.c   |  7 +++----
 fs/overlayfs/dir.c       | 25 +++++++++++++++++--------
 fs/overlayfs/overlayfs.h |  2 ++
 include/linux/namei.h    |  2 ++
 5 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 6040d1e..8e3be1b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2867,6 +2867,26 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
 }
 EXPORT_SYMBOL(unlock_rename);
 
+/*
+ * p1 and p2 are delete locked and so are their parents, all the way
+ * to a common ancestor and they are not decendant of each other,
+ * so it is safe to move children between them without holding
+ * s_vfs_rename_mutex and it is safe to lock them in any order.
+ */
+void lock_rename_safe(struct dentry *p1, struct dentry *p2)
+{
+	inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
+	inode_lock_nested(p2->d_inode, I_MUTEX_PARENT2);
+}
+EXPORT_SYMBOL(lock_rename_safe);
+
+void unlock_rename_safe(struct dentry *p1, struct dentry *p2)
+{
+	inode_unlock(p1->d_inode);
+	inode_unlock(p2->d_inode);
+}
+EXPORT_SYMBOL(unlock_rename_safe);
+
 int vfs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 		bool want_excl)
 {
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f18c1a6..02b5386 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -367,9 +367,8 @@ 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");
+	err = ovl_lock_rename_workdir(workdir, upperdir);
+	if (err) {
 		goto out_unlock;
 	}
 	upperdentry = ovl_dentry_upper(dentry);
@@ -386,7 +385,7 @@ int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 		ovl_set_timestamps(upperdir, &pstat);
 	}
 out_unlock:
-	unlock_rename(workdir, upperdir);
+	ovl_unlock_rename_workdir(workdir, upperdir);
 	do_delayed_call(&done);
 
 	return err;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index f24b6b9..f0e9b8f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -202,15 +202,16 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	return err;
 }
 
-static int ovl_lock_rename_workdir(struct dentry *workdir,
-				   struct dentry *upperdir)
+int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
 {
 	/* Workdir should not be the same as upperdir */
 	if (workdir == upperdir)
 		goto err;
 
-	/* Workdir should not be subdir of upperdir and vice versa */
-	if (lock_rename(workdir, upperdir) != NULL)
+	/* FIXME: check this once in fill_super */
+	if (upperdir->d_parent == workdir->d_parent->d_parent)
+		lock_rename_safe(workdir, upperdir);
+	else if (lock_rename(workdir, upperdir) != NULL)
 		goto err_unlock;
 
 	return 0;
@@ -222,6 +223,14 @@ static int ovl_lock_rename_workdir(struct dentry *workdir,
 	return -EIO;
 }
 
+void ovl_unlock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
+{
+	if (upperdir->d_parent == workdir->d_parent->d_parent)
+		unlock_rename_safe(workdir, upperdir);
+	else
+		unlock_rename(workdir, upperdir);
+}
+
 static struct dentry *ovl_clear_empty(struct dentry *dentry,
 				      struct list_head *list)
 {
@@ -283,7 +292,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 
 	ovl_cleanup_whiteouts(upper, list);
 	ovl_cleanup(wdir, upper);
-	unlock_rename(workdir, upperdir);
+	ovl_unlock_rename_workdir(workdir, upperdir);
 
 	/* dentry's upper doesn't match now, get rid of it */
 	d_drop(dentry);
@@ -295,7 +304,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 out_dput:
 	dput(opaquedir);
 out_unlock:
-	unlock_rename(workdir, upperdir);
+	ovl_unlock_rename_workdir(workdir, upperdir);
 out:
 	return ERR_PTR(err);
 }
@@ -450,7 +459,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 out_dput:
 	dput(newdentry);
 out_unlock:
-	unlock_rename(workdir, upperdir);
+	ovl_unlock_rename_workdir(workdir, upperdir);
 out:
 	if (!hardlink) {
 		posix_acl_release(acl);
@@ -657,7 +666,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 out_dput_upper:
 	dput(upper);
 out_unlock:
-	unlock_rename(workdir, upperdir);
+	ovl_unlock_rename_workdir(workdir, upperdir);
 out_dput:
 	dput(opaquedir);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index f6e4d35..63ca647 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -211,6 +211,8 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct kstat *stat, const char *link,
 		    struct dentry *hardlink, bool debug);
 void ovl_cleanup(struct inode *dir, struct dentry *dentry);
+int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
+void ovl_unlock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
 
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index f29abda..31e3f4f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -88,6 +88,8 @@ extern int follow_up(struct path *);
 
 extern struct dentry *lock_rename(struct dentry *, struct dentry *);
 extern void unlock_rename(struct dentry *, struct dentry *);
+extern void lock_rename_safe(struct dentry *, struct dentry *);
+extern void unlock_rename_safe(struct dentry *, struct dentry *);
 
 extern void nd_jump_link(struct path *path);
 
-- 
2.7.4

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 12:44:43AM +0200, Amir Goldstein wrote:
> If work dir and upper dir have a common parent, then it
> is safe to move files between them without taking the super block
> s_vfs_rename_mutex.
> 
> Since both upper dir and work dir (as well as $workdir/work)
> are delete locked on overlay mount, they cannot be moved.

Do explain.  What do you mean, "delete locked" and why exactly can't they
be moved?

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Thu, Nov 10, 2016 at 11:02:21PM +0000, Al Viro wrote:
> On Fri, Nov 11, 2016 at 12:44:43AM +0200, Amir Goldstein wrote:
> > If work dir and upper dir have a common parent, then it
> > is safe to move files between them without taking the super block
> > s_vfs_rename_mutex.
> > 
> > Since both upper dir and work dir (as well as $workdir/work)
> > are delete locked on overlay mount, they cannot be moved.
> 
> Do explain.  What do you mean, "delete locked" and why exactly can't they
> be moved?

More specifically, what makes you think that may_delete() is called before
any change of parent?

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 1:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Nov 10, 2016 at 11:02:21PM +0000, Al Viro wrote:
>> On Fri, Nov 11, 2016 at 12:44:43AM +0200, Amir Goldstein wrote:
>> > If work dir and upper dir have a common parent, then it
>> > is safe to move files between them without taking the super block
>> > s_vfs_rename_mutex.
>> >
>> > Since both upper dir and work dir (as well as $workdir/work)
>> > are delete locked on overlay mount, they cannot be moved.
>>
>> Do explain.  What do you mean, "delete locked" and why exactly can't they
>> be moved?
>

That is explained in commit message of the first 2 patches, but
foremost I would like to say that the name "delete locked" is just the
best of all the bad choices for names I came up with, so I am expecting
better name suggestions.

The concept is to pin the entry so that it cannot be moved, so can have
certain guaranties about the stability of the directories topology.

> More specifically, what makes you think that may_delete() is called before
> any change of parent?

Because it is checked at the beginning of vfs_rename()
and in order to change a parent, an entry needs to be renamed. no?
What am I missing?

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 01:11:16AM +0200, Amir Goldstein wrote:

> That is explained in commit message of the first 2 patches, but
> foremost I would like to say that the name "delete locked" is just the
> best of all the bad choices for names I came up with, so I am expecting
> better name suggestions.
> 
> The concept is to pin the entry so that it cannot be moved, so can have
> certain guaranties about the stability of the directories topology.
> 
> > More specifically, what makes you think that may_delete() is called before
> > any change of parent?
> 
> Because it is checked at the beginning of vfs_rename()
> and in order to change a parent, an entry needs to be renamed. no?
> What am I missing?

Analysis of the rest of call chains where we have __d_move() called?  Not
to mention anything else, workdir itself might be moved around by
d_splice_alias() for a sufficiently unpleasant filesystem...

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 1:17 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 01:11:16AM +0200, Amir Goldstein wrote:
>
>> That is explained in commit message of the first 2 patches, but
>> foremost I would like to say that the name "delete locked" is just the
>> best of all the bad choices for names I came up with, so I am expecting
>> better name suggestions.
>>
>> The concept is to pin the entry so that it cannot be moved, so can have
>> certain guaranties about the stability of the directories topology.
>>
>> > More specifically, what makes you think that may_delete() is called before
>> > any change of parent?
>>
>> Because it is checked at the beginning of vfs_rename()
>> and in order to change a parent, an entry needs to be renamed. no?
>> What am I missing?
>
> Analysis of the rest of call chains where we have __d_move() called?  Not
> to mention anything else, workdir itself might be moved around by
> d_splice_alias() for a sufficiently unpleasant filesystem...

I am certainly looking for this feedback, because there is no other means for
me to sanity test if relaxing lock_rename() is safe.

When I write "any change of parent" I mean a change between 2 different
connected parents. Both work dir and upper dir are connected and with
reference held after mount.
Are the d_splice_alias() and __d_unalias() cases a real concern for moving
work dir around after the overlay mount??

Plus, if a file system is sufficiently unpleasant, perhaps there is a way to
categorize it as such by ovl_dentry_weird(), if it hasn't been
categorized as such already.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 01:33:08AM +0200, Amir Goldstein wrote:

> I am certainly looking for this feedback, because there is no other means for
> me to sanity test if relaxing lock_rename() is safe.
> 
> When I write "any change of parent" I mean a change between 2 different
> connected parents. Both work dir and upper dir are connected and with
> reference held after mount.
> Are the d_splice_alias() and __d_unalias() cases a real concern for moving
> work dir around after the overlay mount??

Why not?  Again, it's really up to you to provide an analysis of the call
chains.  There's nothing in d_splice_alias() to prohibit an existing alias
being attached - in fact, __d_unalias() is called exactly in that case.
It's a rare case, all right, but it is not impossible.

BTW, your analysis would better be simple and explicit - anything subtle
will be flat-out rejected, since it would have to be stepped around very
carefully in any later work in VFS.

I really wonder what it is that you are getting contention on - what are
you doing, besides the actual renames?  And that needs serialization anyway
(on inode lock of workdir, if nothing else), so any contention would not
disappear from dropped ->s_vfs_rename_mutex...

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 1:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 01:33:08AM +0200, Amir Goldstein wrote:
>
>> I am certainly looking for this feedback, because there is no other means for
>> me to sanity test if relaxing lock_rename() is safe.
>>
>> When I write "any change of parent" I mean a change between 2 different
>> connected parents. Both work dir and upper dir are connected and with
>> reference held after mount.
>> Are the d_splice_alias() and __d_unalias() cases a real concern for moving
>> work dir around after the overlay mount??
>
> Why not?  Again, it's really up to you to provide an analysis of the call
> chains.  There's nothing in d_splice_alias() to prohibit an existing alias
> being attached - in fact, __d_unalias() is called exactly in that case.
> It's a rare case, all right, but it is not impossible.
>
> BTW, your analysis would better be simple and explicit - anything subtle
> will be flat-out rejected, since it would have to be stepped around very
> carefully in any later work in VFS.
>

Sure, I get that.
Wanted to post early, before I have a full proof that this is sane
(if I am able to generate one), so people can shout at me - what the hell?
soon enough.

> I really wonder what it is that you are getting contention on - what are
> you doing, besides the actual renames?  And that needs serialization anyway
> (on inode lock of workdir, if nothing else), so any contention would not
> disappear from dropped ->s_vfs_rename_mutex...

Yeah, I wrote in the cover letter that I did not generate performance
numbers yet,
which is a must for this sort of work, and that I am hoping to get some feedback
from testers.
But the serialization I am trying to avoid is between copy-ups and whiteouts of
different overlay mounts, all on the same fs, which is the case with
docker/rocket
containers.

Not ruling out that I am barking up the wrong tree. The burden of
proof is on me.

Thanks,
Amir.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 02:11:56AM +0200, Amir Goldstein wrote:
> Yeah, I wrote in the cover letter that I did not generate performance
> numbers yet,
> which is a must for this sort of work, and that I am hoping to get some feedback
> from testers.
> But the serialization I am trying to avoid is between copy-ups and whiteouts of
> different overlay mounts, all on the same fs, which is the case with
> docker/rocket
> containers.
> 
> Not ruling out that I am barking up the wrong tree. The burden of
> proof is on me.

Surely, the copying of data itself is outside of that lock, isn't it?
And renames proper, especially if there is any kind of contention going on,
will be on the metadata hot in cache, so I would really like to see the
actual evidence of contention-related performance issues...

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 2:27 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 02:11:56AM +0200, Amir Goldstein wrote:
>> Yeah, I wrote in the cover letter that I did not generate performance
>> numbers yet,
>> which is a must for this sort of work, and that I am hoping to get some feedback
>> from testers.
>> But the serialization I am trying to avoid is between copy-ups and whiteouts of
>> different overlay mounts, all on the same fs, which is the case with
>> docker/rocket
>> containers.
>>
>> Not ruling out that I am barking up the wrong tree. The burden of
>> proof is on me.
>
> Surely, the copying of data itself is outside of that lock, isn't it?

Mmmmmm, no it isn't, but I am going to make it right.

> And renames proper, especially if there is any kind of contention going on,
> will be on the metadata hot in cache, so I would really like to see the
> actual evidence of contention-related performance issues...

I am afraid I won't find these evidence.
I did a small benchmark of 2 parallel rm -rf processes on 2 different
overlay mount
over the same fs.
In that test, every process spends 20% of the time on vfs_whiteout and 10% on
vfs_rename (of those whiteouts).  mutex_lock/unlock of s_vfs_rename_mutex
takes 4% of the time, but the lock covers both the whiteout and rename.

I estimate that after we fix the coarse grained rename_lock, it will be quite
hard to demonstrate contention-related performance issues due to
s_vfs_rename_mutex.

Thanks for taking the time to set me straight.
Amir.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 04:43:57PM +0200, Amir Goldstein wrote:

> > Surely, the copying of data itself is outside of that lock, isn't it?
> 
> Mmmmmm, no it isn't, but I am going to make it right.

Well, if you really have the copyup itself done under lock_rename(), you
are looking at potentially copying gigabytes of data from r/o layer to
workdir under fs-wide lock.  _That_ is really asking for contention from
hell, and for no good reason.  Renames/whiteout creations/removals are
trivial noise compared to that.  I'm somewhat surprised, TBH - I thought
the copyup itself is done outside of that thing.  Are you sure it really
isn't?

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 5:40 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 04:43:57PM +0200, Amir Goldstein wrote:
>
>> > Surely, the copying of data itself is outside of that lock, isn't it?
>>
>> Mmmmmm, no it isn't, but I am going to make it right.
>
> Well, if you really have the copyup itself done under lock_rename(), you
> are looking at potentially copying gigabytes of data from r/o layer to
> workdir under fs-wide lock.  _That_ is really asking for contention from
> hell, and for no good reason.  Renames/whiteout creations/removals are
> trivial noise compared to that.  I'm somewhat surprised, TBH - I thought
> the copyup itself is done outside of that thing.  Are you sure it really
> isn't?

I'm afraid so.
It seems to me that most of the time, lock_rename() is being used in
overlayfs for no better alternative to lock 2 directories (work+upper).

My suggestion is a small modification to the overlayfs locking scheme.
---Instead of this:
assert(lock_rename(workdir, upperdir) != NULL));
   copy_up(src, tmp);
   vfs_rename(tmp, dst);
unlock_rename(workdir, upperdir);

+++Use this:
assert(lock_rename(workdir, upperdir) != NULL));
mutex_unlock(s_vfs_rename_mutex);
   copy_up(src, tmp);
inode_unlock(upperdir);
inode_unlock(workdir);
assert(lock_rename(workdir, upperdir) != NULL));
  vfs_rename(tmp, dst);
unlock_rename(workdir, upperdir);

Miklos,

Do you see any problem with the proposed scheme?
Anything that can go wrong while releasing the workdir lock before vfs_rename()?

Do you think we should do the same for the remove/create functions
or leave the lock_rename() over whiteout+rename+cleanup as it is?

Amir.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Al Viro]

On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote:

> I'm afraid so.
> It seems to me that most of the time, lock_rename() is being used in
> overlayfs for no better alternative to lock 2 directories (work+upper).
> 
> My suggestion is a small modification to the overlayfs locking scheme.
> ---Instead of this:
> assert(lock_rename(workdir, upperdir) != NULL));
>    copy_up(src, tmp);
>    vfs_rename(tmp, dst);
> unlock_rename(workdir, upperdir);
> 
> +++Use this:
> assert(lock_rename(workdir, upperdir) != NULL));
> mutex_unlock(s_vfs_rename_mutex);
>    copy_up(src, tmp);
> inode_unlock(upperdir);
> inode_unlock(workdir);
> assert(lock_rename(workdir, upperdir) != NULL));
>   vfs_rename(tmp, dst);
> unlock_rename(workdir, upperdir);
> 
> Miklos,
> 
> Do you see any problem with the proposed scheme?
> Anything that can go wrong while releasing the workdir lock before vfs_rename()?

Huh???

->rename() definitely counts upon parents being locked; please, read the
damn Documentation/filesystems/locking, it's there for a reason.

The real question is why the fsck do you need to lock the workdir for the
duration of copying at all.  O_TMPFILE open + writes there doesn't need
lock_rename() *or* parents being locked.  You need the parent locked when
you link the sucker in place, but that's it.  IDGI...

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Miklos Szeredi]

On Fri, Nov 11, 2016 at 6:27 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote:
>
>> I'm afraid so.
>> It seems to me that most of the time, lock_rename() is being used in
>> overlayfs for no better alternative to lock 2 directories (work+upper).
>>
>> My suggestion is a small modification to the overlayfs locking scheme.
>> ---Instead of this:
>> assert(lock_rename(workdir, upperdir) != NULL));
>>    copy_up(src, tmp);
>>    vfs_rename(tmp, dst);
>> unlock_rename(workdir, upperdir);
>>
>> +++Use this:
>> assert(lock_rename(workdir, upperdir) != NULL));
>> mutex_unlock(s_vfs_rename_mutex);
>>    copy_up(src, tmp);
>> inode_unlock(upperdir);
>> inode_unlock(workdir);
>> assert(lock_rename(workdir, upperdir) != NULL));
>>   vfs_rename(tmp, dst);
>> unlock_rename(workdir, upperdir);
>>
>> Miklos,
>>
>> Do you see any problem with the proposed scheme?
>> Anything that can go wrong while releasing the workdir lock before vfs_rename()?
>
> Huh???
>
> ->rename() definitely counts upon parents being locked; please, read the
> damn Documentation/filesystems/locking, it's there for a reason.
>
> The real question is why the fsck do you need to lock the workdir for the
> duration of copying at all.  O_TMPFILE open + writes there doesn't need
> lock_rename() *or* parents being locked.  You need the parent locked when
> you link the sucker in place, but that's it.  IDGI...

There's that.  The other thing that lock_rename does is prevent
multiple copy ups on the same file.   Arguably it's an overkill, but a
replacement needs to be added.

Fact is, nobody ever reported this blatant performance bottleneck.
Probably because copying up gigabyte files is not the usual use case
for union filesystems...

Thanks,
Miklos

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 7:44 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Nov 11, 2016 at 6:27 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote:
>>
>>> I'm afraid so.
>>> It seems to me that most of the time, lock_rename() is being used in
>>> overlayfs for no better alternative to lock 2 directories (work+upper).
>>>
>>> My suggestion is a small modification to the overlayfs locking scheme.
>>> ---Instead of this:
>>> assert(lock_rename(workdir, upperdir) != NULL));
>>>    copy_up(src, tmp);
>>>    vfs_rename(tmp, dst);
>>> unlock_rename(workdir, upperdir);
>>>
>>> +++Use this:
>>> assert(lock_rename(workdir, upperdir) != NULL));
>>> mutex_unlock(s_vfs_rename_mutex);
>>>    copy_up(src, tmp);
>>> inode_unlock(upperdir);
>>> inode_unlock(workdir);
>>> assert(lock_rename(workdir, upperdir) != NULL));
>>>   vfs_rename(tmp, dst);
>>> unlock_rename(workdir, upperdir);
>>>
>>> Miklos,
>>>
>>> Do you see any problem with the proposed scheme?
>>> Anything that can go wrong while releasing the workdir lock before vfs_rename()?
>>
>> Huh???
>>
>> ->rename() definitely counts upon parents being locked; please, read the
>> damn Documentation/filesystems/locking, it's there for a reason.
>>
>> The real question is why the fsck do you need to lock the workdir for the
>> duration of copying at all.  O_TMPFILE open + writes there doesn't need
>> lock_rename() *or* parents being locked.  You need the parent locked when
>> you link the sucker in place, but that's it.  IDGI...
>
> There's that.  The other thing that lock_rename does is prevent
> multiple copy ups on the same file.   Arguably it's an overkill, but a
> replacement needs to be added.
>

Miklos,

FYI, I started to work on copy up using O_TMPFILE with i_mutex
held only for upper dir.

> There's that.  The other thing that lock_rename does is prevent
> multiple copy ups on the same file.   Arguably it's an overkill, but a
> replacement needs to be added.
>

I think you suggested here to use a waitqueue for pending copyups?
Did you mean a waitqueue per overlay parent directory inode or
did you mean something else?

> Fact is, nobody ever reported this blatant performance bottleneck.
> Probably because copying up gigabyte files is not the usual use case
> for union filesystems...

Still, it's a loophole for some serious DoS. An unprivileged user inside
container can touch a file and block all copy up on all other containers
on that host and all renames on that host fs as well!

Amir.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Miklos Szeredi]

On Thu, Jan 12, 2017 at 07:42:22AM +0200, Amir Goldstein wrote:
> I think you suggested here to use a waitqueue for pending copyups?
> Did you mean a waitqueue per overlay parent directory inode or
> did you mean something else?

Something like this:

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index d14bca1850d9..a46e7e53cd7a 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;
+	wait_queue_head_t copyup_wq;
 };
 
 /* private information held for every overlayfs dentry */
@@ -38,6 +39,7 @@ struct ovl_entry {
 			u64 version;
 			const char *redirect;
 			bool opaque;
+			bool copying;
 		};
 		struct rcu_head rcu;
 	};
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 952286f4826c..4e2b2c485165 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -263,3 +263,32 @@ 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)
+{
+	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;
+	spin_unlock(&ofs->copyup_wq.lock);
+}

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Thu, Jan 12, 2017 at 12:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 12, 2017 at 07:42:22AM +0200, Amir Goldstein wrote:
>> I think you suggested here to use a waitqueue for pending copyups?
>> Did you mean a waitqueue per overlay parent directory inode or
>> did you mean something else?
>
> Something like this:
>

That's a useful answer :)
I'll give it a try.
Thanks!

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 7:27 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote:
>
>> I'm afraid so.
>> It seems to me that most of the time, lock_rename() is being used in
>> overlayfs for no better alternative to lock 2 directories (work+upper).
>>
>> My suggestion is a small modification to the overlayfs locking scheme.
>> ---Instead of this:
>> assert(lock_rename(workdir, upperdir) != NULL));
>>    copy_up(src, tmp);
>>    vfs_rename(tmp, dst);
>> unlock_rename(workdir, upperdir);
>>
>> +++Use this:
>> assert(lock_rename(workdir, upperdir) != NULL));
>> mutex_unlock(s_vfs_rename_mutex);
>>    copy_up(src, tmp);
>> inode_unlock(upperdir);
>> inode_unlock(workdir);
>> assert(lock_rename(workdir, upperdir) != NULL));
>>   vfs_rename(tmp, dst);
>> unlock_rename(workdir, upperdir);
>>
>> Miklos,
>>
>> Do you see any problem with the proposed scheme?
>> Anything that can go wrong while releasing the workdir lock before vfs_rename()?
>
> Huh???
>
> ->rename() definitely counts upon parents being locked; please, read the
> damn Documentation/filesystems/locking, it's there for a reason.
>

I know it does. lock_rename() before vfs_rename() re-aquires the parent locks.
The reason I am unlocking them is to be able to re-take s_vfs_rename_mutex
and 2 parent dirs in correct order.

> The real question is why the fsck do you need to lock the workdir for the
> duration of copying at all.  O_TMPFILE open + writes there doesn't need
> lock_rename() *or* parents being locked.  You need the parent locked when
> you link the sucker in place, but that's it.  IDGI...

I am starting to understand the reasons of how the current locking scheme for
copy up became to be what it is.

It stems from the fact the the parent upper dir is used as the synchronization
object to avoid double copy up (and my suggested scheme does not preserve
that). Then workdir needs to be locked before we link the temp file.
And then we cannot lock upperdir->workdir in that order without holding
s_vfs_rename_mutex while we do that, so the whole locking became
lock_rename() over the whole thing.

I can see the following solution:

For regular file copy up, don't use workdir at all, use O_TMPFILE instead
then only need to take upperdir lock and it can be the synchronization
object for the entire copy-up, which also allows for concurrent copy up
to different directories.

For non-regular file copy up, use workdir as it is used now and take
lock_rename() for the entire copy up as it is now.

If upper fs does not support O_TMPFILE, we can resort to the
contending copy-up.

I am working on a RFC patch. Will send it out over weekend.
Bare with me guys, I'll get it right eventually...

Amir.

Re: [RFC][PATH 4/4] ovl: relax lock_rename when moving files between work and upper dir

[Amir Goldstein]

On Fri, Nov 11, 2016 at 8:07 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Nov 11, 2016 at 7:27 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Fri, Nov 11, 2016 at 06:17:07PM +0200, Amir Goldstein wrote:
>>
>>> I'm afraid so.
>>> It seems to me that most of the time, lock_rename() is being used in
>>> overlayfs for no better alternative to lock 2 directories (work+upper).
>>>
>> Huh???
>>
>> ->rename() definitely counts upon parents being locked; please, read the
>> damn Documentation/filesystems/locking, it's there for a reason.
>>
>
> I know it does. lock_rename() before vfs_rename() re-aquires the parent locks.
> The reason I am unlocking them is to be able to re-take s_vfs_rename_mutex
> and 2 parent dirs in correct order.
>
>> The real question is why the fsck do you need to lock the workdir for the
>> duration of copying at all.  O_TMPFILE open + writes there doesn't need
>> lock_rename() *or* parents being locked.  You need the parent locked when
>> you link the sucker in place, but that's it.  IDGI...
>
> I am starting to understand the reasons of how the current locking scheme for
> copy up became to be what it is.
>
> It stems from the fact the the parent upper dir is used as the synchronization
> object to avoid double copy up (and my suggested scheme does not preserve
> that). Then workdir needs to be locked before we link the temp file.
> And then we cannot lock upperdir->workdir in that order without holding
> s_vfs_rename_mutex while we do that, so the whole locking became
> lock_rename() over the whole thing.
>
> I can see the following solution:
>
> For regular file copy up, don't use workdir at all, use O_TMPFILE instead
> then only need to take upperdir lock and it can be the synchronization
> object for the entire copy-up, which also allows for concurrent copy up
> to different directories.
>
> For non-regular file copy up, use workdir as it is used now and take
> lock_rename() for the entire copy up as it is now.
>
> If upper fs does not support O_TMPFILE, we can resort to the
> contending copy-up.
>
> I am working on a RFC patch. Will send it out over weekend.
> Bare with me guys, I'll get it right eventually...
>

I posted the new patches. They are much simpler than my previous attempt.
I did not go for the O_TMPFILE solution because I found a solution
that IMO is simple and clean without changing too much code.

In a nut shell, the overlay inode is used to synchronize copy up
regular files (outside the existing locks), so lock_rename can be dropped
for the period of copying data and re-acquired before the final rename.

Amir.