Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380451 > unrolled thread

[PATCHSET][RFC][CFT] parallel lookups

Back to article view | Back to linux.kernel

Contents

  [PATCHSET][RFC][CFT] parallel lookups Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 03:00 +0200
    [PATCH 01/15] security_d_instantiate(): move to the point prior to attaching dentry to inode Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 03:00 +0200
    [PATCH 09/15] lookup_slow(): bugger off on IS_DEADDIR() from the very beginning Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 03:00 +0200
    [PATCH 14/15] parallel lookups machinery, part 4 (and last) Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 03:00 +0200
    [PATCH 15/15] parallel lookups: actual switch to rwsem Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 03:00 +0200
      Re: [PATCH 15/15] parallel lookups: actual switch to rwsem Andreas Dilger <adilger@dilger.ca> - 2016-04-16 05:10 +0200
        Re: [PATCH 15/15] parallel lookups: actual switch to rwsem Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 05:40 +0200
    Re: [PATCHSET][RFC][CFT] parallel lookups Andreas Dilger <adilger@dilger.ca> - 2016-04-16 05:10 +0200
      Re: [PATCHSET][RFC][CFT] parallel lookups Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 05:30 +0200

#1380451 — [PATCHSET][RFC][CFT] parallel lookups

	The thing appears to be working.  It's in vfs.git#work.lookups; the
last 5 commits are the infrastructure (fs/namei.c and fs/dcache.c; no changes
in fs/*/*) + actual switch to rwsem.

	The missing bits: down_write_killable() (there had been a series
posted introducing just that; for now I've replaced mutex_lock_killable()
calls with plain inode_lock() - they are not critical for any testing and
as soon as down_write_killable() gets there I'll replace those), lockdep
bits might need corrections and right now it's only for lookups.

	I'm going to add readdir to the mix; the primitive added in this
series (d_alloc_parallel()) will need to be used in dcache pre-seeding
paths, ncpfs use of dentry_update_name_case() will need to be changed to
something less hacky and syscalls calling iterate_dir() will need to
switch to fdget_pos() (with FMODE_ATOMIC_POS set for directories as well
as regulars).  The last bit is needed for exclusion on struct file
level - there's a bunch of cases where we maintain data structures
hanging off file->private and those really need to be serialized.  Besides,
serializing ->f_pos updates is needed for sane semantics; right now we
tend to use ->i_mutex for that, but it would be easier to go for the same
mechanism as for regular files.  With any luck we'll have working parallel
readdir in addition to parallel lookups in this cycle as well.

	The patchset is on top of switching getxattr to passing dentry and
inode separately; that part will get changes (in particular, the stuff
agruen has posted lately), but the lookups queue proper cares only about
being able to move security_d_instantiate() to the point before dentry
is attached to inode.

1/15: security_d_instantiate(): move to the point prior to attaching dentry
to inode.  Depends on getxattr changes, allows to do the "attach to inode"
and "add to dentry hash" parts without dropping ->d_lock in between.

2/15 -- 8/15: preparations - stuff similar to what went in during the last
cycle; several places switched to lookup_one_len_unlocked(), a bunch of
direct manipulations of ->i_mutex replaced with inode_lock, etc. helpers.

kernfs: use lookup_one_len_unlocked().
configfs_detach_prep(): make sure that wait_mutex won't go away
ocfs2: don't open-code inode_lock/inode_unlock
orangefs: don't open-code inode_lock/inode_unlock
reiserfs: open-code reiserfs_mutex_lock_safe() in reiserfs_unpack()
reconnect_one(): use lookup_one_len_unlocked()
ovl_lookup_real(): use lookup_one_len_unlocked()

9/15: lookup_slow(): bugger off on IS_DEADDIR() from the very beginning
open-code real_lookup() call in lookup_slow(), move IS_DEADDIR check upwards.

10/15: __d_add(): don't drop/regain ->d_lock
that's what 1/15 had been for; might make sense to reorder closer to it.

11/15 -- 14/15: actual machinery for parallel lookups.  This stuff could've
been a single commit, along with the actual switch to rwsem and shared lock
in lookup_slow(), but it's easier to review if carved up like that.  From the
testing POV it's one chunk - it is bisect-safe, but the added code really
comes into play only after we go for shared lock, which happens in 15/15.
That's the core of the series.

beginning of transition to parallel lookups - marking in-lookup dentries
parallel lookups machinery, part 2
parallel lookups machinery, part 3
parallel lookups machinery, part 4 (and last)

15/15: parallel lookups: actual switch to rwsem

Note that filesystems would be free to switch some of their own uses of
inode_lock() to grabbing it shared - it's really up to them.  This series
works only with directories locking, but this field has become an rwsem
for all inodes.  XFS folks in particular might be interested in using it...

I'll post the individual patches in followups.  Again, this is also available
in vfs.git #work.lookups (head at e2d622a right now).  The thing survives
LTP and xfstests without regressions, but more testing would certainly be
appreciated.  So would review, of course.

[toc] | [next] | [standalone]

#1380452 — [PATCH 01/15] security_d_instantiate(): move to the point prior to attaching dentry to inode

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/dcache.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 32ceae3..e9de4d9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1772,11 +1772,11 @@ void d_instantiate(struct dentry *entry, struct inode * inode)
 {
 	BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
 	if (inode) {
+		security_d_instantiate(entry, inode);
 		spin_lock(&inode->i_lock);
 		__d_instantiate(entry, inode);
 		spin_unlock(&inode->i_lock);
 	}
-	security_d_instantiate(entry, inode);
 }
 EXPORT_SYMBOL(d_instantiate);
 
@@ -1793,6 +1793,7 @@ int d_instantiate_no_diralias(struct dentry *entry, struct inode *inode)
 {
 	BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
 
+	security_d_instantiate(entry, inode);
 	spin_lock(&inode->i_lock);
 	if (S_ISDIR(inode->i_mode) && !hlist_empty(&inode->i_dentry)) {
 		spin_unlock(&inode->i_lock);
@@ -1801,7 +1802,6 @@ int d_instantiate_no_diralias(struct dentry *entry, struct inode *inode)
 	}
 	__d_instantiate(entry, inode);
 	spin_unlock(&inode->i_lock);
-	security_d_instantiate(entry, inode);
 
 	return 0;
 }
@@ -1875,6 +1875,7 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 		goto out_iput;
 	}
 
+	security_d_instantiate(tmp, inode);
 	spin_lock(&inode->i_lock);
 	res = __d_find_any_alias(inode);
 	if (res) {
@@ -1897,13 +1898,10 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 	hlist_bl_unlock(&tmp->d_sb->s_anon);
 	spin_unlock(&tmp->d_lock);
 	spin_unlock(&inode->i_lock);
-	security_d_instantiate(tmp, inode);
 
 	return tmp;
 
  out_iput:
-	if (res && !IS_ERR(res))
-		security_d_instantiate(res, inode);
 	iput(inode);
 	return res;
 }
@@ -2369,7 +2367,6 @@ static inline void __d_add(struct dentry *dentry, struct inode *inode)
 		__d_instantiate(dentry, inode);
 		spin_unlock(&inode->i_lock);
 	}
-	security_d_instantiate(dentry, inode);
 	d_rehash(dentry);
 }
 
@@ -2384,8 +2381,10 @@ static inline void __d_add(struct dentry *dentry, struct inode *inode)
 
 void d_add(struct dentry *entry, struct inode *inode)
 {
-	if (inode)
+	if (inode) {
+		security_d_instantiate(entry, inode);
 		spin_lock(&inode->i_lock);
+	}
 	__d_add(entry, inode);
 }
 EXPORT_SYMBOL(d_add);
@@ -2779,6 +2778,7 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 	if (!inode)
 		goto out;
 
+	security_d_instantiate(dentry, inode);
 	spin_lock(&inode->i_lock);
 	if (S_ISDIR(inode->i_mode)) {
 		struct dentry *new = __d_find_any_alias(inode);
@@ -2806,7 +2806,6 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 			} else {
 				__d_move(new, dentry, false);
 				write_sequnlock(&rename_lock);
-				security_d_instantiate(new, inode);
 			}
 			iput(inode);
 			return new;
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1380453 — [PATCH 09/15] lookup_slow(): bugger off on IS_DEADDIR() from the very beginning

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namei.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c0d551f..6fb33a7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1603,8 +1603,15 @@ static struct dentry *lookup_slow(const struct qstr *name,
 				  struct dentry *dir,
 				  unsigned int flags)
 {
-	struct dentry *dentry;
-	inode_lock(dir->d_inode);
+	struct dentry *dentry, *old;
+	struct inode *inode = dir->d_inode;
+
+	inode_lock(inode);
+	/* Don't go there if it's already dead */
+	if (unlikely(IS_DEADDIR(inode))) {
+		inode_unlock(inode);
+		return ERR_PTR(-ENOENT);
+	}
 	dentry = d_lookup(dir, name);
 	if (unlikely(dentry)) {
 		if ((dentry->d_flags & DCACHE_OP_REVALIDATE) &&
@@ -1618,17 +1625,21 @@ static struct dentry *lookup_slow(const struct qstr *name,
 			}
 		}
 		if (dentry) {
-			inode_unlock(dir->d_inode);
+			inode_unlock(inode);
 			return dentry;
 		}
 	}
 	dentry = d_alloc(dir, name);
 	if (unlikely(!dentry)) {
-		inode_unlock(dir->d_inode);
+		inode_unlock(inode);
 		return ERR_PTR(-ENOMEM);
 	}
-	dentry = lookup_real(dir->d_inode, dentry, flags);
-	inode_unlock(dir->d_inode);
+	old = inode->i_op->lookup(inode, dentry, flags);
+	if (unlikely(old)) {
+		dput(dentry);
+		dentry = old;
+	}
+	inode_unlock(inode);
 	return dentry;
 }
 
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1380454 — [PATCH 14/15] parallel lookups machinery, part 4 (and last)

