Re: [PATCH v6 09/18] nvme: Implement cross-controller reset completion

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Wed 2026-10-21 10:43:03 +0200, Daniel Wagner wrote:
> On Tue, Oct 20, 2026 at 06:04:38PM -0700, Mohamed Khalfella wrote:
> > > What happens to the waiters on sctrl->ccr_list when the source
> > > controller enters error recovery while a CCR is pending?
> > 
> > You already answered it below :)
> 
> I tried to figure it out, but I'm still not sure if I really understand
> all the conditions. So bear with me.

No problem. Take your time.

> 
> > > As far as I can tell, ccr_work itself is fine. The reset path cancels
> > > it via nvme_stop_ctrl(), and in the error recovery path the Get Log
> > > Page just fails once the admin queue is torn down. But the target drops
> > > all CCRs issued by the old association when it frees the controller,
> > > so the CCR completed AEN for them will never arrive. Nothing on the
> > > host side walks ccr_list when the controller leaves LIVE, so
> > > nvme_issue_wait_ccr() sleeps until min(kato, deadline - now) runs out
> > > before nvme_fence_ctrl() tries the next source controller. In the
> > > worst case that eats the whole fencing window.
> > 
> > If the remaining of the fencing window (deadline - now) is less than
> > kato then I am fine with this scenario wasting this time anyway. If we
> > hit the deadline then we consider fencing done anyway. If it consumes
> > kato from the deadline then we retry another path.
> 
> Okay, this might be something worth documenting? It seems like a case we
> will get bug reports in the future.

I do not think this behavior worth documenting as stable behavior. We
should be able to refactor/change/optimize this code in the future
without worrying about breaking a documented behavior.

The main point of CCR is to speed up controller fencing in order to avoid
CQT delay. I think of it as a best-effort process attempted in a bounded
amount of time. If it succeeds we skip the CQT delay, otherwise we spend
the time we will have spent in CQT delay anyway.

Can we do better within this bounded amount of time? Sure, this is what
is being suggested here. However, I prefer not to do that part of this
patchset. Also, do not like to clutter the code with comments about
potential changes.

> 
> > > Would it make sense to fail all pending entries (set ccrs to a failure
> > > status and complete them under ctrl->lock) when the source controller
> > > goes into error recovery, so the fencing code can move on to another
> > > path right away?
> > 
> > Failing pending CCR entries that we know we are not going to complete
> > will help in this case. The question is where to do that. A LIVE
> > controller might hit an error and transition to FENCING, or manually 
> > reset and move to RESETTING, or deleted and go to DELETING. Do we need
> > to consider all these places to fail the pending CCRs?
> 
> I'd say yes. Anyway, I don't think this needs to be addressed right away
> if the current code is functional correct and we are just not as fast as
> possible. 

Agree, the goal of the patchset is to add the initial support for
CCR/CQT. If there are functional issues they should be addressed now.
Known gaps and optimizations can come in later patchsets.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ca7f083702f5..6c0703fba8ff 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -863,6 +863,9 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
                wake_up_all(&ctrl->state_wq);
        }

+       if (changed && old_state == NVME_CTRL_LIVE)
+               nvme_fail_pending_ccrs(ctrl); /* TBD */
+
        spin_unlock_irqrestore(&ctrl->lock, flags);
        if (!changed)
                return false;

One way to fail pending CCRs is to add it to controller state machine.
Something like the diff above.

> 
> > You might have also noticed that nvme_issue_wait_ccr() calling
> > __nvme_submit_sync_cmd() might consume the entire fencing window if the
> > CCR command gets held because the source controller hits an error. This
> > is another area be optimized. I am inclined to leave the code as it is
> > today. What is being described here is an optimization that can come
> > later on.
> 
> Yes, I was actually about to ask about __nvme_submit_sync_cmd in this
> path next. Since my questions are about optimization, I don't think we
> should make the current version any more complex than it already is. We
> can handle optimization later, but please document those cases in the
> meantime.

Hannes also raised this point. It is on my todo list to add timeout
parameter to __nvme_submit_sync_cmd() and use that here.