by J. Bruce Fields

[permalink] [raw]

Subject: [PATCH 1/3] nfsd4: hash lockowners to simplify RELEASE_LOCKOWNER

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>
---
fs/nfsd/nfs4state.c | 42 ++++++++++++++----------------------------
1 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1d6812d..eec9900 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3746,15 +3746,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];

/*
@@ -3824,7 +3815,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;

@@ -3847,7 +3838,7 @@ static void hash_lockowner(struct nfs4_lockowner *lo, unsigned int strhashval, s
* 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 *
@@ -3922,7 +3913,7 @@ __be32 lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, struct n
struct nfs4_ol_stateid, st_perstateowner);
return nfs_ok;
}
- strhashval = lock_ownerstr_hashval(fi->fi_inode, cl->cl_clientid.cl_id,
+ strhashval = open_ownerstr_hashval(cl->cl_clientid.cl_id,
&lock->v.new.owner);
lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
if (lo == NULL)
@@ -4286,7 +4277,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
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",
@@ -4301,22 +4292,17 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
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)
--
1.7.5.4

2011-11-08 22:59:09

by J. Bruce Fields

[permalink] [raw]

Subject: [PATCH 3/3] nfsd4: add a separate (lockowner, inode) lookup

From: "J. Bruce Fields" <[email protected]>

Address the possible performance regression mentioned in "nfsd4: hash
lockowners to simplify RELEASE_LOCKOWNER" by providing a separate
(lockowner, inode) hash.

Really, I doubt this matters much, but I think it's likely we'll change
these data structures here and I'd rather that the need for (owner,
inode) lookups be well-documented.

Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 29 +++++++++++++++++++++++------
fs/nfsd/state.h | 1 +
2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dcbe7f3..d4bd698 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -514,6 +514,7 @@ static void unhash_lockowner(struct nfs4_lockowner *lo)

list_del(&lo->lo_owner.so_strhash);
list_del(&lo->lo_perstateid);
+ list_del(&lo->lo_owner_ino_hash);
while (!list_empty(&lo->lo_owner.so_stateids)) {
stp = list_first_entry(&lo->lo_owner.so_stateids,
struct nfs4_ol_stateid, st_perstateowner);
@@ -3722,6 +3723,10 @@ out:

#define LOFF_OVERFLOW(start, len) ((u64)(len) > ~(u64)(start))

+#define LOCKOWNER_INO_HASH_BITS 8
+#define LOCKOWNER_INO_HASH_SIZE (1 << LOCKOWNER_INO_HASH_BITS)
+#define LOCKOWNER_INO_HASH_MASK (LOCKOWNER_INO_HASH_SIZE - 1)
+
static inline u64
end_offset(u64 start, u64 len)
{
@@ -3742,6 +3747,15 @@ last_byte_offset(u64 start, u64 len)
return end > start ? end - 1: NFS4_MAX_UINT64;
}

+static unsigned int lockowner_ino_hashval(struct inode *inode, u32 cl_id, struct xdr_netobj *ownername)
+{
+ return (file_hashval(inode) + cl_id
+ + opaque_hashval(ownername->data, ownername->len))
+ & LOCKOWNER_INO_HASH_MASK;
+}
+
+static struct list_head lockowner_ino_hashtbl[LOCKOWNER_INO_HASH_SIZE];
+
/*
* TODO: Linux file offsets are _signed_ 64-bit quantities, which means that
* we can't properly handle lock requests that go beyond the (2^63 - 1)-th
@@ -3809,14 +3823,10 @@ static struct nfs4_lockowner *
find_lockowner_str(struct inode *inode, clientid_t *clid,
struct xdr_netobj *owner)
{
- unsigned int hashval = ownerstr_hashval(clid->cl_id, owner);
+ unsigned int hashval = lockowner_ino_hashval(inode, clid->cl_id, owner);
struct nfs4_lockowner *lo;
- struct nfs4_stateowner *op;

- list_for_each_entry(op, &ownerstr_hashtbl[hashval], so_strhash) {
- if (op->so_is_open_owner)
- continue;
- lo = lockowner(op);
+ list_for_each_entry(lo, &lockowner_ino_hashtbl[hashval], lo_owner_ino_hash) {
if (same_lockowner_ino(lo, inode, clid, owner))
return lo;
}
@@ -3825,7 +3835,12 @@ find_lockowner_str(struct inode *inode, clientid_t *clid,

static void hash_lockowner(struct nfs4_lockowner *lo, unsigned int strhashval, struct nfs4_client *clp, struct nfs4_ol_stateid *open_stp)
{
+ struct inode *inode = open_stp->st_file->fi_inode;
+ unsigned int inohash = lockowner_ino_hashval(inode,
+ clp->cl_clientid.cl_id, &lo->lo_owner.so_owner);
+
list_add(&lo->lo_owner.so_strhash, &ownerstr_hashtbl[strhashval]);
+ list_add(&lo->lo_owner_ino_hash, &lockowner_ino_hashtbl[inohash]);
list_add(&lo->lo_perstateid, &open_stp->st_lockowners);
}

@@ -4548,6 +4563,8 @@ nfs4_state_init(void)
for (i = 0; i < OWNER_HASH_SIZE; i++) {
INIT_LIST_HEAD(&ownerstr_hashtbl[i]);
}
+ for (i = 0; i < LOCKOWNER_INO_HASH_SIZE; i++)
+ INIT_LIST_HEAD(&lockowner_ino_hashtbl[i]);
memset(&onestateid, ~0, sizeof(stateid_t));
INIT_LIST_HEAD(&close_lru);
INIT_LIST_HEAD(&client_lru);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index a3cf384..89c2cd8 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -366,6 +366,7 @@ struct nfs4_openowner {

struct nfs4_lockowner {
struct nfs4_stateowner lo_owner; /* must be first element */
+ struct list_head lo_owner_ino_hash; /* hash by owner,file */
struct list_head lo_perstateid; /* for lockowners only */
struct list_head lo_list; /* for temporary uses */
};
--
1.7.5.4