Skip to contentSkip to conversation

Extend RCU-walk support to NFS.

PATCH NAK NeilBrown 3 participants 17 messages Mar 5, 2014 – Mar 6, 2014

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

cover Extend RCU-walk support to NFS.

Linux 2.6.38 introduced RCU-walk (https://lwn.net/Articles/419811/)
to improve path-name lookup by avoiding locking in many cases, particularly when everything is in cache.

NFS doesn't benefit from this, returning -ECHILD whenever RCU-walk is detected.

For workloads that can be served entirely from the cache and are metadata-heavy (e.g. 'make') this can be a performance problem particularly if multiple threads are using multiple cores.

The following series extends RCU-walk into NFS allowing a synthetic stat-heavy workload to avoid locking and refcounting most of the time. It provides about 20% performance improvement with 4 threads on 2 core, and will probably have a much more significant effect with 4 or 8 cores (though I haven't tested yet).

The key to providing RCU-walk support is to avoid sleeping and, where possible, avoid locking or refcounting.
So in nfs_permission and nfs{,4}_lookup_revalidate we should return -ECHILD only if a memory allocation or RPC request are required. In other cases we can simply complete as normal.

There are two data-structures which need to be searched for
nfs_permission: the credential cache (in sunrpc) and the nfs access_cache.
The credential cache is already rcu-aware so it is easy to return a credential without locking if LOOKUP_RCU is set.

The access_cache uses an rbtree which is not rcu-aware and cannot easily be made so. There are two approaches we can take here. 1/ take the required locks and perform the access check with locks
held (as nfs_access_get_cached already does). This is easy but requires taking locks which doesn't scale as well as not taking them.
2/ Use rcu to free cache entries and use a simpler rcu-safe search
mechanism when performing an RCU-walk. This search doesn't have to be perfect. If it finds the right value most of the time that will still help. When it doesn't find the required value we fall back on taking the lock.

The early patches assume the first approach. The final patch attempts the second. The rcu-safe search method used is simple to examine the final (most recent) entry on the lru.

This final patch provides about 2% performance increase over not having the patch in my 4-thread tests on 2-core CPU.
It is the most "RFC" part of the series. The earlier patches I am comfortable with (though review is always appreciated of course).

---

NeilBrown (8):
NFS: nfs4_lookup_revalidate: only evaluate parent if it will be used.
NFS: prepare for RCU-walk support but pushing tests later in code. sunrpc/auth: allow lockless (rcu) lookup of credential cache. sunrpc/auth: add 'rcu_walk' arg to rpc_lookup_cred.
NFS: teach nfs_do_access to understand MAY_NOT_BLOCK
NFS: teach nfs_neg_need_reval to understand LOOKUP_RCU
NFS: teach nfs_lookup_verify_inode to handle LOOKUP_RCU
NFS: allow lockless access to access_cache

 fs/nfs/dir.c                |  134 +++++++++++++++++++++++++++++++++----------
 fs/nfs/inode.c              |   11 +++-
 fs/nfs/nfs4proc.c           |    2 -
 fs/nfs/unlink.c             |    4 +
 include/linux/nfs_fs.h      |    2 +
 include/linux/sunrpc/auth.h |    3 +
 net/sunrpc/auth.c           |   17 +++++
 net/sunrpc/auth_generic.c   |    5 +-
 net/sunrpc/auth_null.c      |    2 +
 9 files changed, 139 insertions(+), 41 deletions(-)
Signature · 1 line
Signature
author
#1 lore raw

PATCH 1/8 NFS: nfs4_lookup_revalidate: only evaluate parent if it will be used.

nfs4_lookup_revalidate only uses 'parent' to get 'dir', and only uses 'dir' if 'inode == NULL'.

So we don't need to find out what 'parent' or 'dir' is until we know that 'inode' is NULL.

By moving 'dget_parent' inside the 'if', we can reduce the number of call sites for 'dput(parent)'.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
fs/nfs/dir.c +8 -9
index be38b573495a..f5509f95f261 100644
@@ -1496,9 +1496,7 @@ EXPORT_SYMBOL_GPL(nfs_atomic_open);
 
 static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 {
-	struct dentry *parent = NULL;
 	struct inode *inode;
-	struct inode *dir;
 	int ret = 0;
 
 	if (flags & LOOKUP_RCU)

@@ -1512,34 +1510,35 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 		goto no_open;
 
 	inode = dentry->d_inode;
-	parent = dget_parent(dentry);
-	dir = parent->d_inode;
 
 	/* We can't create new files in nfs_open_revalidate(), so we
 	 * optimize away revalidation of negative dentries.
 	 */
 	if (inode == NULL) {
+		struct dentry *parent;
+		struct inode *dir;
+
+		parent = dget_parent(dentry);
+		dir = parent->d_inode;
 		if (!nfs_neg_need_reval(dir, dentry, flags))
 			ret = 1;
+		dput(parent);
 		goto out;
 	}
 
 	/* NFS only supports OPEN on regular files */
 	if (!S_ISREG(inode->i_mode))
-		goto no_open_dput;
+		goto no_open;
 	/* We cannot do exclusive creation on a positive dentry */
 	if (flags & LOOKUP_EXCL)
-		goto no_open_dput;
+		goto no_open;
 
 	/* Let f_op->open() actually open (and revalidate) the file */
 	ret = 1;
 
 out:
-	dput(parent);
 	return ret;
 
-no_open_dput:
-	dput(parent);
 no_open:
 	return nfs_lookup_revalidate(dentry, flags);
 }
author
#2 lore raw

PATCH 2/8 NFS: prepare for RCU-walk support but pushing tests later in code.

In reply to NeilBrown

nfs_lookup_revalidate, nfs4_lookup_revalidate, and nfs_permission all need to understand and handle RCU-walk for NFS to gain the benefits of RCU-walk for cached information.

Currently these functions all immediately return -ECHILD if the relevant flag (LOOKUP_RCU or MAY_NOT_BLOCK) is set.

This patch pushes those tests later in the code so that we only abort immediately before we enter rcu-unsafe code. As subsequent patches make that rcu-unsafe code rcu-safe, several of these new tests will disappear.

With this patch there are several paths through the code which will no longer return -ECHILD during an RCU-walk. However these are mostly error paths or other uninteresting cases.

A noteworthy change in nfs_lookup_revalidate is that we don't take (or put) the reference to ->d_parent when LOOKUP_RCU is set.

In nfs4_lookup_revalidate we simple avoid testing LOOKUP_RCU on the path that simply calls nfs_lookup_revalidate() as that function already performs the required test.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
fs/nfs/dir.c +24 -12
index f5509f95f261..dcec2c56fe13 100644
@@ -1056,14 +1056,17 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	int error;
 
 	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	parent = dget_parent(dentry);
+		parent = rcu_dereference(dentry->d_parent);
+	else
+		parent = dget_parent(dentry);
 	dir = parent->d_inode;
 	nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);
 	inode = dentry->d_inode;
 
 	if (!inode) {
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		if (nfs_neg_need_reval(dir, dentry, flags))
 			goto out_bad;
 		goto out_valid_noent;

@@ -1078,6 +1081,9 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
 		goto out_set_verifier;
 
+	if (flags & LOOKUP_RCU)
+		return -ECHILD;
+
 	/* Force a full look up iff the parent directory has changed */
 	if (!nfs_is_exclusive_create(dir, flags) && nfs_check_verifier(dir, dentry)) {
 		if (nfs_lookup_verify_inode(inode, flags))

@@ -1120,7 +1126,8 @@ out_set_verifier:
 	/* Success: notify readdir to use READDIRPLUS */
 	nfs_advise_use_readdirplus(dir);
  out_valid_noent:
-	dput(parent);
+	if (!(flags & LOOKUP_RCU))
+		dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is valid\n",
 			__func__, dentry);
 	return 1;

@@ -1147,7 +1154,8 @@ out_zap_parent:
 	if (check_submounts_and_drop(dentry) != 0)
 		goto out_valid;
 
-	dput(parent);
+	if (!(flags & LOOKUP_RCU))
+		dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is invalid\n",
 			__func__, dentry);
 	return 0;

@@ -1155,7 +1163,8 @@ out_error:
 	nfs_free_fattr(fattr);
 	nfs_free_fhandle(fhandle);
 	nfs4_label_free(label);
-	dput(parent);
+	if (!(flags & LOOKUP_RCU))
+		dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) lookup returned error %d\n",
 			__func__, dentry, error);
 	return error;

