[Cluster-devel] [DLM PATCH 6/6] DLM: save / restore all socket callbacks

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 10/02/16 18:55, Bob Peterson wrote:
> Before this patch, DLM was saving off the original error report
> callback before setting its own, but it never restored it. Instead,
> we should be saving off all four socket callbacks before changing
> them, and then restore them once we're done.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/dlm/lowcomms.c | 40 +++++++++++++++++++++++++++++++++++++---
>   1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 4e82285..c196c16 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -124,7 +124,10 @@ struct connection {
>   	struct connection *othercon;
>   	struct work_struct rwork; /* Receive workqueue */
>   	struct work_struct swork; /* Send workqueue */
> -	void (*orig_error_report)(struct sock *sk);
> +	void (*orig_error_report)(struct sock *);
> +	void (*orig_data_ready)(struct sock *);
> +	void (*orig_state_change)(struct sock *);
> +	void (*orig_write_space)(struct sock *);
>   };
>   #define sock2con(x) ((struct connection *)(x)->sk_user_data)
>   
> @@ -513,6 +516,34 @@ out:
>   		orig_report(sk);
>   }
>   
> +/* Note: sk_callback_lock must be locked before calling this function. */
> +static void save_callbacks(struct connection *con, struct sock *sk)
> +{
> +	if (test_bit(CF_IS_OTHERCON, &con->flags))
> +		return;
> +	lock_sock(sk);
> +	con->orig_data_ready = sk->sk_data_ready;
> +	con->orig_state_change = sk->sk_state_change;
> +	con->orig_write_space = sk->sk_write_space;
> +	con->orig_error_report = sk->sk_error_report;
> +	release_sock(sk);
> +}
> +
> +static void restore_callbacks(struct connection *con, struct sock *sk)
> +{
> +	if (test_bit(CF_IS_OTHERCON, &con->flags))
> +		return;
> +	write_lock_bh(&sk->sk_callback_lock);
> +	lock_sock(sk);
> +	sk->sk_user_data = NULL;
> +	sk->sk_data_ready = con->orig_data_ready;
> +	sk->sk_state_change = con->orig_state_change;
> +	sk->sk_write_space = con->orig_write_space;
> +	sk->sk_error_report = con->orig_error_report;
> +	release_sock(sk);
> +	write_unlock_bh(&sk->sk_callback_lock);
> +}
> +
Might be clearer to move the test for CF_IS_OTHERCON outside of these 
functions and into the callers?

Otherwise these patches look like a good set of clean ups,

Steve.

>   /* Make a socket active */
>   static void add_sock(struct socket *sock, struct connection *con)
>   {
> @@ -521,13 +552,13 @@ static void add_sock(struct socket *sock, struct connection *con)
>   	write_lock_bh(&sk->sk_callback_lock);
>   	con->sock = sock;
>   
> +	sk->sk_user_data = con;
> +	save_callbacks(con, sk);
>   	/* Install a data_ready callback */
>   	sk->sk_data_ready = lowcomms_data_ready;
>   	sk->sk_write_space = lowcomms_write_space;
>   	sk->sk_state_change = lowcomms_state_change;
> -	sk->sk_user_data = con;
>   	sk->sk_allocation = GFP_NOFS;
> -	con->orig_error_report = sk->sk_error_report;
>   	sk->sk_error_report = lowcomms_error_report;
>   	write_unlock_bh(&sk->sk_callback_lock);
>   }
> @@ -564,6 +595,7 @@ static void close_connection(struct connection *con, bool and_other,
>   
>   	mutex_lock(&con->sock_mutex);
>   	if (con->sock) {
> +		restore_callbacks(con, con->sock->sk);
>   		sock_release(con->sock);
>   		con->sock = NULL;
>   	}
> @@ -1205,6 +1237,8 @@ static struct socket *tcp_create_listen_sock(struct connection *con,
>   	if (result < 0) {
>   		log_print("Failed to set SO_REUSEADDR on socket: %d", result);
>   	}
> +	sock->sk->sk_user_data = con;
> +
>   	con->rx_action = tcp_accept_from_sock;
>   	con->connect_action = tcp_connect_to_sock;
>