Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1] SUNRPC: Ensure backchannel transports are marked
 connected
Thread-Topic: [PATCH v1] SUNRPC: Ensure backchannel transports are marked
 connected
Thread-Index: AQHXlPXcGICcXaW3sESgYQmy74r7pat6ysSAgAAEDgCAABBYAIAABaKA
Date:   Thu, 19 Aug 2021 14:34:49 +0000
Message-ID: <24B4FABF-C6D5-4FF6-89B7-68EFDC3AB415@oracle.com>
References: <162937592206.2298.13447589794033256951.stgit@klimt.1015granger.net>
 <ed3fbd005a9a2e3a6217085ebe05e80cd78766ba.camel@hammerspace.com>
 <6D4FFB37-B5CB-410B-A3C9-AAC92F611520@oracle.com>
 <69e8d4c3ae302ba51a92e20c927006ad33b2486c.camel@hammerspace.com>
In-Reply-To: <69e8d4c3ae302ba51a92e20c927006ad33b2486c.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SibRUL1iNO72mqaegK1t49OG/9Z1lFF7ouoDdcucbniiBikytiI2+GZE0d9y?=
 =?us-ascii?Q?C9Ss9iOl9PcS+atiktmmn6VupQGhRUanKBuTsbt9RwvAAe+xjib4aQjdqGg/?=
 =?us-ascii?Q?uAeNylKDK4MUdv85AZi7MX6zyzQizj3T2psrjCKpnmVcqfuLoBTWpkty1VqC?=
 =?us-ascii?Q?8B8tepVBgCoSUizUGavEVL/T6HfS1b7kPl58y9wn7WREjwOhtJuFxqlbD6QD?=
 =?us-ascii?Q?iaQFl9KsnWQVck+N719B7tUy6EBKp28+FWLBqsWuIXMSqCIzVbzRcIyeItiw?=
 =?us-ascii?Q?W5BTotRdUy5q5+tv3bgvJbdGulYu3j4ItBMb/34AemrSvPzmEjsQ+p3KNM7n?=
 =?us-ascii?Q?XV+wGO7yJX9OwaBb2VGpsw1j2f3IzCvlkiazRXvkyX5FAptPAIwx8ivsuhfR?=
 =?us-ascii?Q?+aiw5mHUYngVFtlrhOYoJ36DWSq35yWqtTj864h2WJMbSsGkI7RxmMm3Ruee?=
 =?us-ascii?Q?JpjIQycpk7mhByWfyNVf0ES8oJbyZkSuV0i2DEyUSVYNnY30XiUpWRNRnFKL?=
 =?us-ascii?Q?hq6OGSckJE5ajOcNGyC+SDb1PRAxTCqIcGbgl8N+Ao9Dfb2DyEJM61kSXfl2?=
 =?us-ascii?Q?Vzx1/gsQIMcnR+Smb6dl6J2uyqfWpfqSqb6kycZL4vSqCpfqb172WptfMo1u?=
 =?us-ascii?Q?QLDSWPtoJrjGpOwlCF5InZUMmUPpl41lMyEYAOX9MzDh/azP6BMW4w4FmyDv?=
 =?us-ascii?Q?bMLklbIN6LqD5JFjTP86zHp9TABDUiK3Xn9x5kbC3YFf7mZy+YRnJQ0wNxME?=
 =?us-ascii?Q?to/EMKhbnkBwsHT3hjS8z5GzzvrnS62kOWaF7J83NokABr0dr0WVS+fAxzbE?=
 =?us-ascii?Q?OpLfACuOsrIVFVdUb8gmitQ7bEAiVka4Y6ewsoKNREI4pmrk3zm1bNA72nhd?=
 =?us-ascii?Q?01tUGpEaGYs0VI9LeZQhZzUMOgXWJmZ0u1lI2cl8FAhcmMLUJY8aZYAfVvoZ?=
 =?us-ascii?Q?XAFdxEyf2KD93nDUXgBljRnuHS7JA3walVPZfjeniA1lkXhQjY+GZluvs145?=
 =?us-ascii?Q?0cAmjaJX1ZEBGJzeQUdMfEBNlvDZB+93U9R15uAzd85/eNtfbIfVcFw3Lg98?=
 =?us-ascii?Q?EDNsjT+h4hm01QzIYK/wJn1Do3cLDe36fiTGvoOmXt0BQPwHXXedjPSprrif?=
 =?us-ascii?Q?eBnQYJPNIuZd04QkAobC9gADPa80bb1GppD+oWE6hA8IjlvWVjJ7JiK7rB8C?=
 =?us-ascii?Q?fp1C7tw4R89e/1grhzichUuK2iBXWSD0yw3xp+U2a33AvS/WZVBGItfcZdYj?=
 =?us-ascii?Q?Mtqrtgd4qLWTyzuX9dE1uwwv108aEg65u64lPFGAoFhSl3NuVUY9QZBtDQGl?=
 =?us-ascii?Q?CLlaJ6UtWNoKTa+1VA+5cTBw?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <05A0C177ED4BA845BAF0853065298E85@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a9a560d-5417-4e39-adc0-08d9631e7f9f
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Aug 2021 14:34:49.0617
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: taGj8K9EEJo2A/c/l2H+ipuiL8KsGnA/Hg+lTiBZc6/vkPPP6OUBS2oBsSVHZo6M5GBnGhIhOLLhwafVnfQrsQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4510
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10081 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 bulkscore=0
 suspectscore=0 mlxlogscore=999 phishscore=0 mlxscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2107140000
 definitions=main-2108190084
