nfsd: eliminate the client_mutex

From: Trond Myklebust <[email protected]>

Ensure that we only can have a single struct nfs4_file per inode in the file_hashtbl and make addition atomic with respect to lookup.

To prevent an i_lock/state_lock inversion, change nfsd4_init_file to use ihold instead if igrab. That's also more efficient anyway as we definitely hold a reference to the inode at that point.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

fs/nfsd/nfs4state.c +37 -12

index c473bd6d52c8..29788fd0da24 100644
@@ -2611,17 +2611,18 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 {
 	unsigned int hashval = file_hashval(ino);
 
+	lockdep_assert_held(&state_lock);
+
 	atomic_set(&fp->fi_ref, 1);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
-	fp->fi_inode = igrab(ino);
+	ihold(ino);
+	fp->fi_inode = ino;
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
 	memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
 	memset(fp->fi_access, 0, sizeof(fp->fi_access));
-	spin_lock(&state_lock);
 	hlist_add_head(&fp->fi_hash, &file_hashtbl[hashval]);
-	spin_unlock(&state_lock);
 }
 
 void

@@ -2787,23 +2788,49 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
 
 /* search file_hashtbl[] for file */
 static struct nfs4_file *
-find_file(struct inode *ino)
+find_file_locked(struct inode *ino)
 {
 	unsigned int hashval = file_hashval(ino);
 	struct nfs4_file *fp;
 
-	spin_lock(&state_lock);
+	lockdep_assert_held(&state_lock);
+
 	hlist_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
 		if (fp->fi_inode == ino) {
 			get_nfs4_file(fp);
-			spin_unlock(&state_lock);
 			return fp;
 		}
 	}
-	spin_unlock(&state_lock);
 	return NULL;
 }
 
+static struct nfs4_file *
+find_file(struct inode *ino)
+{
+	struct nfs4_file *fp;
+
+	spin_lock(&state_lock);
+	fp = find_file_locked(ino);
+	spin_unlock(&state_lock);
+	return fp;
+}
+
+static struct nfs4_file *
+find_or_add_file(struct inode *ino, struct nfs4_file *new)
+{
+	struct nfs4_file *fp;
+
+	spin_lock(&state_lock);
+	fp = find_file_locked(ino);
+	if (fp == NULL) {
+		nfsd4_init_file(new, ino);
+		fp = new;
+	}
+	spin_unlock(&state_lock);
+
+	return fp;
+}
+
 /*
  * Called to check deny when READ with all zero stateid or
  * WRITE with all zero or all one stateid

@@ -3325,21 +3352,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * and check for delegations in the process of being recalled.
 	 * If not found, create the nfs4_file struct
 	 */
-	fp = find_file(ino);
-	if (fp) {
+	fp = find_or_add_file(ino, open->op_file);
+	if (fp != open->op_file) {
 		if ((status = nfs4_check_open(fp, open, &stp)))
 			goto out;
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
 			goto out;
 	} else {
+		open->op_file = NULL;
 		status = nfserr_bad_stateid;
 		if (nfsd4_is_deleg_cur(open))
 			goto out;
 		status = nfserr_jukebox;
-		fp = open->op_file;
-		open->op_file = NULL;
-		nfsd4_init_file(fp, ino);
 	}
 
 	/*

Signature · 1 line

1.9.3

Quoted 4 lines · Jeff Layton

From: Trond Myklebust <[email protected]>

Ensure that we only can have a single struct nfs4_file per inode in the file_hashtbl and make addition atomic with respect to lookup.

This changelog gives the impression it's a bugfix, and it isn't. I understand it's tedious in this series, but really I think at a minimum a changelog should always make it clear whether a patch is a bugfix or not. Applying with changelog rewritten to:

Current code depends on the client_mutex to guarantee a single struct nfs4_file per inode in the file_hashtbl and make addition atomic with respect to lookup. Rely instead on the state_Lock, to make it easier to stop taking the client_mutex here later.
...

--b.

Quoted 120 lines

To prevent an i_lock/state_lock inversion, change nfsd4_init_file to use ihold instead if igrab. That's also more efficient anyway as we definitely hold a reference to the inode at that point.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfsd/nfs4state.c | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

fs/nfsd/nfs4state.c +37 -12

index c473bd6d52c8..29788fd0da24 100644
@@ -2611,17 +2611,18 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 {
 	unsigned int hashval = file_hashval(ino);
 
+	lockdep_assert_held(&state_lock);
+
 	atomic_set(&fp->fi_ref, 1);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
-	fp->fi_inode = igrab(ino);
+	ihold(ino);
+	fp->fi_inode = ino;
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
 	memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
 	memset(fp->fi_access, 0, sizeof(fp->fi_access));
-	spin_lock(&state_lock);
 	hlist_add_head(&fp->fi_hash, &file_hashtbl[hashval]);
-	spin_unlock(&state_lock);
 }
 
 void

@@ -2787,23 +2788,49 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
 
 /* search file_hashtbl[] for file */
 static struct nfs4_file *
-find_file(struct inode *ino)
+find_file_locked(struct inode *ino)
 {
 	unsigned int hashval = file_hashval(ino);
 	struct nfs4_file *fp;
 
-	spin_lock(&state_lock);
+	lockdep_assert_held(&state_lock);
+
 	hlist_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
 		if (fp->fi_inode == ino) {
 			get_nfs4_file(fp);
-			spin_unlock(&state_lock);
 			return fp;
 		}
 	}
-	spin_unlock(&state_lock);
 	return NULL;
 }
 
+static struct nfs4_file *
+find_file(struct inode *ino)
+{
+	struct nfs4_file *fp;
+
+	spin_lock(&state_lock);
+	fp = find_file_locked(ino);
+	spin_unlock(&state_lock);
+	return fp;
+}
+
+static struct nfs4_file *
+find_or_add_file(struct inode *ino, struct nfs4_file *new)
+{
+	struct nfs4_file *fp;
+
+	spin_lock(&state_lock);
+	fp = find_file_locked(ino);
+	if (fp == NULL) {
+		nfsd4_init_file(new, ino);
+		fp = new;
+	}
+	spin_unlock(&state_lock);
+
+	return fp;
+}
+
 /*
  * Called to check deny when READ with all zero stateid or
  * WRITE with all zero or all one stateid

@@ -3325,21 +3352,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * and check for delegations in the process of being recalled.
 	 * If not found, create the nfs4_file struct
 	 */
