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

[Gerd Rausch <gerd.rausch@oracle.com>]

Hi Allison,

On 2026-09-11 01:19, Allison Henderson wrote:
> On Thu, 2026-09-10 at 16:56 -0700, Gerd Rausch wrote:

>> On 2026-09-10 15:58, Allison Henderson wrote:
>>> On Thu, 2026-09-10 at 10:08 -0700, Gerd Rausch wrote:
>> But rds_ib_cm_handle_connect() issues a rdma_reject(IB_CM_REJ_CONSUMER_DEFINED) with payload
>> err = 1 (aka RDS_RDMA_REJ_INCOMPAT) not only if rds_ib_protocol_compatible()
>> returns zero, but all sorts of other scenarios also.
> 
> Yes, err starts as 1 and the conn-create failure and connect-race exits
> go to the reject without changing it.  So you are correct that the peer can't tell
> "incompatible" from "you raced me".

It definitely can tell that difference.

Also not done well, but there's backoff and counters in
rds_ib_cm_handle_connect() for when a race happens.

> That's a wart, and worth a patch of its
> own to give those exits their own code.  But I still think it's separate from this
> fix, since it doesn't touch the reject side in either direction.
> 

The point I was trying to make is that the party that received the RDS_RDMA_REJ_INCOMPAT
ought to be the one to try the reconnect.
That can only be guaranteed if the other party doesn't attempt to connect.
And that only happens if the other party does a rds_conn_destroy(),
and not a rds_conn_drop(), because the later will keep the connection with the
incompatible version number alive.

A rds_conn_drop() with a valid "i_cm_id" will lead to various CMA rejection
messages to be sent out, depending on state (DREQ, REJ, etc.),
once the RDS code reaches rdma_destroy_id().

The fact this change is in a different function is immaterial.
Keeping a RDS-connection that's considered incompatible alive,
and keep trying, makes the difference.

>> And then the RDS/client (actually the CM layer) sends out an RTU, that makes the RDS/server
>> land inside rds_ib_cm_connect_complete() too, but with "private_data_len==0".
>>
>> So a "major == 0" doesn't imply something being truncated nor it being
>> a fresh conn, but simply is the last step you see on an RDS/server
>> during connection establishment.
> 
> Also right, and I should have specified "on the active side".  On the
> passive side the version was already set from the REQ in
> rds_ib_cm_handle_connect(), and it is always 3.1 or newer there
> because anything else was rejected before the accept.
> So the RTU arriving with no payload leaves a valid c_version in place and the> branch can't fire.  That also means the changed line only runs
> on the active side, and only when the REP says 3.0 or says nothing.
> 

The branch can fire, because the protocol is updated on the client
side via rds_ib_set_protocol().

Both "private_data" payloads from rdma_connect() and rdma_accept()
are seen by RDS.

The one from rdma_connect() is processed by rds_ib_cm_handle_connect()
on the other side (i.e. server).

The one from rdma_accept() is processed by rds_ib_cm_connect_complete()
on the other side (i.e. client).

What version is provided to and seen by the client depends on this
weird function rds_ib_protocol_compatible(), as implemented by the peer,
which apparently has undergone many organic changes over the years.

In the context of this patch submission, we are talking
about the "if (c_version < RDS_PROTOCOL_VERSION)" being true,
because the change is done within that block of code.

And it can be true, because the peer can supply any old version
via rdma_accept().
If it couldn't be true, we could just remove the block.

So we know that the server responded with an incompatible version
that is neither >=RDS_PROTOCOL_VERSION nor ==RDS_PROTOCOL_COMPAT_VERSION,
and thus can't just accept it.

Prior to this suggested patch, that was done via rds_conn_destroy().

This patch suggests to replace that by a rds_conn_drop(),
and thus allow the client to keep trying.

But how can that be correct?

What makes the next connection attempt by this client different
in a way that makes us expect a different response from the server
than it got last time?

What mechanism makes us believe that next time around, we won't
end up in the exact same code-block?

And again. And again.
  >>
>> The important bit that's missing here is that the side that did the
>> "c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION" downgrade
>> is the one that needs to initiate.
>>
>> Or else, we run the risk of running into endless loops of trying the same
>> thing again and again and again.
> 
> Well, there is a downgrade in the REJECTED path, and the downgrading side
> probably should be the one to retry.  But in the branch this
> patch changes, nobody rejected.  The peer accepted, with a version below
> the floor, so there's no downgrade available to either side.
> 

If the version is considered incompatible, there are two options:

1) Downgrade and speak a common version.
2) Just give up. I.e. rds_conn_destroy().

It looks like #2 was the choice in the past.

If we don't want to give up, then there ought to be a downgrade.

How can this patch be accepted while you say
"there's no downgrade available to either side."

How would we expect a different result by just repeatedly proposing
the same incompatible version to the peer?

> So the loop is there either way.  The conn is torn down (via destroy or
> by drop), the peer gets a DISCONNECTED, drops, and reconnects to us on
> its own timer (IB has no smaller-address rule in rds_queue_reconnect()),
> and we reject it in rds_ib_cm_handle_connect() every time.
> 
> Destroy doesn't make the peer back off; it just moves the 1 second loop> to their side of the wire.  The next sendmsg() on our side starts it
> again from ours. The thing that actually stops the loop is "retry only
> when an application sends", which is the follow-up I'd like to see, and
> would need to cover the REJECTED path too.
> 

There's a world of a difference between a sendmsg() triggering a
reconnect, and RDS doing it on its own every second, with no hope
of success, because it is known that both sides are incompatible with eachother.

If RDS is expected to retry, it should do so with a common version,
understood by both sides.

If RDS is not expected to retry, there's no way around rds_conn_destroy().

I don't get the argument that rds_conn_destroy() ought to not
be called except in the rmmod-path.

Is the assumption that there couldn't be two totally incompatible
versions out there in the wild, for which RDS ought to give up,
and thus would have to call rds_conn_destroy() outside
the rmmod-path?

If there are bugs in the rds_conn_destroy() path, they ought to be fixed.

>>
>> But this whole protocol negotation in RDS is rather broken.
>> I guess some would call it "organically grown over the years".
>>
> 
> Agreed, I don't think anyone would dispute that RDS has plenty
> of things to fix.  But to me that's the reason to shepherd small,
> targeted fixes through one at a time rather than gate them on
> each other, or on a rework of the negotiation.
> This one turns a> node-wide hang into the same retry loop the peer already drives today,
> with a one-line change that's easy to revert when we get to larger
> changes.
> 
> So my suggestion would be: take Aohan's fix as is for net, and treat
> the reject-code cleanup, the send-driven retry, and any negotiation
> rework as separate net-next patches.  I'd be glad to review any of
> them, and happy to help with the send-driven one if you'd like to
> take it.
> 

I guess we differ that this is a fix and net improvement.

IMO this change makes things worse.

Thanks,

   Gerd