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

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Tue 2026-11-03 11:51:28 +0100, Daniel Wagner wrote:
> On Mon, Nov 02, 2026 at 05:18:51PM -0800, Mohamed Khalfella wrote:
> > -/* 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) {
> 
> Do we call flush when we don't have a set?

Yes, that is possible in case the controller fail first connect attempt
before the IO tagset is created. Commit ee38469f8849 ("nvme-fc: Do not
cancel requests in io target before it is initialized") added similar
check to __nvme_fc_abort_outstanding_ios() to close this gap.

> 
> nitpick: use the early return pattern if needed.

Done.

> 
> > +		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);
> >  }
> >  
> >  /*
> 
> Yes, I'd prefer this over the introduction of RCU in this code path.
> 

Okay. I spent a lot of time thinking about these two atomic_cmpxchg() to
make sure they cover all scenarios. That is holding all requests to be
held and at the same time not leave a request in a held state and not
get to complete it. Let me know if you think I missed a scenario.