@@ -1499,9 +1508,6 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	struct inode *inode;
 	int ret = 0;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
 	if (!(flags & LOOKUP_OPEN) || (flags & LOOKUP_DIRECTORY))
 		goto no_open;
 	if (d_mountpoint(dentry))

@@ -1518,6 +1524,9 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 		struct dentry *parent;
 		struct inode *dir;
 
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		parent = dget_parent(dentry);
 		dir = parent->d_inode;
 		if (!nfs_neg_need_reval(dir, dentry, flags))

@@ -2273,9 +2282,6 @@ int nfs_permission(struct inode *inode, int mask)
 	struct rpc_cred *cred;
 	int res = 0;
 
-	if (mask & MAY_NOT_BLOCK)
-		return -ECHILD;
-
 	nfs_inc_stats(inode, NFSIOS_VFSACCESS);
 
 	if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)

@@ -2302,6 +2308,9 @@ force_lookup:
 	if (!NFS_PROTO(inode)->access)
 		goto out_notsup;
 
+	if (mask & MAY_NOT_BLOCK)
+		return -ECHILD;
+
 	cred = rpc_lookup_cred();
 	if (!IS_ERR(cred)) {
 		res = nfs_do_access(inode, cred, mask);

@@ -2316,6 +2325,9 @@ out:
 		inode->i_sb->s_id, inode->i_ino, mask, res);
 	return res;
 out_notsup:
+	if (mask & MAY_NOT_BLOCK)
+		return -ECHILD;
+
 	res = nfs_revalidate_inode(NFS_SERVER(inode), inode);
 	if (res == 0)
 		res = generic_permission(inode, mask);
NAK
#3 lore raw

Re: [PATCH 2/8] NFS: prepare for RCU-walk support but pushing tests later in code.

fs/nfs/dir.c static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) · line 1063

Quoted 6 lines · NeilBrown
fs/nfs/dir.c +3 -1
-	parent = dget_parent(dentry);
+		parent = rcu_dereference(dentry->d_parent);
+	else
+		parent = dget_parent(dentry);
 	dir = parent->d_inode;
 	nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);

... and in RCU mode there's nothing to stop parent from

	a) not being dentry->d_parent anymore
	b) having already become negative.

NAK.

author
#4 lore raw

Re: [PATCH 2/8] NFS: prepare for RCU-walk support but pushing tests later in code.

Quoted 8 lines · On Wed, 5 Mar 2014 05:34:41 +0000 Al Viro <[email protected]> wrote:

fs/nfs/dir.c static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) · line 1063

Quoted 6 lines · NeilBrown
fs/nfs/dir.c +3 -1
-	parent = dget_parent(dentry);
+		parent = rcu_dereference(dentry->d_parent);
+	else
+		parent = dget_parent(dentry);
 	dir = parent->d_inode;
 	nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);

Hi Al,
thanks for the review.

Quoted 2 lines

... and in RCU mode there's nothing to stop parent from

	a) not being dentry->d_parent anymore

Does that matter? Surely dentry->d_parent exists, is still a dentry, and was recently the parent to dentry (i.e. since the last grace period). If it has changed (which it must if it is no longer our parent), then the cache validity checks are bound to fail and we will fall back to refcnt-walk as we should.

