2008-07-12 13:17:34

by Jeff Layton

[permalink] [raw]
Subject: [PATCH] lockd: eliminate duplicate calls to nlmsvc_lookup_host in nlmsvc_lock and nlmsvc_testlock

Both nlmsvc_lock and nlmsvc_testlock call nlmsvc_lookup_host to find a
nlm_host struct. All the callers of these functions, however, call
nlmsvc_retrieve_args or nlm4svc_retrieve_args which also return a
nlm_host struct.

Change nlmsvc_lock and nlmsvc_testlock to take a host arg instead of
calling nlmsvc_lookup_host themselves and change the callers to pass
a pointer to the nlm_host they've already found.

Also, get rid of some unneeded nlm_host NULL checks.

I think I've got the reference counting right with this patch, but
I wouldn't mind someone sanity checking it.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/lockd/svc4proc.c | 7 +++----
fs/lockd/svclock.c | 26 +++++++-------------------
fs/lockd/svcproc.c | 7 +++----
include/linux/lockd/lockd.h | 6 ++++--
4 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 2e27176..3994446 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -58,8 +58,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
return 0;

no_locks:
- if (host)
- nlm_release_host(host);
+ nlm_release_host(host);
if (error)
return error;
return nlm_lck_denied_nolocks;
@@ -100,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;

/* Now check for conflicting locks */
- resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
+ resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
else
@@ -146,7 +145,7 @@ nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
#endif

/* Now try to lock the file */
- resp->status = nlmsvc_lock(rqstp, file, &argp->lock,
+ resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
argp->block, &argp->cookie);
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 56a08ab..6680715 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -358,10 +358,10 @@ nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block)
*/
__be32
nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
- struct nlm_lock *lock, int wait, struct nlm_cookie *cookie)
+ struct nlm_host *host, struct nlm_lock *lock, int wait,
+ struct nlm_cookie *cookie)
{
struct nlm_block *block = NULL;
- struct nlm_host *host;
int error;
__be32 ret;

@@ -373,11 +373,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
(long long)lock->fl.fl_end,
wait);

- /* Create host handle for callback */
- host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
- if (host == NULL)
- return nlm_lck_denied_nolocks;
-
/* Lock file against concurrent access */
mutex_lock(&file->f_mutex);
/* Get existing block (in case client is busy-waiting)
@@ -386,7 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
block = nlmsvc_lookup_block(file, lock);
if (block == NULL) {
block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
- lock, cookie);
+ lock, cookie);
ret = nlm_lck_denied_nolocks;
if (block == NULL)
goto out;
@@ -450,7 +445,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
out:
mutex_unlock(&file->f_mutex);
nlmsvc_release_block(block);
- nlm_release_host(host);
dprintk("lockd: nlmsvc_lock returned %u\n", ret);
return ret;
}
@@ -460,8 +454,8 @@ out:
*/
__be32
nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
- struct nlm_lock *lock, struct nlm_lock *conflock,
- struct nlm_cookie *cookie)
+ struct nlm_host *host, struct nlm_lock *lock,
+ struct nlm_lock *conflock, struct nlm_cookie *cookie)
{
struct nlm_block *block = NULL;
int error;
@@ -479,17 +473,11 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,

if (block == NULL) {
struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
- struct nlm_host *host;

if (conf == NULL)
return nlm_granted;
- /* Create host handle for callback */
- host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
- if (host == NULL) {
- kfree(conf);
- return nlm_lck_denied_nolocks;
- }
- block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
+ block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
+ lock, cookie);
if (block == NULL) {
kfree(conf);
return nlm_granted;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index ce6952b..76019d2 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -87,8 +87,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
return 0;

no_locks:
- if (host)
- nlm_release_host(host);
+ nlm_release_host(host);
if (error)
return error;
return nlm_lck_denied_nolocks;
@@ -129,7 +128,7 @@ nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;

/* Now check for conflicting locks */
- resp->status = cast_status(nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie));
+ resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie));
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
else
@@ -176,7 +175,7 @@ nlmsvc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
#endif

