Re: [PATCH 4/9] libceph: fix mutex coverage for ceph_con_close

[Alex Elder <elder@inktank.com>]

On 07/20/2012 07:41 PM, Sage Weil wrote:
> Hold the mutex while twiddling all of the state bits to avoid possible
> races.  While we're here, make not of why we cannot close the socket
> directly.
> 
> Signed-off-by: Sage Weil <sage@inktank.com>

Looks OK to me.  A quick scan seems to show that the
state and flag bits are *almost* always set while the
mutex is held.  The one counterexample I found was for
the STANDBY state bit in clear_standby() (but I really
didn't look very closely).

Anyway, I think this looks fine--it makes things safer
even if there could be another imperfection somewhere.

Reviewed-by: Alex Elder <elder@inktank.com>

> ---
>  net/ceph/messenger.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 7105908..e24310e 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -503,6 +503,7 @@ static void reset_connection(struct ceph_connection *con)
>   */
>  void ceph_con_close(struct ceph_connection *con)
>  {
> +	mutex_lock(&con->mutex);
>  	dout("con_close %p peer %s\n", con,
>  	     ceph_pr_addr(&con->peer_addr.in_addr));
>  	clear_bit(NEGOTIATING, &con->state);
> @@ -515,11 +516,16 @@ void ceph_con_close(struct ceph_connection *con)
>  	clear_bit(KEEPALIVE_PENDING, &con->flags);
>  	clear_bit(WRITE_PENDING, &con->flags);
>  
> -	mutex_lock(&con->mutex);
>  	reset_connection(con);
>  	con->peer_global_seq = 0;
>  	cancel_delayed_work(&con->work);
>  	mutex_unlock(&con->mutex);
> +
> +	/*
> +	 * We cannot close the socket directly from here because the
> +	 * work threads use it without holding the mutex.  Instead, let
> +	 * con_work() do it.
> +	 */
>  	queue_con(con);
>  }
>  EXPORT_SYMBOL(ceph_con_close);
>