-	fp = find_file(ino);
-	if (fp) {
+	fp = find_or_add_file(ino, open->op_file);
+	if (fp != open->op_file) {
 		if ((status = nfs4_check_open(fp, open, &stp)))
 			goto out;
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
 			goto out;
 	} else {
+		open->op_file = NULL;
 		status = nfserr_bad_stateid;
 		if (nfsd4_is_deleg_cur(open))
 			goto out;
 		status = nfserr_jukebox;
-		fp = open->op_file;
-		open->op_file = NULL;
-		nfsd4_init_file(fp, ino);
 	}
 
 	/*

Signature · 1 line

1.9.3

There's a fair chance we won't ever need this work struct, so we might as well delay initializing it until just before we're going to use it. In a later patch, we'll need to intialize the work with a different function for delegation callbacks. By delaying this, we avoid having to do it twice.

Quoted 5 lines · Jeff Layton

There's a fair chance we won't ever need this work struct, so we might as well delay initializing it until just before we're going to use it. In a later patch, we'll need to intialize the work with a different function for delegation callbacks. By delaying this, we avoid having to do it twice.

When I run

	./nfs4.1/testserver.py f19:/exports/xfs/FUBAR --maketree --rundeps COMP1

I get crashes like:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810890b1>] process_one_work+0x31/0x500
PGD 0 
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl lockd sunrpc
CPU: 2 PID: 30 Comm: kworker/u8:1 Not tainted 3.16.0-rc2-00023-g30c1d16 #3051
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffff88003d9cc810 ti: ffff88003d9d0000 task.ti: ffff88003d9d0000
RIP: 0010:[<ffffffff810890b1>]  [<ffffffff810890b1>] process_one_work+0x31/0x500
RSP: 0018:ffff88003d9d3d68  EFLAGS: 00010046
RAX: 0000000000000100 RBX: ffff88003d9caf80 RCX: dead000000200200
RDX: 0000000000000100 RSI: ffff880031429b38 RDI: ffff88003d9caf80
RBP: ffff88003d9d3dd8 R08: ffff88003dcb7800 R09: 0000000001c40000
R10: 0000000000000000 R11: 000000000006b5b0 R12: ffff88003dcb7800
R13: 0000000000000000 R14: ffff880031429b38 R15: ffff88003d9caf80
FS:  0000000000000000(0000) GS:ffff88003fb00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 0000000027744000 CR4: 00000000000006e0
Stack:
 000000003d9d3d88 ffff88003dcb7818 ffff88003d9cafb0 ffff88003dcb7800
 ffff88003dcb7800 ffff88003dcb7800 ffff88003dcb7800 ffff88003d9cafb0
 ffff88003d9d3dd8 ffff88003dcb7800 ffff88003dcb7848 ffff88003d9cafb0
Call Trace:
 [<ffffffff81089b9b>] worker_thread+0x11b/0x4f0
 [<ffffffff81089a80>] ? init_pwq+0x190/0x190
 [<ffffffff81090ac4>] kthread+0xe4/0x100
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81a4ca2c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
Code: 48 89 e5 41 57 41 56 49 89 f6 41 55 45 31 ed 41 54 53 48 89 fb 48 83 ec 48 48 8b 06 4c 8b 67 48 48 89 c2 30 d2 a8 04 4c 0f 45 ea <49> 8b 45 08 48 c7 45 b8 00 00 00 00 48 c7 45 c0 00 00 00 00 8b 
RIP  [<ffffffff810890b1>] process_one_work+0x31/0x500
 RSP <ffff88003d9d3d68>

CR2: 0000000000000008

and I'm suspicious of this:

Quoted 4 lines

fs/nfsd/nfs4callback.c +0 -0

@@ -763,6 +764,8 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 

fs/nfsd/nfs4callback.c static void do_probe_callback(struct nfs4_client *clp) · line 770

Quoted 4 lines

fs/nfsd/nfs4callback.c +2 -0

+	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+
 	run_nfsd4_cb(cb);
 }

This is called all over the place, possibly multiple times on the same client, so we're calling INIT_WORK on something that may already be in use--I doubt that's safe.

I'm backing out this patch for now.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810890b1>] process_one_work+0x31/0x500
PGD 0 
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl lockd sunrpc
CPU: 2 PID: 30 Comm: kworker/u8:1 Not tainted 3.16.0-rc2-00023-g30c1d16 #3051
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffff88003d9cc810 ti: ffff88003d9d0000 task.ti: ffff88003d9d0000
RIP: 0010:[<ffffffff810890b1>]  [<ffffffff810890b1>] process_one_work+0x31/0x500
RSP: 0018:ffff88003d9d3d68  EFLAGS: 00010046
RAX: 0000000000000100 RBX: ffff88003d9caf80 RCX: dead000000200200
RDX: 0000000000000100 RSI: ffff880031429b38 RDI: ffff88003d9caf80
RBP: ffff88003d9d3dd8 R08: ffff88003dcb7800 R09: 0000000001c40000
R10: 0000000000000000 R11: 000000000006b5b0 R12: ffff88003dcb7800
R13: 0000000000000000 R14: ffff880031429b38 R15: ffff88003d9caf80
FS:  0000000000000000(0000) GS:ffff88003fb00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 0000000027744000 CR4: 00000000000006e0
Stack:
 000000003d9d3d88 ffff88003dcb7818 ffff88003d9cafb0 ffff88003dcb7800
 ffff88003dcb7800 ffff88003dcb7800 ffff88003dcb7800 ffff88003d9cafb0
 ffff88003d9d3dd8 ffff88003dcb7800 ffff88003dcb7848 ffff88003d9cafb0
Call Trace:
 [<ffffffff81089b9b>] worker_thread+0x11b/0x4f0
 [<ffffffff81089a80>] ? init_pwq+0x190/0x190
 [<ffffffff81090ac4>] kthread+0xe4/0x100
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81a4ca2c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
Code: 48 89 e5 41 57 41 56 49 89 f6 41 55 45 31 ed 41 54 53 48 89 fb 48 83 ec 48 48 8b 06 4c 8b 67 48 48 89 c2 30 d2 a8 04 4c 0f 45 ea <49> 8b 45 08 48 c7 45 b8 00 00 00 00 48 c7 45 c0 00 00 00 00 8b 
RIP  [<ffffffff810890b1>] process_one_work+0x31/0x500
 RSP <ffff88003d9d3d68>

CR2: 0000000000000008

and I'm suspicious of this:

fs/nfsd/nfs4callback.c static void do_probe_callback(struct nfs4_client *clp) · line 770

Quoted 8 lines

fs/nfsd/nfs4callback.c +2 -0

@@ -763,6 +764,8 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
+	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+
 	run_nfsd4_cb(cb);
 }

This is called all over the place, possibly multiple times on the same client, so we're calling INIT_WORK on something that may already be in use--I doubt that's safe.

I'm backing out this patch for now.

Quoted 49 lines · J. Bruce Fields

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810890b1>] process_one_work+0x31/0x500
PGD 0 
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl lockd sunrpc
CPU: 2 PID: 30 Comm: kworker/u8:1 Not tainted 3.16.0-rc2-00023-g30c1d16 #3051
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffff88003d9cc810 ti: ffff88003d9d0000 task.ti: ffff88003d9d0000
RIP: 0010:[<ffffffff810890b1>]  [<ffffffff810890b1>] process_one_work+0x31/0x500
RSP: 0018:ffff88003d9d3d68  EFLAGS: 00010046
RAX: 0000000000000100 RBX: ffff88003d9caf80 RCX: dead000000200200
RDX: 0000000000000100 RSI: ffff880031429b38 RDI: ffff88003d9caf80
RBP: ffff88003d9d3dd8 R08: ffff88003dcb7800 R09: 0000000001c40000
R10: 0000000000000000 R11: 000000000006b5b0 R12: ffff88003dcb7800
R13: 0000000000000000 R14: ffff880031429b38 R15: ffff88003d9caf80
FS:  0000000000000000(0000) GS:ffff88003fb00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 0000000027744000 CR4: 00000000000006e0
Stack:
 000000003d9d3d88 ffff88003dcb7818 ffff88003d9cafb0 ffff88003dcb7800
 ffff88003dcb7800 ffff88003dcb7800 ffff88003dcb7800 ffff88003d9cafb0
 ffff88003d9d3dd8 ffff88003dcb7800 ffff88003dcb7848 ffff88003d9cafb0
Call Trace:
 [<ffffffff81089b9b>] worker_thread+0x11b/0x4f0
 [<ffffffff81089a80>] ? init_pwq+0x190/0x190
 [<ffffffff81090ac4>] kthread+0xe4/0x100
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81a4ca2c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
Code: 48 89 e5 41 57 41 56 49 89 f6 41 55 45 31 ed 41 54 53 48 89 fb 48 83 ec 48 48 8b 06 4c 8b 67 48 48 89 c2 30 d2 a8 04 4c 0f 45 ea <49> 8b 45 08 48 c7 45 b8 00 00 00 00 48 c7 45 c0 00 00 00 00 8b 
RIP  [<ffffffff810890b1>] process_one_work+0x31/0x500
 RSP <ffff88003d9d3d68>

CR2: 0000000000000008

and I'm suspicious of this:

fs/nfsd/nfs4callback.c static void do_probe_callback(struct nfs4_client *clp) · line 770

Quoted 8 lines

fs/nfsd/nfs4callback.c +2 -0

@@ -763,6 +764,8 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
+	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+
 	run_nfsd4_cb(cb);
 }

This is called all over the place, possibly multiple times on the same client, so we're calling INIT_WORK on something that may already be in use--I doubt that's safe.

Quoted 1 line

I'm backing out this patch for now.

If that's the case all of do_probe_callback seems very fishy to me, as it scrambles all over the callback structure. I guess we need to move more of it to an init function then, and have different init functions for the different callbacks.

Quoted 50 lines · Christoph Hellwig

Quoted 49 lines · J. Bruce Fields

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810890b1>] process_one_work+0x31/0x500
PGD 0 
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl lockd sunrpc
CPU: 2 PID: 30 Comm: kworker/u8:1 Not tainted 3.16.0-rc2-00023-g30c1d16 #3051
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffff88003d9cc810 ti: ffff88003d9d0000 task.ti: ffff88003d9d0000
RIP: 0010:[<ffffffff810890b1>]  [<ffffffff810890b1>] process_one_work+0x31/0x500
RSP: 0018:ffff88003d9d3d68  EFLAGS: 00010046
RAX: 0000000000000100 RBX: ffff88003d9caf80 RCX: dead000000200200
RDX: 0000000000000100 RSI: ffff880031429b38 RDI: ffff88003d9caf80
RBP: ffff88003d9d3dd8 R08: ffff88003dcb7800 R09: 0000000001c40000
R10: 0000000000000000 R11: 000000000006b5b0 R12: ffff88003dcb7800
R13: 0000000000000000 R14: ffff880031429b38 R15: ffff88003d9caf80
FS:  0000000000000000(0000) GS:ffff88003fb00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 0000000027744000 CR4: 00000000000006e0
Stack:
 000000003d9d3d88 ffff88003dcb7818 ffff88003d9cafb0 ffff88003dcb7800
 ffff88003dcb7800 ffff88003dcb7800 ffff88003dcb7800 ffff88003d9cafb0
 ffff88003d9d3dd8 ffff88003dcb7800 ffff88003dcb7848 ffff88003d9cafb0
Call Trace:
 [<ffffffff81089b9b>] worker_thread+0x11b/0x4f0
 [<ffffffff81089a80>] ? init_pwq+0x190/0x190
 [<ffffffff81090ac4>] kthread+0xe4/0x100
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81a4ca2c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
Code: 48 89 e5 41 57 41 56 49 89 f6 41 55 45 31 ed 41 54 53 48 89 fb 48 83 ec 48 48 8b 06 4c 8b 67 48 48 89 c2 30 d2 a8 04 4c 0f 45 ea <49> 8b 45 08 48 c7 45 b8 00 00 00 00 48 c7 45 c0 00 00 00 00 8b 
RIP  [<ffffffff810890b1>] process_one_work+0x31/0x500
 RSP <ffff88003d9d3d68>

CR2: 0000000000000008

and I'm suspicious of this:

fs/nfsd/nfs4callback.c static void do_probe_callback(struct nfs4_client *clp) · line 770

Quoted 8 lines

fs/nfsd/nfs4callback.c +2 -0

@@ -763,6 +764,8 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
+	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+
 	run_nfsd4_cb(cb);
 }

This is called all over the place, possibly multiple times on the same client, so we're calling INIT_WORK on something that may already be in use--I doubt that's safe.

Quoted 6 lines

Quoted 1 line

I'm backing out this patch for now.

If that's the case all of do_probe_callback seems very fishy to me, as it scrambles all over the callback structure. I guess we need to move more of it to an init function then, and have different init functions for the different callbacks.

Taking a look.... It does look fishy, but those fields are constant, so I don't see a bug.

In delegation recall case (nfsd4_cb_recall()) those fields aren't constant, but we guarantee it's only called once.

Quoted 58 lines · On Wed, 9 Jul 2014 15:41:14 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 50 lines · Christoph Hellwig

Quoted 49 lines · J. Bruce Fields

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810890b1>] process_one_work+0x31/0x500
PGD 0 
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl lockd sunrpc
CPU: 2 PID: 30 Comm: kworker/u8:1 Not tainted 3.16.0-rc2-00023-g30c1d16 #3051
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffff88003d9cc810 ti: ffff88003d9d0000 task.ti: ffff88003d9d0000
RIP: 0010:[<ffffffff810890b1>]  [<ffffffff810890b1>] process_one_work+0x31/0x500
RSP: 0018:ffff88003d9d3d68  EFLAGS: 00010046
RAX: 0000000000000100 RBX: ffff88003d9caf80 RCX: dead000000200200
RDX: 0000000000000100 RSI: ffff880031429b38 RDI: ffff88003d9caf80
RBP: ffff88003d9d3dd8 R08: ffff88003dcb7800 R09: 0000000001c40000
R10: 0000000000000000 R11: 000000000006b5b0 R12: ffff88003dcb7800
R13: 0000000000000000 R14: ffff880031429b38 R15: ffff88003d9caf80
FS:  0000000000000000(0000) GS:ffff88003fb00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 0000000027744000 CR4: 00000000000006e0
Stack:
 000000003d9d3d88 ffff88003dcb7818 ffff88003d9cafb0 ffff88003dcb7800
 ffff88003dcb7800 ffff88003dcb7800 ffff88003dcb7800 ffff88003d9cafb0
 ffff88003d9d3dd8 ffff88003dcb7800 ffff88003dcb7848 ffff88003d9cafb0
Call Trace:
 [<ffffffff81089b9b>] worker_thread+0x11b/0x4f0
 [<ffffffff81089a80>] ? init_pwq+0x190/0x190
 [<ffffffff81090ac4>] kthread+0xe4/0x100
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff81a4ca2c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810909e0>] ? __init_kthread_worker+0x70/0x70
Code: 48 89 e5 41 57 41 56 49 89 f6 41 55 45 31 ed 41 54 53 48 89 fb 48 83 ec 48 48 8b 06 4c 8b 67 48 48 89 c2 30 d2 a8 04 4c 0f 45 ea <49> 8b 45 08 48 c7 45 b8 00 00 00 00 48 c7 45 c0 00 00 00 00 8b 
RIP  [<ffffffff810890b1>] process_one_work+0x31/0x500
 RSP <ffff88003d9d3d68>

CR2: 0000000000000008

and I'm suspicious of this:

fs/nfsd/nfs4callback.c static void do_probe_callback(struct nfs4_client *clp) · line 770

Quoted 8 lines

fs/nfsd/nfs4callback.c +2 -0

@@ -763,6 +764,8 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
+	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+
 	run_nfsd4_cb(cb);
 }

This is called all over the place, possibly multiple times on the same client, so we're calling INIT_WORK on something that may already be in use--I doubt that's safe.

Quoted 6 lines

Quoted 1 line

I'm backing out this patch for now.

If that's the case all of do_probe_callback seems very fishy to me, as it scrambles all over the callback structure. I guess we need to move more of it to an init function then, and have different init functions for the different callbacks.

Quoted 6 lines

Taking a look.... It does look fishy, but those fields are constant, so I don't see a bug.

In delegation recall case (nfsd4_cb_recall()) those fields aren't constant, but we guarantee it's only called once.

Yes. I'm inclined to leave well enough alone on this and to leave any cleanup in this area to Christoph since he said he was overhauling that code anyway.

I didn't see where you had reverted the patch in your repo,

so I went
ahead and did it and then rebased the series on top of the revert. There were some merge conflicts, but I at least was able to get rid of the double INIT_WORK calls in the case of a delegation (and do some related cleanup in this area):

http://git.samba.org/?p=jlayton/linux.git;a=commitdiff;h=60b61375d6a84e74bf1c3c7c230712721e14773d
http://git.samba.org/?p=jlayton/linux.git;a=commitdiff;h=491192c3e6f0966722c34ba36adfde7575640544

Unfortunately there are a few (fairly trivial) merge conflicts later in the series due to this change. Bruce, do you want me to repost the whole set, or would you rather just cherry-pick them from my updated branch?

I didn't see where you had reverted the patch in your repo, so I went ahead and did it and then rebased the series on top of the revert. There were some merge conflicts, but I at least was able to get rid of the double INIT_WORK calls in the case of a delegation (and do some related cleanup in this area):

Unfortunately there are a few (fairly trivial) merge conflicts later in the series due to this change. Bruce, do you want me to repost the whole set, or would you rather just cherry-pick them from my updated branch?

Quoted 4 lines

Unfortunately there are a few (fairly trivial) merge conflicts later in the series due to this change. Bruce, do you want me to repost the whole set, or would you rather just cherry-pick them from my updated branch?

Can you resend just the whole fi_fds and access/deny mode patches as a series for the next step? This seems useful and complicated enough to do a standalone review. I also don't think the additional few spinlocks would have enough performance impact to avoid them until the big client lock is gone, although all those logic changes could probably be done easily enough without the new locking if someone cared enough (I don't).

state_lock is a heavily contended global lock. We don't want to grab that while simultaneously holding the inode->i_lock.

Add a new per-nfs4_file lock that we can use to protect the per-nfs4_file delegation list. Hold that while walking the list in the break_deleg callback and queue the workqueue job for each one.

The workqueue job can then take the state_lock and do the list manipulations without the i_lock being held prior to starting the rpc call.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfsd/nfs4callback.c | 28 +++++++++++++++++++++-------
 fs/nfsd/nfs4state.c    | 43 ++++++++++++++++++++++++++++---------------
 fs/nfsd/state.h        |  2 ++
 3 files changed, 51 insertions(+), 22 deletions(-)

fs/nfsd/nfs4callback.c +21 -7

index 00cb9b7a75f6..cba4ca375f5e 100644
@@ -43,7 +43,7 @@
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
 static void nfsd4_mark_cb_fault(struct nfs4_client *, int reason);
-static void nfsd4_do_callback_rpc(struct work_struct *w);
+static void nfsd4_run_cb_null(struct work_struct *w);
 
 #define NFSPROC4_CB_NULL 0
 #define NFSPROC4_CB_COMPOUND 1

@@ -764,7 +764,7 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
-	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+	INIT_WORK(&cb->cb_work, nfsd4_run_cb_null);
 
 	run_nfsd4_cb(cb);
 }

@@ -936,7 +936,7 @@ void nfsd4_shutdown_callback(struct nfs4_client *clp)
 	set_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags);
 	/*
 	 * Note this won't actually result in a null callback;
-	 * instead, nfsd4_do_callback_rpc() will detect the killed
+	 * instead, nfsd4_run_cb_null() will detect the killed
 	 * client, destroy the rpc client, and stop:
 	 */
 	do_probe_callback(clp);

@@ -1014,9 +1014,8 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 		run_nfsd4_cb(cb);
 }
 
