Even though nlmclnt_reclaim() is only one call into the stack frame,
928 bytes on the stack seems like a lot. Recode to dynamically
allocate the request structure once from within the reclaimer task,
then pass this pointer into nlmclnt_reclaim() for reuse on
subsequent calls.
smatch analysis:
fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
928 bytes on stack
Also remove redundant assignment of 0 after memset.
Cc: Trond Myklebust <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: [email protected]
Signed-off-by: Tim Gardner <[email protected]>
---
fs/lockd/clntlock.c | 8 +++++++-
fs/lockd/clntproc.c | 6 ++----
include/linux/lockd/lockd.h | 3 ++-
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 4885b53..5dd23ef 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -220,10 +220,15 @@ reclaimer(void *ptr)
{
struct nlm_host *host = (struct nlm_host *) ptr;
struct nlm_wait *block;
+ struct nlm_rqst *req;
struct file_lock *fl, *next;
u32 nsmstate;
struct net *net = host->net;
+ req = kmalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return -ENOMEM;
+
allow_signal(SIGKILL);
down_write(&host->h_rwsem);
@@ -253,7 +258,7 @@ restart:
*/
if (signalled())
continue;
- if (nlmclnt_reclaim(host, fl) != 0)
+ if (nlmclnt_reclaim(host, fl, req) != 0)
continue;
list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
if (host->h_nsmstate != nsmstate) {
@@ -279,5 +284,6 @@ restart:
/* Release host handle after use */
nlmclnt_release_host(host);
lockd_down(net);
+ kfree(req);
return 0;
}
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 54f9e6c..b43114c 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -615,17 +615,15 @@ out_unlock:
* RECLAIM: Try to reclaim a lock
*/
int
-nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl)
+nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
+ struct nlm_rqst *req)
{
- struct nlm_rqst reqst, *req;
int status;
- req = &reqst;
memset(req, 0, sizeof(*req));
locks_init_lock(&req->a_args.lock.fl);
locks_init_lock(&req->a_res.lock.fl);
req->a_host = host;
- req->a_flags = 0;
/* Set up the argument struct */
nlmclnt_setlockargs(req, fl);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index f5a051a..a395f1e 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -212,7 +212,8 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
__be32 nlmclnt_grant(const struct sockaddr *addr,
const struct nlm_lock *lock);
void nlmclnt_recovery(struct nlm_host *);
-int nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
+int nlmclnt_reclaim(struct nlm_host *, struct file_lock *,
+ struct nlm_rqst *);
void nlmclnt_next_cookie(struct nlm_cookie *);
/*
--
1.7.9.5
Even though nlmclnt_reclaim() is only one call into the stack frame,
928 bytes on the stack seems like a lot. Recode to dynamically
allocate the request structure once from within the reclaimer task,
then pass this pointer into nlmclnt_reclaim() for reuse on
subsequent calls.
smatch analysis:
fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
928 bytes on stack
Also remove redundant assignment of 0 after memset.
Cc: Trond Myklebust <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: [email protected]
Signed-off-by: Tim Gardner <[email protected]>
---
Changes from v1 -- don't return -ENOMEM from a task thread because it is
not propagated to the task creator. Instead print an error message and return.
fs/lockd/clntlock.c | 12 +++++++++++-
fs/lockd/clntproc.c | 6 ++----
include/linux/lockd/lockd.h | 3 ++-
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 4885b53..6cd673d 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -220,10 +220,19 @@ reclaimer(void *ptr)
{
struct nlm_host *host = (struct nlm_host *) ptr;
struct nlm_wait *block;
+ struct nlm_rqst *req;
struct file_lock *fl, *next;
u32 nsmstate;
struct net *net = host->net;
+ req = kmalloc(sizeof(*req), GFP_KERNEL);
+ if (!req) {
+ printk(KERN_ERR "lockd: reclaimer unable to alloc memory."
+ " Locks for %s won't be reclaimed!\n",
+ host->h_name);
+ return 0;
+ }
+
allow_signal(SIGKILL);
down_write(&host->h_rwsem);
@@ -253,7 +262,7 @@ restart:
*/
if (signalled())
continue;
- if (nlmclnt_reclaim(host, fl) != 0)
+ if (nlmclnt_reclaim(host, fl, req) != 0)
continue;
list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
if (host->h_nsmstate != nsmstate) {
@@ -279,5 +288,6 @@ restart:
/* Release host handle after use */
nlmclnt_release_host(host);
lockd_down(net);
+ kfree(req);
return 0;
}
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 54f9e6c..b43114c 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -615,17 +615,15 @@ out_unlock:
* RECLAIM: Try to reclaim a lock
*/
int
-nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl)
+nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
+ struct nlm_rqst *req)
{
- struct nlm_rqst reqst, *req;
int status;
- req = &reqst;
memset(req, 0, sizeof(*req));
locks_init_lock(&req->a_args.lock.fl);
locks_init_lock(&req->a_res.lock.fl);
req->a_host = host;
- req->a_flags = 0;
/* Set up the argument struct */
nlmclnt_setlockargs(req, fl);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index f5a051a..a395f1e 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -212,7 +212,8 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
__be32 nlmclnt_grant(const struct sockaddr *addr,
const struct nlm_lock *lock);
void nlmclnt_recovery(struct nlm_host *);
-int nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
+int nlmclnt_reclaim(struct nlm_host *, struct file_lock *,
+ struct nlm_rqst *);
void nlmclnt_next_cookie(struct nlm_cookie *);
/*
--
1.7.9.5
On Tue, 12 Feb 2013 12:33:15 -0700
Tim Gardner <[email protected]> wrote:
> Even though nlmclnt_reclaim() is only one call into the stack frame,
> 928 bytes on the stack seems like a lot. Recode to dynamically
> allocate the request structure once from within the reclaimer task,
> then pass this pointer into nlmclnt_reclaim() for reuse on
> subsequent calls.
>
> smatch analysis:
>
> fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
> 928 bytes on stack
>
> Also remove redundant assignment of 0 after memset.
>
> Cc: Trond Myklebust <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> fs/lockd/clntlock.c | 8 +++++++-
> fs/lockd/clntproc.c | 6 ++----
> include/linux/lockd/lockd.h | 3 ++-
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 4885b53..5dd23ef 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -220,10 +220,15 @@ reclaimer(void *ptr)
> {
> struct nlm_host *host = (struct nlm_host *) ptr;
> struct nlm_wait *block;
> + struct nlm_rqst *req;
> struct file_lock *fl, *next;
> u32 nsmstate;
> struct net *net = host->net;
>
> + req = kmalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
The basic idea here seems sound, but if this allocation fails then the
user will have no indication that lock reclaim didn't work. Might it
make sense to emit a printk like the one in nlmclnt_recovery() in this
case?
> allow_signal(SIGKILL);
>
> down_write(&host->h_rwsem);
> @@ -253,7 +258,7 @@ restart:
> */
> if (signalled())
> continue;
> - if (nlmclnt_reclaim(host, fl) != 0)
> + if (nlmclnt_reclaim(host, fl, req) != 0)
> continue;
> list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
> if (host->h_nsmstate != nsmstate) {
> @@ -279,5 +284,6 @@ restart:
> /* Release host handle after use */
> nlmclnt_release_host(host);
> lockd_down(net);
> + kfree(req);
> return 0;
> }
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 54f9e6c..b43114c 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -615,17 +615,15 @@ out_unlock:
> * RECLAIM: Try to reclaim a lock
> */
> int
> -nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl)
> +nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
> + struct nlm_rqst *req)
> {
> - struct nlm_rqst reqst, *req;
> int status;
>
> - req = &reqst;
> memset(req, 0, sizeof(*req));
> locks_init_lock(&req->a_args.lock.fl);
> locks_init_lock(&req->a_res.lock.fl);
> req->a_host = host;
> - req->a_flags = 0;
>
> /* Set up the argument struct */
> nlmclnt_setlockargs(req, fl);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index f5a051a..a395f1e 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -212,7 +212,8 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
> __be32 nlmclnt_grant(const struct sockaddr *addr,
> const struct nlm_lock *lock);
> void nlmclnt_recovery(struct nlm_host *);
> -int nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
> +int nlmclnt_reclaim(struct nlm_host *, struct file_lock *,
> + struct nlm_rqst *);
> void nlmclnt_next_cookie(struct nlm_cookie *);
>
> /*
On Wed, 13 Feb 2013 08:40:16 -0700
Tim Gardner <[email protected]> wrote:
> Even though nlmclnt_reclaim() is only one call into the stack frame,
> 928 bytes on the stack seems like a lot. Recode to dynamically
> allocate the request structure once from within the reclaimer task,
> then pass this pointer into nlmclnt_reclaim() for reuse on
> subsequent calls.
>
> smatch analysis:
>
> fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
> 928 bytes on stack
>
> Also remove redundant assignment of 0 after memset.
>
> Cc: Trond Myklebust <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Tim Gardner <[email protected]>
> ---
>
> Changes from v1 -- don't return -ENOMEM from a task thread because it is
> not propagated to the task creator. Instead print an error message and return.
>
> fs/lockd/clntlock.c | 12 +++++++++++-
> fs/lockd/clntproc.c | 6 ++----
> include/linux/lockd/lockd.h | 3 ++-
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 4885b53..6cd673d 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -220,10 +220,19 @@ reclaimer(void *ptr)
> {
> struct nlm_host *host = (struct nlm_host *) ptr;
> struct nlm_wait *block;
> + struct nlm_rqst *req;
> struct file_lock *fl, *next;
> u32 nsmstate;
> struct net *net = host->net;
>
> + req = kmalloc(sizeof(*req), GFP_KERNEL);
> + if (!req) {
> + printk(KERN_ERR "lockd: reclaimer unable to alloc memory."
> + " Locks for %s won't be reclaimed!\n",
> + host->h_name);
> + return 0;
> + }
> +
> allow_signal(SIGKILL);
>
> down_write(&host->h_rwsem);
> @@ -253,7 +262,7 @@ restart:
> */
> if (signalled())
> continue;
> - if (nlmclnt_reclaim(host, fl) != 0)
> + if (nlmclnt_reclaim(host, fl, req) != 0)
> continue;
> list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
> if (host->h_nsmstate != nsmstate) {
> @@ -279,5 +288,6 @@ restart:
> /* Release host handle after use */
> nlmclnt_release_host(host);
> lockd_down(net);
> + kfree(req);
> return 0;
> }
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 54f9e6c..b43114c 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -615,17 +615,15 @@ out_unlock:
> * RECLAIM: Try to reclaim a lock
> */
> int
> -nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl)
> +nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
> + struct nlm_rqst *req)
> {
> - struct nlm_rqst reqst, *req;
> int status;
>
> - req = &reqst;
> memset(req, 0, sizeof(*req));
> locks_init_lock(&req->a_args.lock.fl);
> locks_init_lock(&req->a_res.lock.fl);
> req->a_host = host;
> - req->a_flags = 0;
>
> /* Set up the argument struct */
> nlmclnt_setlockargs(req, fl);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index f5a051a..a395f1e 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -212,7 +212,8 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
> __be32 nlmclnt_grant(const struct sockaddr *addr,
> const struct nlm_lock *lock);
> void nlmclnt_recovery(struct nlm_host *);
> -int nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
> +int nlmclnt_reclaim(struct nlm_host *, struct file_lock *,
> + struct nlm_rqst *);
> void nlmclnt_next_cookie(struct nlm_cookie *);
>
> /*
Oops, just noticed there was a later version of this patch. Please
disregard my earlier mail...
Reviewed-by: Jeff Layton <[email protected]>
On Thu, Feb 14, 2013 at 10:20:38AM -0500, Jeff Layton wrote:
> On Wed, 13 Feb 2013 08:40:16 -0700
> Tim Gardner <[email protected]> wrote:
>
> > Even though nlmclnt_reclaim() is only one call into the stack frame,
> > 928 bytes on the stack seems like a lot. Recode to dynamically
> > allocate the request structure once from within the reclaimer task,
> > then pass this pointer into nlmclnt_reclaim() for reuse on
> > subsequent calls.
> >
> > smatch analysis:
> >
> > fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
> > 928 bytes on stack
> >
> > Also remove redundant assignment of 0 after memset.
> >
> > Cc: Trond Myklebust <[email protected]>
> > Cc: "J. Bruce Fields" <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Tim Gardner <[email protected]>
> > ---
> >
> > Changes from v1 -- don't return -ENOMEM from a task thread because it is
> > not propagated to the task creator. Instead print an error message and return.
...
> Oops, just noticed there was a later version of this patch. Please
> disregard my earlier mail...
>
> Reviewed-by: Jeff Layton <[email protected]>
Thanks, applying.--b.
On Tue, Feb 12, 2013 at 12:33:15PM -0700, Tim Gardner wrote:
> Even though nlmclnt_reclaim() is only one call into the stack frame,
> 928 bytes on the stack seems like a lot. Recode to dynamically
> allocate the request structure once from within the reclaimer task,
> then pass this pointer into nlmclnt_reclaim() for reuse on
> subsequent calls.
>
> smatch analysis:
>
> fs/lockd/clntproc.c:620 nlmclnt_reclaim() warn: 'reqst' puts
> 928 bytes on stack
>
> Also remove redundant assignment of 0 after memset.
>
> Cc: Trond Myklebust <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Tim Gardner <[email protected]>
> ---
> fs/lockd/clntlock.c | 8 +++++++-
> fs/lockd/clntproc.c | 6 ++----
> include/linux/lockd/lockd.h | 3 ++-
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
> index 4885b53..5dd23ef 100644
> --- a/fs/lockd/clntlock.c
> +++ b/fs/lockd/clntlock.c
> @@ -220,10 +220,15 @@ reclaimer(void *ptr)
> {
> struct nlm_host *host = (struct nlm_host *) ptr;
> struct nlm_wait *block;
> + struct nlm_rqst *req;
> struct file_lock *fl, *next;
> u32 nsmstate;
> struct net *net = host->net;
>
> + req = kmalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
It caught my eye that this function has never actually returned anything
but 0 before....
But OK I don't think there's a problem there: it could already fail to
reclaim individual locks due to allocation or other failures, and could
fail to run entirely if kthread_run failed.
But maybe we should log the same "Locks for %s won't be reclaimed!"
error in this case that we would in the case the kthread_run() in
nlmclnt_recovery fails.
--b.
> allow_signal(SIGKILL);
>
> down_write(&host->h_rwsem);
> @@ -253,7 +258,7 @@ restart:
> */
> if (signalled())
> continue;
> - if (nlmclnt_reclaim(host, fl) != 0)
> + if (nlmclnt_reclaim(host, fl, req) != 0)
> continue;
> list_add_tail(&fl->fl_u.nfs_fl.list, &host->h_granted);
> if (host->h_nsmstate != nsmstate) {
> @@ -279,5 +284,6 @@ restart:
> /* Release host handle after use */
> nlmclnt_release_host(host);
> lockd_down(net);
> + kfree(req);
> return 0;
> }
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 54f9e6c..b43114c 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -615,17 +615,15 @@ out_unlock:
> * RECLAIM: Try to reclaim a lock
> */
> int
> -nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl)
> +nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
> + struct nlm_rqst *req)
> {
> - struct nlm_rqst reqst, *req;
> int status;
>
> - req = &reqst;
> memset(req, 0, sizeof(*req));
> locks_init_lock(&req->a_args.lock.fl);
> locks_init_lock(&req->a_res.lock.fl);
> req->a_host = host;
> - req->a_flags = 0;
>
> /* Set up the argument struct */
> nlmclnt_setlockargs(req, fl);
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index f5a051a..a395f1e 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -212,7 +212,8 @@ int nlmclnt_block(struct nlm_wait *block, struct nlm_rqst *req, long timeout)
> __be32 nlmclnt_grant(const struct sockaddr *addr,
> const struct nlm_lock *lock);
> void nlmclnt_recovery(struct nlm_host *);
> -int nlmclnt_reclaim(struct nlm_host *, struct file_lock *);
> +int nlmclnt_reclaim(struct nlm_host *, struct file_lock *,
> + struct nlm_rqst *);
> void nlmclnt_next_cookie(struct nlm_cookie *);
>
> /*
> --
> 1.7.9.5
>