Received-SPF: pass (google.com: domain of linux-nfs+bounces-3170-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 6 May 2024 11:55:52 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: Dan Aloni <dan.aloni@vastdata.com>, linux-nfs@vger.kernel.org,
        Sagi Grimberg <sagi.grimberg@vastdata.com>
Subject: Re: [PATCH v3] rpcrdma: fix handling for RDMA_CM_EVENT_DEVICE_REMOVAL
Message-ID: <Zjj9iOOJ0px+Lvin@tissot.1015granger.net>
References: <20240506093759.2934591-1-dan.aloni@vastdata.com>
 <be4563e5-caa6-4085-98a9-a86e24c99186@grimberg.me>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <be4563e5-caa6-4085-98a9-a86e24c99186@grimberg.me>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?K1yog+VfXqmzR4ldi3MJAKQ0B0Xt4hWH2S9AqO5ri+VSxyskD+PNbaM9+dmB?=
 =?us-ascii?Q?scr5AoEj+eZLEeRFOjd6jCCrfDC+i7j1BkSDYRx+i5e8zy2ovlKk87c2VkgM?=
 =?us-ascii?Q?esbcSgj6o2iEsl2aiLcvewb2QO4+xpx6rVjmAv77uRXi+lvDgitVyfYgpNr3?=
 =?us-ascii?Q?jimsTs4nJZiODDqMdZiRbBpy3mhbxCIhDmIoSocHOJDw5kNMgkHt2ApkbHRH?=
 =?us-ascii?Q?6O7xfISPhVQJVLYzzN4wtDs9FPJRrILPLIC92PzCbo6VoB8+GwhlZhLh48bE?=
 =?us-ascii?Q?UvLgyCCo/SrKuifRi0JAdSseeJY2ugBPh/5fRB0KaylVtzpeSZz783UmdFTW?=
 =?us-ascii?Q?RN8cH7v+mG0fIkM4+ABjPHMUZfTjdgX+U1XE8UFhBBfqPWDnNvAy41+Vg9uE?=
 =?us-ascii?Q?gPtc6dsD5tuLdbafu+ZIJp+vZbbM334CU5y8IsvlS8NbU8OlwLkIVE5LvARH?=
 =?us-ascii?Q?kwxauRnnMQbkwo3xaoNhLqYfI6EAvTsknBqOrNsB7EhdfzyGGNF9PpMTJiXx?=
 =?us-ascii?Q?hccRyJgF1gGDgzWt+mPfHRLmPxIzvEp3+lhaIUYOG2mM3jmyKFgLt/j4li/b?=
 =?us-ascii?Q?ByBdDqUHVVNbbPjVZRVZDEKdKoMj2wi+qucWBD2iEwykWo1hubZ2sgAJENsg?=
 =?us-ascii?Q?cLpaCwthH6ey6DZl1zyICuX5/6Sb1gjVNa91khtnPt6Hqf9NlEll3djFcyfN?=
 =?us-ascii?Q?dBdG6h3o2PC8D+TG1cPFqxaT0du786g5/CPITqqraQidguS42jotVhS34VKn?=
 =?us-ascii?Q?4gDbXfTPKWLBOkDBH8ZaFGmdHyqczKkQZQNxZcKkyg6QsYcybqpowJnNhWCb?=
 =?us-ascii?Q?cHlSYndLTRCK5t63T8uf4ZNJaSTsSjzv7U9iR/zXOujHzNNwWbDfP5d42xPz?=
 =?us-ascii?Q?rSG0YJ2hBz8tERgKnmM90VllEQuKaJsZFq9s6L6Q6ODZTX2/h+O+ErtSVp+M?=
 =?us-ascii?Q?wDNZbPu4HGo4LVYw+glrvh+XZxDo1Qj1JyDe5hiID4C0Lxrbuz3Y6oBhlYns?=
 =?us-ascii?Q?AOUi5Px+MY2OoMSeT7eBAviH9gRhc9VNqsM6qz3/CT7luCIXuD9s9FI0Ao8r?=
 =?us-ascii?Q?jN6xbKc9kJWKdvnY6C+XUPH5w7vFKDaP6FM7T7nV7IXdIhYfNZg261c8nTpN?=
 =?us-ascii?Q?exScL21WUno4zt+2RSa/MmmIUr0jr2DVvj/a/XwD0fKviTxr3yh5UVdD1seH?=
 =?us-ascii?Q?MsXnZ1CaV8ep5nmIyiWr2JiFFbVHG/pHkdERszGx9xmV/cpGJcSGdslS/1Vk?=
 =?us-ascii?Q?U33p+/bO9NFh8vHjTycQdtO31dZbCzOUpTJYRDmF9JdO7/dHUjzsvz2N3sIW?=
 =?us-ascii?Q?2PTqhZI9lFRZk0cgJL1ZhSzfAZql6WzDv42/l3QkKo4kNJSkK/BTxddhAxL2?=
 =?us-ascii?Q?4qnJ8UaKI87Ke3+SFKp3zWLQVSVgPU2EH9BeeYfPlVLhIDpaHp15zxvams6h?=
 =?us-ascii?Q?1B+ohkQhpqyClENqPVK68QgQr4mJu7Jw1LRWZHur6ZdQbMps33oFOZYhXsDl?=
 =?us-ascii?Q?Mx9g6fnt+xDAfV8oixLYn6e11Iulwi2wMC2A/GZAvDXcga8ArOxc0J6eb8kV?=
 =?us-ascii?Q?xa0MONfZqY2GBYBoJUh49YLgs1ny4g1adxGl0l8ex22PzOo6pWV9VLC7uvJ4?=
 =?us-ascii?Q?yQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ehIFmvBng7KYJaHxg9hRMQOouhh9cijU/Vk0qB1OzSK8C/p4PQYTc6LHoB9aRM2bPL36TgQlav6X12vWEJf5tVNVoGEXQD1FE/U6jgMe9iJ4kQ4S9iDOSveXcBO91JZOPS+X8L33QCd5J+/shwOV4Y+iSHCInOy6LAYZRJ/wyTTF29X0p2Vye2ljCwRvPu24Z/yUu+b7TVxbdBx3qbTZ+4QEDYE2sAFRuTGxu9jBnSMDiZomaPVkPS8pVwiHchvRsUa4O6ZDwJJtcDGuwFmJoVyYbg0fNNkF+oZuudZ12IsNjcsTaVPVpsg8sO0KZx/4YiFu0LZvwrhy1l02xb4OoG/Z8I4MWoiTiTXvJGEzhsd5DVXE+mGgTFxKw4WBYCpuzLRWeWvyIPXYWFv7QOmetqM3gPfOIsRX+gMkFIASi8NZGCgjkbfUHpPKASGAb+vTlW335IQVuYhft2otu4heFgOYpwJwtcNNH3gNaHpNxxg5d6qpwIf1tkVbxNvMFdKtuma25e125k5ABdPbyi6zcw3ADRvwedE1jI8yDbbZKWPguF4m6TDpDuIOVoNOHP9dJtc99hBIA44zvd8PUw/upY0bJMb3XmCZ/pCs1PVFgcY=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f20a8a12-254a-471c-345e-08dc6de502f8
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 15:55:54.6906
 (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: 65apTmxCKYypck5pUNk3A9yYiZoJMCSEW9H6G0Zo3TRyoVB32R0ECoXg9F09m0XwjL8UIanVEoowjGggIclUeA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4652
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-06_11,2024-05-06_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 spamscore=0
 suspectscore=0 adultscore=0 phishscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2405060110
X-Proofpoint-GUID: y9g-NFdvv5G0Xrw1bpo7jBYNoa1gA6Yd
X-Proofpoint-ORIG-GUID: y9g-NFdvv5G0Xrw1bpo7jBYNoa1gA6Yd

On Mon, May 06, 2024 at 06:09:51PM +0300, Sagi Grimberg wrote:
> 
> On 06/05/2024 12:37, Dan Aloni wrote:
> > Under the scenario of IB device bonding, when bringing down one of the
> > ports, or all ports, we saw xprtrdma entering a non-recoverable state
> > where it is not even possible to complete the disconnect and shut it
> > down the mount, requiring a reboot. Following debug, we saw that
> > transport connect never ended after receiving the
> > RDMA_CM_EVENT_DEVICE_REMOVAL callback.
> > 
> > The DEVICE_REMOVAL callback is irrespective of whether the CM_ID is
> > connected, and ESTABLISHED may not have happened. So need to work with
> > each of these states accordingly.
> > 
> > Fixes: 2acc5cae2923 ('xprtrdma: Prevent dereferencing r_xprt->rx_ep after it is freed')
> 
> Is this actually the offending commit ?
> 
> commit bebd031866ca ("xprtrdma: Support unplugging an HCA from under an NFS
> mount")
> is the one assuming DEVICE_REMOVAL triggers a disconnect not accounting that
> the
> cm_id may not be ESTABLISHED (where we need to wake the connect waiter?

I'd be OK with discussing possible culprits in the patch description
but leaving off a Fixes: tag for now.

It would be reasonable to demand that the proposed fix be applied
to each LTS kernel and tested individually to ensure there are no
side-effects or pre-requisites.


> Question though, in DEVICE_REMOVAL the device is going away as soon as the
> cm handler callback returns. Shouldn't nfs release all the device resources
> (related to this
> cm_id)? afaict it was changed in:
> e28ce90083f0 ("xprtrdma: kmalloc rpcrdma_ep separate from rpcrdma_xprt")

In the case where a DEVICE_REMOVAL event fires and a connection
hasn't yet been established, my guess is the ep reference count will
go to zero when rpcrdma_ep_put() is called.


> The patch itself looks reasonable (although I do think that the rdma stack
> expects the
> ulp to have the rdma resources released when the callback returns).

Thanks for the review! Should we add:

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>


> FWIW in nvme we avoided the problem altogether by registering an ib_client
> that is
> called on .remove() and its a separate context that doesn't have all the
> intricacies with
> rdma_cm...

I looked at ib_client, years ago, and thought it would be a lot of
added complexity. With a code sample (NVMe host) maybe I can put
something together.


-- 
Chuck Lever