Re: [RFC PATCH 08/14] nvme: Implement cross-controller reset recovery

[Sagi Grimberg]

On 01/01/2026 1:43, Mohamed Khalfella wrote:
> On Sat 2025-12-27 12:14:11 +0200, Sagi Grimberg wrote:
> > On 26/11/2025 4:11, Mohamed Khalfella wrote:
> > > A host that has more than one path connecting to an nvme subsystem
> > > typically has an nvme controller associated with every path. This is
> > > mostly applicable to nvmeof. If one path goes down, inflight IOs on that
> > > path should not be retried immediately on another path because this
> > > could lead to data corruption as described in TP4129. TP8028 defines
> > > cross-controller reset mechanism that can be used by host to terminate
> > > IOs on the failed path using one of the remaining healthy paths. Only
> > > after IOs are terminated, or long enough time passes as defined by
> > > TP4129, inflight IOs should be retried on another path. Implement core
> > > cross-controller reset shared logic to be used by the transports.
> > >
> > > Signed-off-by: Mohamed Khalfella <mkhalfella@xxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/nvme/host/constants.c |   1 +
> > >    drivers/nvme/host/core.c      | 133 ++++++++++++++++++++++++++++++++++
> > >    drivers/nvme/host/nvme.h      |  10 +++
> > >    3 files changed, 144 insertions(+)
> > >
> > > diff --git a/drivers/nvme/host/constants.c b/drivers/nvme/host/constants.c
> > > index dc90df9e13a2..f679efd5110e 100644
> > > --- a/drivers/nvme/host/constants.c
> > > +++ b/drivers/nvme/host/constants.c
> > > @@ -46,6 +46,7 @@ static const char * const nvme_admin_ops[] = {
> > >    	[nvme_admin_virtual_mgmt] = "Virtual Management",
> > >    	[nvme_admin_nvme_mi_send] = "NVMe Send MI",
> > >    	[nvme_admin_nvme_mi_recv] = "NVMe Receive MI",
> > > +	[nvme_admin_cross_ctrl_reset] = "Cross Controller Reset",
> > >    	[nvme_admin_dbbuf] = "Doorbell Buffer Config",
> > >    	[nvme_admin_format_nvm] = "Format NVM",
> > >    	[nvme_admin_security_send] = "Security Send",
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index f5b84bc327d3..f38b70ca9cee 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -554,6 +554,138 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
> > >    }
> > >    EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset);
> > >    
> > > +static struct nvme_ctrl *nvme_find_ccr_ctrl(struct nvme_ctrl *ictrl,
> > > +					    u32 min_cntlid)
> > > +{
> > > +	struct nvme_subsystem *subsys = ictrl->subsys;
> > > +	struct nvme_ctrl *sctrl;
> > > +	unsigned long flags;
> > > +
> > > +	mutex_lock(&nvme_subsystems_lock);
> > This looks like the wrong lock to take here?
> This is similar to nvme_validate_cntlid()?
> What is the correct lock to use?

Not really, its only because it is called from nvme_init_subsystem which 
spans
subsystems.

> > > +	list_for_each_entry(sctrl, &subsys->ctrls, subsys_entry) {
> > > +		if (sctrl->cntlid < min_cntlid)
> > > +			continue;
> > The use of min_cntlid is not clear to me.
> >
> > > +
> > > +		if (atomic_dec_if_positive(&sctrl->ccr_limit) < 0)
> > > +			continue;
> > > +
> > > +		spin_lock_irqsave(&sctrl->lock, flags);
> > > +		if (sctrl->state != NVME_CTRL_LIVE) {
> > > +			spin_unlock_irqrestore(&sctrl->lock, flags);
> > > +			atomic_inc(&sctrl->ccr_limit);
> > > +			continue;
> > > +		}
> > > +
> > > +		/*
> > > +		 * We got a good candidate source controller that is locked and
> > > +		 * LIVE. However, no guarantee sctrl will not be deleted after
> > > +		 * sctrl->lock is released. Get a ref of both sctrl and admin_q
> > > +		 * so they do not disappear until we are done with them.
> > > +		 */
> > > +		WARN_ON_ONCE(!blk_get_queue(sctrl->admin_q));
> > > +		nvme_get_ctrl(sctrl);
> > > +		spin_unlock_irqrestore(&sctrl->lock, flags);
> > > +		goto found;
> > > +	}
> > > +	sctrl = NULL;
> > > +found:
> > > +	mutex_unlock(&nvme_subsystems_lock);
> > > +	return sctrl;
> > > +}
> > > +
> > > +static int nvme_issue_wait_ccr(struct nvme_ctrl *sctrl, struct nvme_ctrl *ictrl)
> > > +{
> > > +	unsigned long flags, tmo, remain;
> > > +	struct nvme_ccr_entry ccr = { };
> > > +	union nvme_result res = { 0 };
> > > +	struct nvme_command c = { };
> > > +	u32 result;
> > > +	int ret = 0;
> > > +
> > > +	init_completion(&ccr.complete);
> > > +	ccr.ictrl = ictrl;
> > > +
> > > +	spin_lock_irqsave(&sctrl->lock, flags);
> > > +	list_add_tail(&ccr.list, &sctrl->ccrs);
> > > +	spin_unlock_irqrestore(&sctrl->lock, flags);
> > > +
> > > +	c.ccr.opcode = nvme_admin_cross_ctrl_reset;
> > > +	c.ccr.ciu = ictrl->ciu;
> > > +	c.ccr.icid = cpu_to_le16(ictrl->cntlid);
> > > +	c.ccr.cirn = cpu_to_le64(ictrl->cirn);
> > > +	ret = __nvme_submit_sync_cmd(sctrl->admin_q, &c, &res,
> > > +				     NULL, 0, NVME_QID_ANY, 0);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	result = le32_to_cpu(res.u32);
> > > +	if (result & 0x01) /* Immediate Reset */
> > > +		goto out;
> > > +
> > > +	tmo = msecs_to_jiffies(max(ictrl->cqt, ictrl->kato * 1000));
> > > +	remain = wait_for_completion_timeout(&ccr.complete, tmo);
> > > +	if (!remain)
> > I think remain is redundant here.
> Deleted 'remain'.
>
> > > +		ret = -EAGAIN;
> > > +out:
> > > +	spin_lock_irqsave(&sctrl->lock, flags);
> > > +	list_del(&ccr.list);
> > > +	spin_unlock_irqrestore(&sctrl->lock, flags);
> > > +	return ccr.ccrs == 1 ? 0 : ret;
> > Why would you still return 0 and not EAGAIN? you expired on timeout but
> > still
> > return success if you have ccrs=1? btw you have ccrs in the ccr struct
> > and in the controller
> > as a list. Lets rename to distinguish the two.
> True, we did expire timeout here. However, after we removed the ccr
> entry we found that it was marked as completed. We return success in
> this case even though we hit timeout.

When does this happen? Why is it worth having the code non-intuitive for
something that effectively never happens (unless I'm missing something?)

> Renamed ctrl->ccrs to ctrl->ccr_list.
>
> > > +}
> > > +
> > > +unsigned long nvme_recover_ctrl(struct nvme_ctrl *ictrl)
> > > +{
> > I'd call it nvme_fence_controller()
> Okay. I will do that. I will also rename the controller state FENCING.
>
> > > +	unsigned long deadline, now, timeout;
> > > +	struct nvme_ctrl *sctrl;
> > > +	u32 min_cntlid = 0;
> > > +	int ret;
> > > +
> > > +	timeout = nvme_recovery_timeout_ms(ictrl);
> > > +	dev_info(ictrl->device, "attempting CCR, timeout %lums\n", timeout);
> > > +
> > > +	now = jiffies;
> > > +	deadline = now + msecs_to_jiffies(timeout);
> > > +	while (time_before(now, deadline)) {
> > > +		sctrl = nvme_find_ccr_ctrl(ictrl, min_cntlid);
> > > +		if (!sctrl) {
> > > +			/* CCR failed, switch to time-based recovery */
> > > +			return deadline - now;
> > It is not clear what is the return code semantics of this function.
> > How about making it success/failure and have the caller choose what to do?
> The function returns 0 on success. On failure it returns the time in
> jiffies to hold requests for before they are canceled. On failure the
> returned time is essentially the hold time defined in TP4129 minus the
> time it took to attempt CCR.

I think it would be cleaner to simple have this function return status 
code and
have the caller worry about time spent.

> > > +		}
> > > +
> > > +		ret = nvme_issue_wait_ccr(sctrl, ictrl);
> > > +		atomic_inc(&sctrl->ccr_limit);
> > inc after you wait for the ccr? shouldn't this be before?
> I think it should be after we wait for CCR. sctrl->ccr_limit is the
> number of concurrent CCRs the controller supports. Only after we are
> done with CCR on this controller we increment it.

Maybe it should be folded into nvme_issue_wait_ccr for symmetry?

> > > +
> > > +		if (!ret) {
> > > +			dev_info(ictrl->device, "CCR succeeded using %s\n",
> > > +				 dev_name(sctrl->device));
> > > +			blk_put_queue(sctrl->admin_q);
> > > +			nvme_put_ctrl(sctrl);
> > > +			return 0;
> > > +		}
> > > +
> > > +		/* Try another controller */
> > > +		min_cntlid = sctrl->cntlid + 1;
> > OK, I see why min_cntlid is used. That is very non-intuitive.
> >
> > I'm wandering if it will be simpler to take one-shot at ccr and
> > if it fails fallback to crt. I mean, if the sctrl is alive, and it was
> > unable
> > to reset the ictrl in time, how would another ctrl do a better job here?
> We need to attempt CCR from multiple controllers for reason explained in
> another response. As you figured out min_cntlid is needed in order to
> not loop controller list forever. Do you have a better idea?

No, just know that I don't like it very much :)

> > > +		blk_put_queue(sctrl->admin_q);
> > > +		nvme_put_ctrl(sctrl);
> > > +		now = jiffies;
> > > +	}
> > > +
> > > +	dev_info(ictrl->device, "CCR reached timeout, call it done\n");
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(nvme_recover_ctrl);
> > > +
> > > +void nvme_end_ctrl_recovery(struct nvme_ctrl *ctrl)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&ctrl->lock, flags);
> > > +	WRITE_ONCE(ctrl->state, NVME_CTRL_RESETTING);
> > This needs to be a proper state transition.
> We do not want to have proper transition from RECOVERING to RESETTING.
> The reason is that we do not want the controller to be reset while it is
> being recovered/fenced because requests should not be canceled. One way
> to keep the transitions in nvme_change_ctrl_state() is to use two
> states. Say FENCING and FENCED.
>
> The allowed transitions are
>
> - LIVE -> FENCING
> - FENCING -> FENCED
> - FENCED -> (RESETTING, DELETING)
>
> This will also git rid of NVME_CTRL_RECOVERED
>
> Does this sound good?

We could do what failfast is doing, in case we get transition FENCING -> 
RESETTING/DELETING we flush
the fence_work...