-static void nfsd4_do_callback_rpc(struct work_struct *w)
+static void nfsd4_run_callback_rpc(struct nfsd4_callback *cb)
 {
-	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback, cb_work);
 	struct nfs4_client *clp = cb->cb_clp;
 	struct rpc_clnt *clnt;
 

@@ -1034,6 +1033,22 @@ static void nfsd4_do_callback_rpc(struct work_struct *w)
 			cb->cb_ops, cb);
 }
 
+static void nfsd4_run_cb_null(struct work_struct *w)
+{
+	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback,
+							cb_work);
+	nfsd4_run_callback_rpc(cb);
+}
+
+static void nfsd4_run_cb_recall(struct work_struct *w)
+{
+	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback,
+							cb_work);
+
+	nfsd4_prepare_cb_recall(cb->cb_op);
+	nfsd4_run_callback_rpc(cb);
+}
+
 void nfsd4_cb_recall(struct nfs4_delegation *dp)
 {
 	struct nfsd4_callback *cb = &dp->dl_recall;

@@ -1050,8 +1065,7 @@ void nfsd4_cb_recall(struct nfs4_delegation *dp)
 
 	INIT_LIST_HEAD(&cb->cb_per_client);
 	cb->cb_done = true;
-
-	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+	INIT_WORK(&cb->cb_work, nfsd4_run_cb_recall);
 
 	run_nfsd4_cb(&dp->dl_recall);
 }