Quoted 1 line

b) having already become negative.

I didn't think dentries ever became negative. When a file is deleted the old positive dentry is unlinked and a new negative dentry is created in it's place.
Or has that changed since last I looked?

If they can become negative, then I could
dir = ACCESS_ONCE(parent->d_inode);
if (!dir)

        return -ECHILD;

Do you think that would be safe?

Thanks,
NeilBrown

Quoted 2 lines

NAK.

#5 lore raw

Re: [PATCH 2/8] NFS: prepare for RCU-walk support but pushing tests later in code.

Quoted 6 lines · NeilBrown
Quoted 1 line

b) having already become negative.

I didn't think dentries ever became negative. When a file is deleted the old positive dentry is unlinked and a new negative dentry is created in it's place.
Or has that changed since last I looked?

It has never been true. See what d_delete() is doing. If there was only one reference to dentry, it *does* become negative.

Quoted 6 lines

If they can become negative, then I could

   dir = ACCESS_ONCE(parent->d_inode);
   if (!dir)
        return -ECHILD;

Do you think that would be safe?

Depends on what you do with it afterwards...

author
#6 lore raw

Re: [PATCH 2/8] NFS: prepare for RCU-walk support but pushing tests later in code.

Quoted 10 lines · On Wed, 5 Mar 2014 06:49:46 +0000 Al Viro <[email protected]> wrote:
Quoted 6 lines · NeilBrown
Quoted 1 line

b) having already become negative.

I didn't think dentries ever became negative. When a file is deleted the old positive dentry is unlinked and a new negative dentry is created in it's place.
Or has that changed since last I looked?

It has never been true. See what d_delete() is doing. If there was only one reference to dentry, it *does* become negative.

Seems I missed that - thanks.

Quoted 9 lines
Quoted 7 lines

If they can become negative, then I could

   dir = ACCESS_ONCE(parent->d_inode);
   if (!dir)
        return -ECHILD;

Do you think that would be safe?

Depends on what you do with it afterwards...

It deferences i_sb, reads a couple of integer fields (never writes) and

    rcu_dereference(NFS_I(inode)->delegation);

which will currently always be NULL on a directory and if we ever did have directory delegation, should clearly be safe to reference under rcu.

Here is my current version of that patch.
Thanks again,
NeilBrown

From 8b85de80ccc7be4e9a31a547737e091fba2ae81f Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Tue, 4 Mar 2014 15:06:58 +1100
Subject: [PATCH] NFS: prepare for RCU-walk support but pushing tests later in
code.

nfs_lookup_revalidate, nfs4_lookup_revalidate, and nfs_permission all need to understand and handle RCU-walk for NFS to gain the benefits of RCU-walk for cached information.

Currently these functions all immediately return -ECHILD if the relevant flag (LOOKUP_RCU or MAY_NOT_BLOCK) is set.

This patch pushes those tests later in the code so that we only abort immediately before we enter rcu-unsafe code. As subsequent patches make that rcu-unsafe code rcu-safe, several of these new tests will disappear.

With this patch there are several paths through the code which will no longer return -ECHILD during an RCU-walk. However these are mostly error paths or other uninteresting cases.

A noteworthy change in nfs_lookup_revalidate is that we don't take (or put) the reference to ->d_parent when LOOKUP_RCU is set. Rather we rcu_dereference ->d_parent, and check that ->d_inode is not NULL. We also check that ->d_parent hasn't changed after all the tests.

In nfs4_lookup_revalidate we simple avoid testing LOOKUP_RCU on the path that simply calls nfs_lookup_revalidate() as that function already performs the required test.

Signed-off-by NeilBrown <[email protected]>
fs/nfs/dir.c +33 -12
index f5509f95f261..7530acdc5c42 100644
@@ -1055,21 +1055,30 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	struct nfs4_label *label = NULL;
 	int error;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	parent = dget_parent(dentry);
-	dir = parent->d_inode;
+	if (flags & LOOKUP_RCU) {
+		parent = rcu_dereference(dentry->d_parent);
+		dir = ACCESS_ONCE(parent->d_inode);
+		if (!dir)
+			return -ECHILD;
+	} else {
+		parent = dget_parent(dentry);
+		dir = parent->d_inode;
+	}
 	nfs_inc_stats(dir, NFSIOS_DENTRYREVALIDATE);
 	inode = dentry->d_inode;
 
 	if (!inode) {
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		if (nfs_neg_need_reval(dir, dentry, flags))
 			goto out_bad;
 		goto out_valid_noent;
 	}
 
 	if (is_bad_inode(inode)) {
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
 		dfprintk(LOOKUPCACHE, "%s: %pd2 has dud inode\n",
 				__func__, dentry);
 		goto out_bad;

@@ -1078,6 +1087,9 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
 		goto out_set_verifier;
 
+	if (flags & LOOKUP_RCU)
+		return -ECHILD;
+
 	/* Force a full look up iff the parent directory has changed */
 	if (!nfs_is_exclusive_create(dir, flags) && nfs_check_verifier(dir, dentry)) {
 		if (nfs_lookup_verify_inode(inode, flags))

@@ -1120,13 +1132,18 @@ out_set_verifier:
 	/* Success: notify readdir to use READDIRPLUS */
 	nfs_advise_use_readdirplus(dir);
  out_valid_noent:
-	dput(parent);
+	if (flags & LOOKUP_RCU) {
+		if (parent != rcu_dereference(dentry->d_parent))
+			return -ECHILD;
+	} else
+		dput(parent);
 	dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is valid\n",
 			__func__, dentry);
 	return 1;
 out_zap_parent:
 	nfs_zap_caches(dir);
  out_bad:
+	BUG_ON(flags & LOOKUP_RCU);
 	nfs_free_fattr(fattr);
 	nfs_free_fhandle(fhandle);
 	nfs4_label_free(label);

@@ -1152,6 +1169,7 @@ out_zap_parent:
 			__func__, dentry);
 	return 0;
 out_error:
