[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#699361: linux-image-3.2.0-0.bpo.4-amd64: nfsd4 RELEASE_LOCKOWNER is slow and, CPU intensive



Control: tag -1 moreinfo fixed-upstream

On Thu, 2013-02-28 at 15:28 +0000, Chris Boot wrote:
> We are also seeing this on an NFS server hosing home directories for a 
> fairly large deployment of Debian desktop systems. The symptoms and perf 
> top agree perfectly with what the reporter is experiencing.
> 
> Please consider backporting said patch to the 3.2 kernel for 
> wheezy/squeeze-backports.

Please test the attached backport as explained here:
http://kernel-handbook.alioth.debian.org/ch-common-tasks.html#s-common-official

Ben.

-- 
Ben Hutchings
Time is nature's way of making sure that everything doesn't happen at once.
From: "J. Bruce Fields" <bfields@redhat.com>
Date: Mon, 7 Nov 2011 16:58:18 -0500
Subject: nfsd4: hash lockowners to simplify RELEASE_LOCKOWNER

commit 06f1f864d4ae5804e83785308d41f14a08e4b980 upstream.

Hash lockowners on just the owner string rather than on (owner, inode).
This makes the owner-string lookup needed for RELEASE_LOCKOWNER simpler
(currently it's doing at a linear search through the entire hash
table!).  That may come at the expense of making (owner, inode) lookups
more expensive if a client reuses the same lockowner across multiple
files.  We might add a separate lookup for that.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
[bwh: Backported to 3.2: lock creation/lookup is in nfsd4_lock() not
 lookup_or_create_lock_state()]
---
 fs/nfsd/nfs4state.c |   42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3747,15 +3747,6 @@ last_byte_offset(u64 start, u64 len)
 	return end > start ? end - 1: NFS4_MAX_UINT64;
 }
 
-static inline unsigned int
-lock_ownerstr_hashval(struct inode *inode, u32 cl_id,
-		struct xdr_netobj *ownername)
-{
-	return (file_hashval(inode) + cl_id
-			+ opaque_hashval(ownername->data, ownername->len))
-		& LOCK_HASH_MASK;
-}
-
 static struct list_head	lock_ownerstr_hashtbl[LOCK_HASH_SIZE];
 
 /*
@@ -3825,7 +3816,7 @@ static struct nfs4_lockowner *
 find_lockowner_str(struct inode *inode, clientid_t *clid,
 		struct xdr_netobj *owner)
 {
-	unsigned int hashval = lock_ownerstr_hashval(inode, clid->cl_id, owner);
+	unsigned int hashval = open_ownerstr_hashval(clid->cl_id, owner);
 	struct nfs4_lockowner *lo;
 	struct nfs4_stateowner *op;
 
@@ -3848,7 +3839,7 @@ static void hash_lockowner(struct nfs4_l
  * Called in nfsd4_lock - therefore, OPEN and OPEN_CONFIRM (if needed) has 
  * occurred. 
  *
- * strhashval = lock_ownerstr_hashval 
+ * strhashval = open_ownerstr_hashval
  */
 
 static struct nfs4_lockowner *
@@ -3968,7 +3959,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struc
 			goto out;
 		/* create lockowner and lock stateid */
 		fp = open_stp->st_file;
-		strhashval = lock_ownerstr_hashval(fp->fi_inode,
+		strhashval = open_ownerstr_hashval(
 				open_sop->oo_owner.so_client->cl_clientid.cl_id,
 				&lock->v.new.owner);
 		/* XXX: Do we need to check for duplicate stateowners on
@@ -4260,7 +4251,7 @@ nfsd4_release_lockowner(struct svc_rqst
 	struct nfs4_ol_stateid *stp;
 	struct xdr_netobj *owner = &rlockowner->rl_owner;
 	struct list_head matches;
-	int i;
+	unsigned int hashval = open_ownerstr_hashval(clid->cl_id, owner);
 	__be32 status;
 
 	dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
@@ -4275,22 +4266,17 @@ nfsd4_release_lockowner(struct svc_rqst
 	nfs4_lock_state();
 
 	status = nfserr_locks_held;
-	/* XXX: we're doing a linear search through all the lockowners.
-	 * Yipes!  For now we'll just hope clients aren't really using
-	 * release_lockowner much, but eventually we have to fix these
-	 * data structures. */
 	INIT_LIST_HEAD(&matches);
-	for (i = 0; i < LOCK_HASH_SIZE; i++) {
-		list_for_each_entry(sop, &lock_ownerstr_hashtbl[i], so_strhash) {
-			if (!same_owner_str(sop, owner, clid))
-				continue;
-			list_for_each_entry(stp, &sop->so_stateids,
-					st_perstateowner) {
-				lo = lockowner(sop);
-				if (check_for_locks(stp->st_file, lo))
-					goto out;
-				list_add(&lo->lo_list, &matches);
-			}
+
+	list_for_each_entry(sop, &lock_ownerstr_hashtbl[hashval], so_strhash) {
+		if (!same_owner_str(sop, owner, clid))
+			continue;
+		list_for_each_entry(stp, &sop->so_stateids,
+				st_perstateowner) {
+			lo = lockowner(sop);
+			if (check_for_locks(stp->st_file, lo))
+				goto out;
+			list_add(&lo->lo_list, &matches);
 		}
 	}
 	/* Clients probably won't expect us to return with some (but not all)

Attachment: signature.asc
Description: This is a digitally signed message part


Reply to: