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

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

[Bob Peterson]

Hi,

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.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index dc9ae6d..f9eb366 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)
 
@@ -465,12 +468,43 @@ int dlm_lowcomms_connect_node(int nodeid)
 	return 0;
 }
 
-static void lowcomms_error_report(struct sock *sk)
+static void save_old_callbacks(struct sock *sk)
+{
+	struct connection *con = sock2con(sk);
+
+	if (sk->sk_error_report) {
+		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;
+	}
+}
+
+static void restore_old_callbacks(struct sock *sk)
 {
 	struct connection *con = sock2con(sk);
+
+	if (con->orig_error_report) {
+		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;
+	}
+}
+
+static void lowcomms_error_report(struct sock *sk)
+{
+	struct connection *con;
 	struct sockaddr_storage saddr;
 	int buflen;
+	void (*orig_report)(struct sock *) = NULL;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	con = sock2con(sk);
+	if (con == NULL)
+		goto out;
 
+	orig_report = con->orig_error_report;
 	if (con->sock == NULL ||
 	    kernel_getpeername(con->sock, (struct sockaddr *)&saddr, &buflen)) {
 		printk_ratelimited(KERN_ERR "dlm: node %d: socket error "
@@ -478,7 +512,6 @@ static void lowcomms_error_report(struct sock *sk)
 				   "sk_err=%d/%d\n", dlm_our_nodeid(),
 				   con->nodeid, dlm_config.ci_tcp_port,
 				   sk->sk_err, sk->sk_err_soft);
-		return;
 	} else if (saddr.ss_family == AF_INET) {
 		struct sockaddr_in *sin4 = (struct sockaddr_in *)&saddr;
 
@@ -501,22 +534,31 @@ static void lowcomms_error_report(struct sock *sk)
 				   dlm_config.ci_tcp_port, sk->sk_err,
 				   sk->sk_err_soft);
 	}
-	con->orig_error_report(sk);
+out:
+	read_unlock_bh(&sk->sk_callback_lock);
+	if (orig_report)
+		orig_report(sk);
 }
 
 /* Make a socket active */
 static void add_sock(struct socket *sock, struct connection *con)
 {
+	struct sock *sk = sock->sk;
+
+	write_lock_bh(&sk->sk_callback_lock);
 	con->sock = sock;
 
-	/* Install a data_ready callback */
-	con->sock->sk->sk_data_ready = lowcomms_data_ready;
-	con->sock->sk->sk_write_space = lowcomms_write_space;
-	con->sock->sk->sk_state_change = lowcomms_state_change;
-	con->sock->sk->sk_user_data = con;
-	con->sock->sk->sk_allocation = GFP_NOFS;
-	con->orig_error_report = con->sock->sk->sk_error_report;
-	con->sock->sk->sk_error_report = lowcomms_error_report;
+	if (sk->sk_error_report != lowcomms_error_report) {
+		sk->sk_user_data = con;
+		save_old_callbacks(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_allocation = GFP_NOFS;
+		sk->sk_error_report = lowcomms_error_report;
+	}
+	write_unlock_bh(&sk->sk_callback_lock);
 }
 
 /* Add the port number to an IPv6 or 4 sockaddr and return the address
@@ -551,6 +593,13 @@ static void close_connection(struct connection *con, bool and_other,
 
 	mutex_lock(&con->sock_mutex);
 	if (con->sock) {
+		struct sock *sk = con->sock->sk;
+
+		if (sk) {
+			write_lock_bh(&sk->sk_callback_lock);
+			restore_old_callbacks(sk);
+			write_unlock_bh(&sk->sk_callback_lock);
+		}
 		sock_release(con->sock);
 		con->sock = NULL;
 	}
@@ -1131,6 +1180,10 @@ static void tcp_connect_to_sock(struct connection *con)
 	if (result == 0)
 		goto out;
 
+	write_lock_bh(&sock->sk->sk_callback_lock);
+	restore_old_callbacks(sock->sk);
+	write_unlock_bh(&sock->sk->sk_callback_lock);
+
 out_err:
 	if (con->sock) {
 		sock_release(con->sock);
@@ -1273,13 +1326,19 @@ static int sctp_listen_for_all(void)
 	if (result < 0)
 		log_print("Could not set SCTP NODELAY error %d\n", result);
 
+	write_lock_bh(&sock->sk->sk_callback_lock);
 	/* Init con struct */
 	sock->sk->sk_user_data = con;
+
+	save_old_callbacks(sock->sk);
+
 	con->sock = sock;
 	con->sock->sk->sk_data_ready = lowcomms_data_ready;
 	con->rx_action = sctp_accept_from_sock;
 	con->connect_action = sctp_connect_to_sock;
 
+	write_unlock_bh(&sock->sk->sk_callback_lock);
+
 	/* Bind to all addresses. */
 	if (sctp_bind_addrs(con, dlm_config.ci_tcp_port))
 		goto create_delsock;
@@ -1293,6 +1352,9 @@ static int sctp_listen_for_all(void)
 	return 0;
 
 create_delsock:
+	write_lock_bh(&sock->sk->sk_callback_lock);
+	restore_old_callbacks(sock->sk);
+	write_unlock_bh(&sock->sk->sk_callback_lock);
 	sock_release(sock);
 	con->sock = NULL;
 out:

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

[Steven Whitehouse]

Hi,

On 26/01/16 17:57, Bob Peterson wrote:
> Hi,
>
> 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.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index dc9ae6d..f9eb366 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)
>   
> @@ -465,12 +468,43 @@ int dlm_lowcomms_connect_node(int nodeid)
>   	return 0;
>   }
>   
> -static void lowcomms_error_report(struct sock *sk)
> +static void save_old_callbacks(struct sock *sk)
> +{
> +	struct connection *con = sock2con(sk);
> +
> +	if (sk->sk_error_report) {
> +		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;
> +	}
> +}
> +
This should not need to be conditional, because sk->sk_error_report() 
should never be NULL, so I think that needs looking at again - where did 
that NULL pointer come from?

Also the other callbacks, data_ready, etc should also have the same 
locking pattern of read_lock_bh(), con = sock2con(sk); if (con) { /* 
original code here */ } read_unlock_bh(); as per the error_report one,

Steve.