From: Al Viro <viro@zeniv.linux.org.uk>

If we *do* run into an in-lookup match, we need to wait for it to
cease being in-lookup.  Fortunately, we do have unused space in
in-lookup dentries - d_lru is never looked at until it stops being
in-lookup.

So we can stash a pointer to wait_queue_head from stack frame of
the caller of ->lookup().  Some precautions are needed while
waiting, but it's not that hard - we do hold a reference to dentry
we are waiting for, so it can't go away.  If it's found to be
in-lookup the wait_queue_head is still alive and will remain so
at least while ->d_lock is held.  Moreover, the condition we
are waiting for becomes true at the same point where everything
on that wq gets woken up, so we can just add ourselves to the
queue once.

d_alloc_parallel() gets a pointer to wait_queue_head_t from its
caller; lookup_slow() adjusted, d_add_ci() taught to use
d_alloc_parallel() if the dentry passed to it happens to be
in-lookup one (i.e. if it's been called from the parallel lookup).

That's pretty much it - all that remains is to switch ->i_mutex
to rwsem and have lookup_slow() take it shared.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/dcache.c            | 94 +++++++++++++++++++++++++++++++++++++++-----------
 fs/namei.c             |  3 +-
 include/linux/dcache.h |  8 +++--
 3 files changed, 82 insertions(+), 23 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 0552002..5965588 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1984,28 +1984,36 @@ EXPORT_SYMBOL(d_obtain_root);
 struct dentry *d_add_ci(struct dentry *dentry, struct inode *inode,
 			struct qstr *name)
 {
-	struct dentry *found;
-	struct dentry *new;
+	struct dentry *found, *res;
 
 	/*
 	 * First check if a dentry matching the name already exists,
 	 * if not go ahead and create it now.
 	 */
 	found = d_hash_and_lookup(dentry->d_parent, name);
-	if (!found) {
-		new = d_alloc(dentry->d_parent, name);
-		if (!new) {
-			found = ERR_PTR(-ENOMEM);
-		} else {
-			found = d_splice_alias(inode, new);
-			if (found) {
-				dput(new);
-				return found;
-			}
-			return new;
+	if (found) {
+		iput(inode);
+		return found;
+	}
+	if (dentry->d_flags & DCACHE_PAR_LOOKUP) {
+		found = d_alloc_parallel(dentry->d_parent, name,
+					dentry->d_wait);
+		if (IS_ERR(found) || !(found->d_flags & DCACHE_PAR_LOOKUP)) {
+			iput(inode);
+			return found;
 		}
+	} else {
+		found = d_alloc(dentry->d_parent, name);
+		if (!found) {
+			iput(inode);
+			return ERR_PTR(-ENOMEM);
+		} 
+	}
+	res = d_splice_alias(inode, found);
+	if (res) {
+		dput(found);
+		return res;
 	}
-	iput(inode);
 	return found;
 }
 EXPORT_SYMBOL(d_add_ci);
@@ -2388,8 +2396,23 @@ static inline void end_dir_add(struct inode *dir, unsigned n)
 	smp_store_release(&dir->i_dir_seq, n + 2);
 }
 
+static void d_wait_lookup(struct dentry *dentry)
+{
+	if (dentry->d_flags & DCACHE_PAR_LOOKUP) {
+		DECLARE_WAITQUEUE(wait, current);
+		add_wait_queue(dentry->d_wait, &wait);
+		do {
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			spin_unlock(&dentry->d_lock);
+			schedule();
+			spin_lock(&dentry->d_lock);
+		} while (dentry->d_flags & DCACHE_PAR_LOOKUP);
+	}
+}
+
 struct dentry *d_alloc_parallel(struct dentry *parent,
-				const struct qstr *name)
+				const struct qstr *name,
+				wait_queue_head_t *wq)
 {
 	unsigned int len = name->len;
 	unsigned int hash = name->hash;
@@ -2444,18 +2467,47 @@ retry:
 		}
 		dget(dentry);
 		hlist_bl_unlock(b);
-		/* impossible until we actually enable parallel lookups */
-		BUG();
-		/* and this will be "wait for it to stop being in-lookup" */
-		/* this one will be handled in the next commit */
+		/* somebody is doing lookup for it right now; wait for it */
+		spin_lock(&dentry->d_lock);
+		d_wait_lookup(dentry);
+		/*
+		 * it's not in-lookup anymore; in principle we should repeat
+		 * everything from dcache lookup, but it's likely to be what
+		 * d_lookup() would've found anyway.  If it is, just return it;
+		 * otherwise we really have to repeat the whole thing.
+		 */
+		if (unlikely(dentry->d_name.hash != hash))
+			goto mismatch;
+		if (unlikely(dentry->d_parent != parent))
+			goto mismatch;
+		if (unlikely(d_unhashed(dentry)))
+			goto mismatch;
+		if (parent->d_flags & DCACHE_OP_COMPARE) {
+			int tlen = dentry->d_name.len;
+			const char *tname = dentry->d_name.name;
+			if (parent->d_op->d_compare(parent, dentry, tlen, tname, name))
+				goto mismatch;
+		} else {
+			if (unlikely(dentry->d_name.len != len))
+				goto mismatch;
+			if (unlikely(dentry_cmp(dentry, str, len)))
+				goto mismatch;
+		}
+		/* OK, it *is* a hashed match; return it */
+		spin_unlock(&dentry->d_lock);
 		dput(new);
 		return dentry;
 	}
 	/* we can't take ->d_lock here; it's OK, though. */
 	new->d_flags |= DCACHE_PAR_LOOKUP;
