Re: [PATCH net v3] net/smc: avoid data corruption caused by decline

Wen Gu
2023-11-20T03:37:29Z

On 2023/11/19 23:28, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
>
> We found a data corruption issue during testing of SMC-R on Redis
> applications.
>
> The benchmark has a low probability of reporting a strange error as
> shown below.
>
> "Error: Protocol error, got "\xe2" as reply type byte"
>
> Finally, we found that the retrieved error data was as follows:
>
> 0xE2 0xD4 0xC3 0xD9 0x04 0x00 0x2C 0x20 0xA6 0x56 0x00 0x16 0x3E 0x0C
> 0xCB 0x04 0x02 0x01 0x00 0x00 0x20 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xE2
>
> It is quite obvious that this is a SMC DECLINE message, which means that
> the applications received SMC protocol message.
> We found that this was caused by the following situations:
>
> client server
> ¦ proposal
> ------------->
> ¦ accept
> <-------------
> ¦ confirm
> ------------->
> wait confirm

I think there may be an ambiguity here, better for 'wait for llc confirm link'.
Could you please add 'clc' and 'llc' prefix to distinguish flows on the diagram?

Thanks.

>
> ¦failed llc confirm
> ¦ x------
> (after 2s)timeout
> wait rsp
>
> wait decline
>
> (after 1s) timeout
> (after 2s) timeout
> ¦ decline
> -------------->
> ¦ decline
> <--------------
>
> As a result, a decline message was sent in the implementation, and this
> message was read from TCP by the already-fallback connection.
>
> This patch double the client timeout as 2x of the server value,
> With this simple change, the Decline messages should never cross or
> collide (during Confirm link timeout).
>
> This issue requires an immediate solution, since the protocol updates
> involve a more long-term solution.
>
> Fixes: 0fb0b02bd6fd ("net/smc: adapt SMC client code to use the LLC flow")
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>

> ---
> net/smc/af_smc.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index abd2667..8615cc0 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -598,8 +598,12 @@ static int smcr_clnt_conf_first_link(struct smc_sock *smc)
> struct smc_llc_qentry *qentry;
> int rc;
>
> - /* receive CONFIRM LINK request from server over RoCE fabric */
> - qentry = smc_llc_wait(link->lgr, NULL, SMC_LLC_WAIT_TIME,
> + /* Receive CONFIRM LINK request from server over RoCE fabric.
> + * Increasing the client's timeout by twice as much as the server's
> + * timeout by default can temporarily avoid decline messages of
> + * both sides crossing or colliding
> + */
> + qentry = smc_llc_wait(link->lgr, NULL, 2 * SMC_LLC_WAIT_TIME,
> SMC_LLC_CONFIRM_LINK);
> if (!qentry) {
> struct smc_clc_msg_decline dclc;