fs/nfsd/nfs4state.c +28 -15

index b49b46b0ce23..461229a01963 100644
@@ -513,7 +513,9 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	lockdep_assert_held(&state_lock);
 
 	dp->dl_stid.sc_type = NFS4_DELEG_STID;
+	spin_lock(&fp->fi_lock);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
+	spin_unlock(&fp->fi_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 }
 

@@ -521,14 +523,18 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 static void
 unhash_delegation(struct nfs4_delegation *dp)
 {
+	struct nfs4_file *fp = dp->dl_file;
+
 	spin_lock(&state_lock);
 	list_del_init(&dp->dl_perclnt);
-	list_del_init(&dp->dl_perfile);
 	list_del_init(&dp->dl_recall_lru);
+	spin_lock(&fp->fi_lock);
+	list_del_init(&dp->dl_perfile);
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	if (dp->dl_file) {
-		nfs4_put_deleg_lease(dp->dl_file);
-		put_nfs4_file(dp->dl_file);
+	if (fp) {
+		nfs4_put_deleg_lease(fp);
+		put_nfs4_file(fp);
 		dp->dl_file = NULL;
 	}
 }

@@ -2612,6 +2618,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	lockdep_assert_held(&state_lock);
 
 	atomic_set(&fp->fi_ref, 1);
+	spin_lock_init(&fp->fi_lock);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
 	ihold(ino);

@@ -2857,26 +2864,32 @@ out:
 	return ret;
 }
 
-static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
+void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp)
 {
 	struct nfs4_client *clp = dp->dl_stid.sc_client;
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
-	lockdep_assert_held(&state_lock);
+	/*
+	 * We can't do this in nfsd_break_deleg_cb because it is
+	 * already holding inode->i_lock
+	 */
+	spin_lock(&state_lock);
+	if (list_empty(&dp->dl_recall_lru)) {
+		dp->dl_time = get_seconds();
+		list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
+	}
+	spin_unlock(&state_lock);
+}
+
+static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
+{
 	/* We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
 	 * callback (and since the lease code is serialized by the kernel
 	 * lock) we know the server hasn't removed the lease yet, we know
 	 * it's safe to take a reference: */
 	atomic_inc(&dp->dl_count);
-
-	list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
-
-	/* Only place dl_time is set; protected by i_lock: */
-	dp->dl_time = get_seconds();
-
 	block_delegations(&dp->dl_fh);
-
 	nfsd4_cb_recall(dp);
 }
 

@@ -2901,11 +2914,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
 	 */
 	fl->fl_break_time = 0;
 
-	spin_lock(&state_lock);
 	fp->fi_had_conflict = true;
+	spin_lock(&fp->fi_lock);
 	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
 		nfsd_break_one_deleg(dp);
