Misc DLM Improvements Regarding Socket Errors

This patch removes a "return" statement from lowcomms_error_report. It need to call the original error report in all paths through the function.

Signed-off-by Bob Peterson <[email protected]>

Function tcp_create_listen_sock has two error paths. One of them was setting con->sock to NULL. The other was not. This patch changes it to be consistent and do the same thing for both error paths.

Signed-off-by Bob Peterson <[email protected]>

---
 fs/dlm/lowcomms.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

fs/dlm/lowcomms.c +1 -7

index e740326..3b780f0 100644
@@ -1199,10 +1199,7 @@ static struct socket *tcp_create_listen_sock(struct connection *con,
        result = sock->ops->bind(sock, (struct sockaddr *) saddr, addr_len);
        if (result < 0) {
                log_print("Can't bind to port %d", dlm_config.ci_tcp_port);
-               sock_release(sock);
-               sock = NULL;
-               con->sock = NULL;
-               goto create_out;
+               goto out_err;
        }
        result = kernel_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
                                 (char *)&one, sizeof(one));

@@ -1213,13 +1210,18 @@ static struct socket *tcp_create_listen_sock(struct connection *con,
        result = sock->ops->listen(sock, 5);
        if (result < 0) {
                log_print("Can't listen on port %d", dlm_config.ci_tcp_port);
-               sock_release(sock);
-               sock = NULL;
-               goto create_out;

fs/dlm/lowcomms.c static struct socket *tcp_create_listen_sock(struct connection *con, · line 1222

Quoted 4 lines

fs/dlm/lowcomms.c +4 -0

+out_err:
+       sock_release(sock);
+       sock = NULL;
+       con->sock = NULL;

Consolidating the error paths makes sense, but con->sock shouldn't be set here at all; the caller does that in add_sock().

Thanks,
Andreas

----- Original Message -----

Quoted 5 lines

Quoted 4 lines

(quoted diff) +4 -0

+out_err:
+       sock_release(sock);
+       sock = NULL;
+       con->sock = NULL;

Quoted 2 lines

Consolidating the error paths makes sense, but con->sock shouldn't be set here at all; the caller does that in add_sock().

I disagree.

The caller doesn't call add_sock() in the error case, which is where we're setting con->sock to NULL.

This patch simply removes a goto from function sctp_listen_for_all. The end result is the same, but makes the code more readable.

Signed-off-by Bob Peterson <[email protected]>

---
 fs/dlm/lowcomms.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/dlm/lowcomms.c +1 -3

index 3b780f0..ec5087a 100644
@@ -1261,7 +1261,7 @@ static int sctp_listen_for_all(void)
                                  SOCK_STREAM, IPPROTO_SCTP, &sock);
        if (result < 0) {
                log_print("Can't create comms socket, check SCTP is loaded");
-               goto out;
+               return result;
        }
 
        result = kernel_setsockopt(sock, SOL_SOCKET, SO_RCVBUFFORCE,

@@ -1296,7 +1296,6 @@ static int sctp_listen_for_all(void)
 create_delsock:
        sock_release(sock);
        con->sock = NULL;
-out:
        return result;
 }
 
--

2.5.0

This patch adds write_lock_bh locking to several places in the code that save and restore the socket callbacks.

Signed-off-by Bob Peterson <[email protected]>

---
 fs/dlm/lowcomms.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

fs/dlm/lowcomms.c +2 -1

index ec5087a..4e82285 100644
@@ -467,10 +467,17 @@ int dlm_lowcomms_connect_node(int nodeid)
 
 static void lowcomms_error_report(struct sock *sk)
 {
-       struct connection *con = sock2con(sk);
+       struct connection *con;
        struct sockaddr_storage saddr;
        int buflen;
+       void (*orig_report)(struct sock *) = NULL;

fs/dlm/lowcomms.c +18 -9

@@ -500,22 +507,29 @@ 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;
+       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);
 }
 
 /* Add the port number to an IPv6 or 4 sockaddr and return the address

@@ -1274,6 +1288,7 @@ 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;
        con->sock = sock;

@@ -1281,6 +1296,8 @@ static int sctp_listen_for_all(void)
        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;
--

2.5.0

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 <[email protected]>

---
  fs/dlm/lowcomms.c | 40 +++++++++++++++++++++++++++++++++++++---
  1 file changed, 37 insertions(+), 3 deletions(-)

fs/dlm/lowcomms.c +22 -1

index 4e82285..c196c16 100644
@@ -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);

fs/dlm/lowcomms.c +5 -2

  /* 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;
 

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.

Hi Steve,

----- Original Message -----

Quoted 6 lines

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 <[email protected]>

I've been doing a bunch of recovery testing with DLM and discovered some issues. This collection of 6 patches addresses those issues. Some of them are of my own making, introduced by the recent patches that made DLM print socket connection errors, and recovery from those errors.

The first patch changes the TCP "connect to sock" function to more closely match the SCTP version of the function. The idea is to not create a kernel socket until we have a valid node address, like it does in the SCTP path.

The second patch removes a "return" from lowcomms_error_report that should not be there. The return was causing it to bypass calling the original error report code, thus skipping an important part in the reporting.

The third patch changes function tcp_create_listen_sock so that its error path is consistent. Only one of its error paths was setting con->sock to NULL, but it should be done in both cases.

The fourth patch eliminates a useless goto, to make the code more clear.

The fifth patch adds a layer of locking by way of the sk->sk_callback_lock which is needed to prevent multiple send/receive sockets from interfering with one another when reporting the socket errors and subsequent recovery. This makes it similar to how sunrpc handles errors.

The sixth and final patch makes the socket error code save and restore all four callbacks, whereas before we were only saving and restoring the error report callback.

I've been doing a bunch of recovery testing with DLM and discovered some issues. This collection of 6 patches addresses those issues. Some of them are of my own making, introduced by the recent patches that made DLM print socket connection errors, and recovery from those errors.

Quoted 4 lines

I've been doing a bunch of recovery testing with DLM and discovered some issues. This collection of 6 patches addresses those issues. Some of them are of my own making, introduced by the recent patches that made DLM print socket connection errors, and recovery from those errors.

Thanks Bob, perhaps I've not been paying close enough attention, but it's unclear to me how this patch set relates the the most accute issue we have at the moment, which are the problems introduced here:

From b3a5bbfd780d9e9291f5f257be06e9ad6db11657 Mon Sep 17 00:00:00 2001
From: Bob Peterson <[email protected]>
Date: Thu, 27 Aug 2015 09:34:47 -0500
Subject: [PATCH] dlm: print error from kernel_sendpage

Print a dlm-specific error when a socket error occurs when sending a dlm message.

Signed-off-by Bob Peterson <[email protected]>

Signed-off-by David Teigland <[email protected]>

Could we begin with one patch that's easy to track that directly resolves the issues with that commit (perhaps even a revert if it's not simple to fix directly)? That brings us back to a known-good place, from which we can look at cleanups and changes.

The problem is: While testing the dlm in multiple recovery situations, Nate and I discovered multiple problems. Until recently, no one has tried to run recovery tests on an upstream DLM,

I agree that some of these patches might be unnecessary improvements. I'll try to pare them down to what is absolutely necessary and what is not. I'll also document exactly why the necessary ones are needed.

I'll also try to post them in order of highest priority and repost them as individual patches rather than a set.

The recovery tests are somewhat slow, so this will take some time.

BTW, Have you had a chance to look at the patch I posted on 18 January, titled "DLM: Replace nodeid_to_addr with kernel_getpeername"? That definitely fixes one bug in patch b3a5bbfd which you mentioned.

I assume you're not suggesting I combine that patch with other patches to stabilize b3a5bbfd, right? As you well know, this is very touchy code and it's easier to diagnose and debug a larger number of smaller patches.

----- Original Message -----

Quoted 18 lines · Bob Peterson

Quoted 4 lines

I've been doing a bunch of recovery testing with DLM and discovered some issues. This collection of 6 patches addresses those issues. Some of them are of my own making, introduced by the recent patches that made DLM print socket connection errors, and recovery from those errors.

Thanks Bob, perhaps I've not been paying close enough attention, but it's unclear to me how this patch set relates the the most accute issue we have at the moment, which are the problems introduced here:

From b3a5bbfd780d9e9291f5f257be06e9ad6db11657 Mon Sep 17 00:00:00 2001
From: Bob Peterson <[email protected]>
Date: Thu, 27 Aug 2015 09:34:47 -0500
Subject: [PATCH] dlm: print error from kernel_sendpage

Print a dlm-specific error when a socket error occurs when sending a dlm message.

Quoted 8 lines

Signed-off-by Bob Peterson <[email protected]>

Signed-off-by David Teigland <[email protected]>

Could we begin with one patch that's easy to track that directly resolves the issues with that commit (perhaps even a revert if it's not simple to fix directly)? That brings us back to a known-good place, from which we can look at cleanups and changes.

Hi Dave,

My goal has always been to attain stability, which I think I've finally achieved.

The problem is: While testing the dlm in multiple recovery situations, Nate and I discovered multiple problems. Until recently, no one has tried to run recovery tests on an upstream DLM, so I think we're finding some old bugs that have been there for a while, as well as bugs with b3a5bbfd, which you mentioned.

I agree that some of these patches might be unnecessary improvements. I'll try to pare them down to what is absolutely necessary and what is not. I'll also document exactly why the necessary ones are needed.

I think the "right thing to do" at this point is this:

1. Patch #1 is already upstream
2. Patch #2 stands on its own, so I think this should go forward.
3. Combine patches 3, 4 and 5, which ought to provide a comprehensive fix for the other problems listed in #1.
4. The rest of the patches, I can post as separate patches because they are code cleanups, not related to the original problems of #1.

Let me know your thoughts on the subject. If you like this plan, I can re-test and post replacement patches tomorrow (hopefully).