From 1447d25eb3a7bbe5bf5e4e7489f09be13e1ec73a Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Fri, 8 Feb 2008 13:03:37 +1100 Subject: knfsd: Remove NLM_HOST_MAX and associated logic. Lockd caches information about hosts that have recently held locks to expedite the taking of further locks. It periodically discards this information for hosts that have not been used for a few minutes. lockd currently has a value NLM_HOST_MAX, and changes the 'garbage collection' behaviour when the number of hosts exceeds this threshold. However its behaviour is strange, and likely not what was intended. When the number of hosts exceeds the max, it scans *less* often (every 2 minutes vs every minute) and allows unused host information to remain around longer (5 minutes instead of 2). Having this limit is of dubious value anyway, and we have not suffered from the code not getting the limit right, so remove the limit altogether. We go with the larger values (discard 5 minute old hosts every 2 minutes) as they are probably safer. Maybe the periodic garbage collection should be replace to with 'shrinker' handler so we just respond to memory pressure.... Acked-by: Jeff Layton Signed-off-by: Neil Brown Signed-off-by: J. Bruce Fields --- fs/lockd/host.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'fs/lockd/host.c') diff --git a/fs/lockd/host.c b/fs/lockd/host.c index f1ef49fff118..c3f119426d83 100644 --- a/fs/lockd/host.c +++ b/fs/lockd/host.c @@ -19,12 +19,11 @@ #define NLMDBG_FACILITY NLMDBG_HOSTCACHE -#define NLM_HOST_MAX 64 #define NLM_HOST_NRHASH 32 #define NLM_ADDRHASH(addr) (ntohl(addr) & (NLM_HOST_NRHASH-1)) #define NLM_HOST_REBIND (60 * HZ) -#define NLM_HOST_EXPIRE ((nrhosts > NLM_HOST_MAX)? 300 * HZ : 120 * HZ) -#define NLM_HOST_COLLECT ((nrhosts > NLM_HOST_MAX)? 120 * HZ : 60 * HZ) +#define NLM_HOST_EXPIRE (300 * HZ) +#define NLM_HOST_COLLECT (120 * HZ) static struct hlist_head nlm_hosts[NLM_HOST_NRHASH]; static unsigned long next_gc; @@ -142,9 +141,7 @@ nlm_lookup_host(int server, const struct sockaddr_in *sin, INIT_LIST_HEAD(&host->h_granted); INIT_LIST_HEAD(&host->h_reclaim); - if (++nrhosts > NLM_HOST_MAX) - next_gc = 0; - + nrhosts++; out: mutex_unlock(&nlm_host_mutex); return host; -- cgit v1.2.3 From 164f98adbbd50c67177b096a59f55c1a56a45c82 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Wed, 20 Feb 2008 14:02:47 -0500 Subject: lockd: fix race in nlm_release() The sm_count is decremented to zero but left on the nsm_handles list. So in the space between decrementing sm_count and acquiring nsm_mutex, it is possible for another task to find this nsm_handle, increment the use count and then enter nsm_release itself. Thus there's nothing to prevent the nsm being freed before we acquire nsm_mutex here. Signed-off-by: J. Bruce Fields --- fs/lockd/host.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'fs/lockd/host.c') diff --git a/fs/lockd/host.c b/fs/lockd/host.c index c3f119426d83..960911c4a11c 100644 --- a/fs/lockd/host.c +++ b/fs/lockd/host.c @@ -529,12 +529,10 @@ nsm_release(struct nsm_handle *nsm) { if (!nsm) return; + mutex_lock(&nsm_mutex); if (atomic_dec_and_test(&nsm->sm_count)) { - mutex_lock(&nsm_mutex); - if (atomic_read(&nsm->sm_count) == 0) { - list_del(&nsm->sm_link); - kfree(nsm); - } - mutex_unlock(&nsm_mutex); + list_del(&nsm->sm_link); + kfree(nsm); } + mutex_unlock(&nsm_mutex); } -- cgit v1.2.3 From a95e56e72c196970a8067cd515c658d064813170 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Wed, 20 Feb 2008 15:27:31 -0500 Subject: lockd: clean up __nsm_find() Use list_for_each_entry(). Also, in keeping with kernel style, make the normal case (kzalloc succeeds) unindented and handle the abnormal case with a goto. Signed-off-by: J. Bruce Fields --- fs/lockd/host.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'fs/lockd/host.c') diff --git a/fs/lockd/host.c b/fs/lockd/host.c index 960911c4a11c..de0ffb6106c4 100644 --- a/fs/lockd/host.c +++ b/fs/lockd/host.c @@ -465,7 +465,7 @@ __nsm_find(const struct sockaddr_in *sin, int create) { struct nsm_handle *nsm = NULL; - struct list_head *pos; + struct nsm_handle *pos; if (!sin) return NULL; @@ -480,16 +480,16 @@ __nsm_find(const struct sockaddr_in *sin, } mutex_lock(&nsm_mutex); - list_for_each(pos, &nsm_handles) { - nsm = list_entry(pos, struct nsm_handle, sm_link); + list_for_each_entry(pos, &nsm_handles, sm_link) { if (hostname && nsm_use_hostnames) { - if (strlen(nsm->sm_name) != hostname_len - || memcmp(nsm->sm_name, hostname, hostname_len)) + if (strlen(pos->sm_name) != hostname_len + || memcmp(pos->sm_name, hostname, hostname_len)) continue; - } else if (!nlm_cmp_addr(&nsm->sm_addr, sin)) + } else if (!nlm_cmp_addr(&pos->sm_addr, sin)) continue; - atomic_inc(&nsm->sm_count); + atomic_inc(&pos->sm_count); + nsm = pos; goto out; } @@ -499,15 +499,15 @@ __nsm_find(const struct sockaddr_in *sin, } nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL); - if (nsm != NULL) { - nsm->sm_addr = *sin; - nsm->sm_name = (char *) (nsm + 1); - memcpy(nsm->sm_name, hostname, hostname_len); - nsm->sm_name[hostname_len] = '\0'; - atomic_set(&nsm->sm_count, 1); - - list_add(&nsm->sm_link, &nsm_handles); - } + if (nsm == NULL) + goto out; + nsm->sm_addr = *sin; + nsm->sm_name = (char *) (nsm + 1); + memcpy(nsm->sm_name, hostname, hostname_len); + nsm->sm_name[hostname_len] = '\0'; + atomic_set(&nsm->sm_count, 1); + + list_add(&nsm->sm_link, &nsm_handles); out: mutex_unlock(&nsm_mutex); -- cgit v1.2.3 From d8421202121ce74daf4625ca9d1d825bbd7ce66a Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Wed, 20 Feb 2008 15:40:15 -0500 Subject: lockd: convert nsm_mutex to a spinlock There's no reason for a mutex here, except to allow an allocation under the lock, which we can avoid with the usual trick of preallocating memory for the new object and freeing it if it turns out to be unnecessary. Signed-off-by: J. Bruce Fields --- fs/lockd/host.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) (limited to 'fs/lockd/host.c') diff --git a/fs/lockd/host.c b/fs/lockd/host.c index de0ffb6106c4..c7854791898f 100644 --- a/fs/lockd/host.c +++ b/fs/lockd/host.c @@ -457,7 +457,7 @@ nlm_gc_hosts(void) * Manage NSM handles */ static LIST_HEAD(nsm_handles); -static DEFINE_MUTEX(nsm_mutex); +static DEFINE_SPINLOCK(nsm_lock); static struct nsm_handle * __nsm_find(const struct sockaddr_in *sin, @@ -479,7 +479,8 @@ __nsm_find(const struct sockaddr_in *sin, return NULL; } - mutex_lock(&nsm_mutex); +retry: + spin_lock(&nsm_lock); list_for_each_entry(pos, &nsm_handles, sm_link) { if (hostname && nsm_use_hostnames) { @@ -489,28 +490,32 @@ __nsm_find(const struct sockaddr_in *sin, } else if (!nlm_cmp_addr(&pos->sm_addr, sin)) continue; atomic_inc(&pos->sm_count); + kfree(nsm); nsm = pos; - goto out; + goto found; } - - if (!create) { - nsm = NULL; - goto out; + if (nsm) { + list_add(&nsm->sm_link, &nsm_handles); + goto found; } + spin_unlock(&nsm_lock); + + if (!create) + return NULL; nsm = kzalloc(sizeof(*nsm) + hostname_len + 1, GFP_KERNEL); if (nsm == NULL) - goto out; + return NULL; + nsm->sm_addr = *sin; nsm->sm_name = (char *) (nsm + 1); memcpy(nsm->sm_name, hostname, hostname_len); nsm->sm_name[hostname_len] = '\0'; atomic_set(&nsm->sm_count, 1); + goto retry; - list_add(&nsm->sm_link, &nsm_handles); - -out: - mutex_unlock(&nsm_mutex); +found: + spin_unlock(&nsm_lock); return nsm; } @@ -529,10 +534,9 @@ nsm_release(struct nsm_handle *nsm) { if (!nsm) return; - mutex_lock(&nsm_mutex); - if (atomic_dec_and_test(&nsm->sm_count)) { + if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) { list_del(&nsm->sm_link); + spin_unlock(&nsm_lock); kfree(nsm); } - mutex_unlock(&nsm_mutex); } -- cgit v1.2.3