/* Now try to lock the file */
- resp->status = cast_status(nlmsvc_lock(rqstp, file, &argp->lock,
+ resp->status = cast_status(nlmsvc_lock(rqstp, file, host, &argp->lock,
argp->block, &argp->cookie));
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 102d928..f81f9dd 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -200,10 +200,12 @@ typedef int (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
* Server-side lock handling
*/
__be32 nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
- struct nlm_lock *, int, struct nlm_cookie *);
+ struct nlm_host *, struct nlm_lock *, int,
+ struct nlm_cookie *);
__be32 nlmsvc_unlock(struct nlm_file *, struct nlm_lock *);
__be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
- struct nlm_lock *, struct nlm_lock *, struct nlm_cookie *);
+ struct nlm_host *, struct nlm_lock *,
+ struct nlm_lock *, struct nlm_cookie *);
__be32 nlmsvc_cancel_blocked(struct nlm_file *, struct nlm_lock *);
unsigned long nlmsvc_retry_blocked(void);
void nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
--
1.5.5.1



2008-07-15 18:46:11

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] lockd: eliminate duplicate calls to nlmsvc_lookup_host in nlmsvc_lock and nlmsvc_testlock

On Sat, Jul 12, 2008 at 09:17:10AM -0400, Jeff Layton wrote:
> Both nlmsvc_lock and nlmsvc_testlock call nlmsvc_lookup_host to find a
> nlm_host struct. All the callers of these functions, however, call
> nlmsvc_retrieve_args or nlm4svc_retrieve_args which also return a
> nlm_host struct.
>
> Change nlmsvc_lock and nlmsvc_testlock to take a host arg instead of
> calling nlmsvc_lookup_host themselves and change the callers to pass
> a pointer to the nlm_host they've already found.

That makes sense to me, thanks.

> Also, get rid of some unneeded nlm_host NULL checks.
>
> I think I've got the reference counting right with this patch, but
> I wouldn't mind someone sanity checking it.

So, the way I do that sanity-checking is by breaking up the individual
changes as finely as I can, as follows....

--b.

2008-07-15 18:48:28

by J.Bruce Fields

[permalink] [raw]
Subject: [PATCH 1/4] lockd: nlm_release_host() checks for NULL, caller needn't

From: Jeff Layton <[email protected]>

No need to check for a NULL argument twice.

Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/lockd/svc4proc.c | 3 +--
fs/lockd/svcproc.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 385437e..006a832 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -58,8 +58,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
return 0;

no_locks:
- if (host)
- nlm_release_host(host);
+ nlm_release_host(host);
if (error)
return error;
return nlm_lck_denied_nolocks;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 88379cc..fce3d70 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -87,8 +87,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
return 0;

no_locks:
- if (host)
- nlm_release_host(host);
+ nlm_release_host(host);
if (error)
return error;
return nlm_lck_denied_nolocks;
--
1.5.5.rc1


2008-07-15 18:48:29

by J.Bruce Fields

[permalink] [raw]
Subject: [PATCH 4/4] lockd: minor svclock.c style fixes

Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/lockd/svclock.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index a6d3ed0..926055c 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -129,9 +129,9 @@ nlmsvc_lookup_block(struct nlm_file *file, struct nlm_lock *lock)

static inline int nlm_cookie_match(struct nlm_cookie *a, struct nlm_cookie *b)
{
- if(a->len != b->len)
+ if (a->len != b->len)
return 0;
- if(memcmp(a->data,b->data,a->len))
+ if (memcmp(a->data, b->data,a->len))
return 0;
return 1;
}
@@ -381,7 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
block = nlmsvc_lookup_block(file, lock);
if (block == NULL) {
block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
- lock, cookie);
+ lock, cookie);
ret = nlm_lck_denied_nolocks;
if (block == NULL)
goto out;
@@ -412,7 +412,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
lock->fl.fl_flags &= ~FL_SLEEP;