-	spin_unlock(&state_lock);
+	spin_unlock(&fp->fi_lock);
 }
 
 static

fs/nfsd/state.h +2 -0

index 9447f86f2778..7d6ba06c1abe 100644
@@ -382,6 +382,7 @@ static inline struct nfs4_lockowner * lockowner(struct nfs4_stateowner *so)
 /* nfs4_file: a file opened by some number of (open) nfs4_stateowners. */
 struct nfs4_file {
 	atomic_t		fi_ref;
+	spinlock_t		fi_lock;
 	struct hlist_node       fi_hash;    /* hash by "struct inode *" */
 	struct list_head        fi_stateids;
 	struct list_head	fi_delegations;

@@ -471,6 +472,7 @@ extern void nfsd4_cb_recall(struct nfs4_delegation *dp);
 extern int nfsd4_create_callback_queue(void);
 extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
+extern void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);

Signature · 1 line

1.9.3

Quoted 10 lines · Jeff Layton

state_lock is a heavily contended global lock. We don't want to grab that while simultaneously holding the inode->i_lock.

Add a new per-nfs4_file lock that we can use to protect the per-nfs4_file delegation list. Hold that while walking the list in the break_deleg callback and queue the workqueue job for each one.

The workqueue job can then take the state_lock and do the list manipulations without the i_lock being held prior to starting the rpc call.

The code tends to assume that the callback thread only works with the delegation struct itself and puts it when done but doesn't otherwise touch other state.

I wonder how this interacts with state shutdown....

E.g. in nfs4_state_shutdown_net() we walk the dl_recall_lru and destroy everything we find, but this callback workqueue is still running so I think another delegation could get added to that list after this? Does that cause bugs?

And it'd also be worth checking delegreturn and destroy_client.

Maybe there's no bug, or they just need to flush the workqueue at the appropriate point.

There's also a preexisting expire_client/laundromat vs break race:

- expire_client/laundromat adds a delegation to its local reaplist using the same dl_recall_lru field that a delegation uses to track its position on the recall lru and drops the state lock.

- a concurrent break_lease adds the delegation to the lru.

- expire/client/laundromat then walks it reaplist and sees the lru head as just another delegation on the list....

Possibly unrelated, but it might be good to fix that first.

--b.

Quoted 209 lines

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfsd/nfs4callback.c | 28 +++++++++++++++++++++-------
 fs/nfsd/nfs4state.c    | 43 ++++++++++++++++++++++++++++---------------
 fs/nfsd/state.h        |  2 ++
 3 files changed, 51 insertions(+), 22 deletions(-)

fs/nfsd/nfs4callback.c +21 -7

index 00cb9b7a75f6..cba4ca375f5e 100644
@@ -43,7 +43,7 @@
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
 static void nfsd4_mark_cb_fault(struct nfs4_client *, int reason);
-static void nfsd4_do_callback_rpc(struct work_struct *w);
+static void nfsd4_run_cb_null(struct work_struct *w);
 
 #define NFSPROC4_CB_NULL 0
 #define NFSPROC4_CB_COMPOUND 1

@@ -764,7 +764,7 @@ static void do_probe_callback(struct nfs4_client *clp)
 
 	cb->cb_ops = &nfsd4_cb_probe_ops;
 
-	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+	INIT_WORK(&cb->cb_work, nfsd4_run_cb_null);
 
 	run_nfsd4_cb(cb);
 }

@@ -936,7 +936,7 @@ void nfsd4_shutdown_callback(struct nfs4_client *clp)
 	set_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags);
 	/*
 	 * Note this won't actually result in a null callback;
-	 * instead, nfsd4_do_callback_rpc() will detect the killed
+	 * instead, nfsd4_run_cb_null() will detect the killed
 	 * client, destroy the rpc client, and stop:
 	 */
 	do_probe_callback(clp);

@@ -1014,9 +1014,8 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 		run_nfsd4_cb(cb);
 }
 
-static void nfsd4_do_callback_rpc(struct work_struct *w)
+static void nfsd4_run_callback_rpc(struct nfsd4_callback *cb)
 {
-	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback, cb_work);
 	struct nfs4_client *clp = cb->cb_clp;
 	struct rpc_clnt *clnt;
 

@@ -1034,6 +1033,22 @@ static void nfsd4_do_callback_rpc(struct work_struct *w)
 			cb->cb_ops, cb);
 }
 
+static void nfsd4_run_cb_null(struct work_struct *w)
+{
+	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback,
+							cb_work);
+	nfsd4_run_callback_rpc(cb);
+}
+
+static void nfsd4_run_cb_recall(struct work_struct *w)
+{
+	struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback,
+							cb_work);
+
+	nfsd4_prepare_cb_recall(cb->cb_op);
+	nfsd4_run_callback_rpc(cb);
+}
+
 void nfsd4_cb_recall(struct nfs4_delegation *dp)
 {
 	struct nfsd4_callback *cb = &dp->dl_recall;

@@ -1050,8 +1065,7 @@ void nfsd4_cb_recall(struct nfs4_delegation *dp)
 
 	INIT_LIST_HEAD(&cb->cb_per_client);
 	cb->cb_done = true;
-
-	INIT_WORK(&cb->cb_work, nfsd4_do_callback_rpc);
+	INIT_WORK(&cb->cb_work, nfsd4_run_cb_recall);
 
 	run_nfsd4_cb(&dp->dl_recall);
 }

fs/nfsd/nfs4state.c +28 -15

index b49b46b0ce23..461229a01963 100644
@@ -513,7 +513,9 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	lockdep_assert_held(&state_lock);
 
 	dp->dl_stid.sc_type = NFS4_DELEG_STID;
+	spin_lock(&fp->fi_lock);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
+	spin_unlock(&fp->fi_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 }
 

@@ -521,14 +523,18 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 static void
 unhash_delegation(struct nfs4_delegation *dp)
 {
+	struct nfs4_file *fp = dp->dl_file;
+
 	spin_lock(&state_lock);
 	list_del_init(&dp->dl_perclnt);
-	list_del_init(&dp->dl_perfile);
 	list_del_init(&dp->dl_recall_lru);
+	spin_lock(&fp->fi_lock);
+	list_del_init(&dp->dl_perfile);
+	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	if (dp->dl_file) {
-		nfs4_put_deleg_lease(dp->dl_file);
-		put_nfs4_file(dp->dl_file);
+	if (fp) {
+		nfs4_put_deleg_lease(fp);
+		put_nfs4_file(fp);
 		dp->dl_file = NULL;
 	}
 }

@@ -2612,6 +2618,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	lockdep_assert_held(&state_lock);
 
 	atomic_set(&fp->fi_ref, 1);
+	spin_lock_init(&fp->fi_lock);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
 	ihold(ino);

@@ -2857,26 +2864,32 @@ out:
 	return ret;
 }
 
-static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
+void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp)
 {
 	struct nfs4_client *clp = dp->dl_stid.sc_client;
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
-	lockdep_assert_held(&state_lock);
+	/*
+	 * We can't do this in nfsd_break_deleg_cb because it is
+	 * already holding inode->i_lock
+	 */
+	spin_lock(&state_lock);
+	if (list_empty(&dp->dl_recall_lru)) {
+		dp->dl_time = get_seconds();
+		list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
+	}
+	spin_unlock(&state_lock);
+}
+
+static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
+{
 	/* We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
 	 * callback (and since the lease code is serialized by the kernel
 	 * lock) we know the server hasn't removed the lease yet, we know
 	 * it's safe to take a reference: */
 	atomic_inc(&dp->dl_count);
-
-	list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
-
-	/* Only place dl_time is set; protected by i_lock: */
-	dp->dl_time = get_seconds();
-
 	block_delegations(&dp->dl_fh);
-
 	nfsd4_cb_recall(dp);
 }
 

