Received-SPF: pass (google.com: domain of linux-nfs+bounces-3169-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <be4563e5-caa6-4085-98a9-a86e24c99186@grimberg.me>
Date: Mon, 6 May 2024 18:09:51 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] rpcrdma: fix handling for RDMA_CM_EVENT_DEVICE_REMOVAL
To: Dan Aloni <dan.aloni@vastdata.com>, chuck.lever@oracle.com
Cc: linux-nfs@vger.kernel.org, Sagi Grimberg <sagi.grimberg@vastdata.com>
References: <20240506093759.2934591-1-dan.aloni@vastdata.com>
Content-Language: he-IL, en-US
From: Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <20240506093759.2934591-1-dan.aloni@vastdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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?

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")

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).

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...