dprintk("lockd: vfs_lock_file returned %d\n", error);
- switch(error) {
+ switch (error) {
case 0:
ret = nlm_granted;
goto out;
@@ -880,7 +880,7 @@ nlmsvc_retry_blocked(void)

if (block->b_when == NLM_NEVER)
break;
- if (time_after(block->b_when,jiffies)) {
+ if (time_after(block->b_when, jiffies)) {
timeout = block->b_when - jiffies;
break;
}
--
1.5.5.rc1


2008-07-15 18:48:29

by J.Bruce Fields

[permalink] [raw]
Subject: [PATCH 3/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_lock

From: Jeff Layton <[email protected]>

nlmsvc_lock calls nlmsvc_lookup_host to find a nlm_host struct. The
callers of this function, however, call nlmsvc_retrieve_args or
nlm4svc_retrieve_args, which also return a nlm_host struct.

Change nlmsvc_lock to take a host arg instead of calling
nlmsvc_lookup_host itself and change the callers to pass a pointer to
the nlm_host they've already found.

Since nlmsvc_testlock() now just uses the caller's reference, we no
longer need to get or release it.

Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/lockd/svc4proc.c | 2 +-
fs/lockd/svclock.c | 12 +++---------
fs/lockd/svcproc.c | 2 +-
include/linux/lockd/lockd.h | 3 ++-
4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 8cfb9da..189b2ce 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -145,7 +145,7 @@ nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
#endif

/* Now try to lock the file */
- resp->status = nlmsvc_lock(rqstp, file, &argp->lock,
+ resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
argp->block, &argp->cookie);
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index f40afb3..a6d3ed0 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -358,10 +358,10 @@ nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block)
*/
__be32
nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
- struct nlm_lock *lock, int wait, struct nlm_cookie *cookie)
+ struct nlm_host *host, struct nlm_lock *lock, int wait,
+ struct nlm_cookie *cookie)
{
struct nlm_block *block = NULL;
- struct nlm_host *host;
int error;
__be32 ret;

@@ -373,11 +373,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
(long long)lock->fl.fl_end,
wait);

- /* Create host handle for callback */
- host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
- if (host == NULL)
- return nlm_lck_denied_nolocks;
-
/* Lock file against concurrent access */
mutex_lock(&file->f_mutex);
/* Get existing block (in case client is busy-waiting)
@@ -450,7 +445,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
out:
mutex_unlock(&file->f_mutex);
nlmsvc_release_block(block);
- nlm_release_host(host);
dprintk("lockd: nlmsvc_lock returned %u\n", ret);
return ret;
}
@@ -483,7 +477,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
if (conf == NULL)
return nlm_granted;
nlm_get_host(host);
- block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
+ block = nlmsvc_create_block(rqstp, file, lock, cookie);
if (block == NULL) {
kfree(conf);
return nlm_granted;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index e099f58..82dc908 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -175,7 +175,7 @@ nlmsvc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
#endif

/* Now try to lock the file */
- resp->status = cast_status(nlmsvc_lock(rqstp, file, &argp->lock,
+ resp->status = cast_status(nlmsvc_lock(rqstp, file, host, &argp->lock,
argp->block, &argp->cookie));
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index b279670..f81f9dd 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -200,7 +200,8 @@ typedef int (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
* Server-side lock handling
*/
__be32 nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
- struct nlm_lock *, int, struct nlm_cookie *);
+ struct nlm_host *, struct nlm_lock *, int,
+ struct nlm_cookie *);
__be32 nlmsvc_unlock(struct nlm_file *, struct nlm_lock *);
__be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
struct nlm_host *, struct nlm_lock *,
--
1.5.5.rc1


2008-07-15 18:48:29

by J.Bruce Fields

[permalink] [raw]
Subject: [PATCH 2/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_testlock

From: Jeff Layton <[email protected]>

nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The
callers of this functions, however, call nlmsvc_retrieve_args or
nlm4svc_retrieve_args, which also return a nlm_host struct.

Change nlmsvc_testlock to take a host arg instead of calling
nlmsvc_lookup_host itself and change the callers to pass a pointer to
the nlm_host they've already found.

We take a reference to host in the place where nlmsvc_testlock()
previous did a new lookup, so the reference counting is unchanged from
before.

Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/lockd/svc4proc.c | 2 +-
fs/lockd/svclock.c | 12 +++---------
fs/lockd/svcproc.c | 2 +-
include/linux/lockd/lockd.h | 3 ++-
4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 006a832..8cfb9da 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;

/* Now check for conflicting locks */
- resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
+ resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
else
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 81aca85..f40afb3 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -460,8 +460,8 @@ out:
*/
__be32
nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
- struct nlm_lock *lock, struct nlm_lock *conflock,
- struct nlm_cookie *cookie)
+ struct nlm_host *host, struct nlm_lock *lock,
+ struct nlm_lock *conflock, struct nlm_cookie *cookie)
{
struct nlm_block *block = NULL;
int error;
@@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,

if (block == NULL) {
struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
- struct nlm_host *host;

if (conf == NULL)
return nlm_granted;
- /* Create host handle for callback */
- host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
- if (host == NULL) {
- kfree(conf);
- return nlm_lck_denied_nolocks;
- }
+ nlm_get_host(host);
block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
if (block == NULL) {
kfree(conf);
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index fce3d70..e099f58 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -128,7 +128,7 @@ nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;

/* Now check for conflicting locks */
- resp->status = cast_status(nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie));
+ resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie));
if (resp->status == nlm_drop_reply)
rc = rpc_drop_reply;
else
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 102d928..b279670 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -203,7 +203,8 @@ __be32 nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
struct nlm_lock *, int, struct nlm_cookie *);
__be32 nlmsvc_unlock(struct nlm_file *, struct nlm_lock *);
__be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
- struct nlm_lock *, struct nlm_lock *, struct nlm_cookie *);
+ struct nlm_host *, struct nlm_lock *,
+ struct nlm_lock *, struct nlm_cookie *);
__be32 nlmsvc_cancel_blocked(struct nlm_file *, struct nlm_lock *);
unsigned long nlmsvc_retry_blocked(void);
void nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
--
1.5.5.rc1


