Received-SPF: pass (google.com: domain of linux-nfs+bounces-3177-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <bb1f1604-8b3f-4fc4-ac31-1f365c1fe257@grimberg.me>
Date: Mon, 6 May 2024 22:10:12 +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: Chuck Lever <chuck.lever@oracle.com>
Cc: Dan Aloni <dan.aloni@vastdata.com>, linux-nfs@vger.kernel.org,
 Sagi Grimberg <sagi.grimberg@vastdata.com>
References: <20240506093759.2934591-1-dan.aloni@vastdata.com>
 <be4563e5-caa6-4085-98a9-a86e24c99186@grimberg.me>
 <Zjj9iOOJ0px+Lvin@tissot.1015granger.net>
Content-Language: he-IL, en-US
From: Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <Zjj9iOOJ0px+Lvin@tissot.1015granger.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 06/05/2024 18:55, Chuck Lever wrote:
> 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.
Agreed.
>
> 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.

Yes, I was actually referring to the case where the connection was 
established.
It looks like rpcrdma_force_disconnect -> xprt_force_disconnect 
schedules async
work to tear things down no?

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

Yes.

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

The plus is that there is no need to handle the DEVICE_REMOVAL cm event, 
which is
always nice...