nfsd: preliminary patches for client_mutex removal

PATCH In review Jeff Layton 3 participants 8 messages Jun 6, 2014 – Jun 7, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Jeff Layton

author

2014-06-06 13:07 UTC #0 lore raw

v2 cover nfsd: preliminary patches for client_mutex removal

This is a respin of the last two patches in the 9 patch series I posted last week. Bruce dropped patch #8, as it caused a lockdep pop due to lock inversion between the i_lock and the state_lock. Patch #9 was dropped due to objections from Christoph. Bruce indicated that he would take the others, so I won't repost them here.

These two patches should address the previous concerns -- let me know if there are any objections.

Jeff Layton (1):
nfsd: avoid taking the state_lock while holding the i_lock

Trond Myklebust (1):
nfsd: Protect addition to the file_hashtbl

 fs/nfsd/nfs4callback.c |   9 +++-
 fs/nfsd/nfs4state.c    | 123 ++++++++++++++++++++++++++++++++++++-------------
 fs/nfsd/state.h        |   2 +
 3 files changed, 101 insertions(+), 33 deletions(-)

Signature · 1 line

1.9.3

Jeff Layton

author

2014-06-06 13:07 UTC #1 lore raw

PATCH v2 1/2 nfsd: Protect addition to the file_hashtbl

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]>

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

fs/nfsd/nfs4state.c +37 ~~-12~~

index a500033a2f87..cbec573e9445 100644
@@ -2519,17 +2519,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

@@ -2695,23 +2696,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

@@ -3230,21 +3257,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

Christoph Hellwig

Reviewed-by on v2 1/2

2014-06-06 14:14 UTC #2 lore raw

Re: [PATCH v2 1/2] nfsd: Protect addition to the file_hashtbl

Quoted 11 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.
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]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Jeff Layton

author

2014-06-06 13:07 UTC #3 lore raw

PATCH v2 2/2 nfsd: avoid taking the state_lock while holding the i_lock

In reply to Jeff Layton

state_lock is a heavily contended global lock. We don't want to grab that while simultaneously holding the inode->i_lock. Avoid doing that in the delegation break callback by ensuring that we add/remove the dl_perfile under a new per-nfs4_file fi_lock, and hold that while walking the fi_delegations list.

We still do need to queue the delegations to the global del_recall_lru list. Do that in the rpc_prepare op for the delegation recall RPC. It's possible though that the allocation of the rpc_task will fail, which would cause the delegation to be leaked.

If that occurs rpc_release is still called, so we also do it there if the rpc_task failed to run. This brings up another dilemma -- how do we know whether it got queued in rpc_prepare or not?

In order to determine that, we set the dl_time to 0 in the delegation break callback from the VFS and only set it when we queue it to the list. If it's still zero by the time we get to rpc_release, then we know that it never got queued and we can do it then.

Cc Christoph Hellwig <[email protected]>

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

---
 fs/nfsd/nfs4callback.c |  9 ++++--
 fs/nfsd/nfs4state.c    | 74 +++++++++++++++++++++++++++++++++++++-------------
 fs/nfsd/state.h        |  2 ++
 3 files changed, 64 insertions(+), 21 deletions(-)

fs/nfsd/nfs4callback.c +7 -2

index 2c73cae9899d..3d01637d950c 100644
@@ -810,12 +810,15 @@ static bool nfsd41_cb_get_slot(struct nfs4_client *clp, struct rpc_task *task)
  * TODO: cb_sequence should support referring call lists, cachethis, multiple
  * slots, and mark callback channel down on communication errors.
  */
