Re: [PATCH v7 07/17] nvme: Introduce FENCING and FENCED controller states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvme: premature non-mpath I/O failure in
  nvme_fail_nonready_command()
- [Medium] nvme: live queues re-enabled in nvme_check_ready() when
  deleting from FENCING
--

commit 78ddca4023a9ef3590c04ed05791d87621f68277
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Introduce FENCING and FENCED controller states

This commit adds the NVME_CTRL_FENCING and NVME_CTRL_FENCED controller
states along with their state machine transitions, readiness checks,
multipath path availability handling, and sysfs state names.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 38b27d8db0119..4be3ce2e72a78 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c

[ ... ]

> @@ -622,6 +641,8 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>  		case NVME_CTRL_LIVE:
>  		case NVME_CTRL_RESETTING:
>  		case NVME_CTRL_CONNECTING:
> +		case NVME_CTRL_FENCING:
> +		case NVME_CTRL_FENCED:
>  			changed = true;
>  			fallthrough;
>  		default:

[Severity: Medium]
When a controller in NVME_CTRL_FENCING or NVME_CTRL_FENCED transitions
to NVME_CTRL_DELETING, can new I/O be dispatched onto its still-live
queues while nvme_stop_ctrl() waits for fencing to finish?

While in NVME_CTRL_FENCING, the transport queues remain live and new
requests are blocked only because __nvme_check_ready() and
nvme_path_get_state() reject NVME_CTRL_FENCING and NVME_CTRL_FENCED.
In nvme_do_delete_ctrl(), ctrl->state stays NVME_CTRL_DELETING across
nvme_stop_ctrl() until nvme_remove_namespaces() sets
NVME_CTRL_DELETING_NOIO, and both nvme_check_ready() and
nvme_path_get_state() treat NVME_CTRL_DELETING on a live queue as ready
and enabled:

drivers/nvme/host/nvme.h:nvme_check_ready() {
	if (likely(state == NVME_CTRL_LIVE))
		return true;
	if (ctrl->ops->flags & NVME_F_FABRICS && state == NVME_CTRL_DELETING)
		return queue_live;
	return __nvme_check_ready(ctrl, rq, queue_live, state);
}

drivers/nvme/host/multipath.c:nvme_path_get_state() {
	if (state != NVME_CTRL_LIVE && state != NVME_CTRL_DELETING)
		return NVME_PATH_DISABLED_CTRL_DOWN;
}

As a result, deleting a fencing controller re-enables multipath
selection and allows new requests to be queued to the broken controller
throughout the fence wait instead of failing over immediately.

> @@ -786,6 +807,8 @@ blk_status_t nvme_fail_nonready_command(struct nvme_ctrl *ctrl,
>  
>  	if (state != NVME_CTRL_DELETING_NOIO &&
>  	    state != NVME_CTRL_DELETING &&
> +	    state != NVME_CTRL_FENCING &&
> +	    state != NVME_CTRL_FENCED &&
>  	    state != NVME_CTRL_DEAD &&
>  	    !test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags) &&
>  	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))

[Severity: High]
Does excluding NVME_CTRL_FENCING and NVME_CTRL_FENCED here cause
non-multipath I/O to fail immediately with BLK_STS_TRANSPORT during
fencing?

Multipath requests (REQ_NVME_MPATH or REQ_FAILFAST_TRANSPORT) already
bypass the BLK_STS_RESOURCE return via the trailing checks and call
nvme_host_path_error(rq), so excluding NVME_CTRL_FENCING and
NVME_CTRL_FENCED here only changes the behavior for normal
non-multipath requests.  Because the queues are not quiesced during
NVME_CTRL_FENCING, nvme_host_path_error() -> nvme_complete_rq() ->
nvme_decide_disposition() returns RETRY, and nvme_retry_req() requeues
the request with zero delay until nvme_max_retries is exhausted in a
tight loop, after which __nvme_end_req() logs an uninitialized
nr->cmd->common.opcode in nvme_log_error() (since nvme_check_ready()
failed before nvme_setup_cmd()) and fails the I/O to the caller.

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