Re: [PATCH v7 08/17] nvme: Implement cross-controller reset recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme: nvme_find_ctrl_ccr() skips valid controllers on
  unsorted list
--

commit 5d2164548bbd55a6b77ef7780332ce46368dbeb8
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Implement cross-controller reset recovery

This commit adds core cross-controller reset (CCR) recovery logic in
nvme_fence_ctrl() and nvme_fence_timeout_ms() to terminate in-flight I/Os
on a failed controller using a healthy controller in the same subsystem.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4be3ce2e72a78..27c33dc49175e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -576,6 +576,157 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset);
>  
> +static struct nvme_ctrl *nvme_find_ctrl_ccr(struct nvme_ctrl *ictrl,
> +					    u32 min_cntlid)
> +{
> +	struct nvme_subsystem *subsys = ictrl->subsys;
> +	struct nvme_ctrl *ctrl, *sctrl = NULL;
> +	unsigned long flags;
> +	int ccr_used;
> +
> +	mutex_lock(&nvme_subsystems_lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		if (ctrl->cntlid < min_cntlid)
> +			continue;
> +
> +		spin_lock_irqsave(&ctrl->lock, flags);
> +		if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE) {
> +			spin_unlock_irqrestore(&ctrl->lock, flags);
> +			continue;
> +		}
> +
> +		ccr_used = atomic_inc_return(&ctrl->ccr_used);
> +		if (ccr_used > ctrl->ccrl) {
> +			atomic_dec(&ctrl->ccr_used);
> +			spin_unlock_irqrestore(&ctrl->lock, flags);
> +			continue;
> +		}
> +
> +		/*
> +		 * We got a good candidate source controller that is locked and
> +		 * LIVE. However, no guarantee ctrl will not be deleted after
> +		 * ctrl->lock is released. Get a ref of both ctrl and admin_q
> +		 * so they do not disappear until we are done with them.
> +		 */
> +		WARN_ON_ONCE(!blk_get_queue(ctrl->admin_q));
> +		nvme_get_ctrl(ctrl);
> +		spin_unlock_irqrestore(&ctrl->lock, flags);
> +		sctrl = ctrl;
> +		break;
> +	}
> +	mutex_unlock(&nvme_subsystems_lock);
> +	return sctrl;
> +}

[ ... ]

> +unsigned long nvme_fence_ctrl(struct nvme_ctrl *ictrl)
> +{
> +	unsigned long now, deadline, timeout;
> +	struct nvme_ctrl *sctrl;
> +	u32 min_cntlid = 0;
> +	int ret;
> +
> +	timeout = nvme_fence_timeout_ms(ictrl);
> +	dev_info(ictrl->device, "attempting CCR, timeout %lums\n", timeout);
> +
> +	now = jiffies;
> +	deadline = jiffies + msecs_to_jiffies(timeout);
> +	while (time_before(now, deadline)) {
> +		sctrl = nvme_find_ctrl_ccr(ictrl, min_cntlid);
> +		if (!sctrl) {
> +			dev_dbg(ictrl->device,
> +				"failed to find source controller\n");
> +			return deadline - now;
> +		}
> +
> +		ret = nvme_issue_wait_ccr(sctrl, ictrl, deadline);
> +		if (!ret) {
> +			dev_info(ictrl->device, "CCR succeeded using %s\n",
> +				 dev_name(sctrl->device));
> +			nvme_put_ctrl_ccr(sctrl);
> +			return 0;
> +		}
> +
> +		/*
> +		 * CCR command or CCR operation failed on this path.
> +		 * Try another path as long as we have time.
> +		 */
> +		dev_err(ictrl->device, "CCR failed using %s, ret = %d\n",
> +			dev_name(sctrl->device), ret);
> +		min_cntlid = sctrl->cntlid + 1;
> +		nvme_put_ctrl_ccr(sctrl);
> +		now = jiffies;
> +	}

[Severity: Medium]
Can nvme_find_ctrl_ccr() skip valid candidate controllers in subsys->ctrls
when retrying after a failed CCR attempt?

nvme_init_subsystem() appends controllers to subsys->ctrls with
list_add_tail(), while nvmf_connect_admin_queue() assigns a new dynamic
ctrl->cntlid on every connect or reconnect without reordering the list.
Because nvme_find_ctrl_ccr() stops at the first live controller with
ctrl->cntlid >= min_cntlid rather than picking the one with the smallest
matching cntlid, failing on an earlier list entry with a higher cntlid and
advancing min_cntlid to sctrl->cntlid + 1 skips any later entries in
subsys->ctrls that have smaller cntlid values, causing nvme_fence_ctrl()
to give up on CCR early and fall back to time-based recovery.

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