Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <4cc5b614a2b8c4bae3aa1fe271f6915bd538ba22.camel@kernel.org>
Subject: Re: [PATCH] lockd: hold a reference to nlmsvc_serv while stopping
 thread.
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>, Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-nfs@vger.kernel.org, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>
Date:   Tue, 10 Oct 2023 08:10:12 -0400
In-Reply-To: <169689454310.26263.15848180396022999880@noble.neil.brown.name>
References: <169689454310.26263.15848180396022999880@noble.neil.brown.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.module_f38+17164+63eeee4a) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-10-10 at 10:35 +1100, NeilBrown wrote:
> We are required to hold a reference to the svc_serv struct while
> stopping the last thread, as doing that could otherwise drop the last
> reference itself and the svc_serv would be freed while still in use.
>=20
> lockd doesn't do this.  After startup, the only reference is held by the
> running thread.
>=20
> So change locked to hold a reference on nlmsvc_serv while-ever the
> service is active, and only drop it after the last thread has been
> stopped.
>=20
> Note: it doesn't really make sense for threads to hold references to the
> svc_serv any more.  The fact threads are included in serv->sv_nrthreads
> is sufficient.  Maybe a future patch could address this.
>=20
> Reported-by: Jeff Layton <jlayton@kernel.org>
> Fixes: 68cc388c3238 ("SUNRPC: change how svc threads are asked to exit.")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>=20
> Thanks for the report Jeff !!!
>=20
>  fs/lockd/svc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>=20
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index b441c706c2b8..7a5c90a00522 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -345,10 +345,10 @@ static int lockd_get(void)
> =20
>  	serv->sv_maxconn =3D nlm_max_connections;
>  	error =3D svc_set_num_threads(serv, NULL, 1);
> -	/* The thread now holds the only reference */
> -	svc_put(serv);
> -	if (error < 0)
> +	if (error < 0) {
> +		svc_put(serv);
>  		return error;
> +	}
> =20
>  	nlmsvc_serv =3D serv;
>  	register_inetaddr_notifier(&lockd_inetaddr_notifier);
> @@ -374,6 +374,7 @@ static void lockd_put(void)
> =20
>  	svc_set_num_threads(nlmsvc_serv, NULL, 0);
>  	timer_delete_sync(&nlmsvc_retry);
> +	svc_put(nlmsvc_serv);
>  	nlmsvc_serv =3D NULL;
>  	dprintk("lockd_down: service destroyed\n");
>  }

Thanks Neil! You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Tested-by: Jeff Layton <jlayton@kernel.org>