+	BUG_ON(flags & LOOKUP_RCU);
 	nfs_free_fattr(fattr);
 	nfs_free_fhandle(fhandle);
 	nfs4_label_free(label);

@@ -1499,9 +1517,6 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	struct inode *inode;
 	int ret = 0;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
 	if (!(flags & LOOKUP_OPEN) || (flags & LOOKUP_DIRECTORY))
 		goto no_open;
 	if (d_mountpoint(dentry))

@@ -1518,6 +1533,9 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 		struct dentry *parent;
 		struct inode *dir;
 
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		parent = dget_parent(dentry);
 		dir = parent->d_inode;
 		if (!nfs_neg_need_reval(dir, dentry, flags))

@@ -2273,9 +2291,6 @@ int nfs_permission(struct inode *inode, int mask)
 	struct rpc_cred *cred;
 	int res = 0;
 
-	if (mask & MAY_NOT_BLOCK)
-		return -ECHILD;
-
 	nfs_inc_stats(inode, NFSIOS_VFSACCESS);
 
 	if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)

@@ -2302,6 +2317,9 @@ force_lookup:
 	if (!NFS_PROTO(inode)->access)
 		goto out_notsup;
 
+	if (mask & MAY_NOT_BLOCK)
+		return -ECHILD;
+
 	cred = rpc_lookup_cred();
 	if (!IS_ERR(cred)) {
 		res = nfs_do_access(inode, cred, mask);

@@ -2316,6 +2334,9 @@ out:
 		inode->i_sb->s_id, inode->i_ino, mask, res);
 	return res;
 out_notsup:
+	if (mask & MAY_NOT_BLOCK)
+		return -ECHILD;
+
 	res = nfs_revalidate_inode(NFS_SERVER(inode), inode);
 	if (res == 0)
 		res = generic_permission(inode, mask);
author
#7 lore raw

PATCH 3/8 sunrpc/auth: allow lockless (rcu) lookup of credential cache.

In reply to NeilBrown

The new flag RPCAUTH_LOOKUP_RCU to credential lookup avoids locking, does not take a reference on the returned credential, and returns -ECHILD if a simple lookup was not possible.

The returned value can only be used within an rcu_read_lock protected region.

Signed-off-by NeilBrown <[email protected]>
---
 include/linux/sunrpc/auth.h |    1 +
 net/sunrpc/auth.c           |   17 +++++++++++++++--
 net/sunrpc/auth_null.c      |    2 ++
 3 files changed, 18 insertions(+), 2 deletions(-)
