From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oi1-f169.google.com (mail-oi1-f169.google.com [209.85.167.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4C65D23D2B1 for ; Tue, 3 Feb 2026 19:19:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770146375; cv=none; b=ALSoUAV33ucUPZw/DlNY5LzV38ucV4zi2fx8BZ4lNmE6sWyCvEqxV4irShN2oIYwU2oPUtu4yNLTl8YxAhNY69uNSUQvJOJ5GEApq/e69QoS4TC5ADjboOedg8QVQf69YM36DEMl+MbYVrYwEchdINOfAAPY+mOs+vHxKoVoYOQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770146375; c=relaxed/simple; bh=L2VcfKqwc9QM1hYop6MrfplEBC1cqUMPXbghqR6b2rI=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=p66Np7xZlLXUPaBznLgkN4yVN4IjK7K6KsAwzxoJHzrwvFQQR3Awgg14Rf+roWCaRv+biaSedLdBR5HWmMDWwHmMBbgNYP9HVDITHQReLDmsOVA3S+vwJS58oSu4EtW8dtmkh2XmRqDdGkDztniTxlx/E95ts2McCl02E6ECoB0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=QKlAj8w1; arc=none smtp.client-ip=209.85.167.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="QKlAj8w1" Received: by mail-oi1-f169.google.com with SMTP id 5614622812f47-45effa36208so4190101b6e.1 for ; Tue, 03 Feb 2026 11:19:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1770146372; x=1770751172; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=2/Mi8mqqtpMlQ6U0+4KsMZZjH7saSPATz+Oc7ONk0U4=; b=QKlAj8w19oJPBcAqtpa/sJIO6FAhGBeJmptowxtwejfY0sFyRgvqWDuL0rglOG0r3J Y93LehA1SpotTet7TFgUnYiIEzCxjMUAn1QYXDa3WWGRIs4A69DVIwRsBUyGP7DOLtfV Y/cLhfr84WMg1j5uujzN6LZUc2yDrOVnThfx1mAaUD3pfGWgFNTd2pRNSEMUUZ7O4bR9 1y5vXI4nwtX4Gm3Euf7/iRiM23cG83FvL4jhEG8+RjvoSSgU9GcU5bA0qIWAfqJMbjho pFDTX+Z8cEMaNrrk8DAnBpRgp2/XFhFo6oPdHGl8l+KP8q8pbteG0MfXuN4a/Yt2u4V1 ksRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770146372; x=1770751172; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=2/Mi8mqqtpMlQ6U0+4KsMZZjH7saSPATz+Oc7ONk0U4=; b=kvRck5ev/jxqK5QuxH0FFv4iknYkUp5vjmZFvYmgATtqsqLsetzukH62kkymnQpOis 6p6j3sXAIDPNNkU7MnGxyM1WWLGpCgTI6x0QOGLlkgqrZybC8iXdzHH0iQfIMfh/cg0t KqnC57CgNTeT3aJBF5CoPway1qQ06lh1sMY42LDEkHF/BCdOyBu9YpKg5FSbjFMY/tZt 1svTpqU9Kji+1tRG3KJZEiZzTAtoV0FxKcllLNJJjxV7O+MGjYh4vrxv86JCBZF3UoQ1 p2abaIBymLvoSzqm+f3Ep19aW6FLCynvvihRy26z/6qbd9vo9sl1yW+CS21HDuGUxfvH AIFA== X-Forwarded-Encrypted: i=1; AJvYcCWrzwPY4x5jxukzU5VwfSZZJpr0+SmrOviHL6eaj76I6R6oSlwUF5ZeV/UjSMrp31bPVUNF6ftIR0U/hu4=@vger.kernel.org X-Gm-Message-State: AOJu0Yw9KQozt7FlAS59A7Gc1X5WwCw93kdN0MzlA6E7l5Wxn9xXHlM0 n8GAwZ9BfkC1xvxDmwBvUFARL4nLpMfEN/YKHE7IByIMUpAZFLO6pdL3 X-Gm-Gg: AZuq6aIKof0U1ngdurTdQ1zS6RsRjLB82OCPW2ONjN6ZtMEWDkixCArWt+TWbTwm45g qnlFtkjF+nWIx1rxnZF8wvidSTfyxIbblj/eBDVz2yrcLvf7V5J2K2CJ3Li4ZJRbheeYUUuF5YW D0j6tRfOO5x3u07Y+z2lVnN4mlnq8efMP4lyTqqBXwUHGlUS5j+M9U7TIe5g5/ymsJsultn+CBc ZJ/YHKlT/qKPQKGD2f+t4fr5CToOJau5QQmWzzwP+wVo/FQ2ruFuqo1l6NeZd6apuaaw+dYX+Xs MuWXxQOBAkQ+T4CJDjw5qgibXfXWZmCjFpDYtuzBYZzWlpZba28U8G9GDQVGHAHPQGAP3B5zk0g yNl8Xa5Snf37OtN3OSdB2JmS4EUwin2HYbJCBQ59Gj3q/XkqlRkrDzzHmjD3FH0kIjtANq4+W1u RkxYpIfugfAJSGaVPP X-Received: by 2002:a05:6808:4f4e:b0:45a:770c:d77b with SMTP id 5614622812f47-462d59edd49mr250710b6e.35.1770146372054; Tue, 03 Feb 2026 11:19:32 -0800 (PST) Received: from [10.69.37.27] ([192.19.223.252]) by smtp.gmail.com with ESMTPSA id 5614622812f47-462d680ca17sm93947b6e.22.2026.02.03.11.19.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 Feb 2026 11:19:31 -0800 (PST) Message-ID: <383bbbe9-6cf5-465c-8811-0dddce34f883@gmail.com> Date: Tue, 3 Feb 2026 11:19:28 -0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: James Smart Subject: Re: [PATCH v2 12/14] nvme-fc: Decouple error recovery from controller reset To: Mohamed Khalfella , Justin Tee , Naresh Gottumukkala , Paul Ely , Chaitanya Kulkarni , Christoph Hellwig , Jens Axboe , Keith Busch , Sagi Grimberg Cc: Aaron Dailey , Randy Jennings , Dhaval Giani , Hannes Reinecke , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org, jsmart833426@gmail.com References: <20260130223531.2478849-1-mkhalfella@purestorage.com> <20260130223531.2478849-13-mkhalfella@purestorage.com> Content-Language: en-US In-Reply-To: <20260130223531.2478849-13-mkhalfella@purestorage.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/30/2026 2:34 PM, Mohamed Khalfella wrote: > nvme_fc_error_recovery() called from nvme_fc_timeout() while controller > in CONNECTING state results in deadlock reported in link below. Update > nvme_fc_timeout() to schedule error recovery to avoid the deadlock. > > Previous to this change if controller was LIVE error recovery resets > the controller and this does not match nvme-tcp and nvme-rdma. It is not intended to match tcp/rda. Using the reset path was done to avoid code duplication of paths to teardown the association. FC, given we interact with an HBA for device and io state and have a lot of async io completions, requires a lot more work than straight data structure teardown in rdma/tcp. I agree with wanting to changeup the execution thread for the deadlock. > Decouple> error recovery from controller reset to match other fabric transports.> > Link: https://lore.kernel.org/all/20250529214928.2112990-1-mkhalfella@purestorage.com/ > Signed-off-by: Mohamed Khalfella > --- > drivers/nvme/host/fc.c | 94 ++++++++++++++++++------------------------ > 1 file changed, 41 insertions(+), 53 deletions(-) > > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c > index 6948de3f438a..f8f6071b78ed 100644 > --- a/drivers/nvme/host/fc.c > +++ b/drivers/nvme/host/fc.c > @@ -227,6 +227,8 @@ static DEFINE_IDA(nvme_fc_ctrl_cnt); > static struct device *fc_udev_device; > > static void nvme_fc_complete_rq(struct request *rq); > +static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, > + char *errmsg); > > /* *********************** FC-NVME Port Management ************************ */ > > @@ -788,7 +790,7 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl) > "Reconnect", ctrl->cnum); > > set_bit(ASSOC_FAILED, &ctrl->flags); > - nvme_reset_ctrl(&ctrl->ctrl); > + nvme_fc_start_ioerr_recovery(ctrl, "Connectivity Loss"); > } > > /** > @@ -985,7 +987,7 @@ fc_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents, > static void nvme_fc_ctrl_put(struct nvme_fc_ctrl *); > static int nvme_fc_ctrl_get(struct nvme_fc_ctrl *); > > -static void nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg); > +static void nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl); > > static void > __nvme_fc_finish_ls_req(struct nvmefc_ls_req_op *lsop) > @@ -1567,9 +1569,8 @@ nvme_fc_ls_disconnect_assoc(struct nvmefc_ls_rcv_op *lsop) > * for the association have been ABTS'd by > * nvme_fc_delete_association(). > */ > - > - /* fail the association */ > - nvme_fc_error_recovery(ctrl, "Disconnect Association LS received"); > + nvme_fc_start_ioerr_recovery(ctrl, > + "Disconnect Association LS received"); > > /* release the reference taken by nvme_fc_match_disconn_ls() */ > nvme_fc_ctrl_put(ctrl); > @@ -1871,7 +1872,7 @@ nvme_fc_ctrl_ioerr_work(struct work_struct *work) > struct nvme_fc_ctrl *ctrl = > container_of(work, struct nvme_fc_ctrl, ioerr_work); > > - nvme_fc_error_recovery(ctrl, "transport detected io error"); > + nvme_fc_error_recovery(ctrl); hmm.. not sure how I feel about this. There is at least a break in reset processing that is no longer present - e.g. prior queued ioerr_work, which would then queue reset_work. This effectively calls the reset_work handler directly. I assume it should be ok. > } > > /* > @@ -1892,6 +1893,17 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req) > } > EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid); > > +static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, > + char *errmsg) > +{ > + if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) > + return; > +> + dev_warn(ctrl->ctrl.device, "NVME-FC{%d}: starting error recovery %s\n", > + ctrl->cnum, errmsg); > + queue_work(nvme_reset_wq, &ctrl->ioerr_work); > +} > + Disagree with this. The clause in error_recovery around the CONNECTING state is pretty important to terminate io occurring during connect/reconnect where the ctrl state should not change. we don't want start_ioerr making it RESETTING. This should be reworked. > static void > nvme_fc_fcpio_done(struct nvmefc_fcp_req *req) > { > @@ -2049,9 +2061,8 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req) > nvme_fc_complete_rq(rq); > > check_error: > - if (terminate_assoc && > - nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_RESETTING) > - queue_work(nvme_reset_wq, &ctrl->ioerr_work); > + if (terminate_assoc) > + nvme_fc_start_ioerr_recovery(ctrl, "io error"); this is ok. the ioerr_recovery will bounce the RESETTING state if it's already in the state. So this is a little cleaner. > } > > static int > @@ -2495,39 +2506,6 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues) > nvme_unquiesce_admin_queue(&ctrl->ctrl); > } > > -static void > -nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg) > -{ > - enum nvme_ctrl_state state = nvme_ctrl_state(&ctrl->ctrl); > - > - /* > - * if an error (io timeout, etc) while (re)connecting, the remote > - * port requested terminating of the association (disconnect_ls) > - * or an error (timeout or abort) occurred on an io while creating > - * the controller. Abort any ios on the association and let the > - * create_association error path resolve things. > - */ > - if (state == NVME_CTRL_CONNECTING) { > - __nvme_fc_abort_outstanding_ios(ctrl, true); > - dev_warn(ctrl->ctrl.device, > - "NVME-FC{%d}: transport error during (re)connect\n", > - ctrl->cnum); > - return; > - } This logic needs to be preserved. Its no longer part of nvme_fc_start_ioerr_recovery(). Failures during CONNECTING should not be "fenced". They should fail immediately. > - > - /* Otherwise, only proceed if in LIVE state - e.g. on first error */ > - if (state != NVME_CTRL_LIVE) > - return; This was to filter out multiple requests of the reset. I guess that is what happens now in start_ioerr when attempting to set state to RESETTING and already RESETTING. There is a small difference here in that The existing code avoids doing the ctrl reset if the controller is NEW. start_ioerr will change the ctrl to RESETTING. I'm not sure how much of an impact that is. > - > - dev_warn(ctrl->ctrl.device, > - "NVME-FC{%d}: transport association event: %s\n", > - ctrl->cnum, errmsg); > - dev_warn(ctrl->ctrl.device, > - "NVME-FC{%d}: resetting controller\n", ctrl->cnum); I haven't paid much attention, but keeping the transport messages for these cases is very very useful for diagnosis. > - > - nvme_reset_ctrl(&ctrl->ctrl); > -} > - > static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq) > { > struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(rq); > @@ -2536,24 +2514,14 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq) > struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu; > struct nvme_command *sqe = &cmdiu->sqe; > > - /* > - * Attempt to abort the offending command. Command completion > - * will detect the aborted io and will fail the connection. > - */ > dev_info(ctrl->ctrl.device, > "NVME-FC{%d.%d}: io timeout: opcode %d fctype %d (%s) w10/11: " > "x%08x/x%08x\n", > ctrl->cnum, qnum, sqe->common.opcode, sqe->fabrics.fctype, > nvme_fabrics_opcode_str(qnum, sqe), > sqe->common.cdw10, sqe->common.cdw11); > - if (__nvme_fc_abort_op(ctrl, op)) > - nvme_fc_error_recovery(ctrl, "io timeout abort failed"); > > - /* > - * the io abort has been initiated. Have the reset timer > - * restarted and the abort completion will complete the io > - * shortly. Avoids a synchronous wait while the abort finishes. > - */ > + nvme_fc_start_ioerr_recovery(ctrl, "io timeout"); Why get rid of the abort logic ? Note: the error recovery/controller reset is only called when the abort failed. I believe you should continue to abort the op. The fence logic will kick in when the op completes later (along with other io completions). If nothing else, it allows a hw resource to be freed up. > return BLK_EH_RESET_TIMER; > } > > @@ -3352,6 +3320,26 @@ nvme_fc_reset_ctrl_work(struct work_struct *work) > } > } > > +static void > +nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl) > +{ > + nvme_stop_keep_alive(&ctrl->ctrl); Curious, why did the stop_keep_alive() call get added to this ? Doesn't hurt. I assume it was due to other transports having it as they originally were calling stop_ctrl, but then moved to stop_keep_alive. Shouldn't this be followed by flush_work((&ctrl->ctrl.async_event_work) ? > + nvme_stop_ctrl(&ctrl->ctrl); > + > + /* will block while waiting for io to terminate */ > + nvme_fc_delete_association(ctrl); > + > + /* Do not reconnect if controller is being deleted */ > + if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) > + return; > + > + if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) { > + queue_delayed_work(nvme_wq, &ctrl->connect_work, 0); > + return; > + } > + > + nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN); > +} This code and that in nvme_fc_reset_ctrl_work() need to be collapsed into a common helper function invoked by the 2 routines. Also addresses the missing flush_delayed work in this routine. > > static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = { > .name = "fc", -- James (new email address. can always reach me at james.smart@broadcom.com as well)