Re: [PATCH v7 12/17] nvme-fc: start error recovery instead of aborting timed out IOs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] nvme-fc: ignored errors in CONNECTING state allow broken
  association to go LIVE
- [Medium] nvme-fc: nvme_fc_timeout() leaves queue live after non-LIVE
  command abort
- [Medium] nvme-fc: missing abort drain before queue deletion in connect
  error paths
- [Low] nvme-fc: missing Fixes trailer for commit 52793d62a696 in commit
  message
--

commit 0a8f4b079afa7a27de6edf488747423c0c7e82ab
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

> nvme-fc: start error recovery instead of aborting timed out IOs
>
> Aborts issued from the timeout handler run outside the FCCTRL_TERMIO
> window, so they are not counted in ctrl->iocnt and
> nvme_fc_delete_association() does not wait for them. The association
> can be torn down while the LLDD is still working on the abort.
>
> Instead of aborting the timed out command, reset the controller like
> the other fabrics transports do. The association teardown aborts the
> outstanding IOs, counts the aborts and waits for them to complete.
>
> Count every abort in ctrl->iocnt, not only the ones issued while
> FCCTRL_TERMIO is set, and do not reset the count when the teardown
> starts. An abort issued before the teardown is then waited for as well.
>
> Only a LIVE controller starts error recovery. In every other state the
> association is being set up, being torn down or already gone, so abort
> the timed out command from the timeout handler instead and return
> BLK_EH_DONE. The command completes through nvme_fc_fcpio_done() once
> the LLDD has finished the abort, and the abort is counted so a teardown
> that follows waits for it. This matters for a controller being deleted.
> The delete path drains the IOs itself and tears the association down in
> nvme_fc_delete_ctrl() once the drain is done, and DELETING cannot move
> to RESETTING, so without the abort a timed out IO would hang the drain
> against a dead target.
>
> The new nvme_fc_start_ioerr_recovery() is the common entry point for IO
> timeouts, transport errors reported by nvme_fc_fcpio_done(), Disconnect
> LS and connectivity loss. It moves the controller to RESETTING and
> queues ioerr_work only if that succeeds, so nothing is ever queued on a
> controller that is CONNECTING or being deleted. A Disconnect LS or a
> transport error received while CONNECTING is resolved when the inflight
> request fails, or times out and is aborted.
>
> ioerr_work still calls nvme_fc_error_recovery(), which returns without
> doing anything for a controller that is not LIVE. With the controller
> already in RESETTING when the work runs, no recovery takes place yet
> and the controller stays in RESETTING. The next patch moves the
> recovery into ioerr_work itself and completes the change.
>
> Link: https://lore.kernel.org/all/20250529214928.2112990-1-mkhalfella@purestorage.com/
> Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>

[Severity: Low]
Should this bug fix include a Fixes tag for the commit that gated
ctrl->iocnt accounting on the FCCTRL_TERMIO flag:

  Fixes: 52793d62a696 ("nvme-fc: fix io timeout to abort I/O")

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0b9ff50d26ff3..c4603be381445 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
[ ... ]
> @@ -1569,7 +1571,8 @@ nvme_fc_ls_disconnect_assoc(struct nvmefc_ls_rcv_op *lsop)
>  	 */
>  
>  	/* 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);
> @@ -1813,11 +1816,18 @@ __nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
>  	unsigned long flags;
>  	int opstate;
>  
> +	/*
> +	 * Every abort is counted, not only those issued by the association
> +	 * teardown: the teardown waits for iocnt to drop to zero before it
> +	 * deletes the hw queues, and an abort issued earlier, by
> +	 * nvme_fc_timeout() while deleting or by the CONNECTING recovery
> +	 * path, must not still be in progress in the LLDD then.
> +	 */
>  	spin_lock_irqsave(&ctrl->lock, flags);
>  	opstate = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
> -	if (opstate != FCPOP_STATE_ACTIVE)
> +	if (opstate != FCPOP_STATE_ACTIVE) {
>  		atomic_set(&op->state, opstate);
> -	else if (test_bit(FCCTRL_TERMIO, &ctrl->flags)) {
> +	} else {
>  		op->flags |= FCOP_FLAGS_TERMIO;
>  		ctrl->iocnt++;
>  	}

[Severity: Medium]
Can the connect failure unwind paths in nvme_fc_create_association(),
nvme_fc_create_io_queues(), and nvme_fc_recreate_io_queues() delete
hardware queues while an abort or asynchronous request is still active in
the lower-level driver?

Previously, a transport error or Disconnect Association link service while
connecting ran nvme_fc_error_recovery() ->
__nvme_fc_abort_outstanding_ios(), which quiesced the queues, aborted busy
requests on both tagsets, and waited for completion.  With
nvme_fc_start_ioerr_recovery() becoming a no-op while connecting and
nvme_fc_timeout() incrementing ctrl->iocnt for timed-out commands, an
in-flight keep-alive command (started by nvme_init_ctrl_finish()) or an
abort from nvme_fc_timeout() can still be running when a later connect step
fails.  The error labels in nvme_fc_create_io_queues() and
nvme_fc_create_association() call nvme_fc_delete_hw_io_queues() and
__nvme_fc_delete_hw_queue() and clear the active association flag without
aborting outstanding requests or waiting on ctrl->ioabort_wait for
ctrl->iocnt to reach zero:

drivers/nvme/host/fc.c:nvme_fc_create_association() {
    ...
out_delete_hw_queue:
	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
out_free_queue:
	nvme_fc_free_queue(&ctrl->queues[0]);
	clear_bit(ASSOC_ACTIVE, &ctrl->flags);
    ...
}

Because the active association flag is cleared there, even a subsequent
nvme_fc_delete_association() returns immediately without waiting for
ctrl->iocnt to drop to zero.

[ ... ]

> @@ -1892,6 +1901,19 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
>  }
>  EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
>  
> +/*
> + * Start recovery only if this caller can claim it with a state change.
> + */
> +static void
> +nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> +{
> +	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) {
> +		dev_warn(ctrl->ctrl.device, "NVME-FC{%d}: starting error recovery %s\n",
> +			 ctrl->cnum, errmsg);
> +		queue_work(nvme_reset_wq, &ctrl->ioerr_work);
> +	}
> +}
> +
>  static void
>  nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>  {
> @@ -2049,9 +2071,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");
>  }