include/linux/sunrpc/auth.h +1 -0
index 790be1472792..3f158ae1d6fd 100644
@@ -103,6 +103,7 @@ struct rpc_auth_create_args {
 
 /* Flags for rpcauth_lookupcred() */
 #define RPCAUTH_LOOKUP_NEW		0x01	/* Accept an uninitialised cred */
+#define RPCAUTH_LOOKUP_RCU		0x02	/* lock-less lookup */
 
 /*
  * Client authentication ops
net/sunrpc/auth.c +15 -2
index 5285ead196c0..82459dd848ff 100644
@@ -523,6 +523,12 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
 	hlist_for_each_entry_rcu(entry, &cache->hashtable[nr], cr_hash) {
 		if (!entry->cr_ops->crmatch(acred, entry, flags))
 			continue;
+		if (flags & RPCAUTH_LOOKUP_RCU) {
+			if (test_bit(RPCAUTH_CRED_HASHED, &entry->cr_flags) &&
+			    !test_bit(RPCAUTH_CRED_NEW, &entry->cr_flags))
+				cred = entry;
+			break;
+		}
 		spin_lock(&cache->lock);
 		if (test_bit(RPCAUTH_CRED_HASHED, &entry->cr_flags) == 0) {
 			spin_unlock(&cache->lock);

@@ -537,6 +543,9 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
 	if (cred != NULL)
 		goto found;
 
+	if (flags & RPCAUTH_LOOKUP_RCU)
+		return ERR_PTR(-ECHILD);
+
 	new = auth->au_ops->crcreate(auth, acred, flags);
 	if (IS_ERR(new)) {
 		cred = new;

@@ -586,10 +595,14 @@ rpcauth_lookupcred(struct rpc_auth *auth, int flags)
 	memset(&acred, 0, sizeof(acred));
 	acred.uid = cred->fsuid;
 	acred.gid = cred->fsgid;
-	acred.group_info = get_group_info(((struct cred *)cred)->group_info);
+	if (flags & RPCAUTH_LOOKUP_RCU)
+		acred.group_info = rcu_dereference(cred->group_info);
+	else
+		acred.group_info = get_group_info(((struct cred *)cred)->group_info);
 
 	ret = auth->au_ops->lookup_cred(auth, &acred, flags);
-	put_group_info(acred.group_info);
+	if (!(flags & RPCAUTH_LOOKUP_RCU))
+		put_group_info(acred.group_info);
 	return ret;
 }
net/sunrpc/auth_null.c +2 -0
index f0ebe07978a2..712c123e04e9 100644
@@ -35,6 +35,8 @@ nul_destroy(struct rpc_auth *auth)
 static struct rpc_cred *
 nul_lookup_cred(struct rpc_auth *auth, struct auth_cred *acred, int flags)
 {
+	if (flags & RPCAUTH_LOOKUP_RCU)
+		return &null_cred;
 	return get_rpccred(&null_cred);
 }
author
#8 lore raw

PATCH 4/8 sunrpc/auth: add 'rcu_walk' arg to rpc_lookup_cred.

In reply to NeilBrown

This arg causes rpc_lookup_cred to set RPCAUTH_LOOKUP_RCU when performing the credential lookup. Most callers pass '0', except nfs_permission() which passes (mask & MAY_NOT_BLOCK).

This doesn't speed up nfs_permission yet as it is not yet safe to call nfs_do_access with the returned cred.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c                |    8 ++++----
 fs/nfs/inode.c              |    2 +-
 fs/nfs/nfs4proc.c           |    2 +-
 fs/nfs/unlink.c             |    4 ++--
 include/linux/sunrpc/auth.h |    2 +-
 net/sunrpc/auth_generic.c   |    5 +++--
 6 files changed, 12 insertions(+), 11 deletions(-)
fs/nfs/dir.c +4 -4
index dcec2c56fe13..36e12f545fd7 100644
@@ -102,7 +102,7 @@ nfs_opendir(struct inode *inode, struct file *filp)
 
 	nfs_inc_stats(inode, NFSIOS_VFSOPEN);
 
-	cred = rpc_lookup_cred();
+	cred = rpc_lookup_cred(0);
 	if (IS_ERR(cred))
 		return PTR_ERR(cred);
 	ctx = alloc_nfs_open_dir_context(inode, cred);

@@ -2308,11 +2308,11 @@ force_lookup:
 	if (!NFS_PROTO(inode)->access)
 		goto out_notsup;
 
-	if (mask & MAY_NOT_BLOCK)
-		return -ECHILD;
 
-	cred = rpc_lookup_cred();
+	cred = rpc_lookup_cred(mask & MAY_NOT_BLOCK);
 	if (!IS_ERR(cred)) {
+		if (mask & MAY_NOT_BLOCK)
+			return -ECHILD;
 		res = nfs_do_access(inode, cred, mask);
 		put_rpccred(cred);
 	} else
fs/nfs/inode.c +1 -1
index 28a0a3cbd3b7..59e57cceeab5 100644
@@ -717,7 +717,7 @@ EXPORT_SYMBOL_GPL(nfs_close_context);
 struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
 {
 	struct nfs_open_context *ctx;
-	struct rpc_cred *cred = rpc_lookup_cred();
+	struct rpc_cred *cred = rpc_lookup_cred(0);
 	if (IS_ERR(cred))
 		return ERR_CAST(cred);
fs/nfs/nfs4proc.c +1 -1
index 42da6af77587..922b924f0a7c 100644
@@ -4733,7 +4733,7 @@ nfs4_set_security_label(struct dentry *dentry, const void *buf, size_t buflen)
 	ilabel.label = (char *)buf;
 	ilabel.len = buflen;
 
-	cred = rpc_lookup_cred();
+	cred = rpc_lookup_cred(0);
 	if (IS_ERR(cred))
 		return PTR_ERR(cred);
fs/nfs/unlink.c +2 -2
index 11d78944de79..34c0b878f2a8 100644
@@ -258,7 +258,7 @@ nfs_async_unlink(struct inode *dir, struct dentry *dentry)
 	if (data == NULL)
 		goto out;
 
-	data->cred = rpc_lookup_cred();
+	data->cred = rpc_lookup_cred(0);
 	if (IS_ERR(data->cred)) {
 		status = PTR_ERR(data->cred);
 		goto out_free;

@@ -418,7 +418,7 @@ nfs_async_rename(struct inode *old_dir, struct inode *new_dir,
 		return ERR_PTR(-ENOMEM);
 	task_setup_data.callback_data = data;
 
-	data->cred = rpc_lookup_cred();
+	data->cred = rpc_lookup_cred(0);
 	if (IS_ERR(data->cred)) {
 		struct rpc_task *task = ERR_CAST(data->cred);
 		kfree(data);
include/linux/sunrpc/auth.h +1 -1
index 3f158ae1d6fd..ef354ccb709d 100644
@@ -153,7 +153,7 @@ void			rpcauth_remove_module(void);
 void			rpc_destroy_generic_auth(void);
 void 			rpc_destroy_authunix(void);
 
-struct rpc_cred *	rpc_lookup_cred(void);
+struct rpc_cred *	rpc_lookup_cred(int rcu_walk);
 struct rpc_cred *	rpc_lookup_machine_cred(const char *service_name);
 int			rpcauth_register(const struct rpc_authops *);
 int			rpcauth_unregister(const struct rpc_authops *);
net/sunrpc/auth_generic.c +3 -2
index ed04869b2d4f..ffbd66b94781 100644
@@ -32,9 +32,10 @@ static const struct rpc_credops generic_credops;
 /*
  * Public call interface
  */
-struct rpc_cred *rpc_lookup_cred(void)
+struct rpc_cred *rpc_lookup_cred(int rcu_walk)
 {
-	return rpcauth_lookupcred(&generic_auth, 0);
+	return rpcauth_lookupcred(&generic_auth,
+				  rcu_walk ? RPCAUTH_LOOKUP_RCU : 0);
 }
 EXPORT_SYMBOL_GPL(rpc_lookup_cred);
#9 lore raw

Re: [PATCH 4/8] sunrpc/auth: add 'rcu_walk' arg to rpc_lookup_cred.

Quoted 4 lines · NeilBrown

This arg causes rpc_lookup_cred to set RPCAUTH_LOOKUP_RCU when performing the credential lookup. Most callers pass '0', except nfs_permission() which passes (mask & MAY_NOT_BLOCK).

Why not just add a function rpc_lookup_cred_rcu() or rpc_lookup_cred_noblock() to cater for that one exception?

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

author
#10 lore raw

Re: [PATCH 4/8] sunrpc/auth: add 'rcu_walk' arg to rpc_lookup_cred.

Quoted 9 lines · On Wed, 5 Mar 2014 09:43:03 -0500 Trond Myklebust…
Quoted 4 lines · NeilBrown

This arg causes rpc_lookup_cred to set RPCAUTH_LOOKUP_RCU when performing the credential lookup. Most callers pass '0', except nfs_permission() which passes (mask & MAY_NOT_BLOCK).

Why not just add a function rpc_lookup_cred_rcu() or rpc_lookup_cred_noblock() to cater for that one exception?

That certainly makes the patch smaller, and is probably a net win.

Thanks.
NeilBrown

From b805a8f79a5527676b96b9654dd26d1b5bc03691 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Tue, 4 Mar 2014 15:28:43 +1100
Subject: [PATCH] sunrpc/auth: add rpc_lookup_cred_nonblock

This version of rpc_lookup_cred sets RPCAUTH_LOOKUP_RCU when performing the credential lookup.
nfs_permission() uses it in RCU-walk mode.

This doesn't speed up nfs_permission yet as it is not yet safe to call nfs_do_access with the returned cred.

Signed-off-by NeilBrown <[email protected]>
fs/nfs/dir.c +5 -3
index 7530acdc5c42..ede873e88acc 100644
@@ -2318,10 +2318,12 @@ force_lookup:
 		goto out_notsup;
 
 	if (mask & MAY_NOT_BLOCK)
-		return -ECHILD;
-
-	cred = rpc_lookup_cred();
+		cred = rpc_lookup_cred_nonblock();
+	else
+		cred = rpc_lookup_cred();
 	if (!IS_ERR(cred)) {
+		if (mask & MAY_NOT_BLOCK)
+			return -ECHILD;
 		res = nfs_do_access(inode, cred, mask);
 		put_rpccred(cred);
 	} else
include/linux/sunrpc/auth.h +1 -0
index 3f158ae1d6fd..28ffc8476875 100644
@@ -154,6 +154,7 @@ void			rpc_destroy_generic_auth(void);
 void 			rpc_destroy_authunix(void);
 
 struct rpc_cred *	rpc_lookup_cred(void);
+struct rpc_cred *	rpc_lookup_cred_nonblock(void);
 struct rpc_cred *	rpc_lookup_machine_cred(const char *service_name);
 int			rpcauth_register(const struct rpc_authops *);
 int			rpcauth_unregister(const struct rpc_authops *);
net/sunrpc/auth_generic.c +5 -0
index ed04869b2d4f..632f6eec8111 100644
@@ -38,6 +38,11 @@ struct rpc_cred *rpc_lookup_cred(void)
 }
 EXPORT_SYMBOL_GPL(rpc_lookup_cred);
 
