On (09/30/15 08:50), santosh shilimkar wrote:
> minor nit though not a strict rule. Just to be consistent based on
> what we are following.
>
> - core RDS patches "RDS:"
> - RDS IB patches "RDS: IB:" or "RDS/IB:"
> - RDS IW patches "RDS: IW:" or
> - RDS TCP can use "RDS: TCP" or "RDS/TCP:"
Ok, but in this case patch 1/3 the changes affect both core and rds-tcp
modules.
Working on patchv2 that will address Sergei's comments and the
kbuild-test-robot warning as well
>
> $subject
> s/net/rds:/RDS:
>
> On 9/30/2015 6:45 AM, Sowmini Varadhan wrote:
> >Commit f711a6ae062c ("net/rds: RDS-TCP: Always create a new rds_sock
> >for an incoming connection.") modified rds-tcp so that an incoming SYN
> >would ignore an existing "client" TCP connection which had the local
> >port set to the transient port. The motivation for ignoring the existing
> >"client" connection in f711a6ae was to avoid race conditions and an
> >endless duel of reconnect attempts triggered by a restart/abort of one
> >of the nodes in the TCP connection.
> >
> >However, having separate sockets for active and passive sides
> >is avoidable, and the simpler model of a single TCP socket for
> >both send and receives of all RDS connections associated with
> >that tcp socket makes for easier observability. We avoid the race
> >conditions from f711a6ae by attempting reconnects in rds_conn_shutdown
> >if, and only if, the (new) c_outgoing bit is set for RDS_TRANS_TCP.
> >The c_outgoing bit is initialized in __rds_conn_create().
> >
> >A side-effect of re-using the client rds_connection for an incoming
> >SYN is the potential of encountering duelling SYNs, i.e., we
> >have an outgoing RDS_CONN_CONNECTING socket when we get the incoming
> >SYN. The logic to arbitrate this criss-crossing SYN exchange in
> >rds_tcp_accept_one() has been modified to emulate the BGP state
> >machine: the smaller IP address should back off from the connection attempt.
> >
> >Signed-off-by: Sowmini Varadhan <sowmini.varad...@oracle.com>
> >---
> > net/rds/connection.c | 22 ++++++----------------
> > net/rds/rds.h | 4 +++-
> > net/rds/tcp_listen.c | 19 +++++++------------
> > 3 files changed, 16 insertions(+), 29 deletions(-)
> >
>
> [...]
>
> >diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> >index 444d78d..ee70d13 100644
> >--- a/net/rds/tcp_listen.c
> >+++ b/net/rds/tcp_listen.c
> >@@ -110,28 +110,23 @@ int rds_tcp_accept_one(struct socket *sock)
> > goto out;
> > }
> > /* An incoming SYN request came in, and TCP just accepted it.
> >- * We always create a new conn for listen side of TCP, and do not
> >- * add it to the c_hash_list.
> > *
> > * If the client reboots, this conn will need to be cleaned up.
> > * rds_tcp_state_change() will do that cleanup
> > */
> > rs_tcp = (struct rds_tcp_connection *)conn->c_transport_data;
> >- WARN_ON(!rs_tcp || rs_tcp->t_sock);
> >+ if (rs_tcp->t_sock && inet->inet_saddr < inet->inet_daddr) {
> >+ struct sock *nsk = new_sock->sk;
> >
> Any reason you dropped the WARN_ON. Note that till we got commit
> 74e98eb0 (" RDS: verify the underlying transport exists before creating
> a connection") merged, we had an issue. That guards it now.
>
> Am curious about WARN_ON() and hence the question.
>
> Rest of the patch looks fine to me.
> Acked-by: Santosh Shilimkar <santosh.shilim...@oracle.com>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
