Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever" <cel@kernel.org>
Cc:     brauner@kernel.org, "Chuck Lever" <chuck.lever@oracle.com>,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH v1] SUNRPC: Remove BUG_ON call sites
In-reply-to: <169513768769.145733.5037542987990908432.stgit@manet.1015granger.net>
References: <169513768769.145733.5037542987990908432.stgit@manet.1015granger.net>
Date:   Wed, 20 Sep 2023 08:11:01 +1000
Message-id: <169516146143.19404.11284116898963519832@noble.neil.brown.name>
Precedence: bulk

On Wed, 20 Sep 2023, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> There is no need to take down the whole system for these assertions.
> 
> I'd rather not attempt a heroic save here, as some bug has occurred
> that has left the transport data structures in an unknown state.
> Just warn and then leak the left-over resources.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
> ---
>  net/sunrpc/svc.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Changes since v1:
> - Use WARN_ONCE() instead of pr_warn()
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 587811a002c9..3237f7dfde1e 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -575,11 +575,12 @@ svc_destroy(struct kref *ref)
>  	timer_shutdown_sync(&serv->sv_temptimer);
>  
>  	/*
> -	 * The last user is gone and thus all sockets have to be destroyed to
> -	 * the point. Check this.
> +	 * Remaining transports at this point are not expected.
>  	 */
> -	BUG_ON(!list_empty(&serv->sv_permsocks));
> -	BUG_ON(!list_empty(&serv->sv_tempsocks));
> +	WARN_ONCE(!list_empty(&serv->sv_permsocks),
> +		  "SVC: permsocks remain for %s\n", serv->sv_program->pg_name);
> +	WARN_ONCE(!list_empty(&serv->sv_tempsocks),
> +		  "SVC: tempsocks remain for %s\n", serv->sv_program->pg_name);
>  
>  	cache_clean_deferred(serv);
>  
> 

Reviewed-by: NeilBrown <neilb@suse.de>

The stack trace might not be helpful, but this circumstance really
really shouldn't happen so if it ever does, I think we really want as
much context as practicable.

Thanks,
NeilBrown