[rds-devel] [External] : Re: [PATCH net] rds: ib: use rds_conn_drop() on protocol version mismatch

[Gerd Rausch]

Hi Allison,

On 2026-09-09 15:55, Allison Henderson wrote:
> On Wed, 2026-09-09 at 10:25 -0700, Gerd Rausch wrote:
>> On 2026-09-09 00:09, Allison Henderson via rds-devel wrote:
>> I don't get this.
>>
>> So RDS should just keep on retrying to connect every second instead
>> of giving up on that incompatible peer? Forever?
>>
>> The version number the peer uses remains the same until it gets upgraded somehow.
> 
> Hi Gerd,
> 
> You're right that this ends up as a ~1s retry loop.  While that may
> not be pretty, I think it's still the right call for a net/stable
> fix, for a few reasons.
> 
> First, it's already what we do for the same condition in the other
> direction.  When the passive side rejects us with RDS_RDMA_REJ_INCOMPAT,
> the REJECTED case in rds_rdma_cm_event_handler_cmn() calls
> rds_conn_drop(), we reconnect with the usual backoff (capped at
> rds_sysctl_reconnect_max_jiffies, one second), and we log a pr_warn
> on every attempt.  That's the common direction, a new node talking to
> an old one.  Aohan's patch just makes the rare direction behave the
> same way instead of wedging.
> 

The important difference is that when this happens, and the client
sees a RDS_RDMA_REJ_INCOMPAT from the peer, it'll downgrade to
RDS_PROTOCOL_COMPAT_VERSION (3.1):
An older version expected to be understood by all RDS modules.

And thus the connection will be retried with version 3.1,
and is expected to be accepted by the peer:

Function rds_ib_cm_connect_complete() explicitly exempts
RDS_PROTOCOL_COMPAT_VERSION from the rds_conn_destroy() treatment,
but allows the connection to proceed.

So it is not the same way at all.

Before this patch, a version 3.1 connection is expected to be established.
After this patch, there's an infinite connection retry attempt every second,
that'll never succeed.

> Second, rds_conn_destroy() doesn't actually give up on the peer either.
> Nothing in RDS remembers that a peer is incompatible.  Destroy frees
> the conn, and the next rds_sendmsg() to that address either reuses the
> socket's cached rs->rs_conn or goesthrough rds_conn_create_outgoing()
> and rds_conn_connect_if_down() and then connects again.  With an
> application actively sending, that's a create/connect/mismatch/destroy
> cycle at the application level.  With an idle application the only
> difference from the drop is whether the retry is timer-driven or
> send-driven.  Deferring the destroy to a worker doesn't change that
> picture, it just avoids the deadlock.
> 

I can't follow.
Are you suggesting to ban an IP-address / peer for life?

A node can always retry an RDS connect, triggered by a sendmsg().
Much like a failed TCP connect() doesn't ban a peer for life.

There's a big difference between an application trying to send data again,
and the RDS module itself spinning around every second and re-trying
a connect with a version# that is known to be incompatible,
and thus never succeeding, because the fallback mechanism became broken.

> 
>>
>> I understand how the rds_conn_destroy() path is full of bugs.
>>
>> But shouldn't that just mean those bugs ought to be fixed,
> 
> While that code path does have bugs, making rds_conn_destroy()
> safe to call from the CM path is a much bigger change, and one that
> should probably go through net-next rather than holding up a small
> fix like this.  The current code hangs every RDS connection on the
> node the first time this fires, and a one second retry on one conn is
> strictly better than that.
> 

It is not only the infinite retry every second that's of concern:

This change also implies that all RDS kernel modules speak
the exact same protocol version.

I don't see how that assumption can ever be valid.

Because after this change, if versions differ, there's no fallback to 3.1,
but connections won't ever get established.

Thanks,

   Gerd