Re: [PATCH v6 15/18] nvme-fc: Hold inflight requests while in FENCING state

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Fri 2026-10-30 11:29:12 +0100, Daniel Wagner wrote:
> On Wed, Oct 21, 2026 at 02:23:57PM -0700, Mohamed Khalfella wrote:
> > Adding new FCPOP_STATE_HELD will result in a more invasive change and it
> > will result in nvme-fc no using nvme_cancel_tagset(). My intent with
> > this change is to bring nvme-fc closer to other transport and only
> > cancel inflight requests. synchronize_rcu() is a simpler in this case.
> 
> I am totally against introducing yet another synchronization method. Fix
> it properly.

Here is a diff that applies on top of the patches in order to remove
synchronize_rcu() and add FCPOP_STATE_HELD as you suggested. I will try
to run more testing on this change to see if it holds up. While I do
that please take a look and let me know what do you think.

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 2bb6cdfbb718..6c260f6b47b2 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -83,6 +83,7 @@ enum nvme_fcpop_state {
 	FCPOP_STATE_ACTIVE	= 2,
 	FCPOP_STATE_ABORTED	= 3,
 	FCPOP_STATE_COMPLETE	= 4,
+	FCPOP_STATE_HELD	= 5,
 };
 
 struct nvme_fc_fcp_op {
@@ -2086,9 +2087,6 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 	 * association to be terminated.
 	 */
 
-	/* Pairs with synchronize_rcu() in __nvme_fc_teardown_association() */
-	rcu_read_lock();
-
 	opstate = atomic_xchg(&op->state, FCPOP_STATE_COMPLETE);
 
 	fc_dma_sync_single_for_cpu(ctrl->lport->dev, op->fcp_req.rspdma,
@@ -2201,12 +2199,18 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 
 	/*
 	 * If we are going to terminate associations and the controller is
-	 * LIVE or FENCING, then do not complete this request now. Let error
-	 * recovery cancel this request when it is safe to do so.
+	 * LIVE or FENCING, then do not complete this request now. Move the
+	 * op to HELD and let nvme_fc_cancel_held_requests() cancel it when
+	 * it is safe to do so. HELD is only reachable from COMPLETE through
+	 * this cmpxchg: if the cancellation has already walked the tagset it
+	 * moved the op out of COMPLETE, the cmpxchg fails and the request is
+	 * completed here instead. Either way exactly one path completes it.
 	 */
 	state = nvme_ctrl_state(&ctrl->ctrl);
 	if (terminate_assoc &&
-	    (state == NVME_CTRL_LIVE || state == NVME_CTRL_FENCING))
+	    (state == NVME_CTRL_LIVE || state == NVME_CTRL_FENCING) &&
+	    atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
+			   FCPOP_STATE_HELD) == FCPOP_STATE_COMPLETE)
 		goto check_op_term;
 
 	if (!nvme_try_complete_req(rq, status, result))
@@ -2218,8 +2222,6 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 check_error:
 	if (terminate_assoc)
 		nvme_fc_start_ioerr_recovery(ctrl, "io error");
-
-	rcu_read_unlock();
 }
 
 static int
@@ -3364,13 +3366,6 @@ __nvme_fc_teardown_association(struct nvme_fc_ctrl *ctrl)
 	clear_bit(FCCTRL_TERMIO, &ctrl->flags);
 	spin_unlock_irq(&ctrl->lock);
 
-	/*
-	 * Wait for a started nvme_fc_fcpio_done() to complete before we
-	 * proceed with cancelling inflight requests. This guarantees a
-	 * request is completed by one of the two codepaths, not both.
-	 */
-	synchronize_rcu();
-
 	nvme_fc_term_aen_ops(ctrl);
 
 	/*
@@ -3405,18 +3400,51 @@ __nvme_fc_teardown_association(struct nvme_fc_ctrl *ctrl)
 	nvme_fc_ctlr_inactive_on_rport(ctrl);
 }
 
-/* Complete the requests held in __nvme_fc_teardown_association() */
+static bool
+nvme_fc_cancel_held_request(struct request *req, void *data)
+{
+	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
+	struct nvme_ctrl *ctrl = data;
+
+	/*
+	 * A single cmpxchg decides who completes this request. If the op is
+	 * HELD, nvme_fc_fcpio_done() is done with it and we cancel it. If it
+	 * is still COMPLETE, nvme_fc_fcpio_done() has not decided yet: move
+	 * it to IDLE so its own cmpxchg to HELD fails and it completes the
+	 * request itself (nvme_fc_complete_rq() sets IDLE anyway). Doing this
+	 * as read-then-cmpxchg would let the op become HELD in between and
+	 * leave it with no one to complete it.
+	 */
+	if (atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
+			   FCPOP_STATE_IDLE) == FCPOP_STATE_HELD)
+		nvme_cancel_request(req, ctrl);
+
+	return true;
+}
+
 static void
-nvme_fc_flush_held_requests(struct nvme_fc_ctrl *ctrl)
+nvme_fc_cancel_held_requests(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
 {
-	nvme_cancel_tagset(&ctrl->ctrl);
-	nvme_cancel_admin_tagset(&ctrl->ctrl);
+	if (set) {
+		blk_mq_tagset_busy_iter(set, nvme_fc_cancel_held_request, ctrl);
+		blk_mq_tagset_wait_completed_request(set);
+	}
+}
+
+/* Complete the requests nvme_fc_fcpio_done() held during the fence */
+static void
+nvme_fc_flush_held_requests(struct nvme_fc_ctrl *fc_ctrl)
+{
+	struct nvme_ctrl *ctrl = &fc_ctrl->ctrl;
+
+	nvme_fc_cancel_held_requests(ctrl, ctrl->tagset);
+	nvme_fc_cancel_held_requests(ctrl, ctrl->admin_tagset);
 
 	/* re-enable the admin_q so anything new can fast fail */
-	nvme_unquiesce_admin_queue(&ctrl->ctrl);
+	nvme_unquiesce_admin_queue(ctrl);
 
 	/* resume the io queues so that things will fast fail */
-	nvme_unquiesce_io_queues(&ctrl->ctrl);
+	nvme_unquiesce_io_queues(ctrl);
 }
 
 /*