+struct rpc_cred *rpc_lookup_cred_nonblock(void)
+{
+	return rpcauth_lookupcred(&generic_auth, 1);
+}
+
 /*
  * Public call interface for looking up machine creds.
  */
author
#11 lore raw

PATCH 5/8 NFS: teach nfs_do_access to understand MAY_NOT_BLOCK

In reply to NeilBrown

If MAY_NOT_BLOCK is set we return -ECHILD in preference to making a network call.
This allows nfs_permission to call it in RCU-walk mode.

Now nfs_permission() can often complete in RCU-walk mode with no error.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
fs/nfs/dir.c +6 -3
index 36e12f545fd7..f3895325ac6a 100644
@@ -2232,6 +2232,10 @@ static int nfs_do_access(struct inode *inode, struct rpc_cred *cred, int mask)
 	if (status == 0)
 		goto out_cached;
 
+	status = -ECHILD;
+	if (mask & MAY_NOT_BLOCK)
+		goto out;
+
 	/* Be clever: ask server to check for all possible rights */
 	cache.mask = MAY_EXEC | MAY_WRITE | MAY_READ;
 	cache.cred = cred;

@@ -2311,10 +2315,9 @@ force_lookup:
 
 	cred = rpc_lookup_cred(mask & MAY_NOT_BLOCK);
 	if (!IS_ERR(cred)) {
-		if (mask & MAY_NOT_BLOCK)
-			return -ECHILD;
 		res = nfs_do_access(inode, cred, mask);
-		put_rpccred(cred);
+		if (!(mask & MAY_NOT_BLOCK))
+			put_rpccred(cred);
 	} else
 		res = PTR_ERR(cred);
 out:
author
#12 lore raw

PATCH 6/8 NFS: teach nfs_neg_need_reval to understand LOOKUP_RCU

In reply to NeilBrown

This requires nfs_check_verifier to take an rcu_walk flag, and requires an rcu version of nfs_revalidate_inode which returns -ECHILD rather than making an RPC call.

With this, nfs_lookup_revalidate can call nfs_neg_need_reval in RCU-walk mode.

We can also move the LOOKUP_RCU check past the nfs_check_verifier() call in nfs_lookup_revalidate.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c           |   47 ++++++++++++++++++++++++++++++++---------------
 fs/nfs/inode.c         |    9 +++++++++
 include/linux/nfs_fs.h |    1 +
 3 files changed, 42 insertions(+), 15 deletions(-)
fs/nfs/dir.c +32 -15
index f3895325ac6a..76b5f40c8587 100644
@@ -956,8 +956,11 @@ EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
  * In the case it has, we assume that the dentries are untrustworthy
  * and may need to be looked up again.
  */
-static int nfs_check_verifier(struct inode *dir, struct dentry *dentry)
+static int nfs_check_verifier(struct inode *dir, struct dentry *dentry,
+			      int rcu_walk)
 {
+	int ret;
+
 	if (IS_ROOT(dentry))
 		return 1;
 	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONE)

@@ -965,7 +968,13 @@ static int nfs_check_verifier(struct inode *dir, struct dentry *dentry)
 	if (!nfs_verify_change_attribute(dir, dentry->d_time))
 		return 0;
 	/* Revalidate nfsi->cache_change_attribute before we declare a match */
-	if (nfs_revalidate_inode(NFS_SERVER(dir), dir) < 0)
+	if (rcu_walk)
+		ret = nfs_revalidate_inode_rcu(NFS_SERVER(dir), dir);
+	else
+		ret = nfs_revalidate_inode(NFS_SERVER(dir), dir);
+	if (ret == -ECHILD)
+		return -ECHILD;
+	if (ret < 0)
 		return 0;
 	if (!nfs_verify_change_attribute(dir, dentry->d_time))
 		return 0;

