RDS: RDS core bug fixes
Subsystems (3)
v2 cover RDS: RDS core bug fixes
This patch-series updates the RDS core and rds-tcp modules with some bug fixes that were originally authored by Andy Grover, Zach Brown, and Chris Mason.
v2: code review comment from Sergei Shtylyov
Sowmini Varadhan (6):
RDS: Re-add pf/sol access via sysctl
RDS: only use passive connections when addresses match
rds: check for excessive looping in rds_send_xmit
RDS: make sure not to loop forever inside rds_send_xmit
RDS: rds_send_xmit is called under a spinlock, lets not do a
cond_resched()
RDS: don't trust the LL_SEND_FULL bit
net/rds/connection.c | 3 +- net/rds/rds.h | 1 + net/rds/send.c | 56 ++++++++++++++++++++++++++++++++++++++++++++----- net/rds/sysctl.c | 20 +++++++++++++++++ net/rds/threads.c | 2 + 5 files changed, 75 insertions(+), 7 deletions(-)
PATCH v2 1/6 RDS: Re-add pf/sol access via sysctl
Although RDS has an official PF_RDS value now, existing software expects to look for rds sysctls to determine it. We need to maintain these for now, for backwards compatibility.
Original patch by Andy Grover
--- net/rds/sysctl.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
net/rds/sysctl.c +20 -0
@@ -49,6 +49,12 @@ unsigned int rds_sysctl_max_unacked_bytes = (16 << 20);unsigned int rds_sysctl_ping_enable =1 ; +/* We have official values, but must maintain the sysctl interface for existing +* software that expects to find these values here. +*/ +static int rds_sysctl_pf_rds =PF_RDS ; +static int rds_sysctl_sol_rds =SOL_RDS ; +static struct ctl_table rds_sysctl_rds_table[] = { { .procname ="reconnect_min_delay_ms" ,
@@ -69,6 +75,20 @@ static struct ctl_table rds_sysctl_rds_table[] = { .extra2 = &rds_sysctl_reconnect_max, }, { + .procname = "pf_rds", + .data = &rds_sysctl_pf_rds, + .maxlen = sizeof(int), + .mode = 0444, + .proc_handler = proc_dointvec, + }, + { + .procname = "sol_rds", + .data = &rds_sysctl_sol_rds, + .maxlen = sizeof(int), + .mode = 0444, + .proc_handler = proc_dointvec, + }, + { .procname = "max_unacked_packets", .data = &rds_sysctl_max_unacked_packets, .maxlen = sizeof(int),
Signature · 1 line
1.7.1
PATCH v2 2/6 RDS: only use passive connections when addresses match
In reply to Sowmini VaradhanPassive connections were added for the case where one loopback IB connection between identical addresses needs another connection to store the second QP. Unfortunately, they were also created in the case where the addesses differ and we already have both QPs.
This lead to a message reordering bug.
- two different IB interfaces and addresses on a machine: A B
- traffic is sent from A to B
- connection from A-B is created, connect request sent
- listening accepts connect request, B-A is created
- traffic flows, next_rx is incremented
- unacked messages exist on the retrans list
- connection A-B is shut down, new connect request sent
- listen sees existing loopback B-A, creates new passive B-A
- retrans messages are sent and delivered because of 0 next_rx
The problem is that the second connection request saw the previously existing parent connection. Instead of using it, and using the existing next_rx_seq state for the traffic between those IPs, it mistakenly thought that it had to create a passive connection.
We fix this by only using passive connections in the special case where laddr and faddr match. In this case we'll only ever have one parent sending connection requests and one passive connection created as the listening path sees the existing parent connection which initiated the request.
Original patch by Zach Brown
--- net/rds/connection.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
net/rds/connection.c +1 -1
@@ -130,7 +130,7 @@ static struct rds_connection *__rds_conn_create(__be32 laddr, __be32 faddr,rcu_read_lock (); conn =rds_conn_lookup (head, laddr, faddr, trans);if (conn && conn->c_loopback && conn->c_trans != &rds_loop_transport && - !is_outgoing) { + laddr == faddr && !is_outgoing) {/* This is a looped back IB connection, and we're * called by the code handling the incoming connect. * We need a second connection object into which we
Signature · 1 line
1.7.1
PATCH v2 3/6 rds: check for excessive looping in rds_send_xmit
In reply to Sowmini VaradhanOriginal patch by Andy Grover
--- net/rds/send.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
net/rds/send.c +13 -0
@@ -140,6 +140,7 @@ int rds_send_xmit(struct rds_connection *conn)struct scatterlist *sg;int ret =0 ;LIST_HEAD (to_be_dropped); +int same_rm =0 ; restart:
@@ -177,6 +178,18 @@ int rds_send_xmit(struct rds_connection *conn) rm = conn->c_xmit_rm; +if (!rm) { + same_rm =0 ; + }else { + same_rm++; +if (same_rm >=4096 ) { +printk_ratelimited (KERN_ERR "RDS: Stuck rm\n" ); +cond_resched (); + ret = -EAGAIN ; +break ; + } + } +/* * If between sending messages, we can send a pending congestion * map update.
Signature · 1 line
1.7.1
David Miller
maintainerRe: [PATCHv2 3/6] rds: check for excessive looping in rds_send_xmit
From: Sowmini Varadhan <[email protected]>
Date: Thu, 2 Apr 2015 10:19:37 -0400
Quoted 6 lines
net/rds/send.c +2
-0@@ -177,6 +178,18 @@ int rds_send_xmit(struct rds_connection *conn) rm = conn->c_xmit_rm; +if (!rm) { + same_rm =0 ;
net/rds/send.c int rds_send_xmit(struct rds_connection *conn) · line 192
Quoted 10 lines
net/rds/send.c +10
-0+ }else { + same_rm++; +if (same_rm >=4096 ) { +printk_ratelimited (KERN_ERR "RDS: Stuck rm\n" ); +cond_resched (); + ret = -EAGAIN ; +break ; + } + } +
I'm not accepting junk like this.
I want to know exactly how this situation can occur, in detail, and then and only then can we start to talk about more reasonable solutions to this bug.
This looks like a minimal effort hack to make a hang go away, and even more concerning is that a change like this is submitted with a commit message that is essentially empty.
Sorry, this is not acceptable.
PATCH v2 4/6 RDS: make sure not to loop forever inside rds_send_xmit
In reply to Sowmini VaradhanIf a determined set of concurrent senders keep the send queue full, we can loop forever inside rds_send_xmit. This fix has two parts.
First we are dropping out of the while(1) loop after we've processed a large batch of messages.
Second we add a generation number that gets bumped each time the xmit bit lock is acquired. If someone else has jumped in and made progress in the queue, we skip our goto restart.
Original patch by Chris Mason.
--- net/rds/connection.c | 1 + net/rds/rds.h | 1 + net/rds/send.c | 47 +++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 4 deletions(-)
net/rds/connection.c +1 -0
@@ -193,6 +193,7 @@ static struct rds_connection *__rds_conn_create(__be32 laddr, __be32 faddr, }atomic_set (&conn->c_state,RDS_CONN_DOWN ); + conn->c_send_gen =0 ; conn->c_reconnect_jiffies =0 ;INIT_DELAYED_WORK (&conn->c_send_w, rds_send_worker);INIT_DELAYED_WORK (&conn->c_recv_w, rds_recv_worker);
net/rds/rds.h +1 -0
@@ -110,6 +110,7 @@ struct rds_connection {void *c_transport_data;atomic_t c_state; +unsigned long c_send_gen;unsigned long c_flags;unsigned long c_reconnect_jiffies;struct delayed_work c_send_w;
net/rds/send.c +43 -4
85 lines not on this page · raw message
Signature · 1 line
1.7.1
PATCH v2 5/6 RDS: rds_send_xmit is called under a spinlock, lets not do a cond_resched()
In reply to Sowmini VaradhanOriginal patch by Chris Mason
--- net/rds/send.c | 14 ++------------ 1 files changed, 2 insertions(+), 12 deletions(-)
net/rds/send.c +2 -12
@@ -199,7 +199,6 @@ int rds_send_xmit(struct rds_connection *conn) same_rm++;if (same_rm >=4096 ) {printk_ratelimited (KERN_ERR "RDS: Stuck rm\n" ); -cond_resched (); ret = -EAGAIN ;break ; }
@@ -427,17 +426,8 @@ int rds_send_xmit(struct rds_connection *conn)smp_mb ();if (!list_empty (&conn->c_send_queue) && send_gen == conn->c_send_gen) { -cond_resched (); -/* repeat our check after the resched in case -* someone else was kind enough to empty or process -* the queue -*/ -smp_mb (); -if (!list_empty (&conn->c_send_queue) && - send_gen == conn->c_send_gen) { -rds_stats_inc (s_send_lock_queue_raced); - goto restart; - } +rds_stats_inc (s_send_lock_queue_raced); + goto restart; } } out:
Signature · 1 line
1.7.1
PATCH v2 6/6 RDS: don't trust the LL_SEND_FULL bit
In reply to Sowmini VaradhanWe are seeing connections stuck with the LL_SEND_FULL bit getting set and never cleared. This changes RDS to stop trusting the LL_SEND_FULL bit and kick krdsd after any time we
see -ENOMEM from the ring allocation code.
Original patch by Chris Mason
---
v2: comments from Sergei Shtylyov
net/rds/send.c | 10 ++++++---- net/rds/threads.c | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-)
net/rds/send.c +6 -4
@@ -1108,8 +1108,9 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)*/ rds_stats_inc (s_send_queued); -if (!test_bit (RDS_LL_SEND_FULL , &conn->c_flags)) -rds_send_xmit (conn); + ret =rds_send_xmit (conn); +if (ret == -ENOMEM || ret == -EAGAIN ) +queue_delayed_work (rds_wq, &conn->c_send_w,1 );rds_message_put (rm);return payload_len;
@@ -1165,8 +1166,9 @@ rds_send_pong(struct rds_connection *conn, __be16 dport)rds_stats_inc (s_send_queued);rds_stats_inc (s_send_pong); -if (!test_bit (RDS_LL_SEND_FULL , &conn->c_flags)) -queue_delayed_work (rds_wq, &conn->c_send_w,0 ); + ret =rds_send_xmit (conn); +if (ret == -ENOMEM || ret == -EAGAIN ) +queue_delayed_work (rds_wq, &conn->c_send_w,1 );rds_message_put (rm);return 0 ;
net/rds/threads.c +2 -0
@@ -162,7 +162,9 @@ void rds_send_worker(struct work_struct *work)int ret;if (rds_conn_state (conn) ==RDS_CONN_UP ) { +clear_bit (RDS_LL_SEND_FULL , &conn->c_flags); ret =rds_send_xmit (conn); +cond_resched ();rdsdebug ("conn %p ret %d\n" , conn, ret);switch (ret) {case -EAGAIN :
Signature · 1 line
1.7.1