[Severity: High]
Can nvme_fc_create_association() transition a broken or disconnected
association to the live state when a timeout, transport error, or
Disconnect Association link service occurs while connecting?

While ctrl->ctrl.state is connecting, nvme_change_ctrl_state() rejects a
transition to resetting, so nvme_fc_start_ioerr_recovery() is a no-op and
neither sets the association failed flag nor calls
__nvme_fc_abort_outstanding_ios() to clear the queue live bit and mark
requests cancelled:

1. When an admin command times out in nvme_fc_timeout() while connecting,
__nvme_fc_abort_op() aborts the command without setting the cancelled
request flag on op->nreq.flags.  nvme_fc_fcpio_done() then completes the
request with positive status NVME_SC_HOST_ABORTED_CMD (0x371) instead of
-EINTR.  If that command is nvme_set_queue_count() in
nvme_fc_create_io_queues(), nvme_set_queue_count() treats positive status
as non-fatal, sets nr_io_queues to 0, and returns 0, allowing
nvme_fc_create_association() to transition the controller to live with zero
I/O queues.

2. Likewise, optional commands in nvme_init_ctrl_finish() (such as
nvme_get_effects_log() or nvme_configure_apst()) and asynchronous
keep-alive commands ignore positive status codes from transport errors or
aborts, and a Disconnect Association request received in
nvme_fc_ls_disconnect_assoc() after ctrl->association_id is set stores
ctrl->rcv_disconn and calls nvme_fc_start_ioerr_recovery() without setting
the association failed flag.  When the remaining setup commands finish,
nvme_fc_create_association() sees no error, moves the controller to live,
and leaves ctrl->rcv_disconn unanswered:

drivers/nvme/host/fc.c:nvme_fc_create_association() {
    ...
	if (!ret && test_bit(ASSOC_FAILED, &ctrl->flags))
		ret = -EIO;
	if (ret)
		goto out_term_aen_ops;

	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
    ...
}

> @@ -2548,24 +2569,32 @@ 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");
> +
> +	if (nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_LIVE) {
> +		/*
> +		 * The association is being set up, being torn down or is
> +		 * already gone, so recovery cannot be started from here.
> +		 * Abort the command so the connect or teardown it would
> +		 * block can make progress. It completes through
> +		 * nvme_fc_fcpio_done() once the LLDD has finished the
> +		 * abort, and the abort is counted so a teardown that
> +		 * follows waits for it.
> +		 */
> +		__nvme_fc_abort_op(ctrl, op);
> +		return BLK_EH_DONE;
> +	}

[Severity: Medium]
Does aborting a timed-out request here without clearing the queue live flag
or marking the request cancelled allow the request to be retried against a
dead target while deleting?

During controller deletion, nvme_do_delete_ctrl() runs
nvme_remove_namespaces() before nvme_fc_delete_ctrl() ->
nvme_fc_delete_association(), so the queue live flag is still set on
op->queue->flags.  When the abort finishes, nvme_fc_fcpio_done() completes
the request with NVME_SC_HOST_ABORTED_CMD (without do-not-retry), so
non-multipath requests (such as del_gendisk() writeback before the queue is
marked dying) or retryable internal commands are retried by
nvme_decide_disposition().  Because op->queue->flags still has the queue
live bit set, nvme_check_ready() returns true in nvme_fc_queue_rq() and
re-dispatches the request to the dead target for up to nvme_max_retries
additional timeouts instead of fast-failing via
nvme_fail_nonready_command().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=12