@@ -1031,7 +1040,7 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry,
 		return 0;
 	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
 		return 1;
-	return !nfs_check_verifier(dir, dentry);
+	return !nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU);
 }
 
 /*

@@ -1064,11 +1073,11 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	inode = dentry->d_inode;
 
 	if (!inode) {
-		if (flags & LOOKUP_RCU)
-			return -ECHILD;
-
-		if (nfs_neg_need_reval(dir, dentry, flags))
+		if (nfs_neg_need_reval(dir, dentry, flags)) {
+			if (flags & LOOKUP_RCU)
+				return -ECHILD;
 			goto out_bad;
+		}
 		goto out_valid_noent;
 	}
 

@@ -1081,11 +1090,13 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
 		goto out_set_verifier;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
 	/* Force a full look up iff the parent directory has changed */
-	if (!nfs_is_exclusive_create(dir, flags) && nfs_check_verifier(dir, dentry)) {
+	if (!nfs_is_exclusive_create(dir, flags) &&
+	    nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) {
+
+		if (flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		if (nfs_lookup_verify_inode(inode, flags))
 			goto out_zap_parent;
 		goto out_valid;

@@ -1094,6 +1105,9 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	if (NFS_STALE(inode))
 		goto out_bad;
 
+	if (flags & LOOKUP_RCU)
+		return -ECHILD;
+
 	error = -ENOMEM;
 	fhandle = nfs_alloc_fhandle();
 	fattr = nfs_alloc_fattr();

@@ -1525,13 +1539,16 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 		struct inode *dir;
 
 		if (flags & LOOKUP_RCU)
-			return -ECHILD;
-
-		parent = dget_parent(dentry);
+			parent = rcu_dereference(dentry);
+		else
+			parent = dget_parent(dentry);
 		dir = parent->d_inode;
 		if (!nfs_neg_need_reval(dir, dentry, flags))
 			ret = 1;
-		dput(parent);
+		else if (flags & LOOKUP_RCU)
+			ret = -ECHILD;
+		if (!(flags & LOOKUP_RCU))
+			dput(parent);
 		goto out;
 	}
fs/nfs/inode.c +9 -0
index 59e57cceeab5..6f39cadfad8c 100644
@@ -962,6 +962,15 @@ int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
 }
 EXPORT_SYMBOL_GPL(nfs_revalidate_inode);
 
+int nfs_revalidate_inode_rcu(struct nfs_server *server, struct inode *inode)
+{
+	if (!(NFS_I(inode)->cache_validity &
+			(NFS_INO_INVALID_ATTR|NFS_INO_INVALID_LABEL))
+			&& !nfs_attribute_cache_expired(inode))
+		return NFS_STALE(inode) ? -ESTALE : 0;
+	return -ECHILD;
+}
+
 static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
include/linux/nfs_fs.h +1 -0
index 0ae5807480f4..9e91f2cfcd01 100644
@@ -351,6 +351,7 @@ extern int nfs_release(struct inode *, struct file *);
 extern int nfs_attribute_timeout(struct inode *inode);
 extern int nfs_attribute_cache_expired(struct inode *inode);
 extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);
+extern int nfs_revalidate_inode_rcu(struct nfs_server *server, struct inode *inode);
 extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
 extern int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping);
 extern int nfs_setattr(struct dentry *, struct iattr *);
author
#13 lore raw

PATCH 7/8 NFS: teach nfs_lookup_verify_inode to handle LOOKUP_RCU

In reply to NeilBrown

It fails with -ECHILD rather than make an RPC call.

This allows nfs_lookup_revalidate to call it in RCU-walk mode.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
fs/nfs/dir.c +6 -4
index 76b5f40c8587..e808db561201 100644
@@ -1018,6 +1018,8 @@ int nfs_lookup_verify_inode(struct inode *inode, unsigned int flags)
 out:
 	return (inode->i_nlink == 0) ? -ENOENT : 0;
 out_force:
+	if (flags & LOOKUP_RCU)
+		return -ECHILD;
 	ret = __nfs_revalidate_inode(server, inode);
 	if (ret != 0)
 		return ret;

@@ -1094,11 +1096,11 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	if (!nfs_is_exclusive_create(dir, flags) &&
 	    nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) {
 
-		if (flags & LOOKUP_RCU)
-			return -ECHILD;
-
-		if (nfs_lookup_verify_inode(inode, flags))
+		if (nfs_lookup_verify_inode(inode, flags)) {
+			if (flags & LOOKUP_RCU)
+				return -ECHILD;
 			goto out_zap_parent;
+		}
 		goto out_valid;
 	}
author
#14 lore raw

PATCH 8/8 NFS: allow lockless access to access_cache

In reply to NeilBrown

The access cache is used during RCU-walk path lookups, so it is best to avoid locking if possible as taking a lock kills concurrency.

The rbtree is not rcu-safe and cannot easily be made so. Instead we simply check the last (i.e. most recent) entry on the LRU list. If this doesn't match, then we return -ECHILD and retry in lock/refcount mode.

This requires freeing the nfs_access_entry struct with rcu, and requires using rcu access primates when adding entries to the lru, and when examining the last entry.

Calling put_rpccred before kfree_rcu looks a bit odd, but as put_rpccred already provide rcu protection, we know that the cred will not actually be freed until the next grace period, so any concurrent access will be safe.

This patch provides about 5% performance improvement on a stat-heavy synthetic work load with 4 threads on a 2-core CPU.

Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/dir.c           |   41 +++++++++++++++++++++++++++++++++++++++--
 include/linux/nfs_fs.h |    1 +
 2 files changed, 40 insertions(+), 2 deletions(-)
