Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Ido Schimmel" <idosch@idosch.org>
Cc:     "J. Bruce Fields" <bfields@fieldses.org>,
        "Chuck Lever" <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 13/20] lockd: move lockd_start_svc() call into lockd_create_svc()
In-reply-to: <ZHoO3GRH6h/bcRjm@shredder>
References: <163816133466.32298.13831616524908720974.stgit@noble.brown>,
 <163816148560.32298.15560175172815507979.stgit@noble.brown>,
 <ZHoO3GRH6h/bcRjm@shredder>
Date:   Sat, 03 Jun 2023 07:28:28 +1000
Message-id: <168574130862.10905.10707785007987424080@noble.neil.brown.name>
Precedence: bulk

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.
> >=20
> > 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.
> >=20
> > 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().
>=20
> Hi,
>=20
> 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.
>=20
> 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@suse.de>
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@idosch.org>
Fixes: b73a2972041b ("lockd: move lockd_start_svc() call into lockd_create_sv=
c()")
Signed-off-by: NeilBrown <neilb@suse.de>
---
 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;
=20
 	if (nlmsvc_serv) {
-		svc_get(nlmsvc_serv);
 		nlmsvc_users++;
 		return 0;
 	}
--=20
2.40.1