@@ -2901,11 +2914,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)
 	 */
 	fl->fl_break_time = 0;
 
-	spin_lock(&state_lock);
 	fp->fi_had_conflict = true;
+	spin_lock(&fp->fi_lock);
 	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile)
 		nfsd_break_one_deleg(dp);
-	spin_unlock(&state_lock);
+	spin_unlock(&fp->fi_lock);
 }
 
 static

fs/nfsd/state.h +2 -0

index 9447f86f2778..7d6ba06c1abe 100644
@@ -382,6 +382,7 @@ static inline struct nfs4_lockowner * lockowner(struct nfs4_stateowner *so)
 /* nfs4_file: a file opened by some number of (open) nfs4_stateowners. */
 struct nfs4_file {
 	atomic_t		fi_ref;
+	spinlock_t		fi_lock;
 	struct hlist_node       fi_hash;    /* hash by "struct inode *" */
 	struct list_head        fi_stateids;
 	struct list_head	fi_delegations;

@@ -471,6 +472,7 @@ extern void nfsd4_cb_recall(struct nfs4_delegation *dp);
 extern int nfsd4_create_callback_queue(void);
 extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
+extern void nfsd4_prepare_cb_recall(struct nfs4_delegation *dp);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);

Signature · 1 line

1.9.3

Quoted 16 lines · On Wed, 2 Jul 2014 17:14:24 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 10 lines · Jeff Layton

state_lock is a heavily contended global lock. We don't want to grab that while simultaneously holding the inode->i_lock.

Add a new per-nfs4_file lock that we can use to protect the per-nfs4_file delegation list. Hold that while walking the list in the break_deleg callback and queue the workqueue job for each one.

The workqueue job can then take the state_lock and do the list manipulations without the i_lock being held prior to starting the rpc call.

The code tends to assume that the callback thread only works with the delegation struct itself and puts it when done but doesn't otherwise touch other state.

Quoted 7 lines

I wonder how this interacts with state shutdown....

E.g. in nfs4_state_shutdown_net() we walk the dl_recall_lru and destroy everything we find, but this callback workqueue is still running so I think another delegation could get added to that list after this? Does that cause bugs?

I don't see what would prevent those bugs today and I'm unclear on why you think this patch will make things worse. All this patch really does is protect the dl_perfile list manipulations with a new per-nfs4_file lock, and have it lock that when walking the perfile list. Then it has the workqueue callback do the work of adding it to the del_recall_lru list.

Why wouldn't you have the same problem if you do the queueing to the LRU list in break_deleg codepath with the code as it is today?

That said, the delegation code is horribly complex so it's possible I've missed something here.

Quoted 12 lines

And it'd also be worth checking delegreturn and destroy_client.

Maybe there's no bug, or they just need to flush the workqueue at the appropriate point.

There's also a preexisting expire_client/laundromat vs break race:

- expire_client/laundromat adds a delegation to its local reaplist using the same dl_recall_lru field that a delegation uses to track its position on the recall lru and drops the state lock.

Quoted 9 lines

- a concurrent break_lease adds the delegation to the lru.

- expire/client/laundromat then walks it reaplist and sees the lru head as just another delegation on the list....

Possibly unrelated, but it might be good to fix that first.

--b.

I was thinking that that would be fixed up in a later patch:

nfsd: Fix delegation revocation

...but now I'm not so sure. Once you drop the fi_lock, you could end up with the race above.

Honestly, the locking around the delegation code is still a mess, even with this series. I don't care much for the state_lock/recall_lock at all. It seems like we ought to be able to do something more granular there. Let me give it some thought -- maybe I can come up with a better way to handle this.

From: Trond Myklebust <[email protected]>

Minor cleanup that should introduce no behavioral changes.

Currently this function just unhashes the stateid and leaves the caller to do the work of the CLOSE processing.

Change nfsd4_close_open_stateid so that it handles doing all of the work of closing a stateid. Move the handling of the unhashed stateid into it instead of doing that work in nfsd4_close. This will help isolate some coming changes to stateid handling from nfsd4_close.

Signed-off-by Trond Myklebust <[email protected]>

Now that we know that we won't have several lockowners with the same, owner->data, we can simplify nfsd4_release_lockowner and get rid of the lo_list in the process.

Signed-off-by Jeff Layton <[email protected]>

We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid.

In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it.

Signed-off-by Trond Myklebust <[email protected]>

fs/nfsd/nfs4state.c +0 -0

@@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
 	return nfserr_bad_seqid;
 }
 

fs/nfsd/nfs4state.c +29 -21

+		return nfs_ok;
+	}
+
+	if (STALE_CLIENTID(clid, nn))
+		return nfserr_stale_clientid;
+
+	/*
+	 * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
+	 * cached already then we know this is for is for v4.0 and "sessions"
+	 * will be false.
+	 */
+	WARN_ON_ONCE(cstate->session);
+	found = find_confirmed_client(clid, false, nn);
+	if (!found)
+		return nfserr_expired;
+
+	/* Cache the nfs4_client in cstate! */
+	cstate->clp = found;
+	atomic_inc(&found->cl_refcount);
+	return nfs_ok;
+}
+
 __be32
 nfsd4_process_open1(struct nfsd4_compound_state *cstate,
 		    struct nfsd4_open *open, struct nfsd_net *nn)

@@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status)
 		free_generic_stateid(open->op_stp);
 }
 
-static __be32 lookup_clientid(clientid_t *clid, bool session, struct nfsd_net *nn, struct nfs4_client **clp)
-{
-	struct nfs4_client *found;
-
-	if (STALE_CLIENTID(clid, nn))
-		return nfserr_stale_clientid;
-	found = find_confirmed_client(clid, session, nn);
-	if (clp)
-		*clp = found;
-	return found ? nfs_ok : nfserr_expired;
-}
-
 __be32
 nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	    clientid_t *clid)

@@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfs4_lock_state();
 	dprintk("process_renew(%08x/%08x): starting\n", 
 			clid->cl_boot, clid->cl_id);
-	status = lookup_clientid(clid, cstate->minorversion, nn, &clp);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;
+	clp = cstate->clp;
 	status = nfserr_cb_path_down;
 	if (!list_empty(&clp->cl_delegations)
 			&& clp->cl_cb_state != NFSD4_CB_UP)

@@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn)
 {
-	struct nfs4_client *cl;
 	__be32 status;
-	bool sessions = cstate->minorversion != 0;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
 		return nfserr_bad_stateid;
-	status = lookup_clientid(&stateid->si_opaque.so_clid, sessions,
-							nn, &cl);
+	status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn);
 	if (status == nfserr_stale_clientid) {
-		if (sessions)
+		if (cstate->session)
 			return nfserr_bad_stateid;
 		return nfserr_stale_stateid;
 	}
 	if (status)
 		return status;
-	*s = find_stateid_by_type(cl, stateid, typemask);
+	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
 	if (!*s)
 		return nfserr_bad_stateid;
 	return nfs_ok;

@@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfs4_lock_state();
 
 	if (!nfsd4_has_session(cstate)) {
-		status = lookup_clientid(&lockt->lt_clientid, false, nn, NULL);
+		status = lookup_clientid(&lockt->lt_clientid, cstate, nn);
 		if (status)
 			goto out;
 	}

@@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 
 	nfs4_lock_state();
 
-	status = lookup_clientid(clid, cstate->minorversion, nn, NULL);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;
 

fs/nfsd/xdr4.h +1 -0