-static void nfsd4_cb_prepare(struct rpc_task *task, void *calldata)
+static void nfsd4_cb_recall_prepare(struct rpc_task *task, void *calldata)
 {
 	struct nfsd4_callback *cb = calldata;
 	struct nfs4_client *clp = cb->cb_clp;
+	struct nfs4_delegation *dp = container_of(cb, struct nfs4_delegation, dl_recall);
 	u32 minorversion = clp->cl_minorversion;
 
+	nfsd4_queue_to_del_recall_lru(dp);
+
 	cb->cb_minorversion = minorversion;
 	if (minorversion) {
 		if (!nfsd41_cb_get_slot(clp, task))

@@ -900,6 +903,8 @@ static void nfsd4_cb_recall_release(void *calldata)
 	struct nfs4_client *clp = cb->cb_clp;
 	struct nfs4_delegation *dp = container_of(cb, struct nfs4_delegation, dl_recall);
 
+	nfsd4_queue_to_del_recall_lru(dp);
+
 	if (cb->cb_done) {
 		spin_lock(&clp->cl_lock);
 		list_del(&cb->cb_per_client);

@@ -909,7 +914,7 @@ static void nfsd4_cb_recall_release(void *calldata)
 }
 
 static const struct rpc_call_ops nfsd4_cb_recall_ops = {
-	.rpc_call_prepare = nfsd4_cb_prepare,
+	.rpc_call_prepare = nfsd4_cb_recall_prepare,
 	.rpc_call_done = nfsd4_cb_recall_done,
 	.rpc_release = nfsd4_cb_recall_release,
 };

fs/nfsd/nfs4state.c +55 ~~-19~~

124 lines not on this page · raw message

fs/nfsd/state.h +2 -0

index 374c66283ac5..eae4fcaa5fd4 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;

@@ -472,6 +473,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_queue_to_del_recall_lru(struct nfs4_delegation *);
 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

Christoph Hellwig

2014-06-07 14:09 UTC #4 lore raw

Re: [PATCH v2 2/2] nfsd: avoid taking the state_lock while holding the i_lock

Quoted 11 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. Avoid doing that in the delegation break callback by ensuring that we add/remove the dl_perfile under a new per-nfs4_file fi_lock, and hold that while walking the fi_delegations list.
We still do need to queue the delegations to the global del_recall_lru list. Do that in the rpc_prepare op for the delegation recall RPC. It's possible though that the allocation of the rpc_task will fail, which would cause the delegation to be leaked.

Quoted 8 lines

If that occurs rpc_release is still called, so we also do it there if the rpc_task failed to run. This brings up another dilemma -- how do we know whether it got queued in rpc_prepare or not?
In order to determine that, we set the dl_time to 0 in the delegation break callback from the VFS and only set it when we queue it to the list. If it's still zero by the time we get to rpc_release, then we know that it never got queued and we can do it then.

Compared to this version I have to say the original one that I objected to looks like the lesser evil. I'll take another deeper look at it.

Jeff Layton

2014-06-07 14:28 UTC #5 lore raw

Re: [PATCH v2 2/2] nfsd: avoid taking the state_lock while holding the i_lock

Quoted 16 lines · On Sat, 7 Jun 2014 07:09:04 -0700 Christoph Hellwig <[email protected]> wrote:

Quoted 15 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. Avoid doing that in the delegation break callback by ensuring that we add/remove the dl_perfile under a new per-nfs4_file fi_lock, and hold that while walking the fi_delegations list.
We still do need to queue the delegations to the global del_recall_lru list. Do that in the rpc_prepare op for the delegation recall RPC. It's possible though that the allocation of the rpc_task will fail, which would cause the delegation to be leaked.
If that occurs rpc_release is still called, so we also do it there if the rpc_task failed to run. This brings up another dilemma -- how do we know whether it got queued in rpc_prepare or not?

Quoted 8 lines

Quoted 4 lines
In order to determine that, we set the dl_time to 0 in the delegation break callback from the VFS and only set it when we queue it to the list. If it's still zero by the time we get to rpc_release, then we know that it never got queued and we can do it then.
Compared to this version I have to say the original one that I objected to looks like the lesser evil. I'll take another deeper look at it.

Well, I think using the fp->fi_lock instead of the i_lock here is reasonable. We at least avoid taking the state_lock (which is likely to be much more contended) within the i_lock. The thing that makes this patch nasty is all of the shenanigans to queue the delegation to the global list from within rpc_prepare or rpc_release.

Personally, I think it'd be cleaner to add some sort of cb_prepare operation to the generic callback framework you're building to handle that, but let me know what you thing.

Signature · 1 line

Jeff Layton <[email protected]>

Christoph Hellwig

2014-06-07 14:31 UTC #6 lore raw

Re: [PATCH v2 2/2] nfsd: avoid taking the state_lock while holding the i_lock

Quoted 3 lines · Jeff Layton

Well, I think using the fp->fi_lock instead of the i_lock here is reasonable. We at least avoid taking the state_lock (which is likely to be much more contended) within the i_lock.

Yes, avoiding i_lock usage inside nfsd is something I'd prefer. But with the current lock manager ops that are called with i_lock held we'll have some leakage into the nfsd lock hierachy anyway unfortunately.

Quoted 7 lines

The thing that makes this
patch nasty is all of the shenanigans to queue the delegation to the global list from within rpc_prepare or rpc_release.
Personally, I think it'd be cleaner to add some sort of cb_prepare operation to the generic callback framework you're building to handle that, but let me know what you thing.

I guess I'll have to do it that way then. It's not like so far unreleased code should be a hard blocker for a bug fix anyway.

Care to prefer a version that uses fi_lock, but otherwise works like the first version?

Jeff Layton

2014-06-07 14:34 UTC #7 lore raw

Re: [PATCH v2 2/2] nfsd: avoid taking the state_lock while holding the i_lock

Quoted 10 lines · On Sat, 7 Jun 2014 07:31:33 -0700 Christoph Hellwig <[email protected]> wrote:

Quoted 3 lines · Jeff Layton
Well, I think using the fp->fi_lock instead of the i_lock here is reasonable. We at least avoid taking the state_lock (which is likely to be much more contended) within the i_lock.
Yes, avoiding i_lock usage inside nfsd is something I'd prefer. But with the current lock manager ops that are called with i_lock held we'll have some leakage into the nfsd lock hierachy anyway unfortunately.

Yeah. Switching the file locking infrastructure over to the i_lock seemed like such a good idea at the time...

Quoted 8 lines

Quoted 7 lines
The thing that makes this
patch nasty is all of the shenanigans to queue the delegation to the global list from within rpc_prepare or rpc_release.
Personally, I think it'd be cleaner to add some sort of cb_prepare operation to the generic callback framework you're building to handle that, but let me know what you thing.

Quoted 6 lines

I guess I'll have to do it that way then. It's not like so far unreleased code should be a hard blocker for a bug fix anyway.
Care to prefer a version that uses fi_lock, but otherwise works like the first version?

Nope, that'd be fine. It might take a few days to respin as I'll be at the bakeathon next week.

Signature · 1 line

Jeff Layton <[email protected]>