From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Thu, 11 Feb 2016 15:31:04 +0000 Subject: [Cluster-devel] [DLM PATCH 6/6] DLM: save / restore all socket callbacks In-Reply-To: <1455130532-9317-7-git-send-email-rpeterso@redhat.com> References: <1455130532-9317-1-git-send-email-rpeterso@redhat.com> <1455130532-9317-7-git-send-email-rpeterso@redhat.com> Message-ID: <56BCA938.8030208@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > --- > 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; >