index 7d8af164523b..312f6483a48e 100644
@@ -55,6 +55,7 @@ struct nfsd4_compound_state {
 	struct svc_fh		current_fh;
 	struct svc_fh		save_fh;
 	struct nfs4_stateowner	*replay_owner;
+	struct nfs4_client	*clp;
 	/* For sessions DRC */
 	struct nfsd4_session	*session;
 	struct nfsd4_slot	*slot;

Signature · 1 line

1.9.3

Quoted 4 lines · Jeff Layton

fs/nfsd/nfs4state.c +0 -0

@@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
 	return nfserr_bad_seqid;
 }
 

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 10 lines

fs/nfsd/nfs4state.c +10 -0

+static __be32 lookup_clientid(clientid_t *clid,
+		struct nfsd4_compound_state *cstate,
+		struct nfsd_net *nn)
+{
+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;

That's new behavior, isn't it?

But sending a single compound that references multiple clients sounds nuts even in the 4.0 case, OK. Applying.

(And I've merged all but the delegation locking change up through here so far.)

--b.

Quoted 121 lines

fs/nfsd/nfs4state.c +8 -0

+		return nfs_ok;
+	}
+
+	if (STALE_CLIENTID(clid, nn))
+		return nfserr_stale_clientid;
+
+	/*
+	 * For v4.1+ we get the client in the SEQUENCE op. If we

don't have one

(quoted diff) +1 -0

+	 * cached already then we know this is for is for v4.0 and

"sessions"

fs/nfsd/nfs4state.c +13 -11

+	 * will be false.
+	 */
+	WARN_ON_ONCE(cstate->session);
+	found = find_confirmed_client(clid, false, nn);
+	if (!found)
+		return nfserr_expired;
+
+	/* Cache the nfs4_client in cstate! */
+	cstate->clp = found;
+	atomic_inc(&found->cl_refcount);
+	return nfs_ok;
+}
+
 __be32
 nfsd4_process_open1(struct nfsd4_compound_state *cstate,
 		    struct nfsd4_open *open, struct nfsd_net *nn)

@@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct
 nfsd4_open *open, __be32 status) free_generic_stateid(open->op_stp);
 }
 
-static __be32 lookup_clientid(clientid_t *clid, bool session,
 struct nfsd_net *nn, struct nfs4_client **clp) -{
-	struct nfs4_client *found;
-
-	if (STALE_CLIENTID(clid, nn))
-		return nfserr_stale_clientid;
-	found = find_confirmed_client(clid, session, nn);
-	if (clp)
-		*clp = found;
-	return found ? nfs_ok : nfserr_expired;
-}
-
 __be32
 nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state

*cstate, clientid_t *clid)

fs/nfsd/nfs4state.c +2 -1

@@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct
 nfsd4_compound_state *cstate, nfs4_lock_state();
 	dprintk("process_renew(%08x/%08x): starting\n", 
 			clid->cl_boot, clid->cl_id);
-	status = lookup_clientid(clid, cstate->minorversion, nn,
 &clp);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;
+	clp = cstate->clp;
 	status = nfserr_cb_path_down;
 	if (!list_empty(&clp->cl_delegations)

fs/nfsd/nfs4state.c +0 -0

 			&& clp->cl_cb_state != NFSD4_CB_UP)

@@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct

nfsd4_compound_state *cstate, stateid_t *stateid, unsigned char typemask, struct nfs4_stid **s, struct nfsd_net *nn)
{

fs/nfsd/nfs4state.c +0 -3

-	struct nfs4_client *cl;
 	__be32 status;
-	bool sessions = cstate->minorversion != 0;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
 		return nfserr_bad_stateid;
-	status = lookup_clientid(&stateid->si_opaque.so_clid,

sessions,

(quoted diff) +1 -1

-							nn, &cl);
+	status = lookup_clientid(&stateid->si_opaque.so_clid,

cstate, nn); if (status == nfserr_stale_clientid) {

fs/nfsd/nfs4state.c +3 -3

-		if (sessions)
+		if (cstate->session)
 			return nfserr_bad_stateid;
 		return nfserr_stale_stateid;
 	}
 	if (status)
 		return status;
-	*s = find_stateid_by_type(cl, stateid, typemask);
+	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
 	if (!*s)
 		return nfserr_bad_stateid;
 	return nfs_ok;

@@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct
 nfsd4_compound_state *cstate, nfs4_lock_state();
 
 	if (!nfsd4_has_session(cstate)) {
-		status = lookup_clientid(&lockt->lt_clientid,
 false, nn, NULL);
+		status = lookup_clientid(&lockt->lt_clientid,
 cstate, nn); if (status)
 			goto out;

fs/nfsd/nfs4state.c +1 -1

 	}

@@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst
 *rqstp, 
 	nfs4_lock_state();
 
-	status = lookup_clientid(clid, cstate->minorversion, nn,
 NULL);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;

fs/nfsd/xdr4.h +1 -0

index 7d8af164523b..312f6483a48e 100644
@@ -55,6 +55,7 @@ struct nfsd4_compound_state {
 	struct svc_fh		current_fh;
 	struct svc_fh		save_fh;
 	struct nfs4_stateowner	*replay_owner;
+	struct nfs4_client	*clp;
 	/* For sessions DRC */
 	struct nfsd4_session	*session;
 	struct nfsd4_slot	*slot;

Signature · 1 line

1.9.3

Quoted 5 lines · On Thu, 3 Jul 2014 11:18:19 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 4 lines · Jeff Layton

fs/nfsd/nfs4state.c +0 -0

@@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
 	return nfserr_bad_seqid;
 }
 

Quoted 13 lines

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 10 lines

fs/nfsd/nfs4state.c +10 -0

+static __be32 lookup_clientid(clientid_t *clid,
+		struct nfsd4_compound_state *cstate,
+		struct nfsd_net *nn)
+{
+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;

That's new behavior, isn't it?

Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it).

Quoted 22 lines · Jeff Layton

Quoted 18 lines · On Thu, 3 Jul 2014 11:18:19 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 4 lines · Jeff Layton

fs/nfsd/nfs4state.c +0 -0

@@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
 	return nfserr_bad_seqid;
 }
 

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 10 lines

fs/nfsd/nfs4state.c +10 -0

+static __be32 lookup_clientid(clientid_t *clid,
+		struct nfsd4_compound_state *cstate,
+		struct nfsd_net *nn)
+{
+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;

That's new behavior, isn't it?

Quoted 4 lines

Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it).

We could ask for a clarification in 3530bis if there's not already something there that clearly forbids this, but I'm not sure if it's even worth it.

Quoted 10 lines

Quoted 9 lines

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 6 lines

fs/nfsd/nfs4state.c +6 -0

+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;

That's new behavior, isn't it?

Quoted 4 lines

Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it).

We could ask for a clarification in 3530bis if there's not already something there that clearly forbids this, but I'm not sure if it's even worth it.

Quoted 15 lines · J. Bruce Fields

Quoted 14 lines

Quoted 9 lines

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 6 lines

fs/nfsd/nfs4state.c +6 -0

+       struct nfs4_client *found;
+
+       if (cstate->clp) {
+               found = cstate->clp;
+               if (!same_clid(&found->cl_clientid, clid))
+                       return nfserr_stale_clientid;

That's new behavior, isn't it?

Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it).

Quoted 3 lines

We could ask for a clarification in 3530bis if there's not already something there that clearly forbids this, but I'm not sure if it's even worth it.

Or just handle it and be done with me. After all we'd just need to put the existing client, and store the new one in the cstate.

Quoted 15 lines · J. Bruce Fields

Quoted 14 lines

Quoted 9 lines

fs/nfsd/nfs4state.c static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4 · line 3021

Quoted 6 lines

fs/nfsd/nfs4state.c +6 -0

+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;

That's new behavior, isn't it?

