On Sat, 03 Jun 2023, Ido Schimmel wrote:
> On Mon, Nov 29, 2021 at 03:51:25PM +1100, NeilBrown wrote:
> > lockd_start_svc() only needs to be called once, just after the svc is
> > created. If the start fails, the svc is discarded too.
> >
> > It thus makes sense to call lockd_start_svc() from lockd_create_svc().
> > This allows us to remove the test against nlmsvc_rqst at the start of
> > lockd_start_svc() - it must always be NULL.
> >
> > lockd_up() only held an extra reference on the svc until a thread was
> > created - then it dropped it. The thread - and thus the extra reference
> > - will remain until kthread_stop() is called.
> > Now that the thread is created in lockd_create_svc(), the extra
> > reference can be dropped there. So the 'serv' variable is no longer
> > needed in lockd_up().
>
> Hi,
>
> I'm seeing the following memory leak [1] after unmounting a network
> share. High level bisection shows that it started between v5.16 and
> v5.17. Using git bisect [2] I've pinpointed it to this patch.
>
> Can you please look into it? I can easily trigger the issue and test
> patches.
Thanks for the report.
Please test this patch.
Thanks,
NeilBrown
>From 4f7278cb24a0331c1d58cc502029f8bf06bd4e9d Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@xxxxxxx>
Date: Sat, 3 Jun 2023 07:14:14 +1000
Subject: [PATCH] lockd: drop inappropriate svc_get() from locked_get()
The below-mentioned patch was intended to simplify refcounting on the
svc_serv used by locked. The goal was to only ever have a single
reference from the single thread. To that end we dropped a call to
lockd_start_svc() (except when creating thread) which would take a
reference, and dropped the svc_put(serv) that would drop that reference.
Unfortunately we didn't also remove the svc_get() from
lockd_create_svc() in the case where the svc_serv already existed.
So after the patch:
- on the first call the svc_serv was allocated and the one reference
was given to the thread, so there are no extra references
- on subsequent calls svc_get() was called so there is now an extra
reference.
This is clearly not consistent.
The inconsistency is also clear in the current code in lockd_get()
takes *two* references, one on nlmsvc_serv and one by incrementing
nlmsvc_users. This clearly does not match lockd_put().
So: drop that svc_get() from lockd_get() (which used to be in
lockd_create_svc().
Reported-by: Ido Schimmel <idosch@xxxxxxxxxx>
Fixes: b73a2972041b ("lockd: move lockd_start_svc() call into lockd_create_svc()")
Signed-off-by: NeilBrown <neilb@xxxxxxx>
---
fs/lockd/svc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 04ba95b83d16..22d3ff3818f5 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -355,7 +355,6 @@ static int lockd_get(void)
int error;
if (nlmsvc_serv) {
- svc_get(nlmsvc_serv);
nlmsvc_users++;
return 0;
}
--
2.40.1
