From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============2358918681511613269==" MIME-Version: 1.0 From: Walker, Benjamin Subject: Re: [SPDK] RDMA qp recovery: follow up on latest changes Date: Thu, 23 Aug 2018 19:33:40 +0000 Message-ID: <8d504c0700e9d95955af4c6e8399c5d67fab4f53.camel@intel.com> In-Reply-To: DM6PR04MB4988BC1E338ABAA708CC64B086370@DM6PR04MB4988.namprd04.prod.outlook.com List-ID: To: spdk@lists.01.org --===============2358918681511613269== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Thu, 2018-08-23 at 04:08 +0000, Philipp Skadorov wrote: > > From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, > > Benjamin > > = > > On Wed, 2018-08-22 at 22:04 +0000, Philipp Skadorov wrote: > > > Hello Benjamin, > > > There have been a series of changes to the QP error recovery that I'd > > > like to follow up: > > > = > > > ** c3756ae3 nvmf: Eliminate spdk_nvmf_rdma_update_ibv_qp > > = > > ibv_query_qp > > > results in a write syscall to the SNIC driver which is a context > > > switch. > > > The original code was calling it when the state is known to change; > > > the cached value was used everywhere else. > > = > > The observation driving this patch was that the cached value was always > > updated before being used, so keeping the cached value wasn't saving any > > calls. Do you see anywhere in the code on master where a call to > > ibv_query_qp could be eliminated? = > = > I thought it is enough to update the qp state after a QP async event is > received only. > Do you see the cases when it is not? So you're saying we probably don't need the one at the top of spdk_nvmf_rdma_qpair_recover() nor the one in _spdk_nvmf_rdma_qp_error() be= cause any unsolicited state change will occur only along with an associated asynchronous event. I think I agree with that statement. I'd revert the pat= ch that changed this, but the code has drifted a bit, so I'll probably have to= cook up a new one. > = > > The only one I see that might be > > questionable is the one in _spdk_nvmf_rdma_qp_error. > > = > > > = > > > ** 65a512c6 nvmf/rdma: Combine spdk_nvmf_rdma_qp_drained and > > > spdk_nvmf_rdma_recover > > > ** 3bec6601 nvmf/rdma: Simplify spdk_nvmf_rdma_qp_drained > > > ** a9b9f0952d6a0c1a37e544ef2977e7db136a8e86 nvmf/rdma: Don't trigger > > > error recovery on IBV_EVENT_SQ_DRAINED De-allocating the resources > > > associated with the requests being processed by SNIC at the time of > > > IBV_EVENT_QP_FATAL is too early. > > > The IBV standard requires SPDK should "wait for the Affiliated > > > Asynchronous Last WQE Reached Event" before manipulating with the QP > > = > > state. > > > Would also assume it is unsafe to return the associated requests and > > > their data back to the pools before "Last WQE Reached" event is recei= ved. > > = > > I agree, but I think the code on master does that already with just one > > twist. > > We observed that the IBV_EVENT_QP_LAST_WQE_REACHED event would > > never occur if an error occured on an RDMA queue pair which had no > > outstanding I/O. So we can't always wait for that event before releasing > > resources. Instead, when we first are notified of the error via > > IBV_EVENT_QP_FATAL, we abort all outstanding commands and then > > attempt to do the recovery. The recovery quits early if there are RDMA > > operations outstanding though. = > = > Looking at rdma.c, func _spdk_nvmf_rdma_qp_error: > 2085 _spdk_nvmf_rdma_qp_cleanup_all_states(rqpair); > = > It seems to be called at all times once qp is in error state. Is the suggestion here to not call _spdk_nvmf_rdma_qp_cleanup_all_states un= til after the check for whether there is I/O outstanding? I'm not sure I'm interpreting what you're saying correctly, but if that's what you're saying= I think I agree with it. > > In that case, we'll later get the > > IBV_EVENT_QP_LAST_WQE_REACHED event and go through the same path, > > but not bail out early. This was all mostly figured out through trial a= nd > > error > > when force disconnecting the NVMe-oF initiator. If you see any flaws in= the > > logic let me know so we can get them corrected. > > = > > > = > > > Thanks, > > > Philipp --===============2358918681511613269==--