Yeah, I suppose it is, but it's hard to understand a valid use-case for sending multiple ops in a compound with different clientids. Certainly no well-behaved client would do that, would it? (Hmm, that might be an interesting pynfs test, come to think of it).

Quoted 3 lines

We could ask for a clarification in 3530bis if there's not already something there that clearly forbids this, but I'm not sure if it's even worth it.

Or just handle it and be done with me. After all we'd just need to put the existing client, and store the new one in the cstate.

We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid.

In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it.

Signed-off-by Trond Myklebust <[email protected]>

---
 fs/nfsd/nfs4state.c | 74 +++++++++++++++++++++++++++++++++++++----------------
 fs/nfsd/xdr4.h      |  1 +
 2 files changed, 53 insertions(+), 22 deletions(-)

fs/nfsd/nfs4state.c +52 -22

index e9bb39ecebae..4d162a0e4c6d 100644
@@ -189,6 +189,15 @@ static void put_client_renew_locked(struct nfs4_client *clp)
 		renew_client_locked(clp);
 }
 
+static void put_client_renew(struct nfs4_client *clp)
+{
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+
+	spin_lock(&nn->client_lock);
+	put_client_renew_locked(clp);
+	spin_unlock(&nn->client_lock);
+}
+
 static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses)
 {
 	__be32 status;

@@ -2395,6 +2404,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 			goto out_put_session;
 		cstate->slot = slot;
 		cstate->session = session;
+		cstate->clp = clp;
 		/* Return the cached reply status and set cstate->status
 		 * for nfsd4_proc_compound processing */
 		status = nfsd4_replay_cache_entry(resp, seq);

@@ -2429,6 +2439,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 
 	cstate->slot = slot;
 	cstate->session = session;
+	cstate->clp = clp;
 
 out:
 	switch (clp->cl_cb_state) {

@@ -2465,7 +2476,8 @@ nfsd4_sequence_done(struct nfsd4_compoundres *resp)
 		}
 		/* Drop session reference that was taken in nfsd4_sequence() */
 		nfsd4_put_session(cs->session);
-	}
+	} else if (cs->clp)
+		put_client_renew(cs->clp);
 }
 
 __be32

@@ -2997,6 +3009,38 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
 	return nfserr_bad_seqid;
 }
 
+static __be32 lookup_clientid(clientid_t *clid,
+		struct nfsd4_compound_state *cstate,
+		struct nfsd_net *nn)
+{
+	struct nfs4_client *found;
+
+	if (cstate->clp) {
+		found = cstate->clp;
+		if (!same_clid(&found->cl_clientid, clid))
+			return nfserr_stale_clientid;
+		return nfs_ok;
+	}
+
+	if (STALE_CLIENTID(clid, nn))
+		return nfserr_stale_clientid;
+
+	/*
+	 * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
+	 * cached already then we know this is for is for v4.0 and "sessions"
+	 * will be false.
+	 */
+	WARN_ON_ONCE(cstate->session);
+	found = find_confirmed_client(clid, false, nn);
+	if (!found)
+		return nfserr_expired;
+
+	/* Cache the nfs4_client in cstate! */
+	cstate->clp = found;
+	atomic_inc(&found->cl_refcount);
+	return nfs_ok;
+}
+
 __be32
 nfsd4_process_open1(struct nfsd4_compound_state *cstate,
 		    struct nfsd4_open *open, struct nfsd_net *nn)

@@ -3509,18 +3553,6 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status)
 		free_generic_stateid(open->op_stp);
 }
 
-static __be32 lookup_clientid(clientid_t *clid, bool session, struct nfsd_net *nn, struct nfs4_client **clp)
-{
-	struct nfs4_client *found;
-
-	if (STALE_CLIENTID(clid, nn))
-		return nfserr_stale_clientid;
-	found = find_confirmed_client(clid, session, nn);
-	if (clp)
-		*clp = found;
-	return found ? nfs_ok : nfserr_expired;
-}
-
 __be32
 nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	    clientid_t *clid)

@@ -3532,9 +3564,10 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfs4_lock_state();
 	dprintk("process_renew(%08x/%08x): starting\n", 
 			clid->cl_boot, clid->cl_id);
-	status = lookup_clientid(clid, cstate->minorversion, nn, &clp);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;
+	clp = cstate->clp;
 	status = nfserr_cb_path_down;
 	if (!list_empty(&clp->cl_delegations)
 			&& clp->cl_cb_state != NFSD4_CB_UP)

@@ -3797,22 +3830,19 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn)
 {
-	struct nfs4_client *cl;
 	__be32 status;
-	bool sessions = cstate->minorversion != 0;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
 		return nfserr_bad_stateid;
-	status = lookup_clientid(&stateid->si_opaque.so_clid, sessions,
-							nn, &cl);
+	status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn);
 	if (status == nfserr_stale_clientid) {
-		if (sessions)
+		if (cstate->session)
 			return nfserr_bad_stateid;
 		return nfserr_stale_stateid;
 	}
 	if (status)
 		return status;
-	*s = find_stateid_by_type(cl, stateid, typemask);
+	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
 	if (!*s)
 		return nfserr_bad_stateid;
 	return nfs_ok;

@@ -4662,7 +4692,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfs4_lock_state();
 
 	if (!nfsd4_has_session(cstate)) {
-		status = lookup_clientid(&lockt->lt_clientid, false, nn, NULL);
+		status = lookup_clientid(&lockt->lt_clientid, cstate, nn);
 		if (status)
 			goto out;
 	}

@@ -4831,7 +4861,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 
 	nfs4_lock_state();
 
-	status = lookup_clientid(clid, cstate->minorversion, nn, NULL);
+	status = lookup_clientid(clid, cstate, nn);
 	if (status)
 		goto out;
 

fs/nfsd/xdr4.h +1 -0

index 7d8af164523b..312f6483a48e 100644
@@ -55,6 +55,7 @@ struct nfsd4_compound_state {
 	struct svc_fh		current_fh;
 	struct svc_fh		save_fh;
 	struct nfs4_stateowner	*replay_owner;
+	struct nfs4_client	*clp;
 	/* For sessions DRC */
 	struct nfsd4_session	*session;
 	struct nfsd4_slot	*slot;

Signature · 1 line

1.9.3

Quoted 7 lines · Jeff Layton

We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid.

In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it.

The connectathon locking test is failing for me in the nfsv4/krb5i case as of this commit.

Which makes no sense to me whatsoever, so it's entirely possible this is some unrelated problem on my side. I'll let you know when I've figured out anything more.

Quoted 9 lines · J. Bruce Fields

Quoted 7 lines · Jeff Layton

We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid.

In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it.

Quoted 6 lines

The connectathon locking test is failing for me in the nfsv4/krb5i case as of this commit.

Which makes no sense to me whatsoever, so it's entirely possible this is some unrelated problem on my side. I'll let you know when I've figured out anything more.

It's intermittent.

I've reproduced it on the previous commit so I know at least that this one isn't at fault.

I doubt it's really dependent on krb5i, at most that's probably just making it more likely to reproduce.

--b.

Quoted 9 lines · J. Bruce Fields

Quoted 7 lines · Jeff Layton

We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid.

In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it.

Quoted 6 lines

The connectathon locking test is failing for me in the nfsv4/krb5i case as of this commit.

Which makes no sense to me whatsoever, so it's entirely possible this is some unrelated problem on my side. I'll let you know when I've figured out anything more.

It's intermittent.

I've reproduced it on the previous commit so I know at least that this one isn't at fault.

I doubt it's really dependent on krb5i, at most that's probably just making it more likely to reproduce.

--b.

From 84884320b983eaeb68c652de8fe4b48aad4052c3 Mon Sep 17 00:00:00 2001