Skip to contentSkip to conversation

rds: tcp: couple of minor bug fixes

PATCH In review Sowmini Varadhan 3 participants 6 messages Apr 1, 2017 – Apr 3, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover rds: tcp: couple of minor bug fixes

A couple of minor bugfixes that showed up during testing

Sowmini Varadhan (2):
rds: tcp: allow progress of rds_conn_shutdown if the rds_connection
is marked ERROR by an intervening FIN
rds: tcp: canonical connection order for all paths with index > 0

 net/rds/connection.c |   10 +++++++++-
 net/rds/threads.c    |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)
#1 lore raw

PATCH 1/2 rds: tcp: allow progress of rds_conn_shutdown if the rds_connection is marked ERROR by an intervening FIN

rds_conn_shutdown() runs in workq context, and marks the rds_connection as DISCONNECTING before quiescing Tx/Rx paths. However, after all I/O has quiesced, we may still find the rds_connection state to be RDS_CONN_ERROR if an intervening FIN was processed in softirq context.

This is not a fatal error: rds_conn_shutdown() should continue the shutdown, and there is no need to log noisy messages about this event.

---
 net/rds/connection.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
net/rds/connection.c +9 -1
index 1fa75ab..6a5ebde 100644
@@ -333,11 +333,19 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		rds_conn_path_reset(cp);
 
 		if (!rds_conn_path_transition(cp, RDS_CONN_DISCONNECTING,
+					      RDS_CONN_DOWN) &&
+		    !rds_conn_path_transition(cp, RDS_CONN_ERROR,
 					      RDS_CONN_DOWN)) {
 			/* This can happen - eg when we're in the middle of tearing
 			 * down the connection, and someone unloads the rds module.
-			 * Quite reproduceable with loopback connections.
+			 * Quite reproducible with loopback connections.
 			 * Mostly harmless.
+			 *
+			 * Note that this also happens with rds-tcp because
+			 * we could have triggered rds_conn_path_drop in irq
+			 * mode from rds_tcp_state change on the receipt of
+			 * a FIN, thus we need to recheck for RDS_CONN_ERROR
+			 * here.
 			 */
 			rds_conn_path_error(cp, "%s: failed to transition "
 					    "to state DOWN, current state "
Signature · 1 line
1.7.1
Acked-by on v1 1/2
#2 lore raw

Re: [PATCH net-next 1/2] rds: tcp: allow progress of rds_conn_shutdown if the rds_connection is marked ERROR by an intervening FIN

Quoted 10 lines · Sowmini Varadhan

rds_conn_shutdown() runs in workq context, and marks the rds_connection as DISCONNECTING before quiescing Tx/Rx paths. However, after all I/O has quiesced, we may still find the rds_connection state to be RDS_CONN_ERROR if an intervening FIN was processed in softirq context.

This is not a fatal error: rds_conn_shutdown() should continue the shutdown, and there is no need to log noisy messages about this event.

---

#3 lore raw

PATCH 2/2 rds: tcp: canonical connection order for all paths with index > 0

In reply to Sowmini Varadhan

The rds_connect_worker() has a bug in the check that enforces the canonical connection order described in the comments of rds_tcp_state_change(). The intention is to make sure that all the multipath connections are always initiated by the smaller IP address via rds_start_mprds. To achieve this, rds_connection_worker should check that cp_index > 0.

---
 net/rds/threads.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/rds/threads.c +1 -1
index e36e333..3e447d0 100644
@@ -156,7 +156,7 @@ void rds_connect_worker(struct work_struct *work)
 	struct rds_connection *conn = cp->cp_conn;
 	int ret;
 
-	if (cp->cp_index > 1 && cp->cp_conn->c_laddr > cp->cp_conn->c_faddr)
+	if (cp->cp_index > 0 && cp->cp_conn->c_laddr > cp->cp_conn->c_faddr)
 		return;
 	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
 	ret = rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_CONNECTING);
Signature · 1 line
1.7.1
Acked-by on v1 2/2
#4 lore raw

Re: [PATCH net-next 2/2] rds: tcp: canonical connection order for all paths with index > 0

Quoted 9 lines · Sowmini Varadhan

The rds_connect_worker() has a bug in the check that enforces the canonical connection order described in the comments of rds_tcp_state_change(). The intention is to make sure that all the multipath connections are always initiated by the smaller IP address via rds_start_mprds. To achieve this, rds_connection_worker should check that cp_index > 0.

---