2023-09-19 15:36:51

by Chuck Lever

Subject: [PATCH v1] SUNRPC: Remove BUG_ON call sites

From: Chuck Lever <[email protected]>

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 <[email protected]>
Acked-by: Christian Brauner <[email protected]>
---
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);

2023-09-19 22:30:16

by NeilBrown

Subject: Re: [PATCH v1] SUNRPC: Remove BUG_ON call sites

On Wed, 20 Sep 2023, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Christian Brauner <[email protected]>
> ---
> 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 <[email protected]>

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