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>,
        Bruce Fields <bfields@redhat.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: AQHXlPXcGICcXaW3sESgYQmy74r7pat6ysSAgAAEDgCAABBYAIAABaKAgACiBACAAOYSgA==
Date:   Fri, 20 Aug 2021 13:58:07 +0000
Message-ID: <B5C44073-6391-4310-B89A-FCF31A5A5E22@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>
 <24B4FABF-C6D5-4FF6-89B7-68EFDC3AB415@oracle.com>
 <ff2b05ffaecf066331c9f74ed0938f86a0e5a4a5.camel@hammerspace.com>
In-Reply-To: <ff2b05ffaecf066331c9f74ed0938f86a0e5a4a5.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?mEhccNjuWNvGgDcdIxP/uw0roT/Mapk4tcUpdxa/yhL764RebHoEXikzyDpM?=
 =?us-ascii?Q?tsoPsxOl2U2jnmagWP6Ek+RKH542U18ZRPNivwLPDE6dCWGz1kNHtOFyvTbY?=
 =?us-ascii?Q?RAjYP4pWlSjiwLgc3Dap757gkC1WmJ0xmmdtD4BND9/UzN6UkSxd0Qybc0Hw?=
 =?us-ascii?Q?KpWHROilJuvKJCeaS4dEKMJtEh8S5MMGpYpXw+El1OWJXVX/h4amvO2zwymQ?=
 =?us-ascii?Q?E8b3KpiQ2v3EBJnxGYph9ERZ2Dn5D5MKPf8CUFVzMWz0lXO5uZQ+bySMbQ4Y?=
 =?us-ascii?Q?6J/eydtWSWCyGWno5xkz7FqizJNu+g+z8IqY1cFlxrWAYmIFAEAoKpP9FU6s?=
 =?us-ascii?Q?3b2wlhsZVZtMLQTEdyydl5qA44kpS62PSl9/C3tr8WU/mdVNRVbZ+08qp7YO?=
 =?us-ascii?Q?8HZQvf6obkbt/obpqdl1Ls1VZ3/4HWtvZMknE8w6A/hBEi/h3tQzkSKVDBQK?=
 =?us-ascii?Q?Dp/ZNqB0tdsP+54Dnmu/s2xcxdDdGuAFub731oGl/e6MfqglZiE/gJOMZO+2?=
 =?us-ascii?Q?rz6xxUCa46K5DD6qJAyWOAhZu+HRNTygjvHXabzPxA+5v+qZRJRnYcNsWT6D?=
 =?us-ascii?Q?S/a8/OcjowJ40xA6941yTdgnAF1NLZ+20SWaPpQIRbu8jVP75kivJbnj8V7v?=
 =?us-ascii?Q?goozWNyRMr3Sn1ehcTR9VF+tbHyYFT0VHhqL9AYRwDpK18stME2bIMwzNYIG?=
 =?us-ascii?Q?ueDSqpNtv49EUxTDwc+CjdZU6vZ2DoNYDe/LtTznkcWbVGOeX1U0BuNhrbXX?=
 =?us-ascii?Q?M+uZIYFFuxeGOTkL459thSNcIVqWxHgpyZ+7j8eZBsaOJHctbKsaUa8nIxtu?=
 =?us-ascii?Q?wId2QpcFdjzOJZ/RRrBnf+SOCUTCf+fRLtYeICNvoNIw8j1MaraFwuOSRM3x?=
 =?us-ascii?Q?WPpNQA7McE6Vdckyo52F/86dZsK9eW61T9m7gvqwosc5Ni7cpeWt7QgGiKBU?=
 =?us-ascii?Q?8vyqNj0kFl1ZH1ASLOvSRyR6sWjEbtz9BXpTlpbYIuAKqNmaH8K3B9eCTpiA?=
 =?us-ascii?Q?ufQwvJHeQUzfSPLmyZpgA++A4t7INSH3WlkQDg3OZuvOLQBK0XT9XlqlCT5a?=
 =?us-ascii?Q?MEhRqkh0eZaKPT57qvqvPU9mddKhaBmKedJPeEBkAYHwjw9IuTf1JzVF9JZG?=
 =?us-ascii?Q?cWPK4VqWyAnVf6IBcD+fOxFj0AGII9YZM4+Jd8dmdiIjHXVbADCzT2kdKhT/?=
 =?us-ascii?Q?0+4mdnsE2QDvmeMGHCbOrqEGI90LSB2GsWbw2mbN3U8T0myDtHqN446cBzt9?=
 =?us-ascii?Q?qW1GMOvPtyKviav3yVAlbKS1kb7IfubqkDMoSPWsLcgIfQlyGVIwvscDYDmq?=
 =?us-ascii?Q?bSQuIabceUTmBC5KqEkcu6QA?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <62DA1DFB25F6C44199A59C467015BBEB@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: a4ab05a6-f8b5-4365-4039-08d963e289ea
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Aug 2021 13:58:07.6229
 (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: m1nWOSwPvy7J0t/AfJGQ4wi1Mfc/VbwKR/1bBHkOfpjA2SJB9sT5yBdV0mUmR91lMcoifV6l8Y2Q0f3+ip6/6g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2440
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-2108200078
X-Proofpoint-GUID: zJjIfkLQI2oMsnQjWP1xE3oRYnwVlXgE
X-Proofpoint-ORIG-GUID: zJjIfkLQI2oMsnQjWP1xE3oRYnwVlXgE
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Aug 19, 2021, at 8:14 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Thu, 2021-08-19 at 14:34 +0000, Chuck Lever III wrote:
>>=20
>>=20
>>> On Aug 19, 2021, at 10:14 AM, Trond Myklebust
>>> <trondmy@hammerspace.com> wrote:
>>>=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?
>>=20
>> The RPC client's autodisconnect is firing. This was fixed in
>> 6820bf77864d
>> ("svcrdma: disable timeouts on rdma backchannel").
>>=20
>> However we're still seeing cases where backchannel recovery is not
>> working. See:
>>=20
>> https://lore.kernel.org/linux-nfs/c3e31e57-15fa-8f70-bbb8-af5452c1f5fc@r=
othenpieler.org/T/#t
>>=20
>> As an experiment, we've reverted 6820bf77864d to try to provoke the
>> bug.
>>=20
>>=20
>>> 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(xprt->xpt_bc_xprt).
>>>=20
>>> The server shouldn't be reusing anything from that svc_xprt after
>>> that.
>>=20
>>>> 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.
>>=20
>> If it's a bug to mark a backchannel transport disconnected, then
>> asserts should be added to capture the problem.
>=20
> It isn't a bug to mark the transport as disconnected. It is a bug to
> mark it as disconnected and then to continue using it as if it were
> connected.

Are you suggesting the server's backchannel logic should
look for -107 status after posting an RPC, and then recover
somehow? Perhaps it could put xpt_bc_xprt then set it to
NULL, and then invoke setup_callback_client() again ?


>> What is the purpose of this code in rpc_create() ?
>>=20
>>  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         }
>>=20
>=20
> Are you asking me or Bruce?

Fair enough, adding Bruce.


> I'm not much of a fan of this code snippet either, because it doesn't
> really appear to have much to do with the normal function of
> rpc_create().
> However as I understand it, the purpose is to create an instance of
> struct rpc_clnt when presented with an existing instance of svc_xprt.
> It does not currently modify that svc_xprt instance in any way that
> breaks layering. All it does is take a reference to the xpt_bc_xprt
> field.

I see that the initial addition of this logic was done by
d531c008d7d9 ("NFSD/SUNRPC: Check rpc_xprt out of xs_setup_bc_tcp").
It was later moved into rpc_create() by d50039ea5ee6 ("nfsd4/rpc:
move backchannel create logic into rpc code").

Probably the essential purpose of this code is explained by a comment
that was deleted by d531c008d7d9:

-       if (args->bc_xprt->xpt_bc_xprt) {
-               /*
-                * This server connection already has a backchannel
-                * transport; we can't create a new one, as we wouldn't
-                * be able to match replies based on xid any more.  So,
-                * reuse the already-existing one:
-                */
-                return args->bc_xprt->xpt_bc_xprt;
-       }

Is this still a sensible expectation?

--
Chuck Lever