+	new->d_wait = wq;
 	hlist_bl_add_head_rcu(&new->d_u.d_in_lookup_hash, b);
 	hlist_bl_unlock(b);
 	return new;
+mismatch:
+	spin_unlock(&dentry->d_lock);
+	dput(dentry);
+	goto retry;
 }
 
 void __d_not_in_lookup(struct dentry *dentry)
@@ -2465,9 +2517,11 @@ void __d_not_in_lookup(struct dentry *dentry)
 	hlist_bl_lock(b);
 	dentry->d_flags &= ~DCACHE_PAR_LOOKUP;
 	__hlist_bl_del(&dentry->d_u.d_in_lookup_hash);
+	wake_up_all(dentry->d_wait);
+	dentry->d_wait = NULL;
 	hlist_bl_unlock(b);
 	INIT_HLIST_NODE(&dentry->d_u.d_alias);
-	/* more stuff will land here */
+	INIT_LIST_HEAD(&dentry->d_lru);
 }
 
 /* inode->i_lock held if inode is non-NULL */
diff --git a/fs/namei.c b/fs/namei.c
index fbce016..eb879d6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1605,13 +1605,14 @@ static struct dentry *lookup_slow(const struct qstr *name,
 {
 	struct dentry *dentry = ERR_PTR(-ENOENT), *old;
 	struct inode *inode = dir->d_inode;
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 
 	inode_lock(inode);
 	/* Don't go there if it's already dead */
 	if (unlikely(IS_DEADDIR(inode)))
 		goto out;
 again:
-	dentry = d_alloc_parallel(dir, name);
+	dentry = d_alloc_parallel(dir, name, &wq);
 	if (IS_ERR(dentry))
 		goto out;
 	if (unlikely(!(dentry->d_flags & DCACHE_PAR_LOOKUP))) {
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 3ab5ce4..bb4b44f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -123,7 +123,10 @@ struct dentry {
 	unsigned long d_time;		/* used by d_revalidate */
 	void *d_fsdata;			/* fs-specific data */
 
-	struct list_head d_lru;		/* LRU list */
+	union {
+		struct list_head d_lru;		/* LRU list */
+		wait_queue_head_t *d_wait;	/* in-lookup ones only */
+	};
 	struct list_head d_child;	/* child of parent list */
 	struct list_head d_subdirs;	/* our children */
 	/*
@@ -249,7 +252,8 @@ extern void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op
 /* allocate/de-allocate */
 extern struct dentry * d_alloc(struct dentry *, const struct qstr *);
 extern struct dentry * d_alloc_pseudo(struct super_block *, const struct qstr *);
-extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *);
+extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *,
+					wait_queue_head_t *);
 extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
 extern struct dentry * d_exact_alias(struct dentry *, struct inode *);
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1380455 — [PATCH 15/15] parallel lookups: actual switch to rwsem

From: Al Viro <viro@zeniv.linux.org.uk>

ta-da!

The main issue is the lack of down_write_killable(), so the places
like readdir.c switched to plain inode_lock(); once killable
variants of rwsem primitives appear, that'll be dealt with.

lockdep side also might need more work

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/btrfs/ioctl.c       | 16 ++++++++++------
 fs/configfs/inode.c    |  2 +-
 fs/dcache.c            |  9 +++++----
 fs/gfs2/ops_fstype.c   |  2 +-
 fs/inode.c             | 12 ++++++------
 fs/namei.c             |  4 ++--
 fs/ocfs2/inode.c       |  2 +-
 fs/overlayfs/readdir.c |  4 +++-
 fs/readdir.c           |  7 ++++---
 include/linux/fs.h     | 12 ++++++------
 10 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 053e677..db1e830 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -837,9 +837,11 @@ static noinline int btrfs_mksubvol(struct path *parent,
 	struct dentry *dentry;
 	int error;
 
-	error = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
-	if (error == -EINTR)
-		return error;
+	inode_lock_nested(dir, I_MUTEX_PARENT);
+	// XXX: should've been
+	// mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
+	// if (error == -EINTR)
+	//	return error;
 
 	dentry = lookup_one_len(name, parent->dentry, namelen);
 	error = PTR_ERR(dentry);
@@ -2366,9 +2368,11 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		goto out;
 
 
-	err = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
-	if (err == -EINTR)
-		goto out_drop_write;
+	inode_lock_nested(dir, I_MUTEX_PARENT);
+	// XXX: should've been
+	// err = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
+	// if (err == -EINTR)
+	//	goto out_drop_write;
 	dentry = lookup_one_len(vol_args->name, parent, namelen);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 03d124a..0387968 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -156,7 +156,7 @@ static void configfs_set_inode_lock_class(struct configfs_dirent *sd,
 
 	if (depth > 0) {
 		if (depth <= ARRAY_SIZE(default_group_class)) {
-			lockdep_set_class(&inode->i_mutex,
+			lockdep_set_class(&inode->i_rwsem,
 					  &default_group_class[depth - 1]);
 		} else {
 			/*
diff --git a/fs/dcache.c b/fs/dcache.c
index 5965588..d110040 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2911,7 +2911,8 @@ struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
 static int __d_unalias(struct inode *inode,
 		struct dentry *dentry, struct dentry *alias)
 {
-	struct mutex *m1 = NULL, *m2 = NULL;
+	struct mutex *m1 = NULL;
+	struct rw_semaphore *m2 = NULL;
 	int ret = -ESTALE;
 
 	/* If alias and dentry share a parent, then no extra locks required */
@@ -2922,15 +2923,15 @@ static int __d_unalias(struct inode *inode,
 	if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex))
 		goto out_err;
 	m1 = &dentry->d_sb->s_vfs_rename_mutex;
-	if (!inode_trylock(alias->d_parent->d_inode))
+	if (!down_read_trylock(&alias->d_parent->d_inode->i_rwsem))
 		goto out_err;
-	m2 = &alias->d_parent->d_inode->i_mutex;
+	m2 = &alias->d_parent->d_inode->i_rwsem;
 out_unalias:
 	__d_move(alias, dentry, false);
 	ret = 0;
 out_err:
 	if (m2)
-		mutex_unlock(m2);
+		up_read(m2);
 	if (m1)
 		mutex_unlock(m1);
 	return ret;
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index c09c63d..4546360 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -824,7 +824,7 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
 	 * i_mutex on quota files is special. Since this inode is hidden system
 	 * file, we are safe to define locking ourselves.
 	 */
-	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
+	lockdep_set_class(&sdp->sd_quota_inode->i_rwsem,
 			  &gfs2_quota_imutex_key);
 
 	error = gfs2_rindex_update(sdp);
diff --git a/fs/inode.c b/fs/inode.c
index 4b884f7..4ccbc21 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -166,8 +166,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	spin_lock_init(&inode->i_lock);
 	lockdep_set_class(&inode->i_lock, &sb->s_type->i_lock_key);
 
-	mutex_init(&inode->i_mutex);
-	lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);
+	init_rwsem(&inode->i_rwsem);
+	lockdep_set_class(&inode->i_rwsem, &sb->s_type->i_mutex_key);
 
 	atomic_set(&inode->i_dio_count, 0);
 
@@ -925,13 +925,13 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
 		struct file_system_type *type = inode->i_sb->s_type;
 
 		/* Set new key only if filesystem hasn't already changed it */
-		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
+		if (lockdep_match_class(&inode->i_rwsem, &type->i_mutex_key)) {
 			/*
 			 * ensure nobody is actually holding i_mutex
 			 */
-			mutex_destroy(&inode->i_mutex);
-			mutex_init(&inode->i_mutex);
-			lockdep_set_class(&inode->i_mutex,
+			// mutex_destroy(&inode->i_mutex);
+			init_rwsem(&inode->i_rwsem);
+			lockdep_set_class(&inode->i_rwsem,
 					  &type->i_mutex_dir_key);
 		}
 	}
diff --git a/fs/namei.c b/fs/namei.c
index eb879d6..877e9ef 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1607,7 +1607,7 @@ static struct dentry *lookup_slow(const struct qstr *name,
 	struct inode *inode = dir->d_inode;
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 
-	inode_lock(inode);
+	down_read(&inode->i_rwsem);
 	/* Don't go there if it's already dead */
 	if (unlikely(IS_DEADDIR(inode)))
 		goto out;
@@ -1638,7 +1638,7 @@ again:
 		}
 	}
 out:
-	inode_unlock(inode);
+	up_read(&inode->i_rwsem);
 	return dentry;
 }
 
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 12f4a9e..0748777 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -262,7 +262,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
 	inode->i_ino = args->fi_ino;
 	OCFS2_I(inode)->ip_blkno = args->fi_blkno;
 	if (args->fi_sysfile_type != 0)
-		lockdep_set_class(&inode->i_mutex,
+		lockdep_set_class(&inode->i_rwsem,
 			&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
 	if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
 	    args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 6ec1e43..da186ee 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -218,7 +218,9 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
 	old_cred = override_creds(override_cred);
 
-	err = mutex_lock_killable(&dir->d_inode->i_mutex);
+	inode_lock(dir->d_inode);
+	err = 0;
+	// XXX: err = mutex_lock_killable(&dir->d_inode->i_mutex);
 	if (!err) {
 		while (rdd->first_maybe_whiteout) {
 			p = rdd->first_maybe_whiteout;
diff --git a/fs/readdir.c b/fs/readdir.c
index e69ef3b..bf583e8 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -32,9 +32,10 @@ int iterate_dir(struct file *file, struct dir_context *ctx)
 	if (res)
 		goto out;
 
-	res = mutex_lock_killable(&inode->i_mutex);
-	if (res)
-		goto out;
+	inode_lock(inode);
+	// res = mutex_lock_killable(&inode->i_mutex);
+	// if (res)
+	//	goto out;
 
 	res = -ENOENT;
 	if (!IS_DEADDIR(inode)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0a32045..313ad28 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -647,7 +647,7 @@ struct inode {
 
 	/* Misc */
 	unsigned long		i_state;
-	struct mutex		i_mutex;
+	struct rw_semaphore	i_rwsem;
 
 	unsigned long		dirtied_when;	/* jiffies of first dirtying */
 	unsigned long		dirtied_time_when;
@@ -734,27 +734,27 @@ enum inode_i_mutex_lock_class
 
 static inline void inode_lock(struct inode *inode)
 {
-	mutex_lock(&inode->i_mutex);
+	down_write(&inode->i_rwsem);
 }
 
 static inline void inode_unlock(struct inode *inode)
 {
-	mutex_unlock(&inode->i_mutex);
+	up_write(&inode->i_rwsem);
 }
 
 static inline int inode_trylock(struct inode *inode)
 {
-	return mutex_trylock(&inode->i_mutex);
+	return down_write_trylock(&inode->i_rwsem);
 }
 
 static inline int inode_is_locked(struct inode *inode)
 {
-	return mutex_is_locked(&inode->i_mutex);
+	return rwsem_is_locked(&inode->i_rwsem);
 }
 
 static inline void inode_lock_nested(struct inode *inode, unsigned subclass)
 {
-	mutex_lock_nested(&inode->i_mutex, subclass);
+	down_write_nested(&inode->i_rwsem, subclass);
 }
 
 void lock_two_nondirectories(struct inode *, struct inode*);
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1380472 — Re: [PATCH 15/15] parallel lookups: actual switch to rwsem

[Multipart message — attachments visible in raw view] — view raw

On Apr 15, 2016, at 6:55 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> ta-da!
> 
> The main issue is the lack of down_write_killable(), so the places
> like readdir.c switched to plain inode_lock(); once killable
> variants of rwsem primitives appear, that'll be dealt with.
> 
> lockdep side also might need more work
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> fs/btrfs/ioctl.c       | 16 ++++++++++------
> fs/configfs/inode.c    |  2 +-
> fs/dcache.c            |  9 +++++----
> fs/gfs2/ops_fstype.c   |  2 +-
> fs/inode.c             | 12 ++++++------
> fs/namei.c             |  4 ++--
> fs/ocfs2/inode.c       |  2 +-
> fs/overlayfs/readdir.c |  4 +++-
> fs/readdir.c           |  7 ++++---
> include/linux/fs.h     | 12 ++++++------
> 10 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 053e677..db1e830 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -837,9 +837,11 @@ static noinline int btrfs_mksubvol(struct path *parent,
> 	struct dentry *dentry;
> 	int error;
> 
> -	error = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
> -	if (error == -EINTR)
> -		return error;
> +	inode_lock_nested(dir, I_MUTEX_PARENT);
> +	// XXX: should've been
> +	// mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
> +	// if (error == -EINTR)
> +	//	return error;
> 
> 	dentry = lookup_one_len(name, parent->dentry, namelen);
> 	error = PTR_ERR(dentry);
> @@ -2366,9 +2368,11 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> 		goto out;
> 
> 
> -	err = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
> -	if (err == -EINTR)
> -		goto out_drop_write;
> +	inode_lock_nested(dir, I_MUTEX_PARENT);
> +	// XXX: should've been
> +	// err = mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT);
> +	// if (err == -EINTR)
> +	//	goto out_drop_write;
> 	dentry = lookup_one_len(vol_args->name, parent, namelen);
> 	if (IS_ERR(dentry)) {
> 		err = PTR_ERR(dentry);
> diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
> index 03d124a..0387968 100644
> --- a/fs/configfs/inode.c
> +++ b/fs/configfs/inode.c
> @@ -156,7 +156,7 @@ static void configfs_set_inode_lock_class(struct configfs_dirent *sd,
> 
> 	if (depth > 0) {
> 		if (depth <= ARRAY_SIZE(default_group_class)) {
> -			lockdep_set_class(&inode->i_mutex,
> +			lockdep_set_class(&inode->i_rwsem,
> 					  &default_group_class[depth - 1]);
> 		} else {
> 			/*
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 5965588..d110040 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2911,7 +2911,8 @@ struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
> static int __d_unalias(struct inode *inode,
> 		struct dentry *dentry, struct dentry *alias)
> {
> -	struct mutex *m1 = NULL, *m2 = NULL;
> +	struct mutex *m1 = NULL;
> +	struct rw_semaphore *m2 = NULL;
> 	int ret = -ESTALE;
> 
> 	/* If alias and dentry share a parent, then no extra locks required */
> @@ -2922,15 +2923,15 @@ static int __d_unalias(struct inode *inode,
> 	if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex))
> 		goto out_err;
> 	m1 = &dentry->d_sb->s_vfs_rename_mutex;
> -	if (!inode_trylock(alias->d_parent->d_inode))
> +	if (!down_read_trylock(&alias->d_parent->d_inode->i_rwsem))
> 		goto out_err;
> -	m2 = &alias->d_parent->d_inode->i_mutex;
> +	m2 = &alias->d_parent->d_inode->i_rwsem;
> out_unalias:
> 	__d_move(alias, dentry, false);
> 	ret = 0;
> out_err:
> 	if (m2)
> -		mutex_unlock(m2);
> +		up_read(m2);
> 	if (m1)
> 		mutex_unlock(m1);
> 	return ret;
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c09c63d..4546360 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -824,7 +824,7 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> 	 * i_mutex on quota files is special. Since this inode is hidden system
> 	 * file, we are safe to define locking ourselves.
> 	 */
> -	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> +	lockdep_set_class(&sdp->sd_quota_inode->i_rwsem,
> 			  &gfs2_quota_imutex_key);
> 
> 	error = gfs2_rindex_update(sdp);
> diff --git a/fs/inode.c b/fs/inode.c
> index 4b884f7..4ccbc21 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -166,8 +166,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> 	spin_lock_init(&inode->i_lock);
> 	lockdep_set_class(&inode->i_lock, &sb->s_type->i_lock_key);
> 
> -	mutex_init(&inode->i_mutex);
> -	lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);
> +	init_rwsem(&inode->i_rwsem);
> +	lockdep_set_class(&inode->i_rwsem, &sb->s_type->i_mutex_key);
> 
> 	atomic_set(&inode->i_dio_count, 0);
> 
> @@ -925,13 +925,13 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
> 		struct file_system_type *type = inode->i_sb->s_type;
> 
> 		/* Set new key only if filesystem hasn't already changed it */
> -		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
> +		if (lockdep_match_class(&inode->i_rwsem, &type->i_mutex_key)) {
> 			/*
> 			 * ensure nobody is actually holding i_mutex
> 			 */
> -			mutex_destroy(&inode->i_mutex);
> -			mutex_init(&inode->i_mutex);
> -			lockdep_set_class(&inode->i_mutex,
> +			// mutex_destroy(&inode->i_mutex);
> +			init_rwsem(&inode->i_rwsem);
> +			lockdep_set_class(&inode->i_rwsem,
> 					  &type->i_mutex_dir_key);
> 		}
> 	}
> diff --git a/fs/namei.c b/fs/namei.c
> index eb879d6..877e9ef 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1607,7 +1607,7 @@ static struct dentry *lookup_slow(const struct qstr *name,
> 	struct inode *inode = dir->d_inode;
> 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> 
> -	inode_lock(inode);
> +	down_read(&inode->i_rwsem);

Wouldn't it make sense to have helpers like "inode_read_lock(inode)" or similar,
so that it is consistent with other parts of the code and easier to find?
It's a bit strange to have the filesystems use "inode_lock()" and some places
here use "inode_lock_nested()", but other places use up_read() and down_read()
directly on &inode->i_rwsem.  That would also simplify delegating the directory
locking to the filesystems in the future.

Cheers, Andreas

> 	/* Don't go there if it's already dead */
> 	if (unlikely(IS_DEADDIR(inode)))
> 		goto out;
> @@ -1638,7 +1638,7 @@ again:
> 		}
> 	}
> out:
> -	inode_unlock(inode);
> +	up_read(&inode->i_rwsem);
> 	return dentry;
> }
> 
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 12f4a9e..0748777 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -262,7 +262,7 @@ static int ocfs2_init_locked_inode(struct inode *inode, void *opaque)
> 	inode->i_ino = args->fi_ino;
> 	OCFS2_I(inode)->ip_blkno = args->fi_blkno;
> 	if (args->fi_sysfile_type != 0)
> -		lockdep_set_class(&inode->i_mutex,
> +		lockdep_set_class(&inode->i_rwsem,
> 			&ocfs2_sysfile_lock_key[args->fi_sysfile_type]);
> 	if (args->fi_sysfile_type == USER_QUOTA_SYSTEM_INODE ||
> 	    args->fi_sysfile_type == GROUP_QUOTA_SYSTEM_INODE ||
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 6ec1e43..da186ee 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -218,7 +218,9 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
> 	cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
> 	old_cred = override_creds(override_cred);
> 
> -	err = mutex_lock_killable(&dir->d_inode->i_mutex);
> +	inode_lock(dir->d_inode);
> +	err = 0;
> +	// XXX: err = mutex_lock_killable(&dir->d_inode->i_mutex);
> 	if (!err) {
> 		while (rdd->first_maybe_whiteout) {
> 			p = rdd->first_maybe_whiteout;
> diff --git a/fs/readdir.c b/fs/readdir.c
> index e69ef3b..bf583e8 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -32,9 +32,10 @@ int iterate_dir(struct file *file, struct dir_context *ctx)
> 	if (res)
> 		goto out;
> 
> -	res = mutex_lock_killable(&inode->i_mutex);
> -	if (res)
> -		goto out;
> +	inode_lock(inode);
> +	// res = mutex_lock_killable(&inode->i_mutex);
> +	// if (res)
> +	//	goto out;
> 
> 	res = -ENOENT;
> 	if (!IS_DEADDIR(inode)) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 0a32045..313ad28 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -647,7 +647,7 @@ struct inode {
> 
> 	/* Misc */
> 	unsigned long		i_state;
> -	struct mutex		i_mutex;
> +	struct rw_semaphore	i_rwsem;
> 
> 	unsigned long		dirtied_when;	/* jiffies of first dirtying */
> 	unsigned long		dirtied_time_when;
> @@ -734,27 +734,27 @@ enum inode_i_mutex_lock_class
> 
> static inline void inode_lock(struct inode *inode)
> {
> -	mutex_lock(&inode->i_mutex);
> +	down_write(&inode->i_rwsem);
> }
> 
> static inline void inode_unlock(struct inode *inode)
> {
> -	mutex_unlock(&inode->i_mutex);
> +	up_write(&inode->i_rwsem);
> }
> 
> static inline int inode_trylock(struct inode *inode)
> {
> -	return mutex_trylock(&inode->i_mutex);
> +	return down_write_trylock(&inode->i_rwsem);
> }
> 
> static inline int inode_is_locked(struct inode *inode)
> {
> -	return mutex_is_locked(&inode->i_mutex);
> +	return rwsem_is_locked(&inode->i_rwsem);
> }
> 
> static inline void inode_lock_nested(struct inode *inode, unsigned subclass)
> {
> -	mutex_lock_nested(&inode->i_mutex, subclass);
> +	down_write_nested(&inode->i_rwsem, subclass);
> }
> 
> void lock_two_nondirectories(struct inode *, struct inode*);
> --
> 2.8.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1380476 — Re: [PATCH 15/15] parallel lookups: actual switch to rwsem

On Fri, Apr 15, 2016 at 09:02:06PM -0600, Andreas Dilger wrote:

> Wouldn't it make sense to have helpers like "inode_read_lock(inode)" or similar,
> so that it is consistent with other parts of the code and easier to find?
> It's a bit strange to have the filesystems use "inode_lock()" and some places
> here use "inode_lock_nested()", but other places use up_read() and down_read()
> directly on &inode->i_rwsem.  That would also simplify delegating the directory
> locking to the filesystems in the future.

FWIW, my preference would be inode_lock_shared(), but that's bikeshedding;
seeing that we have very few callers at the moment *and* there's the missing
down_write_killable() stuff...  This patch will obviously be reworked and
it's small enough to be understandable, open-coding or not.

[toc] | [prev] | [next] | [standalone]

#1380473

[Multipart message — attachments visible in raw view] — view raw

On Apr 15, 2016, at 6:52 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> 	The thing appears to be working.  It's in vfs.git#work.lookups; the
> last 5 commits are the infrastructure (fs/namei.c and fs/dcache.c; no changes
> in fs/*/*) + actual switch to rwsem.
> 
> 	The missing bits: down_write_killable() (there had been a series
> posted introducing just that; for now I've replaced mutex_lock_killable()
> calls with plain inode_lock() - they are not critical for any testing and
> as soon as down_write_killable() gets there I'll replace those), lockdep
> bits might need corrections and right now it's only for lookups.
> 
> 	I'm going to add readdir to the mix; the primitive added in this
> series (d_alloc_parallel()) will need to be used in dcache pre-seeding
> paths, ncpfs use of dentry_update_name_case() will need to be changed to
> something less hacky and syscalls calling iterate_dir() will need to
> switch to fdget_pos() (with FMODE_ATOMIC_POS set for directories as well
> as regulars).  The last bit is needed for exclusion on struct file
> level - there's a bunch of cases where we maintain data structures
> hanging off file->private and those really need to be serialized.  Besides,
> serializing ->f_pos updates is needed for sane semantics; right now we
> tend to use ->i_mutex for that, but it would be easier to go for the same
> mechanism as for regular files.  With any luck we'll have working parallel
> readdir in addition to parallel lookups in this cycle as well.
> 
> 	The patchset is on top of switching getxattr to passing dentry and
> inode separately; that part will get changes (in particular, the stuff
> agruen has posted lately), but the lookups queue proper cares only about
> being able to move security_d_instantiate() to the point before dentry
> is attached to inode.
> 
> 1/15: security_d_instantiate(): move to the point prior to attaching dentry
> to inode.  Depends on getxattr changes, allows to do the "attach to inode"
> and "add to dentry hash" parts without dropping ->d_lock in between.
> 
> 2/15 -- 8/15: preparations - stuff similar to what went in during the last
> cycle; several places switched to lookup_one_len_unlocked(), a bunch of
> direct manipulations of ->i_mutex replaced with inode_lock, etc. helpers.
> 
> kernfs: use lookup_one_len_unlocked().
> configfs_detach_prep(): make sure that wait_mutex won't go away
> ocfs2: don't open-code inode_lock/inode_unlock
> orangefs: don't open-code inode_lock/inode_unlock
> reiserfs: open-code reiserfs_mutex_lock_safe() in reiserfs_unpack()
> reconnect_one(): use lookup_one_len_unlocked()
> ovl_lookup_real(): use lookup_one_len_unlocked()
> 
> 9/15: lookup_slow(): bugger off on IS_DEADDIR() from the very beginning
> open-code real_lookup() call in lookup_slow(), move IS_DEADDIR check upwards.
> 
> 10/15: __d_add(): don't drop/regain ->d_lock
> that's what 1/15 had been for; might make sense to reorder closer to it.
> 
> 11/15 -- 14/15: actual machinery for parallel lookups.  This stuff could've
> been a single commit, along with the actual switch to rwsem and shared lock
> in lookup_slow(), but it's easier to review if carved up like that.  From the
> testing POV it's one chunk - it is bisect-safe, but the added code really
> comes into play only after we go for shared lock, which happens in 15/15.
> That's the core of the series.
> 
> beginning of transition to parallel lookups - marking in-lookup dentries
> parallel lookups machinery, part 2
> parallel lookups machinery, part 3
> parallel lookups machinery, part 4 (and last)
> 
> 15/15: parallel lookups: actual switch to rwsem
> 
> Note that filesystems would be free to switch some of their own uses of
> inode_lock() to grabbing it shared - it's really up to them.  This series
> works only with directories locking, but this field has become an rwsem
> for all inodes.  XFS folks in particular might be interested in using it...

Looks very interesting, and long awaited.  How do you see the parallel
operations moving forward?  Staying as lookup only, or moving on to parallel
modifications as well?

We've been carrying an out-of-tree patch for ext4 for several years to allow
parallel create/unlink for directory entries*, as I discussed a few times with
you in the past.  It is still a bit heavyweight for doing read-only lookups,
but after this patch series it might finally be interesting to merge into ext4,
with a hope that the VFS might allow parallel directory changes in the future?
We can already do this on a Lustre server, and it would be nice to be able to
so on the client, since the files may even be on different servers (hashed by
name at the client to decide which server to contact) and network latency during
parallel file creates (one thread per CPU core, which is getting into the
low hundreds these days) is a much bigger deal than for local filesystems.

The actual inode_*lock() handling would need to be delegated to the filesystems,
with the VFS just using i_rwsem if the filesystem has no ->inode_lock() method,
but calling the inode_lock() method of the filesystem if available, so that
they can do parallel create/unlink for files and directories (though not rename since that way lies insanity).

We can discuss more at LSF, but now that you posted your patch series, I'm
curious. :-)

Cheers, Andreas



[*] Patch not usable as-is since it has no VFS interface and is only uptodate
for 3.12, just linked here in case of interest.  Essentially, it creates a lock
tree for the ext4 htree directory, and opportunistically only locks the leaf
blocks of the directory on the expectation that there will be ~50-60 entries
added to each leaf before it is split, before it needs to back out to write
lock the parent htree block.  The larger the directory, the more leaf and
parent blocks that can be locked concurrently, and the better scaling gets.

http://git.hpdd.intel.com/fs/lustre-release.git/blob/HEAD:/ldiskfs/kernel_patches/patches/sles12/ext4-pdirop.patch

> I'll post the individual patches in followups.  Again, this is also available
> in vfs.git #work.lookups (head at e2d622a right now).  The thing survives
> LTP and xfstests without regressions, but more testing would certainly be
> appreciated.  So would review, of course.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1380474

On Fri, Apr 15, 2016 at 09:02:02PM -0600, Andreas Dilger wrote:

> Looks very interesting, and long awaited.  How do you see the parallel
> operations moving forward?  Staying as lookup only, or moving on to parallel
> modifications as well?

lookup + readdir.  Not even atomic_open at this point, and that's the
route I'd suggest for modifiers - i.e. a combined lookup + mkdir, etc.
operations.  But we'd really need to sort atomic_open pathway out first...

Let's discuss that at LSFMM, corridor track if needed.  With lookups I'd been
able to keep the surgery site pretty much entirely in VFS proper - fs/dcache.c
and (after earlier massage) a single function in fs/namei.c.  With readdir
it'll be somewhat more invasive - pre-seeding dcache is done in a bunch of
filesystems right now (mostly the network ones, where readdir request is
equivalent to bulk lookup, as well as synthetic-inodes ones a-la procfs)
and it'll need to be regularized; ncpfs is particularly nasty, what with its
case-changing crap), but at least it will be reasonably compact.  For
atomic_open, and worse yet - mkdir/mknod/symlink/link/unlink/rmdir/rename
it will really dip into filesystem code.  A lot.

FWIW, I agree that relying on i_mutex^Wi_rwsem for dcache protection is
something worth getting rid of in the longer term.  But that protection is
there right now, and getting rid of that will take quite a bit of careful
massage.  I don't have such a transition plotted yet; not enough information
at the moment, and I seriously suspect that atomic_open would be the best
place to start.  If nothing else, there are reasonably few instances of that
puppy.  Moreover, we badly need to regularize the paths around do_last() -
right now they are messy as hell.  Once that is sorted out, we'll be in better
position to deal with the rest of directory-modifying operations.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web