X-Proofpoint-ORIG-GUID: pyeEiawpLajgqvkHXMmF4dFgYBeTLS2c
X-Proofpoint-GUID: pyeEiawpLajgqvkHXMmF4dFgYBeTLS2c
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Aug 19, 2021, at 10:14 AM, Trond Myklebust <trondmy@hammerspace.com> w=
rote:
>=20
> On Thu, 2021-08-19 at 13:16 +0000, Chuck Lever III wrote:
>>=20
>>=20
>>> On Aug 19, 2021, at 9:01 AM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>>=20
>>> On Thu, 2021-08-19 at 08:29 -0400, Chuck Lever wrote:
>>>> With NFSv4.1+ on RDMA, backchannel recovery appears not to work.
>>>>=20
>>>> xprt_setup_xxx_bc() is invoked by the client's first
>>>> CREATE_SESSION
>>>> operation, and it always marks the rpc_clnt's transport as
>>>> connected.
>>>>=20
>>>> On a subsequent CREATE_SESSION, if rpc_create() is called and
>>>> xpt_bc_xprt is populated, it might not be connected (for
>>>> instance,
>>>> if a backchannel fault has occurred). Ensure that code path
>>>> returns
>>>> a connected xprt also.
>>>>=20
>>>> Reported-by: Timo Rothenpieler <timo@rothenpieler.org>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>  net/sunrpc/clnt.c |    1 +
>>>>  1 file changed, 1 insertion(+)
>>>>=20
>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>> index 8b4de70e8ead..570480a649a3 100644
>>>> --- a/net/sunrpc/clnt.c
>>>> +++ b/net/sunrpc/clnt.c
>>>> @@ -535,6 +535,7 @@ struct rpc_clnt *rpc_create(struct
>>>> rpc_create_args *args)
>>>>                 xprt =3D args->bc_xprt->xpt_bc_xprt;
>>>>                 if (xprt) {
>>>>                         xprt_get(xprt);
>>>> +                       xprt_set_connected(xprt);
>>>>                         return rpc_create_xprt(args, xprt);
>>>>                 }
>>>>         }
>>>>=20
>>>>=20
>>>=20
>>> No. This is wrong. If the connection got disconnected, then the
>>> client
>>> needs to reconnect and build a new connection altogether. We can't
>>> just
>>> make pretend that the old connection still exists.
>>=20
>> The patch description is not clear: the client has not disconnected.
>> The forward channel is functioning properly, and the server has set
>> SEQ4_STATUS_BACKCHANNEL_FAULT.
>>=20
>> To recover, the client sends a DESTROY_SESSION / CREATE_SESSION pair
>> on the existing connection. On the server, setup_callback_client()
>> invokes rpc_create() again -- it's this step that is failing during
>> the second CREATE_SESSION on a connection because the old xprt
>> is returned but it's still marked disconnected.
>=20
> How is that happening?

The RPC client's autodisconnect is firing. This was fixed in 6820bf77864d
("svcrdma: disable timeouts on rdma backchannel").

However we're still seeing cases where backchannel recovery is not
working. See:

https://lore.kernel.org/linux-nfs/c3e31e57-15fa-8f70-bbb8-af5452c1f5fc@roth=
enpieler.org/T/#t

As an experiment, we've reverted 6820bf77864d to try to provoke the bug.


> As far as I can tell, the only thing that can
> cause the backchannel to be marked as closed is a call to
> svc_delete_xprt(), which explicitly calls xprt->xpt_bc_xprt->ops->close(x=
prt->xpt_bc_xprt).
>=20
> The server shouldn't be reusing anything from that svc_xprt after that.

>> An alternative would be to ensure that setup_callback_client()
>> always puts xpt_bc_xprt before it invokes rpc_create(). But it
>> looked to me like rpc_create() already has a bunch of logic to
>> deal with an existing xpt_bc_xprt.
>=20
> The connection status is managed by the transport layer, not the RPC
> client layer.

If it's a bug to mark a backchannel transport disconnected, then
asserts should be added to capture the problem.

What is the purpose of this code in rpc_create() ?

 533         if (args->bc_xprt) {
 534                 WARN_ON_ONCE(!(args->protocol & XPRT_TRANSPORT_BC));
 535                 xprt =3D args->bc_xprt->xpt_bc_xprt;
 536                 if (xprt) {
 537                         xprt_get(xprt);
 538                         return rpc_create_xprt(args, xprt);
 539                 }
 540         }


--
Chuck Lever