fs/nfs/dir.c +39 -2
index e808db561201..edf0458acd4a 100644
@@ -2011,7 +2011,7 @@ static atomic_long_t nfs_access_nr_entries;
 static void nfs_access_free_entry(struct nfs_access_entry *entry)
 {
 	put_rpccred(entry->cred);
-	kfree(entry);
+	kfree_rcu(entry, rcu_head);
 	smp_mb__before_atomic_dec();
 	atomic_long_dec(&nfs_access_nr_entries);
 	smp_mb__after_atomic_dec();

@@ -2166,6 +2166,36 @@ out_zap:
 	return -ENOENT;
 }
 
+static int nfs_access_get_cached_rcu(struct inode *inode, struct rpc_cred *cred, struct nfs_access_entry *res)
+{
+	/* Only check the most recently returned cache entry,
+	 * but do it without locking.
+	 */
+	struct nfs_inode *nfsi = NFS_I(inode);
+	struct nfs_access_entry *cache;
+	int err = -ECHILD;
+	struct list_head *lh;
+
+	if (nfsi->cache_validity & NFS_INO_INVALID_ACCESS)
+		goto out;
+	lh = rcu_dereference(nfsi->access_cache_entry_lru.prev);
+	cache = list_entry(lh, struct nfs_access_entry, lru);
+	if (lh == &nfsi->access_cache_entry_lru ||
+	    cred != cache->cred)
+		cache = NULL;
+	if (cache == NULL)
+		goto out;
+	if (!nfs_have_delegated_attributes(inode) &&
+	    !time_in_range_open(jiffies, cache->jiffies, cache->jiffies + nfsi->attrtimeo))
+		goto out;
+	res->jiffies = cache->jiffies;
+	res->cred = cache->cred;
+	res->mask = cache->mask;
+	err = 0;
+out:
+	return err;
+}
+
 static void nfs_access_add_rbtree(struct inode *inode, struct nfs_access_entry *set)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);

@@ -2209,6 +2239,11 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set)
 	cache->cred = get_rpccred(set->cred);
 	cache->mask = set->mask;
 
+	/* The above fields assignments must be visible
+	 * before this item appears on the lru.  We cannot easily
+	 * use rcu_assign_pointer, so just force the memory barrier.
+	 */
+	smp_wmb();
 	nfs_access_add_rbtree(inode, cache);
 
 	/* Update accounting */

@@ -2247,7 +2282,9 @@ static int nfs_do_access(struct inode *inode, struct rpc_cred *cred, int mask)
 
 	trace_nfs_access_enter(inode);
 
-	status = nfs_access_get_cached(inode, cred, &cache);
+	if (!(mask & MAY_NOT_BLOCK) ||
+	    (status = nfs_access_get_cached_rcu(inode, cred, &cache)) != 0)
+		status = nfs_access_get_cached(inode, cred, &cache);
 	if (status == 0)
 		goto out_cached;
include/linux/nfs_fs.h +1 -0
index 9e91f2cfcd01..0eaf6c48c826 100644
@@ -52,6 +52,7 @@ struct nfs_access_entry {
 	unsigned long		jiffies;
 	struct rpc_cred *	cred;
 	int			mask;
+	struct rcu_head		rcu_head;
 };
 
 struct nfs_lockowner {
#15 lore raw

Re: [PATCH 8/8] NFS: allow lockless access to access_cache

Quoted 12 lines · NeilBrown

The access cache is used during RCU-walk path lookups, so it is best to avoid locking if possible as taking a lock kills concurrency.

The rbtree is not rcu-safe and cannot easily be made so. Instead we simply check the last (i.e. most recent) entry on the LRU list. If this doesn't match, then we return -ECHILD and retry in lock/refcount mode.

This requires freeing the nfs_access_entry struct with rcu, and requires using rcu access primates when adding entries to the lru, and when examining the last entry.

Quoted 8 lines

Calling put_rpccred before kfree_rcu looks a bit odd, but as put_rpccred already provide rcu protection, we know that the cred will not actually be freed until the next grace period, so any concurrent access will be safe.

This patch provides about 5% performance improvement on a stat-heavy synthetic work load with 4 threads on a 2-core CPU.

Have you looked at converting the rb-tree into something a little more RCU-friendly? Since our lookup key is just a pointer value, couldn’t we use a radix-tree?

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

author
#16 lore raw

Re: [PATCH 8/8] NFS: allow lockless access to access_cache

Quoted 20 lines · On Wed, 5 Mar 2014 10:11:02 -0500 Trond Myklebust…
Quoted 17 lines · NeilBrown

The access cache is used during RCU-walk path lookups, so it is best to avoid locking if possible as taking a lock kills concurrency.

The rbtree is not rcu-safe and cannot easily be made so. Instead we simply check the last (i.e. most recent) entry on the LRU list. If this doesn't match, then we return -ECHILD and retry in lock/refcount mode.

This requires freeing the nfs_access_entry struct with rcu, and requires using rcu access primates when adding entries to the lru, and when examining the last entry.

Calling put_rpccred before kfree_rcu looks a bit odd, but as put_rpccred already provide rcu protection, we know that the cred will not actually be freed until the next grace period, so any concurrent access will be safe.

Quoted 6 lines
Quoted 3 lines

This patch provides about 5% performance improvement on a stat-heavy synthetic work load with 4 threads on a 2-core CPU.

Have you looked at converting the rb-tree into something a little more RCU-friendly? Since our lookup key is just a pointer value, couldn’t we use a radix-tree?

Interesting idea. The radix tree would be fairly sparse unless we divided the address by the size of the structure ... would that hit alignment issues? But we don't know real size of the structure (it can depend on auth scheme) so I doubt that would work. So I think a radix tree would waste a lot of space.

Skip lists might work https://lwn.net/Articles/551896/ but they don't appear to be in the kernel yet.

I think my current approach is no worse the existing code and while there is room for further improvement, that should probably wait until a suitable data structure is available.

Thanks,
NeilBrown