Re: [Cluster-devel] [DLM PATCH 6/6][try #2] DLM: save / restore all socket callbacks

[Bob Peterson]

Hi Steve,

- Original Message -
> 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.

Good idea. Here's a replacement patch that implements your suggestion.

Regards,

Bob Peterson
Red Hat File Systems
---
DLM: save / restore all socket callbacks

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 

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 4e82285..aa9371e 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,30 @@ 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)
+{
+   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)
+{
+   write_lock_bh(>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_callback_lock);
+}
+
 /* Make a socket active */
 static void add_sock(struct socket *sock, struct connection *con)
 {
@@ -521,13 +548,14 @@ static void add_sock(struct socket *sock, struct 
connection *con)
write_lock_bh(>sk_callback_lock);
con->sock = sock;
 
+   sk->sk_user_data = con;
+   if (!test_bit(CF_IS_OTHERCON, >flags))
+   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_callback_lock);
 }
@@ -564,6 +592,8 @@ static void close_connection(struct connection *con, bool 
and_other,
 
mutex_lock(>sock_mutex);
if (con->sock) {
+   if (!test_bit(CF_IS_OTHERCON, >flags))
+   restore_callbacks(con, con->sock->sk);
sock_release(con->sock);
con->sock = NULL;
}
@@ -1205,6 +1235,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;
 

Re: [Cluster-devel] [DLM PATCH 6/6][try #2] DLM: save / restore all socket callbacks

[Andreas Gruenbacher]

On Thu, Feb 11, 2016 at 5:43 PM, Bob Peterson  wrote:
> Hi Steve,
>
> - Original Message -
>> 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.
>
> Good idea. Here's a replacement patch that implements your suggestion.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
> ---
> DLM: save / restore all socket callbacks
>
> 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 

Reviewed-by: Andreas Gruenbacher