2008-07-15 18:56:40

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_lock

On Tue, Jul 15, 2008 at 02:48:12PM -0400, J. Bruce Fields wrote:
> From: Jeff Layton <[email protected]>
>
> nlmsvc_lock calls nlmsvc_lookup_host to find a nlm_host struct. The
> callers of this function, however, call nlmsvc_retrieve_args or
> nlm4svc_retrieve_args, which also return a nlm_host struct.
>
> Change nlmsvc_lock to take a host arg instead of calling
> nlmsvc_lookup_host itself and change the callers to pass a pointer to
> the nlm_host they've already found.
>
> Since nlmsvc_testlock() now just uses the caller's reference, we no
> longer need to get or release it.
>
> Signed-off-by: Jeff Layton <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/lockd/svc4proc.c | 2 +-
> fs/lockd/svclock.c | 12 +++---------
> fs/lockd/svcproc.c | 2 +-
> include/linux/lockd/lockd.h | 3 ++-
> 4 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 8cfb9da..189b2ce 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -145,7 +145,7 @@ nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
> #endif
>
> /* Now try to lock the file */
> - resp->status = nlmsvc_lock(rqstp, file, &argp->lock,
> + resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
> argp->block, &argp->cookie);
> if (resp->status == nlm_drop_reply)
> rc = rpc_drop_reply;
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index f40afb3..a6d3ed0 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -358,10 +358,10 @@ nlmsvc_defer_lock_rqst(struct svc_rqst *rqstp, struct nlm_block *block)
> */
> __be32
> nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> - struct nlm_lock *lock, int wait, struct nlm_cookie *cookie)
> + struct nlm_host *host, struct nlm_lock *lock, int wait,
> + struct nlm_cookie *cookie)
> {
> struct nlm_block *block = NULL;
> - struct nlm_host *host;
> int error;
> __be32 ret;
>
> @@ -373,11 +373,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> (long long)lock->fl.fl_end,
> wait);
>
> - /* Create host handle for callback */
> - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
> - if (host == NULL)
> - return nlm_lck_denied_nolocks;
> -
> /* Lock file against concurrent access */
> mutex_lock(&file->f_mutex);
> /* Get existing block (in case client is busy-waiting)
> @@ -450,7 +445,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> out:
> mutex_unlock(&file->f_mutex);
> nlmsvc_release_block(block);
> - nlm_release_host(host);
> dprintk("lockd: nlmsvc_lock returned %u\n", ret);
> return ret;
> }
> @@ -483,7 +477,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> if (conf == NULL)
> return nlm_granted;
> nlm_get_host(host);
> - block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> + block = nlmsvc_create_block(rqstp, file, lock, cookie);
> if (block == NULL) {
> kfree(conf);
> return nlm_granted;

Oh, jeez, except ignore that chunk! Version that actually compiles
pushed to

git://linux-nfs.org/~bfields/linux.git

--b.

> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index e099f58..82dc908 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -175,7 +175,7 @@ nlmsvc_proc_lock(struct svc_rqst *rqstp, struct nlm_args *argp,
> #endif
>
> /* Now try to lock the file */
> - resp->status = cast_status(nlmsvc_lock(rqstp, file, &argp->lock,
> + resp->status = cast_status(nlmsvc_lock(rqstp, file, host, &argp->lock,
> argp->block, &argp->cookie));
> if (resp->status == nlm_drop_reply)
> rc = rpc_drop_reply;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index b279670..f81f9dd 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -200,7 +200,8 @@ typedef int (*nlm_host_match_fn_t)(void *cur, struct nlm_host *ref);
> * Server-side lock handling
> */
> __be32 nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
> - struct nlm_lock *, int, struct nlm_cookie *);
> + struct nlm_host *, struct nlm_lock *, int,
> + struct nlm_cookie *);
> __be32 nlmsvc_unlock(struct nlm_file *, struct nlm_lock *);
> __be32 nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
> struct nlm_host *, struct nlm_lock *,
> --
> 1.5.5.rc1
>

