Re: [PATCH v3 14/21] nvme-fc: Hold inflight requests while in FENCING state

[Randy Jennings]

On Fri, Feb 13, 2026 at 8:28 PM Mohamed Khalfella
<mkhalfella@xxxxxxxxxxxxxxx> wrote:
>
> While in FENCING state, aborted inflight IOs should be held until fencing
> is done. Update nvme_fc_fcpio_done() to not complete aborted requests or
> requests with transport errors. These held requests will be canceled in
> nvme_fc_delete_association() after fencing is done. nvme_fc_fcpio_done()
> avoids racing with canceling aborted requests by making sure we complete
> successful requests before waking up the waiting thread.
>
> Signed-off-by: Mohamed Khalfella <mkhalfella@xxxxxxxxxxxxxxx>
>  static void nvme_fc_fencing_work(struct work_struct *work)
> @@ -1969,7 +1978,8 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>         struct nvme_command *sqe = &op->cmd_iu.sqe;
>         __le16 status = cpu_to_le16(NVME_SC_SUCCESS << 1);
>         union nvme_result result;
> -       bool terminate_assoc = true;
> +       bool op_term, terminate_assoc = true;
> +       enum nvme_ctrl_state state;
>         int opstate;
>
>         /*
> @@ -2102,16 +2112,38 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>  done:
>         if (op->flags & FCOP_FLAGS_AEN) {
>                 nvme_complete_async_event(&queue->ctrl->ctrl, status, &result);
> -               __nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +               if (__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate))
> +                       __nvme_fc_fcpop_count_one_down(ctrl);
>                 atomic_set(&op->state, FCPOP_STATE_IDLE);
>                 op->flags = FCOP_FLAGS_AEN;     /* clear other flags */
>                 nvme_fc_ctrl_put(ctrl);
>                 goto check_error;
>         }
>
> -       __nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +       /*
> +        * We can not access op after the request is completed because it can
> +        * be reused immediately. At the same time we want to wakeup the thread
> +        * waiting for ongoing IOs _after_ requests are completed. This is
> +        * necessary because that thread will start canceling inflight IOs
> +        * and we want to avoid request completion racing with cancellation.
> +        */
> +       op_term = __nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +
> +       /*
> +        * 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.
> +        */
> +       state = nvme_ctrl_state(&ctrl->ctrl);
> +       if (terminate_assoc &&
> +           (state == NVME_CTRL_LIVE || state == NVME_CTRL_FENCING))
> +               goto check_op_term;
> +
>         if (!nvme_try_complete_req(rq, status, result))
>                 nvme_fc_complete_rq(rq);
> +check_op_term:
> +       if (op_term)
> +               __nvme_fc_fcpop_count_one_down(ctrl);

Although it is a more complicated boolean expression, I think it is
easier to grok:
> +       if (!(terminate_assoc &&
> +             (state == NVME_CTRL_LIVE || state == NVME_CTRL_FENCING)) &&
> +           !nvme_try_complete_req(rq, status, result))
>                 nvme_fc_complete_rq(rq);
resulting in one less goto.

Sincerely,
Randy Jennings