2008-07-15 19:09:28

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 2/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_testlock

On Tue, Jul 15, 2008 at 02:48:11PM -0400, J. Bruce Fields wrote:
> From: Jeff Layton <[email protected]>
>
> nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The
> callers of this functions, however, call nlmsvc_retrieve_args or
> nlm4svc_retrieve_args, which also return a nlm_host struct.
>
> Change nlmsvc_testlock to take a host arg instead of calling
> nlmsvc_lookup_host itself and change the callers to pass a pointer to
> the nlm_host they've already found.
>
> We take a reference to host in the place where nlmsvc_testlock()
> previous did a new lookup, so the reference counting is unchanged from
> before.
>
> Signed-off-by: Jeff Layton <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/lockd/svc4proc.c | 2 +-
> fs/lockd/svclock.c | 12 +++---------
> fs/lockd/svcproc.c | 2 +-
> include/linux/lockd/lockd.h | 3 ++-
> 4 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 006a832..8cfb9da 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
> return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
>
> /* Now check for conflicting locks */
> - resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
> + resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
> if (resp->status == nlm_drop_reply)
> rc = rpc_drop_reply;
> else
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 81aca85..f40afb3 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -460,8 +460,8 @@ out:
> */
> __be32
> nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> - struct nlm_lock *lock, struct nlm_lock *conflock,
> - struct nlm_cookie *cookie)
> + struct nlm_host *host, struct nlm_lock *lock,
> + struct nlm_lock *conflock, struct nlm_cookie *cookie)
> {
> struct nlm_block *block = NULL;
> int error;
> @@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>
> if (block == NULL) {
> struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> - struct nlm_host *host;
>
> if (conf == NULL)
> return nlm_granted;
> - /* Create host handle for callback */
> - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
> - if (host == NULL) {
> - kfree(conf);
> - return nlm_lck_denied_nolocks;
> - }
> + nlm_get_host(host);
> block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> if (block == NULL) {
> kfree(conf);

By the way, it'd seem clearer if nlmsvc_create_block() did the job of
taking whatever reference it needed on "host" itself.

Seems like you could do the same for nlm_alloc_host() as well.

--b.

commit cc8c1b0a6514c670b1ab568241210bbdbece7923
Author: J. Bruce Fields <[email protected]>
Date: Tue Jul 15 15:05:45 2008 -0400

lockd: get host reference in nlmsvc_create_block() instead of callers

As it is it looks like there's an obvious reference count leak until you
track down the definition of nlm_alloc_host() and see that it's
guaranteed to consume a reference, success or failure.

Signed-off-by: J. Bruce Fields <[email protected]>

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 92c49d7..80d4f2e 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -180,6 +180,7 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
struct nlm_block *block;
struct nlm_rqst *call = NULL;

+ nlm_get_host(host);
call = nlm_alloc_call(host);
if (call == NULL)
return NULL;
@@ -380,8 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
*/
block = nlmsvc_lookup_block(file, lock);
if (block == NULL) {
- block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
- lock, cookie);
+ block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
ret = nlm_lck_denied_nolocks;
if (block == NULL)
goto out;
@@ -476,7 +476,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,

if (conf == NULL)
return nlm_granted;
- nlm_get_host(host);
block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
if (block == NULL) {
kfree(conf);

2008-07-15 19:13:28

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH] lockd: eliminate duplicate calls to nlmsvc_lookup_host in nlmsvc_lock and nlmsvc_testlock

On Tue, 15 Jul 2008 14:45:54 -0400
"J. Bruce Fields" <[email protected]> wrote:

> On Sat, Jul 12, 2008 at 09:17:10AM -0400, Jeff Layton wrote:
> > Both nlmsvc_lock and nlmsvc_testlock call nlmsvc_lookup_host to find a
> > nlm_host struct. All the callers of these functions, however, call
> > nlmsvc_retrieve_args or nlm4svc_retrieve_args which also return a
> > nlm_host struct.
> >
> > Change nlmsvc_lock and nlmsvc_testlock to take a host arg instead of
> > calling nlmsvc_lookup_host themselves and change the callers to pass
> > a pointer to the nlm_host they've already found.
>
> That makes sense to me, thanks.
>
> > Also, get rid of some unneeded nlm_host NULL checks.
> >
> > I think I've got the reference counting right with this patch, but
> > I wouldn't mind someone sanity checking it.
>
> So, the way I do that sanity-checking is by breaking up the individual
> changes as finely as I can, as follows....
>
> --b.

Thanks Bruce,
The patchset in your for-2.6.27 branch looks fine to me.

Cheers,
--
Jeff Layton <[email protected]>

2008-07-15 19:32:41

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH 2/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_testlock

On Tue, 15 Jul 2008 15:09:02 -0400
"J. Bruce Fields" <[email protected]> wrote:

> On Tue, Jul 15, 2008 at 02:48:11PM -0400, J. Bruce Fields wrote:
> > From: Jeff Layton <[email protected]>
> >
> > nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The
> > callers of this functions, however, call nlmsvc_retrieve_args or
> > nlm4svc_retrieve_args, which also return a nlm_host struct.
> >
> > Change nlmsvc_testlock to take a host arg instead of calling
> > nlmsvc_lookup_host itself and change the callers to pass a pointer to
> > the nlm_host they've already found.
> >
> > We take a reference to host in the place where nlmsvc_testlock()
> > previous did a new lookup, so the reference counting is unchanged from
> > before.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/lockd/svc4proc.c | 2 +-
> > fs/lockd/svclock.c | 12 +++---------
> > fs/lockd/svcproc.c | 2 +-
> > include/linux/lockd/lockd.h | 3 ++-
> > 4 files changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > index 006a832..8cfb9da 100644
> > --- a/fs/lockd/svc4proc.c
> > +++ b/fs/lockd/svc4proc.c
> > @@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
> > return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
> >
> > /* Now check for conflicting locks */
> > - resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
> > + resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
> > if (resp->status == nlm_drop_reply)
> > rc = rpc_drop_reply;
> > else
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index 81aca85..f40afb3 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -460,8 +460,8 @@ out:
> > */
> > __be32
> > nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > - struct nlm_lock *lock, struct nlm_lock *conflock,
> > - struct nlm_cookie *cookie)
> > + struct nlm_host *host, struct nlm_lock *lock,
> > + struct nlm_lock *conflock, struct nlm_cookie *cookie)
> > {
> > struct nlm_block *block = NULL;
> > int error;
> > @@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >
> > if (block == NULL) {
> > struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > - struct nlm_host *host;
> >
> > if (conf == NULL)
> > return nlm_granted;
> > - /* Create host handle for callback */
> > - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
> > - if (host == NULL) {
> > - kfree(conf);
> > - return nlm_lck_denied_nolocks;
> > - }
> > + nlm_get_host(host);
> > block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> > if (block == NULL) {
> > kfree(conf);
>
> By the way, it'd seem clearer if nlmsvc_create_block() did the job of
> taking whatever reference it needed on "host" itself.
>

That does seem like the best thing to do here...

> Seems like you could do the same for nlm_alloc_host() as well.
>
> --b.
>
> commit cc8c1b0a6514c670b1ab568241210bbdbece7923
> Author: J. Bruce Fields <[email protected]>
> Date: Tue Jul 15 15:05:45 2008 -0400
>
> lockd: get host reference in nlmsvc_create_block() instead of callers
>
> As it is it looks like there's an obvious reference count leak until you
> track down the definition of nlm_alloc_host() and see that it's
> guaranteed to consume a reference, success or failure.
>

I'm not sure I follow this. I don't see an nlm_alloc_host(). Do you mean
nlm_alloc_call()? If so, then it looks like it should only consume a
reference on failure (when signaled).

> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 92c49d7..80d4f2e 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -180,6 +180,7 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
> struct nlm_block *block;
> struct nlm_rqst *call = NULL;
>
> + nlm_get_host(host);
> call = nlm_alloc_call(host);
> if (call == NULL)
> return NULL;
> @@ -380,8 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> */
> block = nlmsvc_lookup_block(file, lock);
> if (block == NULL) {
> - block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
> - lock, cookie);
> + block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> ret = nlm_lck_denied_nolocks;
> if (block == NULL)
> goto out;
> @@ -476,7 +476,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>
> if (conf == NULL)
> return nlm_granted;
> - nlm_get_host(host);
> block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> if (block == NULL) {
> kfree(conf);


--
Jeff Layton <[email protected]>

2008-07-15 19:41:29

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 2/4] lockd: eliminate duplicate nlmsvc_lookup_host call from nlmsvc_testlock

On Tue, Jul 15, 2008 at 03:32:22PM -0400, Jeff Layton wrote:
> On Tue, 15 Jul 2008 15:09:02 -0400
> "J. Bruce Fields" <[email protected]> wrote:
>
> > On Tue, Jul 15, 2008 at 02:48:11PM -0400, J. Bruce Fields wrote:
> > > From: Jeff Layton <[email protected]>
> > >
> > > nlmsvc_testlock calls nlmsvc_lookup_host to find a nlm_host struct. The
> > > callers of this functions, however, call nlmsvc_retrieve_args or
> > > nlm4svc_retrieve_args, which also return a nlm_host struct.
> > >
> > > Change nlmsvc_testlock to take a host arg instead of calling
> > > nlmsvc_lookup_host itself and change the callers to pass a pointer to
> > > the nlm_host they've already found.
> > >
> > > We take a reference to host in the place where nlmsvc_testlock()
> > > previous did a new lookup, so the reference counting is unchanged from
> > > before.
> > >
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > ---
> > > fs/lockd/svc4proc.c | 2 +-
> > > fs/lockd/svclock.c | 12 +++---------
> > > fs/lockd/svcproc.c | 2 +-
> > > include/linux/lockd/lockd.h | 3 ++-
> > > 4 files changed, 7 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > > index 006a832..8cfb9da 100644
> > > --- a/fs/lockd/svc4proc.c
> > > +++ b/fs/lockd/svc4proc.c
> > > @@ -99,7 +99,7 @@ nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_args *argp,
> > > return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
> > >
> > > /* Now check for conflicting locks */
> > > - resp->status = nlmsvc_testlock(rqstp, file, &argp->lock, &resp->lock, &resp->cookie);
> > > + resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
> > > if (resp->status == nlm_drop_reply)
> > > rc = rpc_drop_reply;
> > > else
> > > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > > index 81aca85..f40afb3 100644
> > > --- a/fs/lockd/svclock.c
> > > +++ b/fs/lockd/svclock.c
> > > @@ -460,8 +460,8 @@ out:
> > > */
> > > __be32
> > > nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > > - struct nlm_lock *lock, struct nlm_lock *conflock,
> > > - struct nlm_cookie *cookie)
> > > + struct nlm_host *host, struct nlm_lock *lock,
> > > + struct nlm_lock *conflock, struct nlm_cookie *cookie)
> > > {
> > > struct nlm_block *block = NULL;
> > > int error;
> > > @@ -479,16 +479,10 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> > >
> > > if (block == NULL) {
> > > struct file_lock *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > - struct nlm_host *host;
> > >
> > > if (conf == NULL)
> > > return nlm_granted;
> > > - /* Create host handle for callback */
> > > - host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len);
> > > - if (host == NULL) {
> > > - kfree(conf);
> > > - return nlm_lck_denied_nolocks;
> > > - }
> > > + nlm_get_host(host);
> > > block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> > > if (block == NULL) {
> > > kfree(conf);
> >
> > By the way, it'd seem clearer if nlmsvc_create_block() did the job of
> > taking whatever reference it needed on "host" itself.
> >
>
> That does seem like the best thing to do here...
>
> > Seems like you could do the same for nlm_alloc_host() as well.
> >
> > --b.
> >
> > commit cc8c1b0a6514c670b1ab568241210bbdbece7923
> > Author: J. Bruce Fields <[email protected]>
> > Date: Tue Jul 15 15:05:45 2008 -0400
> >
> > lockd: get host reference in nlmsvc_create_block() instead of callers
> >
> > As it is it looks like there's an obvious reference count leak until you
> > track down the definition of nlm_alloc_host() and see that it's
> > guaranteed to consume a reference, success or failure.
> >
>
> I'm not sure I follow this. I don't see an nlm_alloc_host(). Do you mean
> nlm_alloc_call()?

Whoops, yes.

> If so, then it looks like it should only consume a
> reference on failure (when signaled).

It only has an explicit nlm_release_host() in that case, but in the
other case it exits having stored a pointer in a_host which will
eventually have nlm_release_host() called on it.

So I'd say it has "consumed" (either stored, or thrown away) one
reference in either case.

--b.

>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> >
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index 92c49d7..80d4f2e 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -180,6 +180,7 @@ nlmsvc_create_block(struct svc_rqst *rqstp, struct nlm_host *host,
> > struct nlm_block *block;
> > struct nlm_rqst *call = NULL;
> >
> > + nlm_get_host(host);
> > call = nlm_alloc_call(host);
> > if (call == NULL)
> > return NULL;
> > @@ -380,8 +381,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> > */
> > block = nlmsvc_lookup_block(file, lock);
> > if (block == NULL) {
> > - block = nlmsvc_create_block(rqstp, nlm_get_host(host), file,
> > - lock, cookie);
> > + block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> > ret = nlm_lck_denied_nolocks;
> > if (block == NULL)
> > goto out;
> > @@ -476,7 +476,6 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> >
> > if (conf == NULL)
> > return nlm_granted;
> > - nlm_get_host(host);
> > block = nlmsvc_create_block(rqstp, host, file, lock, cookie);
> > if (block == NULL) {
> > kfree(conf);
>
>
> --
> Jeff Layton <[email protected]>